fix(graphql): allow update mutations to set an @id field to its existing value#9702
fix(graphql): allow update mutations to set an @id field to its existing value#9702shiva-istari wants to merge 2 commits into
Conversation
github-actions
Bot
added
area/testing
This comment has been minimized.
This comment has been minimized.
|
@shiva-istari Can you add a proper PR description covering the changes in detail? It's not clear looking at the code what the exact problem this is fixing. |
| " payload because GraphQL debug: id ABC tech already exists for field company" + | ||
| " inside type Employer", | ||
| }, | ||
| // TODO(reviewer): This test case is commented out because the scenario it tests is |
There was a problem hiding this comment.
I think it's correct to remove. Was definitely a false positive (the test case sets the company to its own current value, right?).
| } | ||
| }`, | ||
| }, | ||
| // TODO(reviewer): Commented out for the same reason as the corresponding case in |
There was a problem hiding this comment.
Same response as in add_mutation_test.go, safe to remove
| t.Run("update mutation nullable @id cleared via remove patch succeeds (C3)", updateMutationNullableXIDClearedViaRemove) | ||
| t.Run("update mutation remove patch on non-@id field succeeds (C3-contrast)", updateMutationRemovePatchNonXIDField) | ||
| t.Run("update mutation delete and recreate same @id succeeds (F1)", updateMutationDeleteRecreateXID) | ||
| t.Run("update mutation concurrent unchanged @id all succeed (H1)", updateMutationConcurrentUnchangedXID) |
There was a problem hiding this comment.
Unlikely, but this only asserts that no goroutine sees "already exists for field". If all 10 goroutines abort due to MVCC contention (again unlikely), the test passes — but a real regression (unchanged-@id silently failing) would also pass. Suggest asserting that at least one update returned NumUids == 1.
2 participants
Description
When a GraphQL update mutation included an id directive field in its set patch with the same value already stored on the node, the resolver incorrectly returned null instead of applying the update.
The rewriter ran an existence query for the id value before execution. When it found a matching UID it assumed a conflict and aborted rewriting it had no way to know at that point whether the existing UID was the same node being updated (no-op, should succeed) or a different node (genuine conflict, should error).
The same early-abort also caused a false-positive error in a second case: when a filter matched no nodes at all, the existence of a matching id directive value elsewhere in the database was being flagged as a conflict even though no mutation ran.
A third related bug: the remove patch was issuing unnecessary existence queries for top-level id fields. Removing a scalar value from a node can never steal uniqueness from another node, so those queries were wasted work and could generate spurious conflict errors.
Fix
Deferred conflict detection a two-phase approach:
conflict, succeed. If it is not → different node → return the conflict error. If mutatedUIDs is empty (filter matched nothing) → no verification needed, return nil.
The new XIDConflictVerifier interface on MutationRewriter keeps the post-execution check opt-in only UpdateRewriter implements it.
Additionally, the remove patch path strips top-level id fields before generating existence queries, and rewriteObject skips id processing entirely for UpdateWithRemove at top level.