[Data masking] Warn when using data masking with `no-cache` operations #12121

jerelmiller · 2024-11-13T00:03:41Z

Emits a warning when using no-cache operations with data masking since watchFragment and useFragment rely on the cache to read fragment data. This warning will not be emitted when either dataMasking is disabled or if the operation already contains @unmask for all fragments.

svc-apollo-docs · 2024-11-13T00:03:44Z

✅ Docs Preview Ready

No new or changed pages found.

changeset-bot · 2024-11-13T00:03:44Z

⚠️ No Changeset found

Latest commit: 8a3fece

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 · 2024-11-13T00:04:15Z

src/core/QueryManager.ts

+        }
+
+        invariant.warn(
+          'Fragments masked by data masking when using fetch policy "no-cache" cannot be read by `watchFragment` or `useFragment`. Please add `@unmask` to the fragment to read the fragment data.'


Suggestions welcome on better wording here.

Maybe

-...to read the fragment data.+ +...to make the fragment data available for reading without those hooks.

?

It might also be helpful to include the name of the document.

pkg-pr-new · 2024-11-13T00:04:37Z

npm i https://pkg.pr.new/@apollo/client@12121

commit: f57d5a9

github-actions · 2024-11-13T00:04:55Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	40.6 KB (+0.14% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	50.99 KB (+0.45% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	47.57 KB (+0.24% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	36.31 KB (+0.59% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	33.56 KB (+0.18% 🔺)
import { ApolloProvider } from "dist/react/index.js"	1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.25 KB (-0.08% 🔽)
import { useQuery } from "dist/react/index.js"	5.22 KB (+0.02% 🔺)
import { useQuery } from "dist/react/index.js" (production)	4.3 KB (-0.03% 🔽)
import { useLazyQuery } from "dist/react/index.js"	5.71 KB (+0.02% 🔺)
import { useLazyQuery } from "dist/react/index.js" (production)	4.78 KB (0%)
import { useMutation } from "dist/react/index.js"	3.63 KB (-0.03% 🔽)
import { useMutation } from "dist/react/index.js" (production)	2.85 KB (0%)
import { useSubscription } from "dist/react/index.js"	4.43 KB (+0.03% 🔺)
import { useSubscription } from "dist/react/index.js" (production)	3.48 KB (-0.03% 🔽)
import { useSuspenseQuery } from "dist/react/index.js"	5.52 KB (+0.02% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.17 KB (+0.03% 🔺)
import { useBackgroundQuery } from "dist/react/index.js"	5.02 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.67 KB (+0.03% 🔺)
import { useLoadableQuery } from "dist/react/index.js"	5.1 KB (+0.04% 🔺)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.74 KB (-0.03% 🔽)
import { useReadQuery } from "dist/react/index.js"	3.42 KB (+0.03% 🔺)
import { useReadQuery } from "dist/react/index.js" (production)	3.36 KB (+0.03% 🔺)
import { useFragment } from "dist/react/index.js"	2.34 KB (+0.05% 🔺)
import { useFragment } from "dist/react/index.js" (production)	2.29 KB (+0.05% 🔺)

jerelmiller · 2024-11-13T00:05:04Z

src/core/ApolloClient.ts

@@ -517,6 +517,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
        data: this.queryManager.maskOperation({
          document: options.query,
          data: result.data,
+          fetchPolicy: options.fetchPolicy,


Subscriptions don't have a queryId so this would mean the warning would be emitted for every subscription event. Are we ok with that? Should we use something else as the query ID to try and only emit this warning once per subscription?

Yeah, I think a counter on module level and using something like "subscription"+(id++) as queryId would be fine here. Might make sense to rename the queryId option to something like id then.

phryneas · 2024-11-13T10:38:54Z

src/utilities/graphql/fragments.ts

+        return BREAK;
+      }
+
+      isUnmasked &&= node.directives.some(


Suggested change

isUnmasked &&= node.directives.some(

isUnmasked = node.directives.some(

I think we can safely assume that at this point, isUnmasked is always true, right?

phryneas · 2024-11-13T10:41:37Z

src/utilities/graphql/fragments.ts

+    FragmentSpread: (node) => {
+      if (!node.directives) {
+        isUnmasked = false;
+        return BREAK;
+      }
+
+      isUnmasked &&= node.directives.some(
+        (directive) => directive.name.value === "unmask"
+      );
+
+      if (!isUnmasked) {
+        return BREAK;
+      }
+    },


Suggested change

FragmentSpread: (node) => {

if (!node.directives) {

isUnmasked = false;

return BREAK;

}

isUnmasked &&= node.directives.some(

(directive) => directive.name.value === "unmask"

);

if (!isUnmasked) {

return BREAK;

}

},

FragmentSpread: (node) => {

isUnmasked = !!node.directives && node.directives.some(

(directive) => directive.name.value === "unmask"

);

if (!isUnmasked) {

return BREAK;

}

},

I think this whole block could be simplified like this.

phryneas · 2024-11-13T11:17:06Z

src/__tests__/dataMasking.ts

+    });
+    const stream = new ObservableStream(observable);
+
+    link.simulateResult({


It would be great if the client.subscribe tests would show more than one subscription result.

phryneas

Left a bunch of comments and suggestions, but as a whole: approved :)

jerelmiller · 2024-11-13T16:32:46Z

Reminder to self: don't forget a changeset

jerelmiller added 11 commits November 12, 2024 15:57

Add tests to ensure warning is emitted with no-cache queries

e832e68

Add implementation to warn for no-cache query

4b9ccb5

Don't warn when query is fully unmasked

63124bf

Extract warning to a constant

cb9496d

Add no-cache warnings when using client.query

0552fc2

Remove unused import

40b23f3

Add warning for client.subscribe

fb49a69

Ensure mutations warn with no-cache fetch policy and data masking

1765ee8

Add additional tests for warnings with no-cache on client.mutate

5a3b9e5

Add additional tests for warnings with no-cache on client.subscribe

d3aad6d

Update size limits and api report

75bc952

jerelmiller added the 🎭 data-masking Issues/PRs related to data masking label Nov 13, 2024

jerelmiller added this to the Data masking milestone Nov 13, 2024

jerelmiller requested a review from phryneas November 13, 2024 00:03

jerelmiller commented Nov 13, 2024

View reviewed changes

jerelmiller added 3 commits November 12, 2024 17:06

Update snapshot export

a655b06

Rename isFullyUnmaskedQuery to isFullyUnmaskedOperation

f14f97f

Update size limits and api report

ddc9893

jerelmiller linked an issue Nov 13, 2024 that may be closed by this pull request

[Data masking] Allow useFragment to work with no-cache queries #11678

Closed

phryneas reviewed Nov 13, 2024

View reviewed changes

phryneas approved these changes Nov 13, 2024

View reviewed changes

github-actions bot added the auto-cleanup 🤖 label Nov 13, 2024

jerelmiller added 6 commits November 13, 2024 09:48

Reword warning and provide more info

3a26fd5

Minor refactor to isFullyUnmaskedOperation

61c8a41

Rename queryId to id in maskOperation options

67aa4eb

Ensure warning is only called once per subscription

70f2d60

Require id for maskOperation

81c14b3

Add prefix to operation id in maskOperation

f57d5a9

jerelmiller force-pushed the jerel/no-cache-warning branch from 3c8b82b to f57d5a9 Compare November 13, 2024 17:07

Clean up Prettier, Size-limit, and Api-Extractor

8a3fece

jerelmiller merged commit 1085a95 into release-3.12 Nov 13, 2024
32 of 36 checks passed

jerelmiller deleted the jerel/no-cache-warning branch November 13, 2024 17:17

This was referenced Nov 13, 2024

Add changeset for #12121 #12123

Merged

[Data masking] Allow useFragment to work with no-cache queries #11678

Closed

Version Packages (rc) #12115

Merged

Version Packages (rc) #12125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data masking] Warn when using data masking with `no-cache` operations #12121

[Data masking] Warn when using data masking with `no-cache` operations #12121

jerelmiller commented Nov 13, 2024

svc-apollo-docs commented Nov 13, 2024 •

edited

Loading

changeset-bot bot commented Nov 13, 2024 •

edited

Loading

jerelmiller Nov 13, 2024

phryneas Nov 13, 2024

pkg-pr-new bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

jerelmiller Nov 13, 2024

phryneas Nov 13, 2024

phryneas Nov 13, 2024

phryneas Nov 13, 2024

phryneas Nov 13, 2024

phryneas left a comment

jerelmiller commented Nov 13, 2024

	isUnmasked &&= node.directives.some(
	isUnmasked = node.directives.some(

[Data masking] Warn when using data masking with no-cache operations #12121

[Data masking] Warn when using data masking with no-cache operations #12121

Conversation

jerelmiller commented Nov 13, 2024

svc-apollo-docs commented Nov 13, 2024 • edited Loading

✅ Docs Preview Ready

changeset-bot bot commented Nov 13, 2024 • edited Loading

⚠️ No Changeset found

jerelmiller Nov 13, 2024

Choose a reason for hiding this comment

phryneas Nov 13, 2024

Choose a reason for hiding this comment

pkg-pr-new bot commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024 • edited Loading

size-limit report 📦

jerelmiller Nov 13, 2024

Choose a reason for hiding this comment

phryneas Nov 13, 2024

Choose a reason for hiding this comment

phryneas Nov 13, 2024

Choose a reason for hiding this comment

phryneas Nov 13, 2024

Choose a reason for hiding this comment

phryneas Nov 13, 2024

Choose a reason for hiding this comment

phryneas left a comment

Choose a reason for hiding this comment

jerelmiller commented Nov 13, 2024

[Data masking] Warn when using data masking with `no-cache` operations #12121

[Data masking] Warn when using data masking with `no-cache` operations #12121

svc-apollo-docs commented Nov 13, 2024 •

edited

Loading

changeset-bot bot commented Nov 13, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading