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

Add data masking to fragments read by useFragment/watchFragment #12018

Merged
merged 29 commits into from
Aug 27, 2024

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Aug 23, 2024

Closes #12022

Adds data masking to watchFragment and useFragment.

Copy link

changeset-bot bot commented Aug 23, 2024

⚠️ No Changeset found

Latest commit: 8970b7d

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

@jerelmiller jerelmiller changed the base branch from data-masking to jerel/enable-masking-in-cache-config August 23, 2024 02:58
@jerelmiller jerelmiller force-pushed the jerel/mask-fragments branch 4 times, most recently from f60b539 to c7eb755 Compare August 23, 2024 05:23
@jerelmiller jerelmiller changed the title [WIP] Mask nested fragments read by useFragment/watchFragment Mask nested fragments read by useFragment/watchFragment Aug 23, 2024
@jerelmiller jerelmiller changed the title Mask nested fragments read by useFragment/watchFragment Add data masking to fragments read by useFragment/watchFragment Aug 23, 2024
@jerelmiller jerelmiller added the 🎭 data-masking Issues/PRs related to data masking label Aug 23, 2024
@jerelmiller jerelmiller marked this pull request as ready for review August 23, 2024 05:29
@jerelmiller jerelmiller requested a review from phryneas August 23, 2024 05:29
@jerelmiller
Copy link
Member Author

Moving to draft until I've moved this to work with the client.watchFragment, but not cache.readFragment as discussed in #12019

@jerelmiller jerelmiller marked this pull request as draft August 23, 2024 16:54
@jerelmiller jerelmiller force-pushed the jerel/mask-fragments branch from c7eb755 to e10e358 Compare August 23, 2024 17:14
@jerelmiller jerelmiller changed the base branch from jerel/enable-masking-in-cache-config to data-masking August 23, 2024 17:14
@jerelmiller jerelmiller force-pushed the jerel/mask-fragments branch from e10e358 to dcb805a Compare August 23, 2024 18:28
@jerelmiller jerelmiller force-pushed the jerel/mask-fragments branch from 9569f52 to b54c22f Compare August 23, 2024 20:11
@jerelmiller jerelmiller marked this pull request as ready for review August 23, 2024 20:24
@jerelmiller jerelmiller force-pushed the jerel/mask-fragments branch from b54c22f to 51c0110 Compare August 23, 2024 22:29
@@ -394,6 +419,27 @@ export abstract class ApolloCache<TSerialized> implements DataProxy {
return maskOperation(data, document, this.fragmentMatches.bind(this));
}

public maskFragment<TData = unknown>(options: MaskFragmentOptions<TData>) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies ahead of time. This has been moved in #12029, but wanted to do that together with maskOperation, so its in the other PR.

@jerelmiller jerelmiller requested a review from alessbell August 26, 2024 19:22
src/__tests__/dataMasking.ts Outdated Show resolved Hide resolved
Comment on lines +551 to +563
const { fragment, fragmentName } = options;

const observable = this.cache.watchFragment(options);
let latestResult: WatchFragmentResult<TFragmentData> | undefined;

return new Observable((observer) => {
const subscription = observable.subscribe({
next: (result) => {
result.data = this.queryManager.maskFragment({
fragment,
fragmentName,
data: result.data,
});
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This works, too, I guess.

My assumption while we were discussing this was more along the lines of "masking happens in the cache, but is not enabled by default there".

So, when calling cache.watchFragment, you can pass along a dataMasking option, but it's false by default - and coming from ApolloClient, it would be this.queryManager.dataMasking by default.

So the implementation here would be more along the lines of

const { dataMasking = this.queryManager.dataMasking, ...rest } = options
return cache.watchFragment({...rest, dataMasking})

Of course, implementing it like this works, too. But if someone were to use a cache without a fragment masking implementation, this would bloat it a little 🤔.

Up to you to make the final call here :)

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 initially had something like this, but #12029 moves these methods out of the base cache class. I left it like this to prepare for that PR, but I'm realizing now how I can achieve essentially what you're suggesting here but without the class methods.

I'd prefer to keep this option private if possible, so I might bury this option in a symbol so that cache.watchFragment doesn't have a public option for data masking (we can always add as a public option later, but I'm trying to be strict right now about where/when its added until we finalize the rules on that).

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'll actually go implement this refactor in #12029 since I've moved these methods out of Cache in that PR and can better address along with those changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in #12029 (specifically 62c6a1d)

@github-actions github-actions bot added the auto-cleanup 🤖 label Aug 27, 2024
@jerelmiller jerelmiller merged commit 120ace4 into data-masking Aug 27, 2024
32 checks passed
@jerelmiller jerelmiller deleted the jerel/mask-fragments branch August 27, 2024 15:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-cleanup 🤖 🎭 data-masking Issues/PRs related to data masking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Data masking] Mask named fragments from data returned from watchFragment and useFragment
2 participants