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

TypePolicy merge function called before cache deep merge #9503

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
58 changes: 58 additions & 0 deletions src/cache/inmemory/__tests__/__snapshots__/writeToStore.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,64 @@ Object {
}
`;

exports[`writing to the store root type policy merge is called before cache deep merge 1`] = `
Array [
Array [
Object {
"existing": Object {
"__ref": "Person:123",
},
"incoming": Object {
"__typename": "Person",
"age": 28,
"id": 123,
"name": "Gaston",
"status": "ACTIVE",
"updatedAt": 100000,
},
"times": 1,
},
],
Array [
Object {
"existing": undefined,
"incoming": Object {
"__ref": "Person:123",
},
"times": 2,
},
],
Array [
Object {
"existing": Object {
"__ref": "Person:123",
},
"incoming": Object {
"__typename": "Person",
"id": 123,
"status": "DISABLED",
"updatedAt": 50,
},
"times": 3,
},
],
Array [
Object {
"existing": Object {
"__ref": "Person:123",
},
"incoming": Object {
"__typename": "Person",
"id": 123,
"status": "PENDING",
"updatedAt": 100001,
},
"times": 4,
},
],
]
`;

exports[`writing to the store should not keep reference when type of mixed inlined field changes to non-inlined field 1`] = `
[MockFunction] {
"calls": Array [
Expand Down
165 changes: 165 additions & 0 deletions src/cache/inmemory/__tests__/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2496,6 +2496,20 @@ describe('writing to the store', () => {
},
},

{
existing: {
__ref: "Account:12345",
},
incoming: {
__typename: "Account",
contact: "[email protected]",
id: 12345,
},
merged: {
__ref: "Account:12345",
},
},

{
existing: {
__typename: "Account",
Expand Down Expand Up @@ -2923,6 +2937,157 @@ describe('writing to the store', () => {
});
});

it("root type policy merge is called before cache deep merge", () => {
const personMergeMock = jest.fn();

let times = 0;
const cache = new InMemoryCache({
typePolicies: {
Person: {
merge(existing, incoming, tools) {
times++;

personMergeMock({
times,
existing,
incoming,
});

if (tools.isReference(existing) && !tools.isReference(incoming)) {
const cachedData = tools.cache.data.lookup(existing.__ref);
const existingUpdatedAt = cachedData?.["updatedAt"];
const incomingUpdatedAt = incoming?.["updatedAt"];
if (
typeof existingUpdatedAt === "number" &&
typeof incomingUpdatedAt === "number" &&
existingUpdatedAt > incomingUpdatedAt
) {
return existing;
}
}

return tools.mergeObjects(existing, incoming);
},
},
},
});

expect(times).toEqual(0);

const query = gql`
query Person($offset: Int, $limit: Int) {
person {
id
name
age
status
updatedAt
}
}
`

expect(times).toEqual(0);

cache.writeQuery({
query,
data: {
person: {
__typename: "Person",
id: 123,
name: "Gaston",
age: 28,
status: "ACTIVE",
updatedAt: 100000,
},
},
variables: {},
});

expect(times).toEqual(2); // TODO: ideally this should only be called once

expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
person: {
__ref: "Person:123",
},
},
"Person:123": {
__typename: "Person",
id: 123,
name: "Gaston",
age: 28,
status: "ACTIVE",
updatedAt: 100000,
},
});

cache.writeQuery({
query,
data: {
person: {
__typename: "Person",
id: 123,
status: "DISABLED",
updatedAt: 50,
},
},
variables: {},
});

expect(times).toEqual(3);

expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
person: {
__ref: "Person:123",
},
},
"Person:123": {
__typename: "Person",
id: 123,
name: "Gaston",
age: 28,
status: "ACTIVE",
updatedAt: 100000,
},
});

cache.writeQuery({
query,
data: {
person: {
__typename: "Person",
id: 123,
status: "PENDING",
updatedAt: 100001,
},
},
variables: {},
});

expect(personMergeMock.mock.calls).toMatchSnapshot();
expect(times).toEqual(4);

expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
person: {
__ref: "Person:123",
},
},
"Person:123": {
__typename: "Person",
id: 123,
name: "Gaston",
age: 28,
status: "PENDING",
updatedAt: 100001,
},
});
});

describe("StoreWriter", () => {
const writer = new StoreWriter(new InMemoryCache());

Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export abstract class EntityStore implements NormalizedCache {
}
}

protected lookup(dataId: string, dependOnExistence?: boolean): StoreObject | undefined {
public lookup(dataId: string, dependOnExistence?: boolean): StoreObject | undefined {
// The has method (above) calls lookup with dependOnExistence = true, so
// that it can later be invalidated when we add or remove a StoreObject for
// this dataId. Any consumer who cares about the contents of the StoreObject
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type BroadcastOptions = Pick<
>

export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
private data: EntityStore;
public data: EntityStore;
private optimisticData: EntityStore;

protected config: InMemoryCacheConfig;
Expand Down
7 changes: 5 additions & 2 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ export interface FieldFunctionOptions<
// helper function can be used to merge objects in a way that respects any
// custom merge functions defined for their fields.
mergeObjects: MergeObjectsFunction;

context: ReadMergeModifyContext | undefined;
}

type MergeObjectsFunction = <T extends StoreObject | Reference>(
Expand Down Expand Up @@ -542,7 +544,7 @@ export class Policies {
});
}

private getTypePolicy(typename: string): Policies["typePolicies"][string] {
public getTypePolicy(typename: string): Policies["typePolicies"][string] {
if (!hasOwn.call(this.typePolicies, typename)) {
const policy: Policies["typePolicies"][string] =
this.typePolicies[typename] = Object.create(null);
Expand Down Expand Up @@ -878,7 +880,7 @@ export class Policies {
// that need to deduplicate child objects and references.
void 0,
{ typename,
fieldName: field.name.value,
fieldName: field?.name.value || "ROOT",
field,
variables: context.variables },
context,
Expand Down Expand Up @@ -917,6 +919,7 @@ function makeFieldFunctionOptions(
);
},
mergeObjects: makeMergeObjectsFunction(context.store),
context
};
}

Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export interface InMemoryCacheConfig extends ApolloReducerConfig {
}

export interface MergeInfo {
field: FieldNode;
field: FieldNode | undefined;
typename: string | undefined;
merge: FieldMergeFunction;
};
Expand Down
20 changes: 19 additions & 1 deletion src/cache/inmemory/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class StoreWriter {
context.incomingById.forEach(({ storeObject, mergeTree, fieldNodeSet }, dataId) => {
const entityRef = makeReference(dataId);

if (mergeTree && mergeTree.map.size) {
if (mergeTree && (mergeTree.map.size || mergeTree.info)) {
const applied = this.applyMerges(mergeTree, entityRef, storeObject, context);
if (isReference(applied)) {
// Assume References returned by applyMerges have already been merged
Expand Down Expand Up @@ -421,6 +421,21 @@ export class StoreWriter {
previous.mergeTree = mergeMergeTrees(previous.mergeTree, mergeTree);
fieldNodeSet.forEach(field => previous.fieldNodeSet.add(field));
} else {
// Add the policy type's merge function for individual upcoming payloads
if(typename && mergeTreeIsEmpty(mergeTree)) {
const typePolicy = policies.getTypePolicy(
typename,
);
const merge = typePolicy.merge;
if (merge) {
mergeTree.info = {
field: undefined as any,
Copy link
Author

Choose a reason for hiding this comment

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

Not sure what field should be set here, I don't think we need a field in this case.

typename,
merge,
};
}
}

context.incomingById.set(dataId, {
storeObject: incoming,
// Save a reference to mergeTree only if it is not empty, because
Expand Down Expand Up @@ -660,6 +675,9 @@ export class StoreWriter {
}

if (mergeTree.info) {
if(isReference(existing) && isReference(incoming)){
Copy link
Author

Choose a reason for hiding this comment

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

may be we should check if they are the same ref too?

return incoming;
}
return this.cache.policies.runMergeFunction(
existing,
incoming,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ describe('relayStylePagination', () => {
readField: () => undefined,
canRead: () => false,
mergeObjects: (existing, _incoming) => existing,
context: undefined,
};

it('should maintain endCursor and startCursor with empty edges', () => {
Expand Down