Repository navigation
Keep generated Fusion operation names valid for every hash format - #10388
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Pull request overview
Normalizes Fusion operation short hashes so generated GraphQL operation names remain parseable.
Changes:
- Adds targeted short-hash normalization.
- Applies normalized hashes throughout operation planning.
- Adds coverage for invalid characters and leading digits.
File summaries
| File | Description |
|---|---|
| src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Planning/OperationShortHashTests.cs | Updated as part of this pull request. |
| src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Planning/GeneratedOperationNameTests.cs | Updated as part of this pull request. |
| src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationShortHash.cs | Updated as part of this pull request. |
| src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs | Updated as part of this pull request. |
Review details
Suppressed comments (4)
src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs:82
- This comment says the generated names embed the short hash verbatim, but the next line deliberately replaces characters that are not valid in a GraphQL name. Please update the comment to describe the normalization step so it does not contradict the behavior or the normalized
Operation.ShortHashvalue.
// The generated source schema operation names embed the short hash verbatim.
src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs:83
- This enforces the invariant only on the planner path.
JsonOperationPlanParser.ParseOperationand other directOperationCompiler.Compilecallers bypassCreatePlan, so an unsafe serializedshortHashcan still reach alias batching, whereoperation.ShortHashis written directly into the merged GraphQL operation name. Apply the same normalization at the common operation-compilation path as well, while retaining this planner-side normalization forPlanNode.CreateOperationName.
// The generated source schema operation names embed the short hash verbatim.
shortHash = OperationShortHash.ToNameSafe(shortHash);
src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs:83
- The cancellation guard is intentionally before planning work, and
OperationPlannerCancellationTestscovers the already-canceled case. This normalization now scans and, for a non-name hash, allocates before that guard, so canceled requests do work the method explicitly promises to avoid; move the normalization belowThrowIfCancellationRequested().
// The generated source schema operation names embed the short hash verbatim.
shortHash = OperationShortHash.ToNameSafe(shortHash);
src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs:83
- Normalizing the parameter here changes
Operation.ShortHashfrom the first eight characters of the original document hash into a lossy value. That property is public and is serialized asshortHash, whileOperation.Hashremains unchanged, so hashes containing-,/, or+can no longer be correlated by consumers usingshortHashas the hash prefix. Keep the original short hash for operation metadata and carry a separate name-safe value forPlanNodeand alias batching, or explicitly revise this public contract.
shortHash = OperationShortHash.ToNameSafe(shortHash);
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Patch coverage100.0% of changed lines covered (31/31)
Project coverage: 57.9% (288327/497915 lines) |
There was a problem hiding this comment.
🟢 Approval recommended
The remaining documentation nit is non-blocking, and the reviewed behavior is covered by tests.
Review details
Suppressed comments (1)
src/HotChocolate/Fusion/src/Fusion.Execution/Execution/Nodes/OperationCompiler.cs:68
- This normalization changes
Operation.ShortHashfrom the supplied short hash to a rewritten value (for example,BnjEJe8-becomesBnjEJe8_), while its contract still says it is the short hash of the original document (Execution/Nodes/Operation.cs:90-93). Please update that public property's documentation, and any related serialization contract documentation, to describe the name-safe value returned after this change.
// Operation.ShortHash is embedded in the alias-batched source schema operation name.
shortHash = OperationShortHash.ToNameSafe(shortHash);
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
The gateway names every operation it sends to a source schema
{clientOperationName}_{shortHash}_{stepId}, where the short hash is the first eight characters of the client document hash as rendered by the configuredIDocumentHashProvider.HashFormat.Base64yields URL-safe Base64, whose-is not a GraphQL name character, so roughly one document in four produced an unparsable name. The failure landed in the gateway rather than at the source schema, becauseOperationExecutionNodeparses the operation text the planner generates. A client-supplied hash reaches the same code path.Characters a GraphQL name cannot hold are now replaced with an underscore at the two places a name is built,
PlanNode.CreateOperationNameandAliasBatchedRequestBody. The hash itself is never rewritten, soOperation.ShortHash,PlanNode.ShortHash, and theshortHashfield of the serialized operation plan all keep the real document hash prefix and stay correlatable with the hash the gateway reports.Digits are kept in every position. The short hash always sits behind
Op_or the client operation name, so GraphQL'sNameStartrule does not reach it, andNameUtils.MakeValidGraphQLNameapplies the whole-name grammar instead and would rewrite94a407d5to_4a407d5. A hex short hash is returned unchanged and allocates nothing.Test plan
GeneratedOperationNameTestsgains the counterpart toCreatePlan_Should_KeepTheShortHash_When_ItHoldsNameCharactersOnly, covering the generated name and, separately, thatOperation.ShortHashstill reports the unmodified hash.AliasBatchedRequestBodyTestscovers the merged batch name for a non-name short hash, which had no assertion before.OperationShortHashTestscovers the character classes and pins the leading-digit contract. Mutating the helper to the whole-name rule turns that test red with_4a407d5.dotnet test src/HotChocolate/Fusion/test/Fusion.Execution.Testspasses, 1147 tests.Fusion.AspNetCore.Tests,Fusion.Diagnostics.Tests, andFusion.Connectors.InMemory.Testspass unchanged. No snapshot moved, because a hex short hash is returned as-is.dotnet build src/HotChocolate/Fusion -p:EnforceCodeStyleInBuild=truereports no errors.