Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Data masking] Warn when passing object to useFragment/watchFragment from that is not identifiable #12004

Merged
merged 7 commits into from
Aug 16, 2024

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Aug 14, 2024

Closes #11675

Adds a warning when passing an object to the from property that is not identifiable from the cache (i.e. cache.identify returns undefined). This also adds some tests to check that objects returned from masked queries can be read from watchFragment and useFragment.

Copy link

changeset-bot bot commented Aug 14, 2024

⚠️ No Changeset found

Latest commit: fd66d42

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR


if (!id) {
invariant.warn(
"Could not identify object passed to `from` either because the object is non-normalized or the key fields are missing. If you are masking this object, please ensure the key fields are requested by the parent object."
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, this might be a bit too generic and be hard to find what is triggering this warning. Perhaps we add the fragment name in here as well?

Should we also try and differentiate between watchFragment and useFragment, or would adding in the fragment name be enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragment name added in fd66d42

@@ -245,10 +245,17 @@ export abstract class ApolloCache<TSerialized> implements DataProxy {
): Observable<WatchFragmentResult<TData>> {
const { fragment, fragmentName, from, optimistic = true } = options;
const query = this.getFragmentDoc(fragment, fragmentName);
const id = typeof from === "string" ? from : this.identify(from);

if (!id) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might make sense as a dev-only warning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev-only warning added in 885be47

Comment on lines +1587 to +1590
expect(console.warn).toHaveBeenCalledWith(
"Could not identify object passed to `from` for '%s' fragment, either because the object is non-normalized or the key fields are missing. If you are masking this object, please ensure the key fields are requested by the parent object.",
"UserFields"
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting question: do we want to auto-unmask key fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid that since it might be surprising behavior and instead just recommend that someone selects from the parent entity instead. Definitely something we can keep an eye on and consider down the road!

@github-actions github-actions bot added the auto-cleanup 🤖 label Aug 16, 2024
@jerelmiller jerelmiller merged commit 28bceb0 into data-masking Aug 16, 2024
40 of 46 checks passed
@jerelmiller jerelmiller deleted the jerel/from-property branch August 16, 2024 14:16
jerelmiller added a commit that referenced this pull request Aug 16, 2024
jerelmiller added a commit that referenced this pull request Aug 16, 2024
jerelmiller added a commit that referenced this pull request Aug 28, 2024
jerelmiller added a commit that referenced this pull request Aug 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants