Skip to content

Commit

Permalink
[Data masking] Fix aggressive warnings when using objects marked with…
Browse files Browse the repository at this point in the history
… `@unmask(mode: "migrate")` with `cache.identify` (#12116)
  • Loading branch information
jerelmiller authored Nov 11, 2024
1 parent e5258e5 commit 8ae6e4e
Show file tree
Hide file tree
Showing 10 changed files with 203 additions and 100 deletions.
6 changes: 3 additions & 3 deletions .api-reports/api-report-cache.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1109,9 +1109,9 @@ interface WriteContext extends ReadMergeModifyContext {
// Warnings were encountered during analysis:
//
// src/cache/core/cache.ts:92:7 - (ae-forgotten-export) The symbol "MaybeMasked" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:93:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:139:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)
Expand Down
6 changes: 3 additions & 3 deletions .api-reports/api-report-core.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2432,9 +2432,9 @@ interface WriteContext extends ReadMergeModifyContext {

// Warnings were encountered during analysis:
//
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:93:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:139:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:120:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:121:5 - (ae-forgotten-export) The symbol "QueryInfo" needs to be exported by the entry point index.d.ts
Expand Down
10 changes: 5 additions & 5 deletions .api-reports/api-report-utilities.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2792,11 +2792,11 @@ interface WriteContext extends ReadMergeModifyContext {
// Warnings were encountered during analysis:
//
// src/cache/core/types/DataProxy.ts:147:7 - (ae-forgotten-export) The symbol "MissingFieldError" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:57:3 - (ae-forgotten-export) The symbol "TypePolicy" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "FieldReadFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:163:3 - (ae-forgotten-export) The symbol "FieldMergeFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:58:3 - (ae-forgotten-export) The symbol "TypePolicy" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:163:3 - (ae-forgotten-export) The symbol "FieldReadFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:164:3 - (ae-forgotten-export) The symbol "FieldMergeFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:139:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/writeToStore.ts:65:7 - (ae-forgotten-export) The symbol "MergeTree" needs to be exported by the entry point index.d.ts
// src/core/LocalState.ts:71:3 - (ae-forgotten-export) The symbol "ApolloClient" needs to be exported by the entry point index.d.ts
Expand Down
6 changes: 3 additions & 3 deletions .api-reports/api-report.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3143,9 +3143,9 @@ interface WriteContext extends ReadMergeModifyContext {

// Warnings were encountered during analysis:
//
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:93:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:139:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:120:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:121:5 - (ae-forgotten-export) The symbol "QueryInfo" needs to be exported by the entry point index.d.ts
Expand Down
5 changes: 5 additions & 0 deletions .changeset/early-bobcats-eat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Prevent field accessor warnings when using `@unmask(mode: "migrate")` on objects that are passed into `cache.identify`.
4 changes: 2 additions & 2 deletions .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 41506,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34257
"dist/apollo-client.min.cjs": 41516,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34296
}
194 changes: 133 additions & 61 deletions src/__tests__/dataMasking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,73 @@ describe("client.watchQuery", () => {
}
});

// https://github.com/apollographql/apollo-client/issues/12043
test("does not warn when passing @unmask(mode: 'migrate') object to cache.identify", async () => {
using consoleSpy = spyOnConsole("warn");

type UserFieldsFragment = {
age: number;
} & { " $fragmentName"?: "UserFieldsFragment" };

interface Query {
currentUser: {
__typename: "User";
id: number;
name: string;
/** @deprecated */
age: number;
} & { " $fragmentRefs"?: { UserFieldsFragment: UserFieldsFragment } };
}

const query: MaskedDocumentNode<Query, never> = gql`
query UnmaskedQuery {
currentUser {
id
name
...UserFields @unmask(mode: "migrate")
}
}
fragment UserFields on User {
age
name
}
`;

const mocks = [
{
request: { query },
result: {
data: {
currentUser: {
__typename: "User",
id: 1,
name: "Test User",
age: 34,
},
},
},
delay: 20,
},
];

const client = new ApolloClient({
dataMasking: true,
cache: new InMemoryCache(),
link: new MockLink(mocks),
});

const observable = client.watchQuery({ query });
const stream = new ObservableStream(observable);

const { data } = await stream.takeNext();

const id = client.cache.identify(data.currentUser);

expect(consoleSpy.warn).not.toHaveBeenCalled();
expect(id).toEqual("User:1");
});

test("reads fragment by passing parent object to `from`", async () => {
type UserFieldsFragment = {
age: number;
Expand Down Expand Up @@ -3112,79 +3179,84 @@ describe("client.watchFragment", () => {
});
});

// FIXME: This broke with the changes in https://github.com/apollographql/apollo-client/pull/12114
// which ensure masking works with deferred payloads. Instead of fixing with
// #12114, it will be fixed with https://github.com/apollographql/apollo-client/issues/12043
// which will fix overagressive warnings.
test.failing(
"warns when accessing an unmasked field on a watched fragment while using @unmask with mode: 'migrate'",
async () => {
using consoleSpy = spyOnConsole("warn");
test("warns when accessing an unmasked field on a watched fragment while using @unmask with mode: 'migrate'", async () => {
using consoleSpy = spyOnConsole("warn");

type ProfileFieldsFragment = {
__typename: "User";
age: number;
name: string;
} & { " $fragmentName": "UserFieldsFragment" };
type ProfileFieldsFragment = {
__typename: "User";
age: number;
name: string;
} & { " $fragmentName": "UserFieldsFragment" };

type UserFieldsFragment = {
__typename: "User";
id: number;
name: string;
/** @deprecated */
age: number;
} & { " $fragmentName": "UserFieldsFragment" } & {
" $fragmentRefs": { ProfileFieldsFragment: ProfileFieldsFragment };
};
type UserFieldsFragment = {
__typename: "User";
id: number;
name: string;
/** @deprecated */
age: number;
} & { " $fragmentName": "UserFieldsFragment" } & {
" $fragmentRefs": { ProfileFieldsFragment: ProfileFieldsFragment };
};

const fragment: MaskedDocumentNode<UserFieldsFragment, never> = gql`
fragment UserFields on User {
id
name
...ProfileFields @unmask(mode: "migrate")
}
const fragment: MaskedDocumentNode<UserFieldsFragment, never> = gql`
fragment UserFields on User {
id
name
...ProfileFields @unmask(mode: "migrate")
}
fragment ProfileFields on User {
age
name
}
`;
fragment ProfileFields on User {
age
name
}
`;

const client = new ApolloClient({
dataMasking: true,
cache: new InMemoryCache(),
});
const client = new ApolloClient({
dataMasking: true,
cache: new InMemoryCache(),
});

const observable = client.watchFragment({
fragment,
fragmentName: "UserFields",
from: { __typename: "User", id: 1 },
});
const stream = new ObservableStream(observable);
client.writeFragment({
id: client.cache.identify({ __typename: "User", id: 1 }),
fragment,
fragmentName: "UserFields",
data: {
__typename: "User",
id: 1,
age: 30,
name: "Test User",
},
});

{
const { data } = await stream.takeNext();
data.__typename;
data.id;
data.name;
const observable = client.watchFragment({
fragment,
fragmentName: "UserFields",
from: { __typename: "User", id: 1 },
});
const stream = new ObservableStream(observable);

expect(consoleSpy.warn).not.toHaveBeenCalled();
{
const { data } = await stream.takeNext();
data.__typename;
data.id;
data.name;

data.age;
expect(consoleSpy.warn).not.toHaveBeenCalled();

expect(consoleSpy.warn).toHaveBeenCalledTimes(1);
expect(consoleSpy.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.",
"fragment 'UserFields'",
"age"
);
data.age;

// Ensure we only warn once
data.age;
expect(consoleSpy.warn).toHaveBeenCalledTimes(1);
}
expect(consoleSpy.warn).toHaveBeenCalledTimes(1);
expect(consoleSpy.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.",
"fragment 'UserFields'",
"age"
);

// Ensure we only warn once
data.age;
expect(consoleSpy.warn).toHaveBeenCalledTimes(1);
}
);
});

test("can lookup unmasked fragments from the fragment registry in watched fragments", async () => {
const fragments = createFragmentRegistry();
Expand Down
20 changes: 12 additions & 8 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
keyArgsFnFromSpecifier,
keyFieldsFnFromSpecifier,
} from "./key-extractor.js";
import { disableWarningsSlot } from "../../core/masking.js";

export type TypePolicies = {
[__typename: string]: TypePolicy;
Expand Down Expand Up @@ -391,15 +392,18 @@ export class Policies {

const policy = typename && this.getTypePolicy(typename);
let keyFn = (policy && policy.keyFn) || this.config.dataIdFromObject;
while (keyFn) {
const specifierOrId = keyFn({ ...object, ...storeObject }, context);
if (isArray(specifierOrId)) {
keyFn = keyFieldsFnFromSpecifier(specifierOrId);
} else {
id = specifierOrId;
break;

disableWarningsSlot.withValue(true, () => {
while (keyFn) {
const specifierOrId = keyFn({ ...object, ...storeObject }, context);
if (isArray(specifierOrId)) {
keyFn = keyFieldsFnFromSpecifier(specifierOrId);
} else {
id = specifierOrId;
break;
}
}
}
});

id = id ? String(id) : void 0;
return context.keyObject ? [id, context.keyObject] : [id];
Expand Down
20 changes: 12 additions & 8 deletions src/core/__tests__/masking.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2244,7 +2244,7 @@ describe("maskFragment", () => {

test("warns when accessing unmasked fields when using `@unmask` directive with mode 'migrate'", () => {
using _ = spyOnConsole("warn");
const query = gql`
const fragment = gql`
fragment UnmaskedFragment on User {
id
name
Expand All @@ -2258,18 +2258,22 @@ describe("maskFragment", () => {

const data = maskFragment(
deepFreeze({
currentUser: {
__typename: "User",
id: 1,
name: "Test User",
age: 30,
},
__typename: "User",
id: 1,
name: "Test User",
age: 30,
}),
query,
fragment,
new InMemoryCache(),
"UnmaskedFragment"
);

data.__typename;
data.id;
data.name;

expect(console.warn).not.toHaveBeenCalled();

data.age;

expect(console.warn).toHaveBeenCalledTimes(1);
Expand Down
Loading

0 comments on commit 8ae6e4e

Please sign in to comment.