fix(graphql): allow update mutations to set an @id field to its existing value#9702
Open
shiva-istari wants to merge 2 commits into
Open
fix(graphql): allow update mutations to set an @id field to its existing value#9702shiva-istari wants to merge 2 commits into
shiva-istari wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.