From 654d788ae211d574a18c8650cceb12593d988d24 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Fri, 15 Nov 2024 18:26:18 +0100 Subject: [PATCH 01/25] alternative implementation without object recreation. No migration mode yet. --- src/core/__tests__/masking.test.ts | 6 +- src/core/masking.ts | 195 +++++++---------------------- 2 files changed, 49 insertions(+), 152 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 3a6eb47251e..bb9fe37267f 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -4,7 +4,7 @@ import { deepFreeze } from "../../utilities/common/maybeDeepFreeze.js"; import { InvariantError } from "../../utilities/globals/index.js"; import { spyOnConsole, withProdMode } from "../../testing/internal/index.js"; -describe("maskOperation", () => { +describe.only("maskOperation", () => { test("throws when passing document with no operation to maskOperation", () => { const document = gql` fragment Foo on Bar { @@ -1608,7 +1608,7 @@ describe("maskOperation", () => { ); }); - test("masks child fragments of @unmask(mode: 'migrate')", () => { + test.failing("masks child fragments of @unmask(mode: 'migrate')", () => { using _ = spyOnConsole("warn"); const query = gql` @@ -3441,7 +3441,7 @@ describe("maskFragment", () => { expect(data.drinks[1]).toBe(drinks[1]); }); - test("masks child fragments of @unmask(mode: 'migrate')", () => { + test.failing("masks child fragments of @unmask(mode: 'migrate')", () => { using _ = spyOnConsole("warn"); const fragment = gql` diff --git a/src/core/masking.ts b/src/core/masking.ts index fdede0508ac..33ff507ddf1 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -23,6 +23,7 @@ interface MaskingContext { operationName: string | undefined; fragmentMap: FragmentMap; cache: ApolloCache; + mutableTargets: WeakMap; } // Contextual slot that allows us to disable accessor warnings on fields when in @@ -59,6 +60,7 @@ export function maskOperation( operationName: definition.name?.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, + mutableTargets: new WeakMap(), }; const [masked, changed] = maskSelectionSet( @@ -129,6 +131,7 @@ export function maskFragment( operationName: fragment.name.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, + mutableTargets: new WeakMap(), }; const [masked, changed] = maskSelectionSet( @@ -144,6 +147,19 @@ export function maskFragment( return changed ? masked : data; } +function getMutableTarget( + data: Record, + context: MaskingContext +): typeof data { + if (context.mutableTargets.has(data)) { + return context.mutableTargets.get(data); + } + + const mutableTarget = Array.isArray(data) ? [] : Object.create(null); + context.mutableTargets.set(data, mutableTarget); + return mutableTarget; +} + function maskSelectionSet( data: any, selectionSet: SelectionSetNode, @@ -152,10 +168,12 @@ function maskSelectionSet( ): [data: any, changed: boolean] { if (Array.isArray(data)) { let changed = false; - - const masked = data.map((item, index) => { + const target = getMutableTarget(data, context); + for (const [index, item] of Array.from(data.entries())) { if (item === null) { - return null; + // what about other primitives - what about an array of Int? + target[index] = null; + continue; } const [masked, itemChanged] = maskSelectionSet( @@ -166,13 +184,13 @@ function maskSelectionSet( ); changed ||= itemChanged; - return itemChanged ? masked : item; - }); + target[index] = masked; + } - return [changed ? masked : data, changed]; + return [changed ? target : data, changed]; } - const result = selectionSet.selections + let [target, changed] = selectionSet.selections .concat() .sort(sortFragmentsLast) .reduce<[any, boolean]>( @@ -182,7 +200,9 @@ function maskSelectionSet( const keyName = resultKeyNameFromField(selection); const childSelectionSet = selection.selectionSet; - memo[keyName] = data[keyName]; + if (!(keyName in memo)) { + memo[keyName] = data[keyName]; + } if (memo[keyName] === void 0) { delete memo[keyName]; @@ -200,13 +220,7 @@ function maskSelectionSet( __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 - ) { + if (childChanged) { memo[keyName] = masked; changed = true; } @@ -222,20 +236,13 @@ function maskSelectionSet( return [memo, changed]; } - const [fragmentData, childChanged] = maskSelectionSet( + const [, childChanged] = maskSelectionSet( data, selection.selectionSet, context, path ); - - return [ - { - ...memo, - ...fragmentData, - }, - changed || childChanged, - ]; + return [memo, changed || childChanged]; } case Kind.FRAGMENT_SPREAD: { const fragmentName = selection.name.value; @@ -255,146 +262,36 @@ function maskSelectionSet( return [memo, true]; } - if (__DEV__) { - if (mode === "migrate") { - return [ - addFieldAccessorWarnings( - memo, - data, - fragment.selectionSet, - path || "", - context - ), - true, - ]; - } - } - - const [fragmentData, changed] = maskSelectionSet( + const [, changed] = maskSelectionSet( data, fragment.selectionSet, context, path ); - return [{ ...memo, ...fragmentData }, changed]; + return [memo, changed]; } } }, - [Object.create(null), false] + [getMutableTarget(data, context), false] ); - if (data && "__typename" in data && !("__typename" in result[0])) { - result[0].__typename = data.__typename; + if (data && "__typename" in data && !("__typename" in target)) { + target.__typename = data.__typename; } - return result; -} - -function addFieldAccessorWarnings( - memo: Record, - data: Record, - selectionSetNode: SelectionSetNode, - path: string, - context: MaskingContext -) { - if (Array.isArray(data)) { - return data.map((item, index): unknown => { - return addFieldAccessorWarnings( - memo[index] || Object.create(null), - item, - selectionSetNode, - `${path}[${index}]`, - context - ); - }); - } - - 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 && !childSelectionSet) { - return memo; - } - - let value = data[keyName]; - - if (childSelectionSet) { - value = addFieldAccessorWarnings( - memo[keyName] || - (Array.isArray(data[keyName]) ? [] : Object.create(null)), - data[keyName] as Record, - childSelectionSet, - `${path}.${keyName}`, - context - ); - } - - if (__DEV__) { - if (keyName in memo) { - memo[keyName] = value; - } else { - addAccessorWarning(memo, value, keyName, path, context); - } - } - - 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; - } - - return addFieldAccessorWarnings( - memo, - data, - selection.selectionSet, - path, - context - ); - } - case Kind.FRAGMENT_SPREAD: { - const fragment = context.fragmentMap[selection.name.value]; - const mode = getFragmentMaskMode(selection); + // console.log( + // "markSelectionSet on %s changed: %s", + // path || "", + // changed || Object.keys(target).length !== Object.keys(data).length + // ); - if (mode === "mask") { - return memo; - } + // 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. + changed ||= Object.keys(target).length !== Object.keys(data).length; - if (mode === "unmask") { - const [fragmentData] = maskSelectionSet( - data, - fragment.selectionSet, - context, - path - ); - - return Object.assign(memo, fragmentData); - } - - return addFieldAccessorWarnings( - memo, - data, - fragment.selectionSet, - path, - context - ); - } - } - }, memo); + return [changed ? target : data, changed]; } function addAccessorWarning( From 04187223ceb09f1c2e5718d9d8db6616dd843548 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 18 Nov 2024 15:29:47 +0100 Subject: [PATCH 02/25] move assignment around --- src/core/masking.ts | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 33ff507ddf1..fa5adc0f2db 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -200,19 +200,8 @@ function maskSelectionSet( const keyName = resultKeyNameFromField(selection); const childSelectionSet = selection.selectionSet; - if (!(keyName in memo)) { - memo[keyName] = data[keyName]; - } - - if (memo[keyName] === void 0) { - delete memo[keyName]; - } - - if ( - keyName in memo && - childSelectionSet && - data[keyName] !== null - ) { + let newValue = memo[keyName] || data[keyName]; + if (childSelectionSet && data[keyName] !== null) { const [masked, childChanged] = maskSelectionSet( data[keyName], childSelectionSet, @@ -221,11 +210,15 @@ function maskSelectionSet( ); if (childChanged) { - memo[keyName] = masked; + newValue = masked; changed = true; } } + if (newValue !== void 0) { + memo[keyName] = newValue; + } + return [memo, changed]; } case Kind.INLINE_FRAGMENT: { From 9c5210cc93eba1d46d3db96b474e0c29099238f6 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 18 Nov 2024 16:08:45 +0100 Subject: [PATCH 03/25] down to two failing tests... --- src/core/__tests__/masking.test.ts | 4 +- src/core/masking.ts | 83 +++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index bb9fe37267f..407cf3de051 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -4,7 +4,7 @@ import { deepFreeze } from "../../utilities/common/maybeDeepFreeze.js"; import { InvariantError } from "../../utilities/globals/index.js"; import { spyOnConsole, withProdMode } from "../../testing/internal/index.js"; -describe.only("maskOperation", () => { +describe("maskOperation", () => { test("throws when passing document with no operation to maskOperation", () => { const document = gql` fragment Foo on Bar { @@ -1608,7 +1608,7 @@ describe.only("maskOperation", () => { ); }); - test.failing("masks child fragments of @unmask(mode: 'migrate')", () => { + test("masks child fragments of @unmask(mode: 'migrate')", () => { using _ = spyOnConsole("warn"); const query = gql` diff --git a/src/core/masking.ts b/src/core/masking.ts index fa5adc0f2db..72e2dc4c774 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -24,6 +24,10 @@ interface MaskingContext { fragmentMap: FragmentMap; cache: ApolloCache; mutableTargets: WeakMap; + migration: { + unmasked: WeakMap>; + migrated: WeakMap>; + }; } // Contextual slot that allows us to disable accessor warnings on fields when in @@ -61,14 +65,24 @@ export function maskOperation( fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, mutableTargets: new WeakMap(), + migration: { + unmasked: new Map(), + migrated: new Map(), + }, }; const [masked, changed] = maskSelectionSet( data, definition.selectionSet, - context + context, + undefined, + false ); + if (__DEV__) { + addMigrationWarnings(context, masked); + } + if (Object.isFrozen(data)) { disableWarningsSlot.withValue(true, maybeDeepFreeze, [masked]); } @@ -132,14 +146,24 @@ export function maskFragment( fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, mutableTargets: new WeakMap(), + migration: { + unmasked: new WeakMap(), + migrated: new WeakMap(), + }, }; const [masked, changed] = maskSelectionSet( data, fragment.selectionSet, - context + context, + undefined, + false ); + if (__DEV__) { + addMigrationWarnings(context, masked); + } + if (Object.isFrozen(data)) { disableWarningsSlot.withValue(true, maybeDeepFreeze, [masked]); } @@ -164,7 +188,8 @@ function maskSelectionSet( data: any, selectionSet: SelectionSetNode, context: MaskingContext, - path?: string | undefined + path: string | undefined, + migration: boolean ): [data: any, changed: boolean] { if (Array.isArray(data)) { let changed = false; @@ -180,7 +205,8 @@ function maskSelectionSet( item, selectionSet, context, - __DEV__ ? `${path || ""}[${index}]` : void 0 + __DEV__ ? `${path || ""}[${index}]` : void 0, + migration ); changed ||= itemChanged; @@ -206,7 +232,8 @@ function maskSelectionSet( data[keyName], childSelectionSet, context, - __DEV__ ? `${path || ""}.${keyName}` : void 0 + __DEV__ ? `${path || ""}.${keyName}` : void 0, + migration ); if (childChanged) { @@ -217,6 +244,25 @@ function maskSelectionSet( if (newValue !== void 0) { memo[keyName] = newValue; + if (__DEV__ && context.migration) { + if (migration) { + if (!context.migration.migrated.has(memo)) { + context.migration.migrated.set(memo, new Map()); + } + context.migration.migrated.get(memo)!.set(keyName, path); + console.log( + "migrated:", + memo, + keyName, + `${path || ""}.${keyName}` + ); + } else { + if (!context.migration.unmasked.has(memo)) { + context.migration.unmasked.set(memo, new Set()); + } + context.migration.unmasked.get(memo)!.add(keyName); + } + } } return [memo, changed]; @@ -233,7 +279,8 @@ function maskSelectionSet( data, selection.selectionSet, context, - path + path, + migration ); return [memo, changed || childChanged]; } @@ -259,10 +306,11 @@ function maskSelectionSet( data, fragment.selectionSet, context, - path + path, + mode === "migrate" ); - return [memo, changed]; + return [memo, changed || mode === "migrate"]; } } }, @@ -287,6 +335,25 @@ function maskSelectionSet( return [changed ? target : data, changed]; } +function addMigrationWarnings(context: MaskingContext, masked: any) { + JSON.stringify(masked, function (this: any, key, value) { + const u = context.migration["unmasked"].get(this); + const m = context.migration["migrated"].get(this); + console.log({ + t: this, + key, + value, + m, + mv: m && m.has(key), + u, + uv: u && u.has(key), + }); + if (!m || !m.has(key) || (u && u.has(key))) return value; + addAccessorWarning(this, value, key, m.get(key)!, context); + return value; + }); +} + function addAccessorWarning( data: Record, value: any, From 78ff7491bb35283e4a409e966c2191dcabe27286 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 18 Nov 2024 16:51:33 +0100 Subject: [PATCH 04/25] all green --- src/core/__tests__/masking.test.ts | 4 +- src/core/masking.ts | 86 ++++++++++++------------------ 2 files changed, 35 insertions(+), 55 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 407cf3de051..5f7ed0f5880 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -1797,7 +1797,7 @@ describe("maskOperation", () => { data.currentUser.skills[0].description; data.currentUser.skills[1].description; - expect(console.warn).toHaveBeenCalledTimes(9); + //expect(console.warn).toHaveBeenCalledTimes(9); expect(console.warn).toHaveBeenCalledWith( "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", "query 'UnmaskedQuery'", @@ -3441,7 +3441,7 @@ describe("maskFragment", () => { expect(data.drinks[1]).toBe(drinks[1]); }); - test.failing("masks child fragments of @unmask(mode: 'migrate')", () => { + test("masks child fragments of @unmask(mode: 'migrate')", () => { using _ = spyOnConsole("warn"); const fragment = gql` diff --git a/src/core/masking.ts b/src/core/masking.ts index 72e2dc4c774..337a40467d9 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -23,11 +23,15 @@ interface MaskingContext { operationName: string | undefined; fragmentMap: FragmentMap; cache: ApolloCache; - mutableTargets: WeakMap; - migration: { - unmasked: WeakMap>; - migrated: WeakMap>; - }; + mutableTargets: Map; + migration: Map< + any, + { + path: string; + unmasked: Set; + migrated: Set; + } + >; } // Contextual slot that allows us to disable accessor warnings on fields when in @@ -64,11 +68,8 @@ export function maskOperation( operationName: definition.name?.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, - mutableTargets: new WeakMap(), - migration: { - unmasked: new Map(), - migrated: new Map(), - }, + mutableTargets: new Map(), + migration: new Map(), }; const [masked, changed] = maskSelectionSet( @@ -145,11 +146,8 @@ export function maskFragment( operationName: fragment.name.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, - mutableTargets: new WeakMap(), - migration: { - unmasked: new WeakMap(), - migrated: new WeakMap(), - }, + mutableTargets: new Map(), + migration: new Map(), }; const [masked, changed] = maskSelectionSet( @@ -244,26 +242,21 @@ function maskSelectionSet( if (newValue !== void 0) { memo[keyName] = newValue; - if (__DEV__ && context.migration) { - if (migration) { - if (!context.migration.migrated.has(memo)) { - context.migration.migrated.set(memo, new Map()); - } - context.migration.migrated.get(memo)!.set(keyName, path); - console.log( - "migrated:", - memo, - keyName, - `${path || ""}.${keyName}` - ); - } else { - if (!context.migration.unmasked.has(memo)) { - context.migration.unmasked.set(memo, new Set()); - } - context.migration.unmasked.get(memo)!.add(keyName); - } + } + if (__DEV__) { + const m = context.migration; + if (!m.has(memo)) { + m.set(memo, { + path: path || "", + unmasked: new Set(), + migrated: new Set(), + }); } + m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); } + // we later want to add acessor warnings to the final result + // so we need a new object to add the accessor warning to + changed ||= migration; return [memo, changed]; } @@ -310,7 +303,7 @@ function maskSelectionSet( mode === "migrate" ); - return [memo, changed || mode === "migrate"]; + return [memo, changed]; } } }, @@ -321,12 +314,6 @@ function maskSelectionSet( target.__typename = data.__typename; } - // console.log( - // "markSelectionSet on %s changed: %s", - // path || "", - // changed || Object.keys(target).length !== Object.keys(data).length - // ); - // 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. @@ -337,19 +324,10 @@ function maskSelectionSet( function addMigrationWarnings(context: MaskingContext, masked: any) { JSON.stringify(masked, function (this: any, key, value) { - const u = context.migration["unmasked"].get(this); - const m = context.migration["migrated"].get(this); - console.log({ - t: this, - key, - value, - m, - mv: m && m.has(key), - u, - uv: u && u.has(key), - }); - if (!m || !m.has(key) || (u && u.has(key))) return value; - addAccessorWarning(this, value, key, m.get(key)!, context); + const obj = context.migration.get(this); + if (obj && obj.migrated.has(key) && !obj.unmasked.has(key)) { + addAccessorWarning(this, value, key, obj.path, context); + } return value; }); } @@ -359,6 +337,8 @@ function addAccessorWarning( value: any, fieldName: string, path: string, + // TODO: this is effectively a memory leak, we need to be more granular here, + // passing all required values individually instead of one big object context: MaskingContext ) { // In order to preserve the original shape of the data as much as possible, we From bdc3e2484473657e6d657d851daf1831b0a411d6 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 18 Nov 2024 16:54:57 +0100 Subject: [PATCH 05/25] uncomment --- src/core/__tests__/masking.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 5f7ed0f5880..3a6eb47251e 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -1797,7 +1797,7 @@ describe("maskOperation", () => { data.currentUser.skills[0].description; data.currentUser.skills[1].description; - //expect(console.warn).toHaveBeenCalledTimes(9); + expect(console.warn).toHaveBeenCalledTimes(9); expect(console.warn).toHaveBeenCalledWith( "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", "query 'UnmaskedQuery'", From 54b8344663c344210bbbded0173f811ad7615085 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 15:27:44 -0700 Subject: [PATCH 06/25] Reenable all tests --- src/core/__tests__/masking.test.ts | 717 ++++++++++++++--------------- 1 file changed, 346 insertions(+), 371 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 3a6eb47251e..87e8477c4db 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -718,140 +718,132 @@ describe("maskOperation", () => { }); }); - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'can handle fragment spreads in inline fragments with mix masked and @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); - - const query = gql` - query GetUser { - user { - id - ... @defer { - ...UserFields - ...ProfileFields @unmask(mode: "migrate") - } - } - } - - fragment UserFields on User { - name - } + test('can handle fragment spreads in inline fragments with mix masked and @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); - fragment ProfileFields on User { - username + const query = gql` + query GetUser { + user { + id + ... @defer { + ...UserFields + ...ProfileFields @unmask(mode: "migrate") + } } - `; - - const data = maskOperation( - deepFreeze({ - user: { - __typename: "User", - id: 1, - name: "Test User", - username: "testuser", - }, - }), - query, - new InMemoryCache() - ); - - data.user.__typename; - data.user.id; - - expect(console.warn).not.toHaveBeenCalled(); + } - data.user.username; + fragment UserFields on User { + name + } - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith( - "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", - "query 'GetUser'", - "user.username" - ); + fragment ProfileFields on User { + username + } + `; - expect(data).toEqual({ + const data = maskOperation( + deepFreeze({ user: { __typename: "User", id: 1, + name: "Test User", username: "testuser", }, - }); - } - ); - - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'can handle overlapping fragment spreads in inline fragments with mix masked and @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); - const query = gql` - query { - user { - id - ... { - ...UserFields - ...ProfileFields @unmask(mode: "migrate") - } - } - } + }), + query, + new InMemoryCache() + ); - fragment UserFields on User { - username - name - } + data.user.__typename; + data.user.id; - fragment ProfileFields on User { - username - email - } - `; + expect(console.warn).not.toHaveBeenCalled(); - const data = maskOperation( - deepFreeze({ - user: { - __typename: "User", - id: 1, - name: "Test User", - username: "testuser", - email: "testuser@example.com", - }, - }), - query, - new InMemoryCache() - ); + data.user.username; - data.user.__typename; - data.user.id; + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'GetUser'", + "user.username" + ); - expect(console.warn).not.toHaveBeenCalled(); + expect(data).toEqual({ + user: { + __typename: "User", + id: 1, + username: "testuser", + }, + }); + }); - data.user.username; - data.user.email; + test('can handle overlapping fragment spreads in inline fragments with mix masked and @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); + const query = gql` + query { + user { + id + ... { + ...UserFields + ...ProfileFields @unmask(mode: "migrate") + } + } + } - expect(console.warn).toHaveBeenCalledTimes(2); - expect(console.warn).toHaveBeenCalledWith( - "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", - "query 'GetUser'", - "user.username" - ); - expect(console.warn).toHaveBeenCalledWith( - "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", - "query 'GetUser'", - "user.email" - ); + fragment UserFields on User { + username + name + } - expect(data).toEqual({ + fragment ProfileFields on User { + username + email + } + `; + + const data = maskOperation( + deepFreeze({ user: { __typename: "User", id: 1, + name: "Test User", username: "testuser", email: "testuser@example.com", }, - }); - } - ); + }), + query, + new InMemoryCache() + ); + + data.user.__typename; + data.user.id; + + expect(console.warn).not.toHaveBeenCalled(); + + data.user.username; + data.user.email; + + expect(console.warn).toHaveBeenCalledTimes(2); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'GetUser'", + "user.username" + ); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'GetUser'", + "user.email" + ); + + expect(data).toEqual({ + user: { + __typename: "User", + id: 1, + username: "testuser", + email: "testuser@example.com", + }, + }); + }); test("handles field aliases", () => { const query = gql` @@ -2432,104 +2424,100 @@ describe("maskOperation", () => { } }); - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'unmasks partial data with warnings with @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); + test('unmasks partial data with warnings with @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); - const query = gql` - query { - greeting { - message - ...GreetingFragment @unmask(mode: "migrate") - } + const query = gql` + query { + greeting { + message + ...GreetingFragment @unmask(mode: "migrate") } + } - fragment GreetingFragment on Greeting { - sentAt - recipient { - name - } + fragment GreetingFragment on Greeting { + sentAt + recipient { + name } - `; - - { - const data = maskOperation( - deepFreeze({ - greeting: { message: "Hello world", __typename: "Greeting" }, - }), - query, - new InMemoryCache() - ); - - expect(data).toEqual({ - greeting: { message: "Hello world", __typename: "Greeting" }, - }); } + `; - { - const data = maskOperation( - deepFreeze({ - greeting: { - __typename: "Greeting", - message: "Hello world", - sentAt: "2024-01-01", - }, - }), - query, - new InMemoryCache() - ); - - data.greeting.__typename; - data.greeting.message; - - expect(console.warn).not.toHaveBeenCalled(); + { + const data = maskOperation( + deepFreeze({ + greeting: { message: "Hello world", __typename: "Greeting" }, + }), + query, + new InMemoryCache() + ); - data.greeting.sentAt; - expect(console.warn).toHaveBeenCalledTimes(1); + expect(data).toEqual({ + greeting: { message: "Hello world", __typename: "Greeting" }, + }); + } - expect(data).toEqual({ + { + const data = maskOperation( + deepFreeze({ greeting: { __typename: "Greeting", message: "Hello world", sentAt: "2024-01-01", }, - }); - } + }), + query, + new InMemoryCache() + ); - { - const data = maskOperation( - deepFreeze({ - greeting: { - __typename: "Greeting", - message: "Hello world", - recipient: { __typename: "__Person" }, - }, - }), - query, - new InMemoryCache() - ); + data.greeting.__typename; + data.greeting.message; - data.greeting.__typename; - data.greeting.message; + expect(console.warn).not.toHaveBeenCalled(); - expect(console.warn).not.toHaveBeenCalled(); + data.greeting.sentAt; + expect(console.warn).toHaveBeenCalledTimes(1); - data.greeting.recipient; - data.greeting.recipient.__typename; - expect(console.warn).toHaveBeenCalledTimes(1); + expect(data).toEqual({ + greeting: { + __typename: "Greeting", + message: "Hello world", + sentAt: "2024-01-01", + }, + }); + } - expect(data).toEqual({ + { + const data = maskOperation( + deepFreeze({ greeting: { __typename: "Greeting", message: "Hello world", recipient: { __typename: "__Person" }, }, - }); - } + }), + query, + new InMemoryCache() + ); + + data.greeting.__typename; + data.greeting.message; + + expect(console.warn).not.toHaveBeenCalled(); + + data.greeting.recipient; + data.greeting.recipient.__typename; + expect(console.warn).toHaveBeenCalledTimes(1); + + expect(data).toEqual({ + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "__Person" }, + }, + }); } - ); + }); test("masks partial deferred data", () => { const query = gql` @@ -2646,80 +2634,76 @@ describe("maskOperation", () => { } }); - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'unmasks partial deferred data with warnings with @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); + test('unmasks partial deferred data with warnings with @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); - const query = gql` - query { - greeting { - message - ... @defer { - sentAt - ...GreetingFragment @unmask(mode: "migrate") - } + const query = gql` + query { + greeting { + message + ... @defer { + sentAt + ...GreetingFragment @unmask(mode: "migrate") } } + } - fragment GreetingFragment on Greeting { - recipient { - name - } + fragment GreetingFragment on Greeting { + recipient { + name } - `; - - { - const data = maskOperation( - deepFreeze({ - greeting: { message: "Hello world", __typename: "Greeting" }, - }), - query, - new InMemoryCache() - ); - - expect(data).toEqual({ - greeting: { message: "Hello world", __typename: "Greeting" }, - }); } + `; - { - const data = maskOperation( - deepFreeze({ - greeting: { - __typename: "Greeting", - message: "Hello world", - sentAt: "2024-01-01", - recipient: { __typename: "__Person", name: "Alice" }, - }, - }), - query, - new InMemoryCache() - ); - - data.greeting.message; - data.greeting.sentAt; - data.greeting.__typename; - - expect(console.warn).not.toHaveBeenCalled(); + { + const data = maskOperation( + deepFreeze({ + greeting: { message: "Hello world", __typename: "Greeting" }, + }), + query, + new InMemoryCache() + ); - data.greeting.recipient; - data.greeting.recipient.__typename; - data.greeting.recipient.name; - expect(console.warn).toHaveBeenCalledTimes(3); + expect(data).toEqual({ + greeting: { message: "Hello world", __typename: "Greeting" }, + }); + } - expect(data).toEqual({ + { + const data = maskOperation( + deepFreeze({ greeting: { __typename: "Greeting", message: "Hello world", sentAt: "2024-01-01", recipient: { __typename: "__Person", name: "Alice" }, }, - }); - } + }), + query, + new InMemoryCache() + ); + + data.greeting.message; + data.greeting.sentAt; + data.greeting.__typename; + + expect(console.warn).not.toHaveBeenCalled(); + + data.greeting.recipient; + data.greeting.recipient.__typename; + data.greeting.recipient.name; + expect(console.warn).toHaveBeenCalledTimes(3); + + expect(data).toEqual({ + greeting: { + __typename: "Greeting", + message: "Hello world", + sentAt: "2024-01-01", + recipient: { __typename: "__Person", name: "Alice" }, + }, + }); } - ); + }); test("masks results with primitive arrays", () => { const query = gql` @@ -3496,8 +3480,7 @@ describe("maskFragment", () => { ); }); - // TODO: Remove .failing when refactoring migrate mode - test.failing("masks child fragments of @unmask", () => { + test("masks child fragments of @unmask", () => { using _ = spyOnConsole("warn"); const fragment = gql` @@ -3676,96 +3659,92 @@ describe("maskFragment", () => { } }); - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'unmasks partial data with warnings with @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); + test('unmasks partial data with warnings with @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); - const fragment = gql` - fragment GreetingFields on Greeting { - message - ...AdditionalFields @unmask(mode: "migrate") - } + const fragment = gql` + fragment GreetingFields on Greeting { + message + ...AdditionalFields @unmask(mode: "migrate") + } - fragment AdditionalFields on Greeting { - sentAt - recipient { - name - } + fragment AdditionalFields on Greeting { + sentAt + recipient { + name } - `; - - { - const data = maskFragment( - deepFreeze({ message: "Hello world", __typename: "Greeting" }), - fragment, - new InMemoryCache(), - "GreetingFields" - ); - - expect(data).toEqual({ - message: "Hello world", - __typename: "Greeting", - }); } + `; - { - const data = maskFragment( - deepFreeze({ - __typename: "Greeting", - message: "Hello world", - sentAt: "2024-01-01", - }), - fragment, - new InMemoryCache(), - "GreetingFields" - ); - - data.__typename; - data.message; - - expect(console.warn).not.toHaveBeenCalled(); + { + const data = maskFragment( + deepFreeze({ message: "Hello world", __typename: "Greeting" }), + fragment, + new InMemoryCache(), + "GreetingFields" + ); - data.sentAt; - expect(console.warn).toHaveBeenCalledTimes(1); + expect(data).toEqual({ + message: "Hello world", + __typename: "Greeting", + }); + } - expect(data).toEqual({ + { + const data = maskFragment( + deepFreeze({ __typename: "Greeting", message: "Hello world", sentAt: "2024-01-01", - }); - } + }), + fragment, + new InMemoryCache(), + "GreetingFields" + ); - { - const data = maskFragment( - deepFreeze({ - __typename: "Greeting", - message: "Hello world", - recipient: { __typename: "__Person" }, - }), - fragment, - new InMemoryCache(), - "GreetingFields" - ); + data.__typename; + data.message; - data.__typename; - data.message; + expect(console.warn).not.toHaveBeenCalled(); - expect(console.warn).not.toHaveBeenCalled(); + data.sentAt; + expect(console.warn).toHaveBeenCalledTimes(1); - data.recipient; - data.recipient.__typename; - expect(console.warn).toHaveBeenCalledTimes(1); + expect(data).toEqual({ + __typename: "Greeting", + message: "Hello world", + sentAt: "2024-01-01", + }); + } - expect(data).toEqual({ + { + const data = maskFragment( + deepFreeze({ __typename: "Greeting", message: "Hello world", recipient: { __typename: "__Person" }, - }); - } + }), + fragment, + new InMemoryCache(), + "GreetingFields" + ); + + data.__typename; + data.message; + + expect(console.warn).not.toHaveBeenCalled(); + + data.recipient; + data.recipient.__typename; + expect(console.warn).toHaveBeenCalledTimes(1); + + expect(data).toEqual({ + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "__Person" }, + }); } - ); + }); test("masks partial deferred data", () => { const fragment = gql` @@ -3872,75 +3851,71 @@ describe("maskFragment", () => { } }); - // TODO: Remove .failing when refactoring migrate mode - test.failing( - 'unmasks partial deferred data with warnings with @unmask(mode: "migrate")', - () => { - using _ = spyOnConsole("warn"); + test('unmasks partial deferred data with warnings with @unmask(mode: "migrate")', () => { + using _ = spyOnConsole("warn"); - const fragment = gql` - fragment GreetingFields on Greeting { - message - ... @defer { - sentAt - ...AdditionalFields @unmask(mode: "migrate") - } + const fragment = gql` + fragment GreetingFields on Greeting { + message + ... @defer { + sentAt + ...AdditionalFields @unmask(mode: "migrate") } + } - fragment AdditionalFields on Greeting { - recipient { - name - } + fragment AdditionalFields on Greeting { + recipient { + name } - `; - - { - const data = maskFragment( - deepFreeze({ message: "Hello world", __typename: "Greeting" }), - fragment, - new InMemoryCache(), - "GreetingFields" - ); - - expect(data).toEqual({ - message: "Hello world", - __typename: "Greeting", - }); } + `; - { - const data = maskFragment( - deepFreeze({ - __typename: "Greeting", - message: "Hello world", - sentAt: "2024-01-01", - recipient: { __typename: "__Person", name: "Alice" }, - }), - fragment, - new InMemoryCache(), - "GreetingFields" - ); - - data.message; - data.sentAt; - data.__typename; - - expect(console.warn).not.toHaveBeenCalled(); + { + const data = maskFragment( + deepFreeze({ message: "Hello world", __typename: "Greeting" }), + fragment, + new InMemoryCache(), + "GreetingFields" + ); - data.recipient; - data.recipient.__typename; - data.recipient.name; - expect(console.warn).toHaveBeenCalledTimes(3); + expect(data).toEqual({ + message: "Hello world", + __typename: "Greeting", + }); + } - expect(data).toEqual({ + { + const data = maskFragment( + deepFreeze({ __typename: "Greeting", message: "Hello world", sentAt: "2024-01-01", recipient: { __typename: "__Person", name: "Alice" }, - }); - } + }), + fragment, + new InMemoryCache(), + "GreetingFields" + ); + + data.message; + data.sentAt; + data.__typename; + + expect(console.warn).not.toHaveBeenCalled(); + + data.recipient; + data.recipient.__typename; + data.recipient.name; + expect(console.warn).toHaveBeenCalledTimes(3); + + expect(data).toEqual({ + __typename: "Greeting", + message: "Hello world", + sentAt: "2024-01-01", + recipient: { __typename: "__Person", name: "Alice" }, + }); } - ); + }); test("masks results with primitive arrays", () => { const fragment = gql` From e38adb81d338dbf20f8a0dfa9157262b89b83e1a Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 15:28:11 -0700 Subject: [PATCH 07/25] Fix incorrect test name --- src/core/__tests__/masking.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 87e8477c4db..28e17840f9d 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -780,7 +780,7 @@ describe("maskOperation", () => { test('can handle overlapping fragment spreads in inline fragments with mix masked and @unmask(mode: "migrate")', () => { using _ = spyOnConsole("warn"); const query = gql` - query { + query GetUser { user { id ... { From b16079b0eb88aac3f1201ef3e770f1088019a5d7 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 16:54:27 -0700 Subject: [PATCH 08/25] Remove fragment sorting --- src/core/masking.ts | 173 +++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 92 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 337a40467d9..5f16284c9db 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -214,101 +214,98 @@ function maskSelectionSet( return [changed ? target : data, changed]; } - let [target, changed] = 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; - - let newValue = memo[keyName] || data[keyName]; - if (childSelectionSet && data[keyName] !== null) { - const [masked, childChanged] = maskSelectionSet( - data[keyName], - childSelectionSet, - context, - __DEV__ ? `${path || ""}.${keyName}` : void 0, - migration - ); - - if (childChanged) { - newValue = masked; - changed = true; - } - } + let [target, changed] = selectionSet.selections.reduce<[any, boolean]>( + ([memo, changed], selection) => { + switch (selection.kind) { + case Kind.FIELD: { + const keyName = resultKeyNameFromField(selection); + const childSelectionSet = selection.selectionSet; + + let newValue = memo[keyName] || data[keyName]; + if (childSelectionSet && data[keyName] !== null) { + const [masked, childChanged] = maskSelectionSet( + data[keyName], + childSelectionSet, + context, + __DEV__ ? `${path || ""}.${keyName}` : void 0, + migration + ); - if (newValue !== void 0) { - memo[keyName] = newValue; + if (childChanged) { + newValue = masked; + changed = true; } - if (__DEV__) { - const m = context.migration; - if (!m.has(memo)) { - m.set(memo, { - path: path || "", - unmasked: new Set(), - migrated: new Set(), - }); - } - m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); - } - // we later want to add acessor warnings to the final result - // so we need a new object to add the accessor warning to - changed ||= migration; + } - return [memo, changed]; + if (newValue !== void 0) { + memo[keyName] = newValue; } - case Kind.INLINE_FRAGMENT: { - if ( - selection.typeCondition && - !context.cache.fragmentMatches!(selection, data.__typename) - ) { - return [memo, changed]; + if (__DEV__) { + const m = context.migration; + if (!m.has(memo)) { + m.set(memo, { + path: path || "", + unmasked: new Set(), + migrated: new Set(), + }); } - - const [, childChanged] = maskSelectionSet( - data, - selection.selectionSet, - context, - path, - migration - ); - return [memo, changed || childChanged]; + m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); } - 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 - ); + // we later want to add acessor warnings to the final result + // so we need a new object to add the accessor warning to + changed ||= migration; - const mode = getFragmentMaskMode(selection); + return [memo, changed]; + } + case Kind.INLINE_FRAGMENT: { + if ( + selection.typeCondition && + !context.cache.fragmentMatches!(selection, data.__typename) + ) { + return [memo, changed]; + } - if (mode === "mask") { - return [memo, true]; - } + const [, childChanged] = maskSelectionSet( + data, + selection.selectionSet, + context, + path, + migration + ); + return [memo, 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); + + if (mode === "mask") { + return [memo, true]; + } - const [, changed] = maskSelectionSet( - data, - fragment.selectionSet, - context, - path, - mode === "migrate" - ); + const [, changed] = maskSelectionSet( + data, + fragment.selectionSet, + context, + path, + mode === "migrate" + ); - return [memo, changed]; - } + return [memo, changed]; } - }, - [getMutableTarget(data, context), false] - ); + } + }, + [getMutableTarget(data, context), false] + ); if (data && "__typename" in data && !("__typename" in target)) { target.__typename = data.__typename; @@ -389,11 +386,3 @@ function warnOnImproperCacheImplementation() { ); } } - -function sortFragmentsLast(a: SelectionNode, b: SelectionNode) { - if (a.kind === b.kind) { - return 0; - } - - return a.kind === Kind.FRAGMENT_SPREAD ? 1 : -1; -} From cf87e13f40200aa2afe7d7d8602cac3b603bd2e1 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 16:56:30 -0700 Subject: [PATCH 09/25] Remove unneeded comment --- src/core/masking.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 5f16284c9db..9cbd46e1c7f 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -194,7 +194,6 @@ function maskSelectionSet( const target = getMutableTarget(data, context); for (const [index, item] of Array.from(data.entries())) { if (item === null) { - // what about other primitives - what about an array of Int? target[index] = null; continue; } From f01564585a90d01851fa9948856de04e08a4d488 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 17:00:30 -0700 Subject: [PATCH 10/25] Remove unused import --- src/core/masking.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 9cbd46e1c7f..f62d1ea0c2d 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -1,9 +1,5 @@ import { Kind } from "graphql"; -import type { - FragmentDefinitionNode, - SelectionNode, - SelectionSetNode, -} from "graphql"; +import type { FragmentDefinitionNode, SelectionSetNode } from "graphql"; import { createFragmentMap, resultKeyNameFromField, From f4d2cc8c9719bd5e873660831d1ae16c96eb92dd Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 17:07:26 -0700 Subject: [PATCH 11/25] Guard against undefined data --- src/core/masking.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index f62d1ea0c2d..aadbc814403 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -217,7 +217,7 @@ function maskSelectionSet( const childSelectionSet = selection.selectionSet; let newValue = memo[keyName] || data[keyName]; - if (childSelectionSet && data[keyName] !== null) { + if (keyName in data && childSelectionSet && data[keyName] !== null) { const [masked, childChanged] = maskSelectionSet( data[keyName], childSelectionSet, From 6f5ce4f1b38e05b62e7c5a00bc44b3b2318c12a6 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 17:19:13 -0700 Subject: [PATCH 12/25] Ensure console mocks are cleared between checks --- src/core/__tests__/masking.test.ts | 44 ++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 28e17840f9d..a053e0d896a 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -2425,7 +2425,7 @@ describe("maskOperation", () => { }); test('unmasks partial data with warnings with @unmask(mode: "migrate")', () => { - using _ = spyOnConsole("warn"); + using consoleSpy = spyOnConsole("warn"); const query = gql` query { @@ -2452,11 +2452,18 @@ describe("maskOperation", () => { new InMemoryCache() ); + data.greeting.__typename; + data.greeting.message; + + expect(console.warn).not.toHaveBeenCalled(); + expect(data).toEqual({ greeting: { message: "Hello world", __typename: "Greeting" }, }); } + consoleSpy.warn.mockClear(); + { const data = maskOperation( deepFreeze({ @@ -2487,6 +2494,8 @@ describe("maskOperation", () => { }); } + consoleSpy.warn.mockClear(); + { const data = maskOperation( deepFreeze({ @@ -2635,7 +2644,7 @@ describe("maskOperation", () => { }); test('unmasks partial deferred data with warnings with @unmask(mode: "migrate")', () => { - using _ = spyOnConsole("warn"); + using consoleSpy = spyOnConsole("warn"); const query = gql` query { @@ -2664,11 +2673,18 @@ describe("maskOperation", () => { new InMemoryCache() ); + data.greeting.message; + data.greeting.__typename; + + expect(console.warn).not.toHaveBeenCalled(); + expect(data).toEqual({ greeting: { message: "Hello world", __typename: "Greeting" }, }); } + consoleSpy.warn.mockClear(); + { const data = maskOperation( deepFreeze({ @@ -2690,9 +2706,8 @@ describe("maskOperation", () => { expect(console.warn).not.toHaveBeenCalled(); data.greeting.recipient; - data.greeting.recipient.__typename; data.greeting.recipient.name; - expect(console.warn).toHaveBeenCalledTimes(3); + expect(console.warn).toHaveBeenCalledTimes(2); expect(data).toEqual({ greeting: { @@ -3660,7 +3675,7 @@ describe("maskFragment", () => { }); test('unmasks partial data with warnings with @unmask(mode: "migrate")', () => { - using _ = spyOnConsole("warn"); + using consoleSpy = spyOnConsole("warn"); const fragment = gql` fragment GreetingFields on Greeting { @@ -3684,12 +3699,19 @@ describe("maskFragment", () => { "GreetingFields" ); + data.message; + data.__typename; + + expect(console.warn).not.toHaveBeenCalled(); + expect(data).toEqual({ message: "Hello world", __typename: "Greeting", }); } + consoleSpy.warn.mockClear(); + { const data = maskFragment( deepFreeze({ @@ -3717,6 +3739,8 @@ describe("maskFragment", () => { }); } + consoleSpy.warn.mockClear(); + { const data = maskFragment( deepFreeze({ @@ -3735,6 +3759,7 @@ describe("maskFragment", () => { expect(console.warn).not.toHaveBeenCalled(); data.recipient; + // We do not warn on access to __typename data.recipient.__typename; expect(console.warn).toHaveBeenCalledTimes(1); @@ -3852,7 +3877,7 @@ describe("maskFragment", () => { }); test('unmasks partial deferred data with warnings with @unmask(mode: "migrate")', () => { - using _ = spyOnConsole("warn"); + using consoleSpy = spyOnConsole("warn"); const fragment = gql` fragment GreetingFields on Greeting { @@ -3884,6 +3909,8 @@ describe("maskFragment", () => { }); } + consoleSpy.warn.mockClear(); + { const data = maskFragment( deepFreeze({ @@ -3904,9 +3931,10 @@ describe("maskFragment", () => { expect(console.warn).not.toHaveBeenCalled(); data.recipient; - data.recipient.__typename; data.recipient.name; - expect(console.warn).toHaveBeenCalledTimes(3); + // We do not warn on access to __typename + data.recipient.__typename; + expect(console.warn).toHaveBeenCalledTimes(2); expect(data).toEqual({ __typename: "Greeting", From 9c2b770fea825d97193345bd2728950f470ebd03 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 18:28:20 -0700 Subject: [PATCH 13/25] Tweak test to check which fields warned --- src/core/__tests__/masking.test.ts | 32 +++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index a053e0d896a..1a8cd201364 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -1985,7 +1985,7 @@ describe("maskOperation", () => { }); test('handles overlapping types when subtype has accessor warnings with @unmask(mode: "migrate")', async () => { - using consoleSpy = spyOnConsole("warn"); + using _ = spyOnConsole("warn"); const query = gql` query PlaylistQuery { playlist { @@ -2070,9 +2070,7 @@ describe("maskOperation", () => { new InMemoryCache() ); - expect(consoleSpy.warn).not.toHaveBeenCalled(); - - consoleSpy.warn.mockClear(); + expect(console.warn).not.toHaveBeenCalled(); data.playlist.album; data.playlist.album.id; @@ -2080,11 +2078,35 @@ describe("maskOperation", () => { data.playlist.artist; data.playlist.artist.id; data.playlist.artist.__typename; + expect(console.warn).not.toHaveBeenCalled(); data.playlist.album.images; + data.playlist.album.tracks[0].name; data.playlist.artist.images; - expect(console.warn).toHaveBeenCalledTimes(2); + data.playlist.artist.topTracks[0].name; + + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'PlaylistQuery'", + "playlist.album.images" + ); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'PlaylistQuery'", + "playlist.album.tracks[0].name" + ); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'PlaylistQuery'", + "playlist.artist.images" + ); + expect(console.warn).toHaveBeenCalledWith( + "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", + "query 'PlaylistQuery'", + "playlist.artist.topTracks[0].name" + ); + expect(console.warn).toHaveBeenCalledTimes(4); expect(data).toEqual({ playlist: { From ad2e2e6402609a692a4b6e6256707caf95a356bb Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 18 Nov 2024 18:31:45 -0700 Subject: [PATCH 14/25] Add additional check for fields --- src/core/__tests__/masking.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index 1a8cd201364..d804b068198 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -2075,9 +2075,13 @@ describe("maskOperation", () => { data.playlist.album; data.playlist.album.id; data.playlist.album.__typename; + data.playlist.album.tracks[0].id; + data.playlist.album.tracks[0].__typename; data.playlist.artist; data.playlist.artist.id; data.playlist.artist.__typename; + data.playlist.artist.topTracks[0].id; + data.playlist.artist.topTracks[0].__typename; expect(console.warn).not.toHaveBeenCalled(); From d0c33d0fb204de18b000d880b3706e7cb9bfc073 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 09:51:19 +0100 Subject: [PATCH 15/25] use a loop instead of reduce --- src/core/masking.ts | 172 ++++++++++++++++++++++---------------------- 1 file changed, 86 insertions(+), 86 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index aadbc814403..c58d3f83361 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -209,109 +209,109 @@ function maskSelectionSet( return [changed ? target : data, changed]; } - let [target, changed] = selectionSet.selections.reduce<[any, boolean]>( - ([memo, changed], selection) => { - switch (selection.kind) { - case Kind.FIELD: { - const keyName = resultKeyNameFromField(selection); - const childSelectionSet = selection.selectionSet; - - let newValue = memo[keyName] || data[keyName]; - if (keyName in data && childSelectionSet && data[keyName] !== null) { - const [masked, childChanged] = maskSelectionSet( - data[keyName], - childSelectionSet, - context, - __DEV__ ? `${path || ""}.${keyName}` : void 0, - migration - ); - - if (childChanged) { - newValue = masked; - changed = true; - } - } + let changed = false; + const memo = getMutableTarget(data, context); + for (const selection of selectionSet.selections) { + switch (selection.kind) { + case Kind.FIELD: { + const keyName = resultKeyNameFromField(selection); + const childSelectionSet = selection.selectionSet; + + let newValue = memo[keyName] || data[keyName]; + if (keyName in data && childSelectionSet && data[keyName] !== null) { + const [masked, childChanged] = maskSelectionSet( + data[keyName], + childSelectionSet, + context, + __DEV__ ? `${path || ""}.${keyName}` : void 0, + migration + ); - if (newValue !== void 0) { - memo[keyName] = newValue; - } - if (__DEV__) { - const m = context.migration; - if (!m.has(memo)) { - m.set(memo, { - path: path || "", - unmasked: new Set(), - migrated: new Set(), - }); - } - m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); + if (childChanged) { + newValue = masked; + changed = true; } - // we later want to add acessor warnings to the final result - // so we need a new object to add the accessor warning to - changed ||= migration; + } - return [memo, changed]; + if (newValue !== void 0) { + memo[keyName] = newValue; } - case Kind.INLINE_FRAGMENT: { - if ( - selection.typeCondition && - !context.cache.fragmentMatches!(selection, data.__typename) - ) { - return [memo, changed]; + if (__DEV__) { + const m = context.migration; + if (!m.has(memo)) { + m.set(memo, { + path: path || "", + unmasked: new Set(), + migrated: new Set(), + }); } - - const [, childChanged] = maskSelectionSet( - data, - selection.selectionSet, - context, - path, - migration - ); - return [memo, changed || childChanged]; + m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); + } + // we later want to add acessor warnings to the final result + // so we need a new object to add the accessor warning to + changed ||= migration; + break; + } + case Kind.INLINE_FRAGMENT: { + if ( + selection.typeCondition && + !context.cache.fragmentMatches!(selection, data.__typename) + ) { + break; } - 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); - if (mode === "mask") { - return [memo, true]; - } + const [, childChanged] = maskSelectionSet( + data, + selection.selectionSet, + context, + path, + migration + ); + changed ||= childChanged; + break; + } + 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); + + if (mode === "mask") { + break; + } - const [, changed] = maskSelectionSet( - data, - fragment.selectionSet, - context, - path, - mode === "migrate" - ); + const [, selectionChanged] = maskSelectionSet( + data, + fragment.selectionSet, + context, + path, + mode === "migrate" + ); - return [memo, changed]; - } + changed ||= selectionChanged; + break; } - }, - [getMutableTarget(data, context), false] - ); + } + } - if (data && "__typename" in data && !("__typename" in target)) { - target.__typename = data.__typename; + if (data && "__typename" in data && !("__typename" in memo)) { + memo.__typename = data.__typename; } // 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. - changed ||= Object.keys(target).length !== Object.keys(data).length; + changed ||= Object.keys(memo).length !== Object.keys(data).length; - return [changed ? target : data, changed]; + return [changed ? memo : data, changed]; } function addMigrationWarnings(context: MaskingContext, masked: any) { From 80fe2e509349cce103bc352d37dc2452a0801bbc Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 10:20:28 +0100 Subject: [PATCH 16/25] fix last test --- src/core/masking.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/core/masking.ts b/src/core/masking.ts index c58d3f83361..339cd75f8db 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -20,6 +20,7 @@ interface MaskingContext { fragmentMap: FragmentMap; cache: ApolloCache; mutableTargets: Map; + knownChanged: Set; migration: Map< any, { @@ -65,6 +66,7 @@ export function maskOperation( fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, mutableTargets: new Map(), + knownChanged: new Set(), migration: new Map(), }; @@ -143,6 +145,7 @@ export function maskFragment( fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, mutableTargets: new Map(), + knownChanged: new Set(), migration: new Map(), }; @@ -311,6 +314,15 @@ function maskSelectionSet( // additional fields from the same child selection. changed ||= Object.keys(memo).length !== Object.keys(data).length; + // If the object has been changed in another subtree, but not in this, + // we still have to return "changed" as true, as otherwise a call parent + // would use original data instead of the masked one. + if (changed) { + context.knownChanged.add(memo); + } else { + changed ||= context.knownChanged.has(memo); + } + return [changed ? memo : data, changed]; } From 04020e50f63e2c055c6f97ef4ee69b427da9c714 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 10:31:48 +0100 Subject: [PATCH 17/25] property accessor memory optimizations --- src/core/masking.ts | 51 ++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 339cd75f8db..a303b69e6ad 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -330,30 +330,36 @@ function addMigrationWarnings(context: MaskingContext, masked: any) { JSON.stringify(masked, function (this: any, key, value) { const obj = context.migration.get(this); if (obj && obj.migrated.has(key) && !obj.unmasked.has(key)) { - addAccessorWarning(this, value, key, obj.path, context); + // In order to preserve the original shape of the data as much as possible, we + // want to skip adding a property with warning to the final result when the + // value is missing, otherwise our final result will contain additional + // properties that our original result did not have. This could happen with a + // deferred payload for example. + if (value !== void 0) { + Object.defineProperty( + this, + key, + getAccessorWarningDescriptor( + value, + key, + obj.path, + context.operationName, + context.operationType + ) + ); + } } return value; }); } -function addAccessorWarning( - data: Record, +function getAccessorWarningDescriptor( value: any, fieldName: string, path: string, - // TODO: this is effectively a memory leak, we need to be more granular here, - // passing all required values individually instead of one big object - context: MaskingContext -) { - // In order to preserve the original shape of the data as much as possible, we - // want to skip adding a property with warning to the final result when the - // value is missing, otherwise our final result will contain additional - // properties that our original result did not have. This could happen with a - // deferred payload for example. - if (value === void 0) { - return; - } - + operationName: string | undefined, + operationType: string +): PropertyDescriptor { let getValue = () => { if (disableWarningsSlot.getValue()) { return value; @@ -361,9 +367,9 @@ function addAccessorWarning( invariant.warn( "Accessing unmasked field on %s at path '%s'. This field will not be available when masking is enabled. Please read the field from the fragment instead.", - context.operationName ? - `${context.operationType} '${context.operationName}'` - : `anonymous ${context.operationType}`, + operationName ? + `${operationType} '${operationName}'` + : `anonymous ${operationType}`, `${path}.${fieldName}`.replace(/^\./, "") ); @@ -372,16 +378,17 @@ function addAccessorWarning( return value; }; - Object.defineProperty(data, fieldName, { + return { get() { return getValue(); }, - set(value) { + set(v) { + value = v; getValue = () => value; }, enumerable: true, configurable: true, - }); + }; } let issuedWarning = false; From 37e9086d2008843e281d6f4032ba4daebee0e62f Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 10:43:35 +0100 Subject: [PATCH 18/25] use WeakMap/WeakSet where possible This should not have any immediate effect, but will prevent additional memory leaks in case the `context` object leaks anywhere. --- src/core/masking.ts | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index a303b69e6ad..4085448a1d8 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -7,6 +7,8 @@ import { getFragmentMaskMode, getOperationDefinition, maybeDeepFreeze, + canUseWeakMap, + canUseWeakSet, } from "../utilities/index.js"; import type { FragmentMap } from "../utilities/index.js"; import type { ApolloCache, DocumentNode, TypedDocumentNode } from "./index.js"; @@ -19,9 +21,9 @@ interface MaskingContext { operationName: string | undefined; fragmentMap: FragmentMap; cache: ApolloCache; - mutableTargets: Map; - knownChanged: Set; - migration: Map< + mutableTargets: WeakMap; + knownChanged: WeakSet; + migration: WeakMap< any, { path: string; @@ -31,6 +33,9 @@ interface MaskingContext { >; } +const MapImpl = canUseWeakMap ? WeakMap : Map; +const SetImpl = canUseWeakSet ? WeakSet : Set; + // Contextual slot that allows us to disable accessor warnings on fields when in // migrate mode. export const disableWarningsSlot = new Slot(); @@ -65,9 +70,9 @@ export function maskOperation( operationName: definition.name?.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, - mutableTargets: new Map(), - knownChanged: new Set(), - migration: new Map(), + mutableTargets: new MapImpl(), + knownChanged: new SetImpl(), + migration: new MapImpl(), }; const [masked, changed] = maskSelectionSet( @@ -144,9 +149,9 @@ export function maskFragment( operationName: fragment.name.value, fragmentMap: createFragmentMap(getFragmentDefinitions(document)), cache, - mutableTargets: new Map(), - knownChanged: new Set(), - migration: new Map(), + mutableTargets: new MapImpl(), + knownChanged: new SetImpl(), + migration: new MapImpl(), }; const [masked, changed] = maskSelectionSet( From daaa9022ad24dab41b7849038f6160c4c1f1b062 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 10:49:35 +0100 Subject: [PATCH 19/25] update size limits --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 45bba8c25f1..927749909f2 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 41638, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34394 + "dist/apollo-client.min.cjs": 41784, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34491 } From c166f45578eae9712de2a6de610da19413de9274 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 11:38:22 +0100 Subject: [PATCH 20/25] inline accessor warnings --- .size-limits.json | 4 +- src/core/masking.ts | 130 ++++++++++++++++++-------------------------- 2 files changed, 56 insertions(+), 78 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 927749909f2..0fc972b6396 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 41784, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34491 + "dist/apollo-client.min.cjs": 41761, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34507 } diff --git a/src/core/masking.ts b/src/core/masking.ts index 4085448a1d8..913b6ae0a60 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -23,14 +23,6 @@ interface MaskingContext { cache: ApolloCache; mutableTargets: WeakMap; knownChanged: WeakSet; - migration: WeakMap< - any, - { - path: string; - unmasked: Set; - migrated: Set; - } - >; } const MapImpl = canUseWeakMap ? WeakMap : Map; @@ -72,24 +64,22 @@ export function maskOperation( cache, mutableTargets: new MapImpl(), knownChanged: new SetImpl(), - migration: new MapImpl(), }; - const [masked, changed] = maskSelectionSet( - data, - definition.selectionSet, - context, - undefined, - false - ); - - if (__DEV__) { - addMigrationWarnings(context, masked); - } + const [masked, changed] = disableWarningsSlot.withValue(true, () => { + const maskedTuple = maskSelectionSet( + data, + definition.selectionSet, + context, + undefined, + false + ); - if (Object.isFrozen(data)) { - disableWarningsSlot.withValue(true, maybeDeepFreeze, [masked]); - } + if (Object.isFrozen(data)) { + maybeDeepFreeze(maskedTuple[0]); + } + return maskedTuple; + }); return changed ? masked : data; } @@ -151,24 +141,22 @@ export function maskFragment( cache, mutableTargets: new MapImpl(), knownChanged: new SetImpl(), - migration: new MapImpl(), }; - const [masked, changed] = maskSelectionSet( - data, - fragment.selectionSet, - context, - undefined, - false - ); - - if (__DEV__) { - addMigrationWarnings(context, masked); - } + const [masked, changed] = disableWarningsSlot.withValue(true, () => { + const maskedTuple = maskSelectionSet( + data, + fragment.selectionSet, + context, + undefined, + false + ); - if (Object.isFrozen(data)) { - disableWarningsSlot.withValue(true, maybeDeepFreeze, [masked]); - } + if (Object.isFrozen(data)) { + maybeDeepFreeze(maskedTuple[0]); + } + return maskedTuple; + }); return changed ? masked : data; } @@ -242,18 +230,35 @@ function maskSelectionSet( } if (newValue !== void 0) { - memo[keyName] = newValue; - } - if (__DEV__) { - const m = context.migration; - if (!m.has(memo)) { - m.set(memo, { - path: path || "", - unmasked: new Set(), - migrated: new Set(), - }); + if (!__DEV__) { + memo[keyName] = newValue; + } + if (__DEV__) { + if ( + migration && + keyName !== "__typename" && + // either the field is not present in the memo object + // or it has a `get` descriptor, not a `value` descriptor + // => it is a warning accessor and we can overwrite it + // with another accessor + !Object.getOwnPropertyDescriptor(memo, keyName)?.value + ) { + Object.defineProperty( + memo, + keyName, + getAccessorWarningDescriptor( + newValue, + keyName, + path || "", + context.operationName, + context.operationType + ) + ); + } else { + delete memo[keyName]; + memo[keyName] = newValue; + } } - m.get(memo)![migration ? "migrated" : "unmasked"].add(keyName); } // we later want to add acessor warnings to the final result // so we need a new object to add the accessor warning to @@ -331,33 +336,6 @@ function maskSelectionSet( return [changed ? memo : data, changed]; } -function addMigrationWarnings(context: MaskingContext, masked: any) { - JSON.stringify(masked, function (this: any, key, value) { - const obj = context.migration.get(this); - if (obj && obj.migrated.has(key) && !obj.unmasked.has(key)) { - // In order to preserve the original shape of the data as much as possible, we - // want to skip adding a property with warning to the final result when the - // value is missing, otherwise our final result will contain additional - // properties that our original result did not have. This could happen with a - // deferred payload for example. - if (value !== void 0) { - Object.defineProperty( - this, - key, - getAccessorWarningDescriptor( - value, - key, - obj.path, - context.operationName, - context.operationType - ) - ); - } - } - return value; - }); -} - function getAccessorWarningDescriptor( value: any, fieldName: string, From 158031d1792cabe0d861ab32ba9b525a89fcd56d Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 19 Nov 2024 14:06:58 +0100 Subject: [PATCH 21/25] add additional test --- src/core/__tests__/masking.test.ts | 101 +++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/src/core/__tests__/masking.test.ts b/src/core/__tests__/masking.test.ts index d804b068198..548d7ea0a2d 100644 --- a/src/core/__tests__/masking.test.ts +++ b/src/core/__tests__/masking.test.ts @@ -211,6 +211,107 @@ describe("maskOperation", () => { }); }); + test("masks fragments from nested objects when query gets fields from same object", () => { + const query = gql` + query { + user { + ...FragmentA @unmask + ...FragmentB + ...FragmentC @unmask + } + } + + fragment FragmentA on User { + this { + is { + very { + deeply { + nested { + a + } + } + } + } + } + } + fragment FragmentB on User { + this { + is { + very { + deeply { + nested { + b + } + } + } + } + } + } + fragment FragmentC on User { + this { + is { + very { + deeply { + nested { + c + } + } + } + } + } + } + `; + + const data = maskOperation( + deepFreeze({ + user: { + __typename: "User", + this: { + is: [ + { + very: { + deeply: [ + { + nested: { + a: 1, + b: 2, + c: 3, + }, + }, + ], + }, + }, + ], + }, + }, + }), + query, + new InMemoryCache() + ); + + expect(data).toEqual({ + user: { + __typename: "User", + this: { + is: [ + { + very: { + deeply: [ + { + nested: { + a: 1, + c: 3, + }, + }, + ], + }, + }, + ], + }, + }, + }); + }); + test("handles nulls in child selection sets", () => { const query = gql` query { From 104c86e618521f38c0ad0852f6c0b4a7ec90a8cc Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 19 Nov 2024 10:05:44 -0700 Subject: [PATCH 22/25] Swap order of arguments --- src/core/masking.ts | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 913b6ae0a60..96743fac47c 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -71,7 +71,6 @@ export function maskOperation( data, definition.selectionSet, context, - undefined, false ); @@ -148,7 +147,6 @@ export function maskFragment( data, fragment.selectionSet, context, - undefined, false ); @@ -178,8 +176,8 @@ function maskSelectionSet( data: any, selectionSet: SelectionSetNode, context: MaskingContext, - path: string | undefined, - migration: boolean + migration: boolean, + path?: string | undefined ): [data: any, changed: boolean] { if (Array.isArray(data)) { let changed = false; @@ -194,8 +192,8 @@ function maskSelectionSet( item, selectionSet, context, - __DEV__ ? `${path || ""}[${index}]` : void 0, - migration + migration, + __DEV__ ? `${path || ""}[${index}]` : void 0 ); changed ||= itemChanged; @@ -219,8 +217,8 @@ function maskSelectionSet( data[keyName], childSelectionSet, context, - __DEV__ ? `${path || ""}.${keyName}` : void 0, - migration + migration, + __DEV__ ? `${path || ""}.${keyName}` : void 0 ); if (childChanged) { @@ -277,8 +275,8 @@ function maskSelectionSet( data, selection.selectionSet, context, - path, - migration + migration, + path ); changed ||= childChanged; break; @@ -305,8 +303,8 @@ function maskSelectionSet( data, fragment.selectionSet, context, - path, - mode === "migrate" + mode === "migrate", + path ); changed ||= selectionChanged; From ada2dee46e1935bdb57c64310a40e339e6b5ae40 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 19 Nov 2024 10:06:26 -0700 Subject: [PATCH 23/25] Swap order of arguments on getAccessorWarningDescriptor --- src/core/masking.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/masking.ts b/src/core/masking.ts index 96743fac47c..0fa811ad755 100644 --- a/src/core/masking.ts +++ b/src/core/masking.ts @@ -245,8 +245,8 @@ function maskSelectionSet( memo, keyName, getAccessorWarningDescriptor( - newValue, keyName, + newValue, path || "", context.operationName, context.operationType @@ -335,8 +335,8 @@ function maskSelectionSet( } function getAccessorWarningDescriptor( - value: any, fieldName: string, + value: any, path: string, operationName: string | undefined, operationType: string From 9259df306cb87d8762e29f5064de5102fc025dac Mon Sep 17 00:00:00 2001 From: jerelmiller Date: Tue, 19 Nov 2024 17:18:49 +0000 Subject: [PATCH 24/25] Clean up Prettier, Size-limit, and Api-Extractor --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 0fc972b6396..faaed486eb9 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 41761, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34507 + "dist/apollo-client.min.cjs": 41756, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34505 } From dd9004f564715e068f4bc7313476cb227754a73b Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 19 Nov 2024 11:25:15 -0700 Subject: [PATCH 25/25] Add changesets --- .changeset/itchy-penguins-worry.md | 5 +++++ .changeset/mean-bottles-travel.md | 5 +++++ .changeset/wicked-pans-appear.md | 5 +++++ 3 files changed, 15 insertions(+) create mode 100644 .changeset/itchy-penguins-worry.md create mode 100644 .changeset/mean-bottles-travel.md create mode 100644 .changeset/wicked-pans-appear.md diff --git a/.changeset/itchy-penguins-worry.md b/.changeset/itchy-penguins-worry.md new file mode 100644 index 00000000000..4459d490662 --- /dev/null +++ b/.changeset/itchy-penguins-worry.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix issue where masked data would sometimes get returned when the field was part of a child fragment from a fragment unmasked by the parent query. diff --git a/.changeset/mean-bottles-travel.md b/.changeset/mean-bottles-travel.md new file mode 100644 index 00000000000..a2e1fd300d2 --- /dev/null +++ b/.changeset/mean-bottles-travel.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix issue where the warning emitted by `@unmask(mode: "migrate")` would trigger unnecessarily when the fragment was used alongside a masked fragment inside an inline fragment. diff --git a/.changeset/wicked-pans-appear.md b/.changeset/wicked-pans-appear.md new file mode 100644 index 00000000000..fc4a3b5eb6d --- /dev/null +++ b/.changeset/wicked-pans-appear.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix issue that threw errors when masking partial data with `@unmask(mode: "migrate")`.