From cfaf4efc6dea56ae46a5b5199d8ed9414b0f17d8 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Fri, 15 Nov 2024 10:57:03 -0700 Subject: [PATCH] [Data masking] Fix issue with `@unmask(mode: "migrate")` with nested partial objects from parent query (#12134) --- .changeset/gorgeous-zebras-confess.md | 5 + .size-limits.json | 4 +- src/core/__tests__/masking.test.ts | 122 ++++++++++ src/core/masking.ts | 324 ++++++++++++++------------ 4 files changed, 304 insertions(+), 151 deletions(-) create mode 100644 .changeset/gorgeous-zebras-confess.md diff --git a/.changeset/gorgeous-zebras-confess.md b/.changeset/gorgeous-zebras-confess.md new file mode 100644 index 0000000000..3d7e4265ed --- /dev/null +++ b/.changeset/gorgeous-zebras-confess.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix issue where data went missing when an unmasked fragment in migrate mode selected fields that the parent did not. diff --git a/.size-limits.json b/.size-limits.json index 91e88ff5ac..45bba8c25f 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 41601, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34359 + "dist/apollo-client.min.cjs": 41638, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34394 } diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index db44191ac2..44eddab72f 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -1717,6 +1717,128 @@ describe("maskOperation", () => { }); }); + test('handles overlapping types when subtype has accessor warnings with @unmask(mode: "migrate")', async () => { + using consoleSpy = spyOnConsole("warn"); + const query = gql` + query PlaylistQuery { + playlist { + ...PlaylistFragment @unmask(mode: "migrate") + id + name + album { + id + tracks { + id + __typename + } + __typename + } + artist { + id + topTracks { + id + __typename + } + __typename + } + __typename + + ...PlaylistTitleCell @unmask(mode: "migrate") + } + } + + fragment PlaylistFragment on Playlist { + album { + id + images { + url + __typename + } + tracks { + id + name + __typename + } + __typename + } + } + + fragment PlaylistTitleCell on Playlist { + artist { + id + images { + url + __typename + } + topTracks { + id + name + __typename + } + __typename + } + } + `; + + const data = maskOperation( + { + playlist: { + id: "1", + name: "Playlist", + album: { + id: "2RSIoPew2TOy41ASHpzOx3", + __typename: "Album", + images: [{ url: "https://i.scdn.co/image/1", __typename: "Image" }], + tracks: [{ id: "1", name: "Track 1", __typename: "Track" }], + }, + artist: { + id: "2", + __typename: "Artist", + images: [{ url: "https://i.scdn.co/image/1", __typename: "Image" }], + topTracks: [{ id: "2", name: "Track 2", __typename: "Track" }], + }, + }, + }, + query, + new InMemoryCache() + ); + + expect(consoleSpy.warn).not.toHaveBeenCalled(); + + consoleSpy.warn.mockClear(); + + data.playlist.album; + data.playlist.album.id; + data.playlist.album.__typename; + data.playlist.artist; + data.playlist.artist.id; + data.playlist.artist.__typename; + expect(console.warn).not.toHaveBeenCalled(); + + data.playlist.album.images; + data.playlist.artist.images; + expect(console.warn).toHaveBeenCalledTimes(2); + + expect(data).toEqual({ + playlist: { + id: "1", + name: "Playlist", + album: { + id: "2RSIoPew2TOy41ASHpzOx3", + __typename: "Album", + images: [{ url: "https://i.scdn.co/image/1", __typename: "Image" }], + tracks: [{ id: "1", name: "Track 1", __typename: "Track" }], + }, + artist: { + id: "2", + __typename: "Artist", + images: [{ url: "https://i.scdn.co/image/1", __typename: "Image" }], + topTracks: [{ id: "2", name: "Track 2", __typename: "Track" }], + }, + }, + }); + }); + test("masks fragments in subscription documents", () => { const subscription = gql` subscription { diff --git a/src/core/masking.ts b/src/core/masking.ts index 2ead4710af..fdede0508a 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -1,5 +1,9 @@ import { Kind } from "graphql"; -import type { FragmentDefinitionNode, SelectionSetNode } from "graphql"; +import type { + FragmentDefinitionNode, + SelectionNode, + SelectionSetNode, +} from "graphql"; import { createFragmentMap, resultKeyNameFromField, @@ -19,7 +23,6 @@ interface MaskingContext { operationName: string | undefined; fragmentMap: FragmentMap; cache: ApolloCache; - disableWarnings?: boolean; } // Contextual slot that allows us to disable accessor warnings on fields when in @@ -169,110 +172,117 @@ function maskSelectionSet( return [changed ? masked : data, changed]; } - const result = selectionSet.selections.reduce<[any, boolean]>( - ([memo, changed], selection) => { - switch (selection.kind) { - case Kind.FIELD: { - const keyName = resultKeyNameFromField(selection); - const childSelectionSet = selection.selectionSet; - - memo[keyName] = data[keyName]; + const result = selectionSet.selections + .concat() + .sort(sortFragmentsLast) + .reduce<[any, boolean]>( + ([memo, changed], selection) => { + switch (selection.kind) { + case Kind.FIELD: { + const keyName = resultKeyNameFromField(selection); + const childSelectionSet = selection.selectionSet; - if (memo[keyName] === void 0) { - delete memo[keyName]; - } + memo[keyName] = data[keyName]; - if (keyName in memo && childSelectionSet && data[keyName] !== null) { - const [masked, childChanged] = maskSelectionSet( - data[keyName], - childSelectionSet, - context, - __DEV__ ? `${path || ""}.${keyName}` : void 0 - ); + if (memo[keyName] === void 0) { + delete memo[keyName]; + } if ( - childChanged || - // This check prevents cases where masked fields may accidentally be - // returned as part of this object when the fragment also selects - // additional fields from the same child selection. - Object.keys(masked).length !== Object.keys(data[keyName]).length + keyName in memo && + childSelectionSet && + data[keyName] !== null ) { - memo[keyName] = masked; - changed = true; + const [masked, childChanged] = maskSelectionSet( + data[keyName], + childSelectionSet, + context, + __DEV__ ? `${path || ""}.${keyName}` : void 0 + ); + + if ( + childChanged || + // This check prevents cases where masked fields may accidentally be + // returned as part of this object when the fragment also selects + // additional fields from the same child selection. + Object.keys(masked).length !== Object.keys(data[keyName]).length + ) { + memo[keyName] = masked; + changed = true; + } } - } - return [memo, changed]; - } - case Kind.INLINE_FRAGMENT: { - if ( - selection.typeCondition && - !context.cache.fragmentMatches!(selection, data.__typename) - ) { return [memo, changed]; } + case Kind.INLINE_FRAGMENT: { + if ( + selection.typeCondition && + !context.cache.fragmentMatches!(selection, data.__typename) + ) { + return [memo, changed]; + } - const [fragmentData, childChanged] = maskSelectionSet( - data, - selection.selectionSet, - context, - path - ); + const [fragmentData, childChanged] = maskSelectionSet( + data, + selection.selectionSet, + context, + path + ); - return [ - { - ...memo, - ...fragmentData, - }, - changed || childChanged, - ]; - } - case Kind.FRAGMENT_SPREAD: { - const fragmentName = selection.name.value; - let fragment: FragmentDefinitionNode | null = - context.fragmentMap[fragmentName] || - (context.fragmentMap[fragmentName] = - context.cache.lookupFragment(fragmentName)!); - invariant( - fragment, - "Could not find fragment with name '%s'.", - fragmentName - ); + return [ + { + ...memo, + ...fragmentData, + }, + changed || childChanged, + ]; + } + case Kind.FRAGMENT_SPREAD: { + const fragmentName = selection.name.value; + let fragment: FragmentDefinitionNode | null = + context.fragmentMap[fragmentName] || + (context.fragmentMap[fragmentName] = + context.cache.lookupFragment(fragmentName)!); + invariant( + fragment, + "Could not find fragment with name '%s'.", + fragmentName + ); - const mode = getFragmentMaskMode(selection); + const mode = getFragmentMaskMode(selection); - if (mode === "mask") { - return [memo, true]; - } + if (mode === "mask") { + return [memo, true]; + } - if (__DEV__) { - if (mode === "migrate") { - return [ - addFieldAccessorWarnings( - memo, - data, - fragment.selectionSet, - path || "", - context - ), - true, - ]; + if (__DEV__) { + if (mode === "migrate") { + return [ + addFieldAccessorWarnings( + memo, + data, + fragment.selectionSet, + path || "", + context + ), + true, + ]; + } } - } - const [fragmentData, changed] = maskSelectionSet( - data, - fragment.selectionSet, - context, - path - ); + const [fragmentData, changed] = maskSelectionSet( + data, + fragment.selectionSet, + context, + path + ); - return [{ ...memo, ...fragmentData }, changed]; + return [{ ...memo, ...fragmentData }, changed]; + } } - } - }, - [Object.create(null), false] - ); + }, + [Object.create(null), false] + ); if (data && "__typename" in data && !("__typename" in result[0])) { result[0].__typename = data.__typename; @@ -300,83 +310,91 @@ function addFieldAccessorWarnings( }); } - return selectionSetNode.selections.reduce((memo, selection) => { - switch (selection.kind) { - case Kind.FIELD: { - const keyName = resultKeyNameFromField(selection); - const childSelectionSet = selection.selectionSet; + return selectionSetNode.selections + .concat() + .sort(sortFragmentsLast) + .reduce((memo, selection) => { + switch (selection.kind) { + case Kind.FIELD: { + const keyName = resultKeyNameFromField(selection); + const childSelectionSet = selection.selectionSet; - if (keyName in memo) { - return memo; - } + if (keyName in memo && !childSelectionSet) { + return memo; + } - let value = data[keyName]; + let value = data[keyName]; - if (childSelectionSet) { - value = addFieldAccessorWarnings( - memo[keyName] || Object.create(null), - data[keyName] as Record, - childSelectionSet, - `${path}.${keyName}`, - context - ); - } + if (childSelectionSet) { + value = addFieldAccessorWarnings( + memo[keyName] || + (Array.isArray(data[keyName]) ? [] : Object.create(null)), + data[keyName] as Record, + childSelectionSet, + `${path}.${keyName}`, + context + ); + } - if (__DEV__) { - addAccessorWarning(memo, value, keyName, path, context); - } + if (__DEV__) { + if (keyName in memo) { + memo[keyName] = value; + } else { + addAccessorWarning(memo, value, keyName, path, context); + } + } - if (!__DEV__) { - memo[keyName] = data[keyName]; - } + if (!__DEV__) { + memo[keyName] = data[keyName]; + } - return memo; - } - case Kind.INLINE_FRAGMENT: { - if ( - selection.typeCondition && - !context.cache.fragmentMatches!(selection, (data as any).__typename) - ) { return memo; } + case Kind.INLINE_FRAGMENT: { + if ( + selection.typeCondition && + !context.cache.fragmentMatches!(selection, (data as any).__typename) + ) { + return memo; + } - return addFieldAccessorWarnings( - memo, - data, - selection.selectionSet, - path, - context - ); - } - case Kind.FRAGMENT_SPREAD: { - const fragment = context.fragmentMap[selection.name.value]; - const mode = getFragmentMaskMode(selection); - - if (mode === "mask") { - return memo; + return addFieldAccessorWarnings( + memo, + data, + selection.selectionSet, + path, + context + ); } + case Kind.FRAGMENT_SPREAD: { + const fragment = context.fragmentMap[selection.name.value]; + const mode = getFragmentMaskMode(selection); + + if (mode === "mask") { + return memo; + } - if (mode === "unmask") { - const [fragmentData] = maskSelectionSet( + if (mode === "unmask") { + const [fragmentData] = maskSelectionSet( + data, + fragment.selectionSet, + context, + path + ); + + return Object.assign(memo, fragmentData); + } + + return addFieldAccessorWarnings( + memo, data, fragment.selectionSet, - context, - path + path, + context ); - - return Object.assign(memo, fragmentData); } - - return addFieldAccessorWarnings( - memo, - data, - fragment.selectionSet, - path, - context - ); } - } - }, memo); + }, memo); } function addAccessorWarning( @@ -434,3 +452,11 @@ function warnOnImproperCacheImplementation() { ); } } + +function sortFragmentsLast(a: SelectionNode, b: SelectionNode) { + if (a.kind === b.kind) { + return 0; + } + + return a.kind === Kind.FRAGMENT_SPREAD ? 1 : -1; +}