Conversation
This reverts commit cd47b12.
The router makes sure to only pass normalized operations to the variable normalizer. This means in practice that both, named and inlined fragments, have already been inlined into the query and the variable normalizer visitor never sees any fragments. Therefore we don't need to explicitely support them for field argument mapping. I removed it because it would be expensive (if used), adds code complexity and in practice is dead code.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional field-argument mapping to variable normalization: the normalizer constructor now accepts a boolean to enable mapping and NormalizeOperation returns a result struct (uploads mapping + optional field-argument mapping) instead of a bare slice. Also changes Path.DotDelimitedString to accept a fragment-ref flag and updates call sites. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds the ability to enable or disable field arg mapping when the variable normalizer is created. Since this option is only useful on the variable normalizer it is disabled in code for the variable extraction on the operation normalizer.
dkorittki
requested review from
Noroth,
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/astnormalization/variables_extraction.go (1)
87-95:⚠️ Potential issue | 🟠 MajorBug: field argument mapping not recorded when a literal value is reused via
variableExists.When two field arguments have identical literal values,
variableExistsfinds the already-extracted variable and reuses it (lines 88-94). However,recordFieldArgumentMappingis never called on this path, so the second field argument's mapping entry is silently dropped.Example scenario:
query { firstAlias: user(id: "123") { name } secondAlias: user(id: "123") { name } }Only
"query.firstAlias.id" -> "a"would be recorded;"query.secondAlias.id"would be missing.Proposed fix
if exists, name, _ := v.variableExists(valueBytes, inputValueDefinition); exists { variable := ast.VariableValue{ Name: v.operation.Input.AppendInputBytes(name), } value := v.operation.AddVariableValue(variable) v.operation.Arguments[ref].Value.Kind = ast.ValueKindVariable v.operation.Arguments[ref].Value.Ref = value + if v.fieldArgumentMapping.enabled { + v.recordFieldArgumentMapping(ref, string(name)) + } return }
🧹 Nitpick comments (1)
v2/pkg/astnormalization/astnormalization_test.go (1)
1196-1204: Test "empty mapping when no arguments" uses a query that would fail GraphQL validation.The operation
query GetUser { user { name } }omits the requiredid: ID!argument on theuserfield (per the schema at line 1073). While this tests the normalizer's behavior on invalid input (which is fine — it correctly returns an empty mapping), consider adding a comment noting this is intentionally invalid to aid future readers.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Outdated
Show resolved
Hide resolved
| func (v *VariablesNormalizer) NormalizeOperation(operation, definition *ast.Document, report *operationreport.Report) []uploads.UploadPathMapping { | ||
| // NormalizeOperation processes GraphQL operation variables. | ||
| // It detects and removes unused variables, extracts variables from inline values | ||
| // and coerces variable types. It modifies the operation in place and |
There was a problem hiding this comment.
it coerses not types, but variable values, we could also mention what part of graphql spec it is https://spec.graphql.org/September2025/#sec-Coercing-Variable-Values
| // It detects and removes unused variables, extracts variables from inline values | ||
| // and coerces variable types. It modifies the operation in place and | ||
| // returns metadata including field argument mappings and upload paths. | ||
| // Field argument mapping is done only when v is configured to do so via NewVariablesNormalizer, |
There was a problem hiding this comment.
when v is configured
when VariablesNormalizer
or when collecting arguments mapping enabled
There was a problem hiding this comment.
This is what "when v is configured to do so via NewVariablesNormalizer" wanted to express. I'll rephrase it.
| variables string | ||
| expectedMapping FieldArgumentMapping | ||
| }{ | ||
| { |
There was a problem hiding this comment.
missing test case for complex input type, smth like this
type Query {
users(paging: Paging!): [String]!
}
input Paging {
limit: Int!
offset: Int!
}
such query
query Search($limit: Int!, $offset: Int!) {
users(paging: {limit: $limit, offset: $offset})
}
variables: {"limit": 10, "offset": 5}
will be normalized into
query Search($a: Paging!) {
users(paging: $a)
}
variables: {"limit": 10, "offset": 5, "a": {"limit": 10, "offset": 5}}
| // of v in v.fieldArgumentMapping, alongside its matching variable name or literal value. | ||
| // If varName is empty, it looks up the variable name from the operation or stores the literal value. | ||
| func (v *variablesExtractionVisitor) recordFieldArgumentMapping(ref int, varName string) { | ||
| // Guard to prevent nil panics. |
There was a problem hiding this comment.
when it is enabled we should always have value
potentially we could just put a check here
if v.fieldArgumentMapping.enabled {
There was a problem hiding this comment.
The relationship between v.fieldArgumentMapping.enabled == true and v.fieldArgumentMapping.result != nil is loose. It's enforced in EnterDocument. It could change by accident in the future and hypothetically we could end up with v.fieldArgumentMapping.enabled == true and v.fieldArgumentMapping.result == nil. Then if v.fieldArgumentMapping.enabled would not return early and we would run into a nil panic.
I did it this way to be maximum defensive. This function wants to work with v.fieldArgumentMapping.result so it must exist.
Will change the comments to make this intend more clear
| if v.operation.Arguments[ref].Value.Kind != ast.ValueKindVariable { | ||
| // We expect the operation on this visitor to be normalized before the visitor walks it. | ||
| // This means that values of any field arguments should be extracted to variables. | ||
| // If we still land here it means the query hasn't been normalized before. |
There was a problem hiding this comment.
If we still land here it means the query hasn't been normalized before.
a bit strange comment, because this visitor is doing this normalization
| type FieldArgumentMapping map[string]string | ||
|
|
||
| // VariablesNormalizerResult contains the results of variable normalization. | ||
| type VariablesNormalizerResult struct { |
There was a problem hiding this comment.
what do you think about VariablesNormalizationResult name?
I propose to move VariablesNormalizer into its own file variables_normalizer or variables_normalization and add these types there
| // NewVariablesNormalizer creates a new variable normalizer. | ||
| // If withFieldArgMapping is true then the normalizer creates a | ||
| // mapping of field arguments as a "side product" when NormalizeOperation is called. | ||
| func NewVariablesNormalizer(withFieldArgMapping bool) *VariablesNormalizer { |
There was a problem hiding this comment.
what do you think about introducing either an options or config argument?
e.g.
type VariablesNormalizerOptions struct {
withFieldArgMapping bool
}
WithFieldArgMapping()
or just
type VariablesNormalizerConfig struct {
WithFieldArgMapping bool
}
…g-support-to-engine
…g-support-to-engine
…g-support-to-engine
3 participants
Checklist
Context
Extends the engines variable normalizer to map field arguments as a side product of it's actual work and return the results alongside the file upload mapping.
This adds a useful feature which the router uses, called field argument mapping. It's described in detail in the corresponding router pull request.
The principle of field argument mapping is to allow Cosmo Streams hook users to be able to access values of field arguments to filter subscription events based on this. The idea is to give access the values via operation variables.
Values of any field arguments are available via variables, either because the users already use variables or because they have been extracted from literal values via variable extraction. The map that is being build holds the path to the field argument as a dot notated string and the variable name where the argument value is hold. Later on, when hook users want to access the value, the variable name can be resolved from the argument path and we can provide the variable value.
Here's an example:
The map thats being build is:
If a hook user later accesses
query.user.orders.limitwe resolve the value of variableato get12.If the hook accesses
query.user.idwe can resolveuserId, even if the engine performed variable remapping (userIdbeing renamed toaorb, etc.)This works because the variables which later are accessed are the ones from the operation context from the router and those are unaffected of variable remapping. In other words: The router config option
engine.disable_variables_remappingdoes not have an effect on this. It works regardless.The first implementation of this feature was done solely on the router. It worked without engine changes. A newly created walker would walk the operation AST and collect and map all field arguments. While this worked the problem was the introduction of a new walker during the request processing right in the hot path and it wasn't cachable either.
With this pull request the actual walking is done by an already existing visitor as a side product:
variablesExtractionVisitor. This visitor is part of the variable normalizer and already walks field arguments. I just placed the new functionality into the visitorsEnterArgumenthook.The variable normalizer does it's work by invoking the
NormalizeOperationmethod. This method, in it's current form, returns the file upload mapping, which is cached by the router. This method has been modified to extend the return type by the field arg mapping results. Since the router already cached the result we get caching field arg mapping for free.Field Argument Mapping can be enabled or disabled when the variable normalizer is instanciated.
This option is then passed down to the
variablesExtractionVisitor, where the field argument mapping is happening.The same visitor exists for the operation normalizer. There, however, we disable field argument mapping because it's a useful feature only on the variable normalizer.
Performance Considerations
This code hooks into an already existing walker, which walks all field arguments of an operation. The code added will allocate a string out of a slice byte via string convertion
(string(myByteSlice)for most arguments. Also it will repeatedly grow a map, because the map initially is not allocated with a predetermined size. This could have performance implications on queries with vast amounts of field arguments but I think in reality we don't have a lot of them. Also once it's cached by the router, it does not matter anyway.Summary by CodeRabbit