Open
Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new V2 variable name generation mechanism for GraphQL operation definitions. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/ast/ast_operation_definition_test.go (1)
3-12:⚠️ Potential issue | 🟡 MinorFix the import grouping to unblock CI.
gciis already failing on this block. Theastimport is mixed into the stdlib section, so this file will not pass formatting as-is.Suggested import order
import ( "fmt" - "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" "github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafeparser" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/ast/ast_operation_definition_test.go` around lines 3 - 12, The import block mixes a non-stdlib import (github.com/wundergraph/graphql-go-tools/v2/pkg/ast) into the stdlib group and fails gci; reorder the imports so stdlib packages (fmt, strings, testing) come first, then third-party packages (github.com/wundergraph/graphql-go-tools/v2/pkg/ast and github.com/stretchr/testify/assert), and finally the internal package (github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafeparser), with a blank line between each group to match gci formatting.
🧹 Nitpick comments (3)
v2/pkg/ast/ast_operation_definition.go (2)
157-166: This lookup pattern can go quadratic on large operations.
GenerateUnusedVariableDefinitionNameV2probes every candidate fromaupward and does an existence lookup each time. For the large cases this PR explicitly targets, that can get expensive quickly ifVariableDefinitionByNameAndOperationis scanning the operation’s variable list. Building a used-name set once per call would keep the new sequence without turning the hot path into repeated linear searches.One way to keep this O(n)
func (d *Document) GenerateUnusedVariableDefinitionNameV2(operationDefinition int) []byte { + used := make(map[string]struct{}, len(d.OperationDefinitions[operationDefinition].VariableDefinitions.Refs)) + for _, ref := range d.OperationDefinitions[operationDefinition].VariableDefinitions.Refs { + value := d.VariableDefinitions[ref].VariableValue.Ref + used[d.VariableValueNameString(value)] = struct{}{} + } + l := NewDefaultLetterIndices() for { - bytes := l.Bytes() - if _, exists := d.VariableDefinitionByNameAndOperation(operationDefinition, bytes); !exists { - return bytes + name := l.Render() + if _, exists := used[name]; !exists { + return []byte(name) } l.Increment() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/ast/ast_operation_definition.go` around lines 157 - 166, GenerateUnusedVariableDefinitionNameV2 currently probes every candidate and calls VariableDefinitionByNameAndOperation repeatedly, which can be quadratic; instead, iterate once over the operation’s variable definitions to build a set/map of used names (e.g., map[string]struct{}) and then use NewDefaultLetterIndices/Bytes/Increment to produce candidates, checking membership against that set until an unused name is found; update the function to reference GenerateUnusedVariableDefinitionNameV2, VariableDefinitionByNameAndOperation (only for building the set or directly iterate variable list), NewDefaultLetterIndices, Bytes, and Increment accordingly so lookups are O(1) per candidate.
168-214: Avoid exportingLetterIndicesas a mutable public contract.This helper looks like an implementation detail of the generator, but
LetterIndices,NewLetterIndices,NewDefaultLetterIndices,Bytes, andRenderare now part of the package API. That also exposes a fragile invariant: callers can pass mismatchedindices/bytesslices or mutate the returned/input slices and leave the instance in an invalid state. If external construction is not required, I’d keep this type unexported; otherwise the constructor should at least copy and validate its inputs and avoid returning the internal buffer directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/ast/ast_operation_definition.go` around lines 168 - 214, The LetterIndices helper is leaking mutable internals and a fragile invariant; either make the type unexported (rename LetterIndices -> letterIndices) and keep constructors private, or harden the public API: in NewLetterIndices validate that len(indices)==len(bytes), copy both input slices into a new struct, and in Bytes/Render return copies (Bytes should return a new slice copy) so callers can't mutate internal buffers; also ensure NewDefaultLetterIndices returns a safe copied instance and keep methods (Increment, incrementAt, resetAt, maxIndex) operating on the internal fields only.v2/pkg/ast/ast_operation_definition_test.go (1)
110-128: Add one sparse-name regression case.These assertions only cover contiguous prefixes (
a..z,a..zz, ...), so an implementation that just returns “next after max” would still pass. A case like($a, $c)→bwould lock in the actual contract: first unused name, not highest+1.Example test to add
+ t.Run("fills the first gap, not just the tail", func(t *testing.T) { + op := unsafeparser.ParseGraphqlDocumentString(`query ($a: Int! $c: Int!) { + fielda(arg: $a) + fieldc(arg: $c) +}`) + assert.Equal(t, "b", string(op.GenerateUnusedVariableDefinitionNameV2(0))) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/ast/ast_operation_definition_test.go` around lines 110 - 128, The tests for GenerateUnusedVariableDefinitionNameV2 only check contiguous sequences and miss sparse cases; add a test using unsafeparser.ParseGraphqlDocumentString with a schemaString that defines non-contiguous variable names (e.g., variables "a" and "c") and assert that op.GenerateUnusedVariableDefinitionNameV2(0) returns "b" to ensure the function picks the first unused name rather than simply returning the next after the max; place this new t.Run alongside the existing cases and reference the same helpers (schemaString, unsafeparser.ParseGraphqlDocumentString, GenerateUnusedVariableDefinitionNameV2) so the regression is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@v2/pkg/ast/ast_operation_definition_test.go`:
- Around line 3-12: The import block mixes a non-stdlib import
(github.com/wundergraph/graphql-go-tools/v2/pkg/ast) into the stdlib group and
fails gci; reorder the imports so stdlib packages (fmt, strings, testing) come
first, then third-party packages
(github.com/wundergraph/graphql-go-tools/v2/pkg/ast and
github.com/stretchr/testify/assert), and finally the internal package
(github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafeparser), with a
blank line between each group to match gci formatting.
---
Nitpick comments:
In `@v2/pkg/ast/ast_operation_definition_test.go`:
- Around line 110-128: The tests for GenerateUnusedVariableDefinitionNameV2 only
check contiguous sequences and miss sparse cases; add a test using
unsafeparser.ParseGraphqlDocumentString with a schemaString that defines
non-contiguous variable names (e.g., variables "a" and "c") and assert that
op.GenerateUnusedVariableDefinitionNameV2(0) returns "b" to ensure the function
picks the first unused name rather than simply returning the next after the max;
place this new t.Run alongside the existing cases and reference the same helpers
(schemaString, unsafeparser.ParseGraphqlDocumentString,
GenerateUnusedVariableDefinitionNameV2) so the regression is covered.
In `@v2/pkg/ast/ast_operation_definition.go`:
- Around line 157-166: GenerateUnusedVariableDefinitionNameV2 currently probes
every candidate and calls VariableDefinitionByNameAndOperation repeatedly, which
can be quadratic; instead, iterate once over the operation’s variable
definitions to build a set/map of used names (e.g., map[string]struct{}) and
then use NewDefaultLetterIndices/Bytes/Increment to produce candidates, checking
membership against that set until an unused name is found; update the function
to reference GenerateUnusedVariableDefinitionNameV2,
VariableDefinitionByNameAndOperation (only for building the set or directly
iterate variable list), NewDefaultLetterIndices, Bytes, and Increment
accordingly so lookups are O(1) per candidate.
- Around line 168-214: The LetterIndices helper is leaking mutable internals and
a fragile invariant; either make the type unexported (rename LetterIndices ->
letterIndices) and keep constructors private, or harden the public API: in
NewLetterIndices validate that len(indices)==len(bytes), copy both input slices
into a new struct, and in Bytes/Render return copies (Bytes should return a new
slice copy) so callers can't mutate internal buffers; also ensure
NewDefaultLetterIndices returns a safe copied instance and keep methods
(Increment, incrementAt, resetAt, maxIndex) operating on the internal fields
only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a388bbae-2720-4013-b1e5-9e3884da1fa2
📒 Files selected for processing (2)
v2/pkg/ast/ast_operation_definition.gov2/pkg/ast/ast_operation_definition_test.go
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
3 participants
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.
Currently, the 27th variable will be
aabut the 53rd will beaaa. This means the size of the variable grows quickly.In this approach, the variable names grows much slower:
a, ..., z, aa, ..., az, ba, ..., zz, aaa, aab, ...2 chars at 27 variables,
3 chars at 703 variables,
4 chars at 18279 variables, etc.
Summary by CodeRabbit
New Features
Tests