Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Data masking] Fix aggressive warnings when using objects marked with @unmask(mode: "migrate") with cache.identify #12116

Merged
merged 14 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)": 34299
}
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({
Copy link
Member Author

Choose a reason for hiding this comment

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

As per my other comment, I had to make this change in the test since the keys are no longer present on the returned masked object since they weren't on the original data object (due to no data yet in the cache). This gets the test passing again.

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
5 changes: 4 additions & 1 deletion 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 @@ -392,7 +393,9 @@ 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);
const specifierOrId = disableWarningsSlot.withValue(true, () => {
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to wrap this slot around the while loop, so if this iterates multiple times, the slot does less work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Updated in 0a6a3fb

return keyFn!({ ...object, ...storeObject }, context);
});
if (isArray(specifierOrId)) {
keyFn = keyFieldsFnFromSpecifier(specifierOrId);
} else {
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: {
Copy link
Member Author

@jerelmiller jerelmiller Nov 8, 2024

Choose a reason for hiding this comment

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

This test was written incorrectly the first time 😅. This just happen to pass because we added accessor warnings for fields on the masked object that weren't present on the original data object. Yet another reason why I think the change not to add these properties unless they were on the original data object make sense.

__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
Loading