diff --git a/change/@graphitation-apollo-forest-run-6b6eb87f-21ce-43f0-b729-efe2047dfa19.json b/change/@graphitation-apollo-forest-run-6b6eb87f-21ce-43f0-b729-efe2047dfa19.json new file mode 100644 index 000000000..4e6fb27a2 --- /dev/null +++ b/change/@graphitation-apollo-forest-run-6b6eb87f-21ce-43f0-b729-efe2047dfa19.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix(apollo-forest-run): tolerate divergent null/object node chunks during update", + "packageName": "@graphitation/apollo-forest-run", + "email": "vrazuvaev@microsoft.com_msteamsmdb", + "dependentChangeType": "patch" +} diff --git a/packages/apollo-forest-run/src/__tests__/regression.test.ts b/packages/apollo-forest-run/src/__tests__/regression.test.ts index 0de2b346b..c3247617c 100644 --- a/packages/apollo-forest-run/src/__tests__/regression.test.ts +++ b/packages/apollo-forest-run/src/__tests__/regression.test.ts @@ -1559,6 +1559,152 @@ test("correctly handles optimistic fragment write for deeply nested node", () => ]); }); +test("update must not crash when a node field is null in one chunk and an object in another", () => { + // Aliasing produces an Object chunk and a CompositeNull chunk for the same + // node; a later ObjectDifference is applied to both, asserting + // isObjectValue(base) on the null chunk. + const cache = new ForestRun(); + + const aliasedQuery = gql` + query Aliased { + x: foo { + __typename + id + details { + __typename + a + b + } + } + y: foo { + __typename + id + details { + __typename + a + b + } + } + } + `; + const singleQuery = gql` + query Single { + foo { + __typename + id + details { + __typename + a + b + } + } + } + `; + + // Same node written twice in one operation: object via `x`, null via `y`. + cache.write({ + query: aliasedQuery, + result: { + x: { + __typename: "Foo", + id: "1", + details: { __typename: "Detail", a: 1, b: 1 }, + }, + y: { + __typename: "Foo", + id: "1", + details: null, + }, + }, + }); + + // Changing details yields one ObjectDifference applied to every chunk, + // including the CompositeNull one. + const writeChanged = () => + cache.write({ + query: singleQuery, + result: { + foo: { + __typename: "Foo", + id: "1", + details: { __typename: "Detail", a: 2, b: 1 }, + }, + }, + }); + + expect(writeChanged).not.toThrow(); +}); + +test("update must not crash when the same node id appears twice with divergent details (no alias)", () => { + // Same regression via a normalized node-id collision rather than aliasing: + // the same node id appears twice in one list result with `details` an object + // in one occurrence and null in the other, yielding an Object chunk and a + // CompositeNull chunk without aliasing. + const cache = new ForestRun(); + + const listQuery = gql` + query List { + foos { + __typename + id + details { + __typename + a + b + } + } + } + `; + const singleQuery = gql` + query Single { + foo { + __typename + id + details { + __typename + a + b + } + } + } + `; + + // Same node id twice in one list result: object then null. + cache.write({ + query: listQuery, + result: { + foos: [ + { + __typename: "Foo", + id: "1", + details: { __typename: "Detail", a: 1, b: 1 }, + }, + { + __typename: "Foo", + id: "1", + details: null, + }, + ], + }, + }); + + // Changing details yields one ObjectDifference applied to every chunk, + // including the CompositeNull one. + const writeChanged = () => + cache.write({ + query: singleQuery, + result: { + foo: { + __typename: "Foo", + id: "1", + details: { __typename: "Detail", a: 2, b: 1 }, + }, + }, + }); + + expect(writeChanged).not.toThrow(); +}); + test("merge policy on embedded object must not crash when a prior operation wrote null (draftHelpers:72)", () => { // A keyless ("embedded") type with a field `merge` policy. If a previous // operation wrote that position as an explicit `null`, the merge machinery @@ -1695,3 +1841,77 @@ test("merge policy on embedded object must not crash when a prior null operation }, }); }); + +test("update must not crash when a list item is null in one chunk and an object in another", () => { + // Same divergent-chunk regression, but the null/object divergence is on a *list item* + // rather than a field. The list difference is applied to every chunk of the node, so the + // per-item ObjectDifference reaches the CompositeNull element of the divergent chunk + // (updateValue is called for list items without a null guard). It must be skipped, not crash. + const cache = new ForestRun(); + + const aliasedQuery = gql` + query Aliased { + x: foo { + __typename + id + items { + __typename + value + } + } + y: foo { + __typename + id + items { + __typename + value + } + } + } + `; + const singleQuery = gql` + query Single { + foo { + __typename + id + items { + __typename + value + } + } + } + `; + + // Same node written twice in one operation: item object via `x`, null item via `y`. + cache.write({ + query: aliasedQuery, + result: { + x: { + __typename: "Foo", + id: "1", + items: [{ __typename: "Item", value: 1 }], + }, + y: { + __typename: "Foo", + id: "1", + items: [null], + }, + }, + }); + + // Changing the item yields one CompositeListDifference (with a per-item ObjectDifference) + // applied to every chunk, including the one whose item is CompositeNull. + const writeChanged = () => + cache.write({ + query: singleQuery, + result: { + foo: { + __typename: "Foo", + id: "1", + items: [{ __typename: "Item", value: 2 }], + }, + }, + }); + + expect(writeChanged).not.toThrow(); +}); diff --git a/packages/apollo-forest-run/src/forest/__tests__/updateObject.test.ts b/packages/apollo-forest-run/src/forest/__tests__/updateObject.test.ts index 5f8d28170..a5502c5c8 100644 --- a/packages/apollo-forest-run/src/forest/__tests__/updateObject.test.ts +++ b/packages/apollo-forest-run/src/forest/__tests__/updateObject.test.ts @@ -35,6 +35,41 @@ describe("replaceValue invariant", () => { 'Invariant violation: Failed to update "query ReplaceScalar": cannot replace Scalar with Object (of type Widget)', ); }); + + it("still reports the invariant when path/node resolution throws", () => { + const operation = createTestOperation(gql` + query ReplaceObject { + node { + id + } + } + `); + // base is an addressable, keyless embedded object, so both path resolution and + // enclosing-node resolution must ascend the tree via findParent. A corrupt tree can + // make findParent throw while an invariant is being reported; the message builder must + // swallow that and still surface the invariant rather than the internal exception. + const objectBase = resolveFieldValue( + generateChunk(`query ObjectSource { embedded { field } }`).value, + "embedded", + ) as ObjectChunk; + const listReplacement = resolveFieldValue( + generateChunk(`query ListSource { items @mock(count: 2) { id } }`).value, + "items", + ); + + const context = { + operation, + findParent: () => { + throw new Error("corrupt tree: findParent exploded"); + }, + } as unknown as UpdateTreeContext; + + expect(() => + replaceValue(context, objectBase as any, listReplacement as any), + ).toThrow( + 'Invariant violation: Failed to update "query ReplaceObject": cannot replace Object with CompositeList', + ); + }); }); const nestedFeedListQuery = gql` @@ -83,6 +118,9 @@ const NESTED_INCOMPATIBLE_OBJECT_DIFF_ERROR = 'Invariant violation: Failed to update "query BaseFeedQuery" ' + "at path listContainer.items.0.value (in Entity): expected CompositeList, got ObjectDifference"; +const NESTED_DIVERGENT_NULL_WARNING_PATH = + "at path listContainer.items.0.value (in Entity)"; + describe("incompatible object difference invariant", () => { it("reports operation and path for a field two levels deep below a list", () => { const cache = new ForestRun(); @@ -131,4 +169,53 @@ describe("incompatible object difference invariant", () => { NESTED_INCOMPATIBLE_OBJECT_DIFF_ERROR, ); }); + + it("reports the full path in the divergent-null warning without throwing", () => { + // A divergent null/object chunk no longer throws: the null chunk is skipped with a + // warning. That warning must still report the full path, even though a CompositeNull's + // source data is `null` and is not addressable by the path utils on its own - the path + // is derived from its parent chunk, yielding "listContainer.items.0.value". + const warnings: string[] = []; + const cache = new ForestRun({ + logger: { + ...console, + warn: (...args: unknown[]) => warnings.push(args.join(" ")), + }, + }); + + // The same Entity's "value" is a null (with a sub-selection, i.e. a CompositeNull) + // under "listContainer" and an object under "objectContainer". + cache.write({ + query: nestedFeedListQuery, + result: { + objectContainer: { + __typename: "Container", + id: "object", + items: [{ __typename: "Entity", id: "1", value: { note: "old" } }], + }, + listContainer: { + __typename: "Container", + id: "list", + items: [{ __typename: "Entity", id: "1", value: null }], + }, + }, + }); + + const run = () => + cache.write({ + query: nestedFeedObjectQuery, + result: { + objectContainer: { + __typename: "Container", + id: "object", + items: [{ __typename: "Entity", id: "1", value: { note: "new" } }], + }, + }, + }); + + expect(run).not.toThrow(); + expect( + warnings.some((w) => w.includes(NESTED_DIVERGENT_NULL_WARNING_PATH)), + ).toBe(true); + }); }); diff --git a/packages/apollo-forest-run/src/forest/updateObject.ts b/packages/apollo-forest-run/src/forest/updateObject.ts index 0bf8841cb..37d48b1b2 100644 --- a/packages/apollo-forest-run/src/forest/updateObject.ts +++ b/packages/apollo-forest-run/src/forest/updateObject.ts @@ -101,7 +101,8 @@ function updateObjectValue( const updated = updateValue(context, value, fieldDiff, { kind: "field", - fieldName: fieldInfo.name, + parent: base, + field: fieldInfo, }); if (valueIsMissing && updated !== undefined) { @@ -152,8 +153,8 @@ function updateObjectValue( } type UpdateValueLocation = - | { kind: "field"; fieldName: string } - | { kind: "listItem"; index: number }; + | { kind: "field"; parent: GraphChunk; field: FieldInfo } + | { kind: "listItem"; parent: GraphChunk; index: number }; function updateValue( context: UpdateTreeContext, @@ -169,6 +170,18 @@ function updateValue( // Note: building the diagnostic message is expensive (path/type lookups), so it is // constructed only on the failure path rather than eagerly passed to assert(). if (!Value.isObjectValue(base)) { + // A node can have divergent chunks for the same position: an explicit null + // (CompositeNull) or a missing value (CompositeUndefined) in one chunk and a + // composite object in another (e.g. aliased selections, a repeated node id in a + // single write, or a partial/errored write). The one difference computed for the + // node is applied to every chunk, so skip the null/missing chunk instead of + // crashing. Genuine type mismatches (e.g. leaf null, scalars) still assert. + if (isNullOrMissingCompositeBase(base)) { + context.env.logger?.warn( + divergentCompositeMessage(context, base, difference, location), + ); + return getSourceValue(base); + } assert( false, incompatibleDifferenceMessage(context, base, difference, location), @@ -179,6 +192,12 @@ function updateValue( case DifferenceKind.CompositeListDifference: { if (!Value.isCompositeListValue(base)) { + if (isNullOrMissingCompositeBase(base)) { + context.env.logger?.warn( + divergentCompositeMessage(context, base, difference, location), + ); + return getSourceValue(base); + } assert( false, incompatibleDifferenceMessage(context, base, difference, location), @@ -218,6 +237,55 @@ function incompatibleDifferenceMessage( ); } +// True when an incoming composite (Object/CompositeList) difference is being applied to a +// base that is an explicit null (CompositeNull) or missing (CompositeUndefined). This +// happens when a node has divergent chunks for the same field/list-item across a single +// operation; such chunks are skipped rather than crashing. Genuine type mismatches (leaf +// null, scalars, complex scalars, etc.) are excluded and still assert. +function isNullOrMissingCompositeBase(base: GraphChunk): boolean { + return Value.isCompositeNullValue(base) || Value.isMissingValue(base); +} + +// Diagnostic for a field or list item that resolves to an explicit null (CompositeNull) or +// a missing value (CompositeUndefined) in one chunk while an incoming composite +// Object/CompositeList difference expects an object/list in another. Such divergent chunks +// are skipped (left unchanged) rather than crashing; this message stays at least as +// descriptive as the previous invariant and adds best-effort, actionable advice. +function divergentCompositeMessage( + context: UpdateTreeContext, + base: GraphChunk, + difference: ObjectDifference | CompositeListDifference, + location: UpdateValueLocation, +): string { + const isObject = Difference.isObjectDifference(difference); + const differenceKind = isObject + ? "ObjectDifference" + : "CompositeListDifference"; + const expectedShape = isObject ? "object" : "list"; + const typeClause = incomingTypeClause(differenceTypeName(difference)); + const isNull = Value.isCompositeNullValue(base); + const baseDesc = isNull + ? "an explicit null (CompositeNull)" + : "missing (CompositeUndefined)"; + const otherDesc = isNull ? "null" : "missing"; + const position = location.kind === "field" ? "field" : "list item"; + const detail = + `${position} is ${baseDesc} in this chunk, but the incoming ${differenceKind}` + + `${typeClause} expects a composite ${expectedShape}; the node has divergent chunks for ` + + `this ${position} (${otherDesc} in one, ${expectedShape} in another). ` + + `Leaving the ${otherDesc} value unchanged. This usually means the same node was written ` + + `with conflicting values in a single operation (e.g. aliased selections or a repeated ` + + `node id) or via a partial/errored write; ensure such selections resolve this ` + + `${position} consistently.`; + return updateFailureMessage( + context, + base, + detail, + location, + `Skipping update for "${context.operation.debugName}"`, + ); +} + function incompatibleReplacementMessage( context: UpdateTreeContext, base: GraphChunk, @@ -294,48 +362,87 @@ function updateFailureMessage( base: GraphChunk, detail: string, location?: UpdateValueLocation, + prefix = `Failed to update "${context.operation.debugName}"`, ): string { - const prefix = `Failed to update "${context.operation.debugName}"`; - const basePath = chunkPath(context, base); - const pathString = basePath?.length - ? basePath.join(".") - : location - ? describeLocation(location) - : undefined; - const nodeClause = nodeTypeClause(context, base); + const pathString = resolveFailurePath(context, base, location); + const nodeClause = nodeTypeClause(context, base, location); return pathString ? `${prefix} at path ${pathString}${nodeClause}: ${detail}` : `${prefix}${nodeClause}: ${detail}`; } +// Full path to the failing value, e.g. "listContainer.items.0.value". +// The failing value itself may not be addressable by the path utils (e.g. a CompositeNull +// or CompositeUndefined, whose source data is null/undefined and cannot be located in the +// tree). In that case its own path resolves to nothing and we would otherwise only report +// the final field/index. So we prefer to derive the path from the failing value's parent +// chunk (always an ObjectChunk or CompositeListChunk, which *is* addressable) and append +// the field/index step, falling back to the value's own path or the bare step. +function resolveFailurePath( + context: UpdateTreeContext, + base: GraphChunk, + location?: UpdateValueLocation, +): string | undefined { + try { + if (location) { + const parentPath = chunkPath(context, location.parent); + if (parentPath) { + return parentPath.concat(describeLocation(location)).join("."); + } + } + const basePath = chunkPath(context, base); + if (basePath?.length) { + return basePath.join("."); + } + return location ? describeLocation(location) : undefined; + } catch { + // Diagnostics only: the tree is already inconsistent when an invariant fires, + // so path resolution is best-effort and must never throw over the invariant. + return undefined; + } +} + function describeLocation(location: UpdateValueLocation): string { return location.kind === "field" - ? location.fieldName + ? location.field.dataKey : String(location.index); } // Best-effort " (in )" clause naming the closest enclosing node's __typename. // Defensive: the tree may already be inconsistent when an invariant fires, so it is // wrapped in try/catch and falls back to an empty clause (and aggregates are skipped, -// as they are not addressable by the path utils). +// as they are not addressable by the path utils). When the failing value itself is not +// addressable (e.g. a CompositeNull), the enclosing node is resolved from its parent. function nodeTypeClause( env: { findParent: ParentLocator }, chunk: GraphChunk, + location?: UpdateValueLocation, ): string { - if ( - (!Value.isObjectValue(chunk) && !Value.isCompositeListValue(chunk)) || - chunk.isAggregate - ) { + const anchor = nodeAnchor(chunk) ?? nodeAnchor(location?.parent); + if (!anchor) { return ""; } try { - const node = Value.findClosestNode(chunk, env.findParent); + const node = Value.findClosestNode(anchor, env.findParent); return node.type ? ` (in ${node.type})` : ""; } catch { return ""; } } +function nodeAnchor( + chunk: GraphChunk | undefined, +): ObjectChunk | CompositeListChunk | undefined { + if ( + chunk && + (Value.isObjectValue(chunk) || Value.isCompositeListValue(chunk)) && + !chunk.isAggregate + ) { + return chunk; + } + return undefined; +} + // Path of a chunk within its tree, e.g. ["listContainer", "items", 0, "value"]. // Returns undefined for values that are not addressable by the path utils (e.g. leaf values). // Wrapped in try/catch: this runs while reporting an invariant, so the tree may already @@ -406,7 +513,7 @@ function updateCompositeListValue( context, Value.resolveListItemChunk(base, index), itemDiff, - { kind: "listItem", index }, + { kind: "listItem", parent: base, index }, ); if (updatedValue === base.data[index]) { continue;