From c166d9138c8f56d763f38cca0fee7d46952a1390 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Mon, 6 Jul 2026 23:22:55 +0530 Subject: [PATCH 01/21] feat: add new visitor --- v2/pkg/astnormalization/inline_arguments.go | 119 ++++++++++ .../astnormalization/inline_arguments_test.go | 220 ++++++++++++++++++ 2 files changed, 339 insertions(+) create mode 100644 v2/pkg/astnormalization/inline_arguments.go create mode 100644 v2/pkg/astnormalization/inline_arguments_test.go diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go new file mode 100644 index 0000000000..a122e6ebe5 --- /dev/null +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -0,0 +1,119 @@ +package astnormalization + +import ( + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" + "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/position" + "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" +) + +// InlineArgument describes a single argument in an operation whose value was +// supplied inline (a literal) instead of as a variable. +type InlineArgument struct { + ArgumentName string + EnclosingName string + EnclosingKind ast.NodeKind + ValueKind ast.ValueKind + Position position.Position +} + +func (a InlineArgument) QualifiedName() string { + switch a.EnclosingKind { + case ast.NodeKindField: + return a.EnclosingName + "." + a.ArgumentName + case ast.NodeKindDirective: + return "@" + a.EnclosingName + "." + a.ArgumentName + default: + return a.ArgumentName + } +} + +type InlineArgumentsValidationOptions struct { + Enforce bool + ErrorMessage string + ErrorCode string + StatusCode int +} + +type InlineArgumentsValidator struct { + Options InlineArgumentsValidationOptions + Findings []InlineArgument + Disabled bool +} + +func (v *InlineArgumentsValidator) ClearFindings() { + v.Findings = v.Findings[:0] +} + +func (v *InlineArgumentsValidator) HadInlineArguments() bool { + return len(v.Findings) > 0 +} + +// InlineArgumentsRule returns a prevalidation rule that flags every argument +// whose value is an inline literal instead of a variable, in any context: field +// arguments, directive arguments (@skip/@include and any custom directive), and +// introspection-field arguments. Register it via WithPrevalidationRules; results +// land on the given validator. +// +// Variable-definition default values (e.g. `$x: Int = 5`) are naturally excluded +// — they are not arguments and are never visited as one. +func InlineArgumentsRule(validator *InlineArgumentsValidator) func(walker *astvisitor.Walker) { + return func(walker *astvisitor.Walker) { + visitor := &inlineArgumentsVisitor{ + Walker: walker, + validator: validator, + } + walker.RegisterEnterDocumentVisitor(visitor) + walker.RegisterEnterArgumentVisitor(visitor) + } +} + +type inlineArgumentsVisitor struct { + *astvisitor.Walker + operation, definition *ast.Document + validator *InlineArgumentsValidator +} + +func (v *inlineArgumentsVisitor) EnterDocument(operation, definition *ast.Document) { + v.operation = operation + v.definition = definition +} + +func (v *inlineArgumentsVisitor) EnterArgument(ref int) { + if v.validator.Disabled { + return + } + valueKind := v.operation.Arguments[ref].Value.Kind + if valueKind == ast.ValueKindVariable { + return + } + + if v.validator.Options.Enforce { + v.StopWithExternalErr(operationreport.ExternalError{ + Message: v.validator.Options.ErrorMessage, + ExtensionCode: v.validator.Options.ErrorCode, + StatusCode: v.validator.Options.StatusCode, + Locations: operationreport.LocationsFromPosition(v.operation.Arguments[ref].Position), + }) + return + } + + finding := InlineArgument{ + ArgumentName: v.operation.ArgumentNameString(ref), + ValueKind: valueKind, + Position: v.operation.Arguments[ref].Position, + } + + if len(v.Ancestors) > 0 { + parent := v.Ancestors[len(v.Ancestors)-1] + finding.EnclosingKind = parent.Kind + switch parent.Kind { + case ast.NodeKindField: + finding.EnclosingName = v.operation.FieldNameString(parent.Ref) + case ast.NodeKindDirective: + finding.EnclosingName = v.operation.DirectiveNameString(parent.Ref) + } + } + + v.validator.Findings = append(v.validator.Findings, finding) +} diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go new file mode 100644 index 0000000000..a33c1990b7 --- /dev/null +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -0,0 +1,220 @@ +package astnormalization + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/asttransform" + "github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafeparser" + "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" +) + +const inlineArgumentsTestSchema = ` + schema { query: Query } + type Query { + userById(userId: ID!): User + user: User + field(order: Sort, flag: Boolean, by: [Int], obj: Filter): String + } + type User { + loginName: String + posts(first: Int): String + } + enum Sort { ASC DESC } + input Filter { a: Int } +` + +func runInlineArgumentsRule(t *testing.T, operation string, opts InlineArgumentsValidationOptions) (*InlineArgumentsValidator, *operationreport.Report) { + t.Helper() + + definitionDocument := unsafeparser.ParseGraphqlDocumentString(inlineArgumentsTestSchema) + require.NoError(t, asttransform.MergeDefinitionWithBaseSchema(&definitionDocument)) + + operationDocument := unsafeparser.ParseGraphqlDocumentString(operation) + report := &operationreport.Report{} + + validator := &InlineArgumentsValidator{Options: opts} + normalizer := NewWithOpts(WithPrevalidationRules(InlineArgumentsRule(validator))) + normalizer.NormalizeOperation(&operationDocument, &definitionDocument, report) + + return validator, report +} + +func TestInlineArgumentsRule_Detection(t *testing.T) { + tests := []struct { + name string + operation string + expected []InlineArgument + }{ + { + name: "inline string field argument", + operation: `query GetUserById { userById(userId: "12345") { loginName } }`, + expected: []InlineArgument{ + {ArgumentName: "userId", EnclosingName: "userById", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, + }, + }, + { + name: "variable field argument is compliant", + operation: `query GetUserById($userId: ID!) { userById(userId: $userId) { loginName } }`, + expected: nil, + }, + { + name: "inline enum argument", + operation: `query { field(order: ASC) }`, + expected: []InlineArgument{ + {ArgumentName: "order", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, + }, + }, + { + name: "inline null argument", + operation: `query { field(flag: null) }`, + expected: []InlineArgument{ + {ArgumentName: "flag", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindNull}, + }, + }, + { + name: "inline list argument recorded once", + operation: `query { field(by: [1, 2, 3]) }`, + expected: []InlineArgument{ + {ArgumentName: "by", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindList}, + }, + }, + { + name: "inline object argument recorded once", + operation: `query { field(obj: { a: 1 }) }`, + expected: []InlineArgument{ + {ArgumentName: "obj", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindObject}, + }, + }, + { + name: "mixed variable and literal flags only the literal", + operation: `query q($flag: Boolean) { field(flag: $flag, order: DESC) }`, + expected: []InlineArgument{ + {ArgumentName: "order", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, + }, + }, + { + name: "inline directive argument (@include)", + operation: `query q($userId: ID!) { userById(userId: $userId) @include(if: true) { loginName } }`, + expected: []InlineArgument{ + {ArgumentName: "if", EnclosingName: "include", EnclosingKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, + }, + }, + { + name: "variable directive argument is compliant", + operation: `query q($userId: ID!, $show: Boolean!) { userById(userId: $userId) @include(if: $show) { loginName } }`, + expected: nil, + }, + { + name: "introspection field argument", + operation: `query { __type(name: "User") { name } }`, + expected: []InlineArgument{ + {ArgumentName: "name", EnclosingName: "__type", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, + }, + }, + { + // Proves detection runs before @skip/@include prunes the node: both the + // directive's own `if` and the child field's `first` are reported even + // though normalization would delete the `user` selection. + name: "argument under a @skip(if:true)-removed node still flagged", + operation: `query q($userId: ID!) { user @skip(if: true) { posts(first: 10) } userById(userId: $userId) { loginName } }`, + expected: []InlineArgument{ + {ArgumentName: "if", EnclosingName: "skip", EnclosingKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, + {ArgumentName: "first", EnclosingName: "posts", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindInteger}, + }, + }, + { + name: "no inline arguments", + operation: `query q($userId: ID!) { userById(userId: $userId) { loginName } }`, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + validator, report := runInlineArgumentsRule(t, tt.operation, InlineArgumentsValidationOptions{Enforce: false}) + require.False(t, report.HasErrors(), "log-only mode must never error: %s", report.Error()) + + if len(tt.expected) == 0 { + assert.Empty(t, validator.Findings) + return + } + + require.Len(t, validator.Findings, len(tt.expected)) + got := make([]InlineArgument, len(validator.Findings)) + for i, f := range validator.Findings { + f.Position = tt.expected[i].Position // ignore position in this comparison + got[i] = f + } + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestInlineArgumentsRule_Position(t *testing.T) { + // The reported position points at the argument in the operation as parsed. + // `userId` starts at column 30 on line 1 of the operation below. + operation := `query GetUserById { userById(userId: "12345") { loginName } }` + validator, report := runInlineArgumentsRule(t, operation, InlineArgumentsValidationOptions{ + Enforce: false, + }) + require.False(t, report.HasErrors()) + require.Len(t, validator.Findings, 1) + + pos := validator.Findings[0].Position + assert.Equal(t, uint32(1), pos.LineStart) + assert.Equal(t, uint32(30), pos.CharStart) +} + +func TestInlineArgumentsRule_Enforce(t *testing.T) { + t.Run("stops at first inline argument and reports a typed error", func(t *testing.T) { + validator, report := runInlineArgumentsRule(t, + `query { userById(userId: "12345") { loginName } field(order: ASC) }`, + InlineArgumentsValidationOptions{ + Enforce: true, + ErrorMessage: "Inline argument values are not allowed. Use variables instead.", + ErrorCode: "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", + StatusCode: 400, + }, + ) + + require.True(t, report.HasErrors()) + require.Len(t, report.ExternalErrors, 1) + extErr := report.ExternalErrors[0] + assert.Equal(t, "Inline argument values are not allowed. Use variables instead.", extErr.Message) + assert.Equal(t, "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", extErr.ExtensionCode) + assert.Equal(t, 400, extErr.StatusCode) + + // Enforce rejects via the report and does not collect findings — the + // operation is rejected, so nothing reads them. + assert.Empty(t, validator.Findings) + }) + + t.Run("compliant operation passes enforce mode", func(t *testing.T) { + validator, report := runInlineArgumentsRule(t, + `query q($userId: ID!) { userById(userId: $userId) { loginName } }`, + InlineArgumentsValidationOptions{Enforce: true, ErrorMessage: "nope", ErrorCode: "CODE", StatusCode: 400}, + ) + require.False(t, report.HasErrors(), "compliant operation must not error: %s", report.Error()) + assert.False(t, validator.HadInlineArguments()) + }) + + t.Run("disabled validator records nothing", func(t *testing.T) { + definitionDocument := unsafeparser.ParseGraphqlDocumentString(inlineArgumentsTestSchema) + require.NoError(t, asttransform.MergeDefinitionWithBaseSchema(&definitionDocument)) + operationDocument := unsafeparser.ParseGraphqlDocumentString(`query { userById(userId: "12345") { loginName } }`) + report := &operationreport.Report{} + + validator := &InlineArgumentsValidator{Options: InlineArgumentsValidationOptions{Enforce: true, ErrorMessage: "x", ErrorCode: "C", StatusCode: 400}} + validator.Disabled = true + + normalizer := NewWithOpts(WithPrevalidationRules(InlineArgumentsRule(validator))) + normalizer.NormalizeOperation(&operationDocument, &definitionDocument, report) + + require.False(t, report.HasErrors()) + assert.False(t, validator.HadInlineArguments()) + }) +} From 937a0f26887a76ff3a0ece240bc2719c45fd5d39 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 13:28:03 +0530 Subject: [PATCH 02/21] fix: changes --- v2/pkg/astnormalization/inline_arguments.go | 35 ++++++++---- .../astnormalization/inline_arguments_test.go | 28 ++++++++-- v2/pkg/engine/resolve/const.go | 3 ++ v2/pkg/engine/resolve/context.go | 3 ++ v2/pkg/engine/resolve/resolvable.go | 53 +++++++++++++++++++ 5 files changed, 109 insertions(+), 13 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index a122e6ebe5..dfd06a821d 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -1,6 +1,8 @@ package astnormalization import ( + "fmt" + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/position" @@ -33,6 +35,11 @@ type InlineArgumentsValidationOptions struct { ErrorMessage string ErrorCode string StatusCode int + // ReturnInResponseExtensions, when true and enforcing, names the offending + // inline argument in the rejection error message (e.g. `... argument "user.id".`). + // In non-enforcing mode the router reports findings via the top-level response + // extensions instead, so this option has no effect on the walker there. + ReturnInResponseExtensions bool } type InlineArgumentsValidator struct { @@ -88,16 +95,6 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { return } - if v.validator.Options.Enforce { - v.StopWithExternalErr(operationreport.ExternalError{ - Message: v.validator.Options.ErrorMessage, - ExtensionCode: v.validator.Options.ErrorCode, - StatusCode: v.validator.Options.StatusCode, - Locations: operationreport.LocationsFromPosition(v.operation.Arguments[ref].Position), - }) - return - } - finding := InlineArgument{ ArgumentName: v.operation.ArgumentNameString(ref), ValueKind: valueKind, @@ -115,5 +112,23 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { } } + if v.validator.Options.Enforce { + // Reject on the first inline argument and stop the walk — a single error is + // enough to signal that the operation is non-compliant. When configured, the + // offending argument is named in the message rather than in a dedicated + // extension. + message := v.validator.Options.ErrorMessage + if v.validator.Options.ReturnInResponseExtensions { + message = fmt.Sprintf("%s Inline value provided for argument %q.", message, finding.QualifiedName()) + } + v.StopWithExternalErr(operationreport.ExternalError{ + Message: message, + ExtensionCode: v.validator.Options.ErrorCode, + StatusCode: v.validator.Options.StatusCode, + Locations: operationreport.LocationsFromPosition(finding.Position), + }) + return + } + v.validator.Findings = append(v.validator.Findings, finding) } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index a33c1990b7..a10f2478be 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -170,7 +170,7 @@ func TestInlineArgumentsRule_Position(t *testing.T) { } func TestInlineArgumentsRule_Enforce(t *testing.T) { - t.Run("stops at first inline argument and reports a typed error", func(t *testing.T) { + t.Run("stops at the first inline argument and reports a typed error", func(t *testing.T) { validator, report := runInlineArgumentsRule(t, `query { userById(userId: "12345") { loginName } field(order: ASC) }`, InlineArgumentsValidationOptions{ @@ -188,11 +188,33 @@ func TestInlineArgumentsRule_Enforce(t *testing.T) { assert.Equal(t, "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", extErr.ExtensionCode) assert.Equal(t, 400, extErr.StatusCode) - // Enforce rejects via the report and does not collect findings — the - // operation is rejected, so nothing reads them. + // Enforce rejects on the first inline argument and stops the walk, so no + // findings are collected. assert.Empty(t, validator.Findings) }) + t.Run("names the offending argument in the message when ReturnInResponseExtensions is set", func(t *testing.T) { + _, report := runInlineArgumentsRule(t, + `query { userById(userId: "12345") { loginName } field(order: ASC) }`, + InlineArgumentsValidationOptions{ + Enforce: true, + ErrorMessage: "Inline argument values are not allowed. Use variables instead.", + ErrorCode: "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", + StatusCode: 400, + ReturnInResponseExtensions: true, + }, + ) + + require.True(t, report.HasErrors()) + require.Len(t, report.ExternalErrors, 1) + // The first offending argument is named in the message; the walk still stops + // there, so only that one is reported. + assert.Equal(t, + `Inline argument values are not allowed. Use variables instead. Inline value provided for argument "userById.userId".`, + report.ExternalErrors[0].Message, + ) + }) + t.Run("compliant operation passes enforce mode", func(t *testing.T) { validator, report := runInlineArgumentsRule(t, `query q($userId: ID!) { userById(userId: $userId) { loginName } }`, diff --git a/v2/pkg/engine/resolve/const.go b/v2/pkg/engine/resolve/const.go index 03c1720dd8..df02daf286 100644 --- a/v2/pkg/engine/resolve/const.go +++ b/v2/pkg/engine/resolve/const.go @@ -30,6 +30,9 @@ var ( literalQueryPlan = []byte("queryPlan") literalValueCompletion = []byte("valueCompletion") literalRateLimit = []byte("rateLimit") + literalInlineArguments = []byte("inlineArguments") + literalCount = []byte("count") + literalArguments = []byte("arguments") literalAuthorization = []byte("authorization") literalIncremental = []byte("incremental") literalHasNext = []byte("hasNext") diff --git a/v2/pkg/engine/resolve/context.go b/v2/pkg/engine/resolve/context.go index 76a5d1ca99..ec6e32f01b 100644 --- a/v2/pkg/engine/resolve/context.go +++ b/v2/pkg/engine/resolve/context.go @@ -39,6 +39,8 @@ type Context struct { Extensions []byte LoaderHooks LoaderHooks + InlineArguments []string + authorizer Authorizer rateLimiter RateLimiter fieldRenderer FieldValueRenderer @@ -313,6 +315,7 @@ func (c *Context) Free() { c.RemapVariables = nil c.TracingOptions.DisableAll() c.Extensions = nil + c.InlineArguments = nil c.subgraphErrors = nil c.authorizer = nil c.LoaderHooks = nil diff --git a/v2/pkg/engine/resolve/resolvable.go b/v2/pkg/engine/resolve/resolvable.go index 71c395025a..a8be43d210 100644 --- a/v2/pkg/engine/resolve/resolvable.go +++ b/v2/pkg/engine/resolve/resolvable.go @@ -866,6 +866,17 @@ func (r *Resolvable) printExtensions(ctx context.Context, fetchTree *FetchTreeNo } } + if len(r.ctx.InlineArguments) > 0 { + if writeComma { + r.printBytes(comma) + } + writeComma = true + err := r.printInlineArgumentsExtension() + if err != nil { + return err + } + } + if r.ctx.TracingOptions.Enable && r.ctx.TracingOptions.IncludeTraceOutputInResponseExtensions { if writeComma { r.printBytes(comma) @@ -976,6 +987,45 @@ func getDefaultReservedExtensions() map[string]struct{} { } } +// printInlineArgumentsExtension renders the non-enforcing disallow-inline-arguments +// findings as `"inlineArguments":{"count":N,"arguments":["field.arg",...]}`. It is +// only called when r.ctx.InlineArguments is non-empty. +func (r *Resolvable) printInlineArgumentsExtension() error { + r.printBytes(quote) + r.printBytes(literalInlineArguments) + r.printBytes(quote) + r.printBytes(colon) + r.printBytes(lBrace) + + r.printBytes(quote) + r.printBytes(literalCount) + r.printBytes(quote) + r.printBytes(colon) + r.printBytes(strconv.AppendInt(nil, int64(len(r.ctx.InlineArguments)), 10)) + r.printBytes(comma) + + r.printBytes(quote) + r.printBytes(literalArguments) + r.printBytes(quote) + r.printBytes(colon) + r.printBytes(lBrack) + for i, name := range r.ctx.InlineArguments { + if i > 0 { + r.printBytes(comma) + } + // json.Marshal yields a correctly-escaped, quoted JSON string. + encoded, err := json.Marshal(name) + if err != nil { + return err + } + r.printBytes(encoded) + } + r.printBytes(rBrack) + + r.printBytes(rBrace) + return r.printErr +} + func (r *Resolvable) hasExtensions() bool { // Apply the filter first to avoid missing extensions or applying empty extensions. if r.filterAllowedSubgraphExtensions(getDefaultReservedExtensions()) { @@ -993,6 +1043,9 @@ func (r *Resolvable) hasExtensions() bool { if r.ctx.ExecutionOptions.IncludeQueryPlanInResponse { return true } + if len(r.ctx.InlineArguments) > 0 { + return true + } if !r.skipValueCompletion && r.valueCompletion != nil { return true } From 3e3675dd7f27ae7e64c609a0647947ed78fa3b19 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 13:38:56 +0530 Subject: [PATCH 03/21] fix: linting --- v2/pkg/astnormalization/inline_arguments.go | 38 +++++++------------ .../astnormalization/inline_arguments_test.go | 23 +---------- 2 files changed, 15 insertions(+), 46 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index dfd06a821d..858d9b345e 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -1,8 +1,6 @@ package astnormalization import ( - "fmt" - "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/position" @@ -35,11 +33,6 @@ type InlineArgumentsValidationOptions struct { ErrorMessage string ErrorCode string StatusCode int - // ReturnInResponseExtensions, when true and enforcing, names the offending - // inline argument in the rejection error message (e.g. `... argument "user.id".`). - // In non-enforcing mode the router reports findings via the top-level response - // extensions instead, so this option has no effect on the walker there. - ReturnInResponseExtensions bool } type InlineArgumentsValidator struct { @@ -77,6 +70,7 @@ func InlineArgumentsRule(validator *InlineArgumentsValidator) func(walker *astvi type inlineArgumentsVisitor struct { *astvisitor.Walker + operation, definition *ast.Document validator *InlineArgumentsValidator } @@ -95,6 +89,18 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { return } + if v.validator.Options.Enforce { + // Reject on the first inline argument and stop the walk. A single generic + // error is enough to signal that the operation is non-compliant; we don't + // name the argument or point at its location. + v.StopWithExternalErr(operationreport.ExternalError{ + Message: v.validator.Options.ErrorMessage, + ExtensionCode: v.validator.Options.ErrorCode, + StatusCode: v.validator.Options.StatusCode, + }) + return + } + finding := InlineArgument{ ArgumentName: v.operation.ArgumentNameString(ref), ValueKind: valueKind, @@ -112,23 +118,5 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { } } - if v.validator.Options.Enforce { - // Reject on the first inline argument and stop the walk — a single error is - // enough to signal that the operation is non-compliant. When configured, the - // offending argument is named in the message rather than in a dedicated - // extension. - message := v.validator.Options.ErrorMessage - if v.validator.Options.ReturnInResponseExtensions { - message = fmt.Sprintf("%s Inline value provided for argument %q.", message, finding.QualifiedName()) - } - v.StopWithExternalErr(operationreport.ExternalError{ - Message: message, - ExtensionCode: v.validator.Options.ErrorCode, - StatusCode: v.validator.Options.StatusCode, - Locations: operationreport.LocationsFromPosition(finding.Position), - }) - return - } - v.validator.Findings = append(v.validator.Findings, finding) } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index a10f2478be..5c4c75683a 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -191,28 +191,9 @@ func TestInlineArgumentsRule_Enforce(t *testing.T) { // Enforce rejects on the first inline argument and stops the walk, so no // findings are collected. assert.Empty(t, validator.Findings) - }) - t.Run("names the offending argument in the message when ReturnInResponseExtensions is set", func(t *testing.T) { - _, report := runInlineArgumentsRule(t, - `query { userById(userId: "12345") { loginName } field(order: ASC) }`, - InlineArgumentsValidationOptions{ - Enforce: true, - ErrorMessage: "Inline argument values are not allowed. Use variables instead.", - ErrorCode: "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", - StatusCode: 400, - ReturnInResponseExtensions: true, - }, - ) - - require.True(t, report.HasErrors()) - require.Len(t, report.ExternalErrors, 1) - // The first offending argument is named in the message; the walk still stops - // there, so only that one is reported. - assert.Equal(t, - `Inline argument values are not allowed. Use variables instead. Inline value provided for argument "userById.userId".`, - report.ExternalErrors[0].Message, - ) + // The rejection is a generic error: no per-argument location is attached. + assert.Empty(t, extErr.Locations) }) t.Run("compliant operation passes enforce mode", func(t *testing.T) { From 201ed9ae2c687026485441d53da4a3a2e0e5cb2b Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 13:56:58 +0530 Subject: [PATCH 04/21] fix: nil check --- v2/pkg/astnormalization/inline_arguments.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index 858d9b345e..288ce6e572 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -42,6 +42,9 @@ type InlineArgumentsValidator struct { } func (v *InlineArgumentsValidator) ClearFindings() { + if v == nil { + return + } v.Findings = v.Findings[:0] } From 7c1f178230abc827d25f320e83e715d1b16c1d24 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 14:49:42 +0530 Subject: [PATCH 05/21] fix: tests --- v2/pkg/engine/resolve/extensions_test.go | 37 ++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/v2/pkg/engine/resolve/extensions_test.go b/v2/pkg/engine/resolve/extensions_test.go index 66f7e2f21e..1efd9172b1 100644 --- a/v2/pkg/engine/resolve/extensions_test.go +++ b/v2/pkg/engine/resolve/extensions_test.go @@ -141,4 +141,41 @@ func TestExtensions(t *testing.T) { `{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test.","extensions":{"code":"UNAUTHORIZED_FIELD_OR_TYPE"}},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"Failed to render Fetch Input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"Failed to render Fetch Input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0},"trace":{"version":"1","info":{"trace_start_time":"","trace_start_unix":0,"parse_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"normalize_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"validate_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"planner_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""}},"fetches":{"kind":"Sequence","children":[{"kind":"Single","fetch":{"kind":"Single","path":"query","source_id":"users","source_name":"users","trace":{"raw_input_data":{},"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}}},{"kind":"Single","fetch":{"kind":"Single","path":"query.me","source_id":"reviews","source_name":"reviews","trace":{"raw_input_data":null,"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}}},{"kind":"Single","fetch":{"kind":"Single","path":"query.me.reviews.@.product","source_id":"products","source_name":"products","trace":{"raw_input_data":null,"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}}}]}}}}`, func(t *testing.T) {} })) + t.Run("inline arguments", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) { + + res := generateTestFederationGraphQLResponse(t, ctrl) + + resolveCtx := NewContext(context.Background()) + resolveCtx.InlineArguments = []string{"user.filter", "@include.if"} + return res, resolveCtx, + `{"data":{"me":{"id":"1234","username":"Me","reviews":[{"body":"A highly effective form of birth control.","product":{"upc":"top-1","name":"Trilby"}},{"body":"Fedoras are one of the most fashionable hats around and can look great with a variety of outfits.","product":{"upc":"top-2","name":"Fedora"}}]}},"extensions":{"inlineArguments":{"count":2,"arguments":["user.filter","@include.if"]}}}`, + func(t *testing.T) {} + })) + t.Run("rate limit deny & inline arguments", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) { + + authorizer := createTestAuthorizer(func(ctx *Context, dataSourceID string, input json.RawMessage, coordinate GraphCoordinate) (result *AuthorizationDeny, err error) { + return nil, nil + }, func(ctx *Context, dataSourceID string, object json.RawMessage, coordinate GraphCoordinate) (result *AuthorizationDeny, err error) { + return nil, nil + }) + + limiter := &testRateLimiter{ + policy: "policy", + allowed: 0, + allowFn: func(ctx *Context, info *FetchInfo, input json.RawMessage) (*RateLimitDeny, error) { + return &RateLimitDeny{Reason: "rate limit exceeded"}, nil + }, + } + + res := generateTestFederationGraphQLResponse(t, ctrl) + + resolveCtx := NewContext(context.Background()) + resolveCtx.authorizer = authorizer + resolveCtx.rateLimiter = limiter + resolveCtx.RateLimitOptions = RateLimitOptions{Enable: true, IncludeStatsInResponseExtension: true} + resolveCtx.InlineArguments = []string{"user.filter", "@include.if"} + return res, resolveCtx, + `{"errors":[{"message":"Rate limit exceeded for Subgraph 'users' at Path 'query', Reason: rate limit exceeded."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"Failed to render Fetch Input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"Failed to render Fetch Input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"rateLimit":{"Policy":"policy","Allowed":0,"Used":1},"inlineArguments":{"count":2,"arguments":["user.filter","@include.if"]}}}`, + func(t *testing.T) {} + })) } From 9dba9d66de5596eba3021f1ebacd85d41dd1904d Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:12:36 +0300 Subject: [PATCH 06/21] fix: do not charge children of null-parents (#1574) This fix includes basic and often used cases, but it does not include some combinations of abstract fields and fragments. For those I have included the test cases (disabled right now). I have simplified how parents are used in calculations. I have added children multiplier to distiungish them from the field cost which is always multiplied even when null was returned on that field. This solution can be sophisticated further if there is a need for it. Potentially, I could take different approach of walking the response from subgraphs and match them with Cost Tree. That would remove many heuristics and exceptions and make things much more simple. But that would happen in a separate PR. `typeNameStats` is populated only when CC is enabled. --- .../engine/execution_engine_cost_test.go | 934 +++++++++++++++++- execution/engine/execution_engine_test.go | 1 + execution/graphql/request.go | 22 +- v2/pkg/engine/plan/cost.go | 168 ++-- v2/pkg/engine/resolve/resolvable.go | 28 +- 5 files changed, 1064 insertions(+), 89 deletions(-) diff --git a/execution/engine/execution_engine_cost_test.go b/execution/engine/execution_engine_cost_test.go index e62041a9c6..c14f8f66f8 100644 --- a/execution/engine/execution_engine_cost_test.go +++ b/execution/engine/execution_engine_cost_test.go @@ -244,7 +244,8 @@ func TestExecutionEngine_Cost(t *testing.T) { }, expectedResponse: `{"data":{"hero":{"name":"Luke Skywalker","height":"12"}}}`, expectedEstimatedCost: intPtr(22), // Query.hero (2) + Human.height (3) + Droid.name (17=max(7, 17)) - expectedActualCost: intPtr(22), + // hero resolved to Human: the interface-selected name is billed at Human.name, not max. + expectedActualCost: intPtr(12), // Query.hero (2) + Human.height (3) + Human.name (7) }, computeCosts(), )) @@ -406,8 +407,8 @@ func TestExecutionEngine_Cost(t *testing.T) { }, expectedResponse: `{"data":{"hero":{"name":"Luke Skywalker"}}}`, expectedEstimatedCost: intPtr(30), // Query.Human (13) + Droid.name (17=max(7, 17)) - // name is interface so the actual cost is taken as max - expectedActualCost: intPtr(30), + // name is selected on the interface; hero resolved to Human, so its actual weight is Human.name. + expectedActualCost: intPtr(20), // Human (13) + Human.name (7) }, computeCosts(), )) @@ -573,7 +574,7 @@ func TestExecutionEngine_Cost(t *testing.T) { }, expectedResponse: `{"data":{"hero":{"friends":[{"name":"Luke Skywalker","height":"12"},{"name":"R2DO","primaryFunction":"joke"}]}}}`, expectedEstimatedCost: intPtr(147), // hero(max(7,5))+ 20 * (4+max(2, 2+1)) - expectedActualCost: intPtr(20), // hero(7) + 2 * (4+0.5*(2+2+1)) + expectedActualCost: intPtr(18), // 7 + 2 * (3+0.5*(2+2+1)) }, computeCosts(), )) @@ -785,7 +786,9 @@ func TestExecutionEngine_Cost(t *testing.T) { // Character type: max(Human=2, Droid=3) = 3 // name: max(Human.name=3, Droid.name=5) = 5 expectedEstimatedCost: intPtr(55), // 2 + 1*(5 + 6*(3 + 1*5)) - expectedActualCost: intPtr(15), // 2 + 1*(5 + 1*(3 + 1*5)) + // hero returned __typename Human, so its name is billed at Human.name (3). + // friends items carry no __typename, so their type weight and name keep the max (3, 5). + expectedActualCost: intPtr(13), // 2 + 1*(3 + 1*(3 + 1*5)) }, computeCosts(), )) @@ -846,7 +849,8 @@ func TestExecutionEngine_Cost(t *testing.T) { // name: max(Human.name=3, Droid.name=5) = 5 // Total: 2 + 5 + 6 * (3 + 5) expectedEstimatedCost: intPtr(55), - expectedActualCost: intPtr(12), // 2 + 1*5 + 1*(2 + 1*3) + // Both hero and the friends item resolved to Human: both names billed at Human.name (3). + expectedActualCost: intPtr(10), // 2 + 1*3 + 1*(2 + 1*3) }, computeCosts(), )) @@ -6806,4 +6810,922 @@ func TestExecutionEngine_Cost(t *testing.T) { )) }) }) + + t.Run("cost of children with parent as null", func(t *testing.T) { + // It verifies that child fields nested under a nullable object field + // are not charged when that object resolves to null at runtime. + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { + items(ids: [ID!]!): [Item] + item: Item + } + type Item { + id: ID! + parent_item: Item + group: Group + board: Board + } + type Group { id: ID! } + type Board { id: ID! } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"items", "item"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "Item", FieldNames: []string{"id", "parent_item", "group", "board"}}, + {TypeName: "Group", FieldNames: []string{"id"}}, + {TypeName: "Board", FieldNames: []string{"id"}}, + } + itemsFieldConfig := []plan.FieldConfiguration{ + { + TypeName: "Query", FieldName: "items", + Arguments: []plan.ArgumentConfiguration{ + { + Name: "ids", + SourceType: plan.FieldArgumentSource, + RenderConfig: plan.RenderArgumentAsGraphQLValue, + }, + }, + }, + } + + makeCase := func(query, response, expectedResponse string, costConfig *plan.DataSourceCostConfig, estimatedCost, actualCost int) ExecutionEngineTestCase { + if expectedResponse == "" { + expectedResponse = response + } + return ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{Query: query} + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: response, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{RootNodes: rootNodes, ChildNodes: childNodes, CostConfig: costConfig}, + customConfig, + ), + }, + fields: itemsFieldConfig, + expectedResponse: expectedResponse, + expectedEstimatedCost: intPtr(estimatedCost), + expectedActualCost: intPtr(actualCost), + } + } + t.Run("with child fields group and board", runWithoutError( + makeCase(`query getItems { + items(ids: ["1", "2", "3"]) { + id + parent_item { + id + group { id } + board { id } + } + } + }`, + `{"data":{"items":[`+ + `{"id":"1","parent_item":null},`+ + `{"id":"2","parent_item":{"id":"1","group":{"id":"1"},"board":{"id":"2"}}},`+ + `{"id":"3","parent_item":null}]}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5, "Board": 7}, + }, + 160, // 10 * (2 + (2 + 5 + 7)) + 24, // 3 * (2 + (2 + 0.33*(5 + 7))) + ), + computeCosts(), + )) + t.Run("without child fields group and board", runWithoutError( + makeCase(`query getItems { + items(ids: ["1", "2", "3"]) { + id + parent_item { + id + } + } + }`, + `{"data":{"items":[`+ + `{"id":"1","parent_item":null},`+ + `{"id":"2","parent_item":{"id":"1"}},`+ + `{"id":"3","parent_item":null}]}}`, + // group/board are not selected, so the engine strips them from the response. + `{"data":{"items":[`+ + `{"id":"1","parent_item":null},`+ + `{"id":"2","parent_item":{"id":"1"}},`+ + `{"id":"3","parent_item":null}]}}`, + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5, "Board": 7}, + }, + 40, // 10 * (2 + (2)) + 12, // 3 * (2 + (2)) + ), + computeCosts(), + )) + t.Run("weighted field two levels under null parent is charged once", runWithoutError( + makeCase(`query getItems { + items(ids: ["1", "2", "3"]) { + id + parent_item { + id + group { id } + } + } + }`, + `{"data":{"items":[`+ + `{"id":"1","parent_item":null},`+ + `{"id":"2","parent_item":{"id":"1","group":{"id":"1"}}},`+ + `{"id":"3","parent_item":null}]}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Group", FieldName: "id"}: {HasWeight: true, Weight: 30}, + }, + }, + 390, // 10 * (2 + (2 + (5 + 30))) + 47, // 3 * (2 + (2 + 0.33*(5 + 30))) + ), + computeCosts(), + )) + + t.Run("weighted field two levels under not-null parents is not charged", runWithoutError( + makeCase(`query getItems { + items(ids: ["1", "2", "3"]) { + id + parent_item { + id + group { id } + } + } + }`, + `{"data":{"items":[`+ + `{"id":"1","parent_item":{"id":"1","group":null}},`+ + `{"id":"2","parent_item":{"id":"2","group":null}},`+ + `{"id":"3","parent_item":{"id":"3","group":null}}]}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Group", FieldName: "id"}: {HasWeight: true, Weight: 30}, + }, + }, + 390, // 10 * (2 + (2 + 5)) + 27, // 3 * (2 + (2 + 5)) + ), + computeCosts(), + )) + t.Run("weighted field two levels under parent that is always null is never charged", runWithoutError( + makeCase(`query getItems { + items(ids: ["1", "2", "3"]) { + id + parent_item { + id + group { id } + } + } + }`, + `{"data":{"items":[`+ + `{"id":"1","parent_item":null},`+ + `{"id":"2","parent_item":null},`+ + `{"id":"3","parent_item":null}]}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Group", FieldName: "id"}: {HasWeight: true, Weight: 30}, + }, + }, + 390, // 10 * (2 + (2 + (5 + 30))) + 12, // 3 * (2 + 2 + 0*(5 + 30)) + ), + computeCosts(), + )) + t.Run("children of null top-level object are not charged", runWithoutError( + makeCase(`query getItem { + item { + id + group { id } + } + }`, + `{"data":{"item":null}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Group", FieldName: "id"}: {HasWeight: true, Weight: 30}, + }, + }, + 37, // 2 + (5 + 30) + 2, // 2 + 0*(5 + 30) + ), + computeCosts(), + )) + t.Run("children of non-null top-level object are charged", runWithoutError( + makeCase(`query getItem { + item { + id + group { id } + } + }`, + `{"data":{"item":{"id":"1","group":{"id":"g1"}}}}`, + "", + &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 2, "Group": 5}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Group", FieldName: "id"}: {HasWeight: true, Weight: 30}, + }, + }, + 37, // 2 + (5 + 30) + 37, // 2 + 1*(5 + 30) + ), + computeCosts(), + )) + }) + + t.Run("a list nested under a partially-null object that is itself under a list", func(t *testing.T) { + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { users: [User] } + type User { id: ID! profile: Profile } + type Profile { id: ID! tags: [Tag] } + type Tag { id: ID! name: String } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"users"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "User", FieldNames: []string{"id", "profile"}}, + {TypeName: "Profile", FieldNames: []string{"id", "tags"}}, + {TypeName: "Tag", FieldNames: []string{"id", "name"}}, + } + + response := `{"data":{"users":[` + + `{"id":"1","profile":null},` + + `{"id":"2","profile":{"id":"p2","tags":[` + + `{"id":"t1","name":"a"},{"id":"t2","name":"b"},{"id":"t3","name":"c"}]}}]}}` + + t.Run("list under a partially-null object", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `query getUsers { + users { + id + profile { + id + tags { id name } + } + } + }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: response, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + ChildNodes: childNodes, + CostConfig: &plan.DataSourceCostConfig{ + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Tag", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + }, + }, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: response, + expectedEstimatedCost: intPtr(3120), // 10 * (1 + (1 + 10 * (1 + 30))) + expectedActualCost: intPtr(97), // 2 * (1 + (1 + 0.5 * (3 * (1 + 30)))) + }, + computeCosts(), + )) + }) + + t.Run("an abstract non-list field that is null for some elements of an enclosing list", func(t *testing.T) { + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { items: [Item] } + type Item { id: ID! hero: Character } + interface Character { id: ID! } + type Human implements Character { id: ID! name: String } + type Droid implements Character { id: ID! name: String } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"items"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "Item", FieldNames: []string{"id", "hero"}}, + {TypeName: "Character", FieldNames: []string{"id"}}, + {TypeName: "Human", FieldNames: []string{"id", "name"}}, + {TypeName: "Droid", FieldNames: []string{"id", "name"}}, + } + + sendResponse := `{"data":{"items":[` + + `{"id":"1","hero":null},` + + `{"id":"2","hero":{"__typename":"Human","id":"h1","name":"Luke"}},` + + `{"id":"3","hero":null}]}}` + + t.Run("abstract null object under list", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `query getItems { + items { + id + hero { ... on Human { name } } + } + }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: sendResponse, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + ChildNodes: childNodes, + CostConfig: &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 0, "Human": 0, "Droid": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Human", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + }, + }, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"items":[` + + `{"id":"1","hero":null},` + + `{"id":"2","hero":{"name":"Luke"}},` + + `{"id":"3","hero":null}]}}`, + expectedEstimatedCost: intPtr(300), // 10 * (1 * 30) + // Human.name is resolved exactly once (only 1 of 3 heroes is non-null) => 30. + expectedActualCost: intPtr(30), // 3 * (0.33 * 30) + }, + computeCosts(), + )) + + t.Run("abstract mixed types under list scales fragment by type count", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `query getItems { + items { + id + hero { + id + ... on Human { name } + } + } + }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: `{"data":{"items":[` + + `{"id":"1","hero":{"__typename":"Human","id":"h1","name":"Luke"}},` + + `{"id":"2","hero":{"__typename":"Human","id":"h2","name":"Han"}},` + + `{"id":"3","hero":{"__typename":"Droid","id":"d1"}}]}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + ChildNodes: childNodes, + CostConfig: &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 0, "Human": 0, "Droid": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Human", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + }, + }, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"items":[` + + `{"id":"1","hero":{"id":"h1","name":"Luke"}},` + + `{"id":"2","hero":{"id":"h2","name":"Han"}},` + + `{"id":"3","hero":{"id":"d1"}}]}}`, + expectedEstimatedCost: intPtr(300), // 10 * (1 * 30) + expectedActualCost: intPtr(60), // 3 * (0.67 * 30) + }, + computeCosts(), + )) + + t.Run("abstract mixed types with nulls under list", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `query getItems { + items { + id + hero { id ... on Human { name } } + } + }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: `{"data":{"items":[` + + `{"id":"1","hero":{"__typename":"Human","id":"h1","name":"Luke"}},` + + `{"id":"2","hero":{"__typename":"Human","id":"h2","name":"Han"}},` + + `{"id":"3","hero":{"__typename":"Droid","id":"d1"}},` + + `{"id":"4","hero":null}]}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + ChildNodes: childNodes, + CostConfig: &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 0, "Human": 0, "Droid": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Human", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + }, + }, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"items":[` + + `{"id":"1","hero":{"id":"h1","name":"Luke"}},` + + `{"id":"2","hero":{"id":"h2","name":"Han"}},` + + `{"id":"3","hero":{"id":"d1"}},` + + `{"id":"4","hero":null}]}}`, + expectedEstimatedCost: intPtr(300), // 10 * (1 * (1 * 30)) + expectedActualCost: intPtr(60), // 4 * (0.75 * (0.67 * 30)) + }, + computeCosts(), + )) + }) + + t.Run("interface-selected field without explicit weights keeps its type weight", func(t *testing.T) { + // pet is selected on the interface Character and has no explicit weight on any + // implementing type, so its weight is the returned type's default (Pet = 1). + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { heroes: [Character] } + interface Character { id: ID! pet: Pet } + type Human implements Character { id: ID! pet: Pet } + type Droid implements Character { id: ID! pet: Pet } + type Pet { id: ID! name: String } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"heroes"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "Character", FieldNames: []string{"id", "pet"}}, + {TypeName: "Human", FieldNames: []string{"id", "pet"}}, + {TypeName: "Droid", FieldNames: []string{"id", "pet"}}, + {TypeName: "Pet", FieldNames: []string{"id", "name"}}, + } + + response := `{"data":{"heroes":[` + + `{"__typename":"Human","pet":{"id":"p1","name":"a"}},` + + `{"__typename":"Droid","pet":{"id":"p2","name":"b"}}]}}` + + t.Run("under abstract list", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `{ heroes { pet { id name } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: response, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + ChildNodes: childNodes, + CostConfig: &plan.DataSourceCostConfig{ + Types: map[string]int{"Human": 0, "Droid": 0}, + }, + }, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"heroes":[{"pet":{"id":"p1","name":"a"}},{"pet":{"id":"p2","name":"b"}}]}}`, + expectedEstimatedCost: intPtr(10), // 10 * (0 + (Pet 1)) + expectedActualCost: intPtr(2), // 2 * (0 + (Pet 1)) + }, + computeCosts(), + )) + }) + + t.Run("interface field weights on an abstract object under a concrete list", func(t *testing.T) { + // name is selected on the interface Character and has a different weight per implementing type. + // In actual mode, each occurrence must be billed at the weight of the concrete type + // that was returned, not at the max implementing weight. + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { items: [Item] } + type Item { id: ID! hero: Character } + interface Character { id: ID! name: String } + type Human implements Character { id: ID! name: String } + type Droid implements Character { id: ID! name: String } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"items"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "Item", FieldNames: []string{"id", "hero"}}, + {TypeName: "Character", FieldNames: []string{"id", "name"}}, + {TypeName: "Human", FieldNames: []string{"id", "name"}}, + {TypeName: "Droid", FieldNames: []string{"id", "name"}}, + } + costConfig := &plan.DataSourceCostConfig{ + Types: map[string]int{"Item": 0, "Human": 0, "Droid": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Human", FieldName: "name"}: {HasWeight: true, Weight: 7}, + {TypeName: "Droid", FieldName: "name"}: {HasWeight: true, Weight: 17}, + }, + } + + t.Run("with typenames bills actual type weights", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `{ items { hero { name } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: `{"data":{"items":[` + + `{"hero":{"__typename":"Human","name":"Luke"}},` + + `{"hero":{"__typename":"Human","name":"Han"}},` + + `{"hero":{"__typename":"Droid","name":"R2D2"}}]}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{RootNodes: rootNodes, ChildNodes: childNodes, CostConfig: costConfig}, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"items":[` + + `{"hero":{"name":"Luke"}},` + + `{"hero":{"name":"Han"}},` + + `{"hero":{"name":"R2D2"}}]}}`, + expectedEstimatedCost: intPtr(170), // 10 * (0 + (0 + max(7, 17))) + // 2 Human heroes and 1 Droid hero: name billed per returned type. + expectedActualCost: intPtr(31), // 2*7 + 1*17 + }, + computeCosts(), + )) + + t.Run("without typenames keeps max weight", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `{ items { hero { name } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: `{"data":{"items":[` + + `{"hero":{"name":"Luke"}},` + + `{"hero":{"name":"Han"}},` + + `{"hero":{"name":"R2D2"}}]}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{RootNodes: rootNodes, ChildNodes: childNodes, CostConfig: costConfig}, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: `{"data":{"items":[` + + `{"hero":{"name":"Luke"}},` + + `{"hero":{"name":"Han"}},` + + `{"hero":{"name":"R2D2"}}]}}`, + expectedEstimatedCost: intPtr(170), // 10 * (0 + (0 + max(7, 17))) + // Subgraph returned no __typename for hero: no per-type info, keep the max. + expectedActualCost: intPtr(51), // 3 * 17 + }, + computeCosts(), + )) + + }) + + t.Run("fragment fields sharing a response path under an abstract list", func(t *testing.T) { + // Several cost-tree nodes resolve into the same response path when the same field + // is selected in multiple fragments. Runtime type stats are keyed by response path, + // so they aggregate occurrences across those nodes and cannot be attributed to a + // single node. The cases below document the correct actual costs. + t.Skip("not implemented yet") + t.Parallel() + + schema, err := graphql.NewSchemaFromString(` + type Query { heroes: [Character] } + interface Character { id: ID! } + type Human implements Character { id: ID! pet: Pet friends: [Friend] } + type Droid implements Character { id: ID! pet: Pet friends: [Friend] } + type Pet { id: ID! name: String toy: Toy } + type Toy { id: ID! name: String } + type Friend { id: ID! name: String } + `) + require.NoError(t, err) + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "GET", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, string(schema.RawSchema())), + }) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"heroes"}}, + } + childNodes := []plan.TypeField{ + {TypeName: "Character", FieldNames: []string{"id"}}, + {TypeName: "Human", FieldNames: []string{"id", "pet", "friends"}}, + {TypeName: "Droid", FieldNames: []string{"id", "pet", "friends"}}, + {TypeName: "Pet", FieldNames: []string{"id", "name", "toy"}}, + {TypeName: "Toy", FieldNames: []string{"id", "name"}}, + {TypeName: "Friend", FieldNames: []string{"id", "name"}}, + } + + makeCase := func(query string, costConfig *plan.DataSourceCostConfig, sendResponse, expectedResponse string, estimatedCost, actualCost int) ExecutionEngineTestCase { + return ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{Query: query} + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", expectedPath: "/", expectedBody: "", + sendResponseBody: sendResponse, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{RootNodes: rootNodes, ChildNodes: childNodes, CostConfig: costConfig}, + customConfig, + ), + }, + fields: []plan.FieldConfiguration{}, + expectedResponse: expectedResponse, + expectedEstimatedCost: intPtr(estimatedCost), + expectedActualCost: intPtr(actualCost), + } + } + + petNameCostConfig := &plan.DataSourceCostConfig{ + Types: map[string]int{"Human": 0, "Droid": 0, "Pet": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Pet", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + } + + // pet is selected in the Human fragment only and is null for one of the two Humans: + // its child name (weight 30) is resolved exactly once and must be billed once. + t.Run("null pet is not charged for its children", runWithoutError( + makeCase( + `{ + heroes { + ...on Human { + pet { name } + } + } + }`, + petNameCostConfig, + `{"data":{"heroes":[`+ + `{"__typename":"Human","pet":{"id":"p1","name":"a"}},`+ + `{"__typename":"Human","pet":null},`+ + `{"__typename":"Droid"}]}}`, + `{"data":{"heroes":[`+ + `{"pet":{"name":"a"}},`+ + `{"pet":null},`+ + `{}]}}`, + 300, // 10 * (0 + (0 + 30)) + // name is resolved once: pet is present for 1 of 2 Humans (3rd hero is a Droid). + 30, // 3 * (0.67 * (0.5 * 30)) + ), + computeCosts(), + )) + + // The same nullable pet is selected in BOTH fragments; the shared-path guard fires + // and disables the null-discount for both nodes, so children are charged at the full + // type-share even where pet was null. + // 2 Humans (one null pet) and 1 Droid with a pet => 2 names resolved in total. + t.Run("nullable object selected in both fragments", runWithoutError( + makeCase( + `{ + heroes { + ...on Human { + pet { name } + } + ...on Droid { + pet { name } + } + } + }`, + petNameCostConfig, + `{"data":{"heroes":[`+ + `{"__typename":"Human","pet":{"name":"a"}},`+ + `{"__typename":"Human","pet":null},`+ + `{"__typename":"Droid","pet":{"name":"b"}}]}}`, + `{"data":{"heroes":[`+ + `{"pet":{"name":"a"}},`+ + `{"pet":null},`+ + `{"pet":{"name":"b"}}]}}`, + 300, // 10 * (0 + max(30, 30)) + 60, // 2 names * 30 + ), + computeCosts(), + )) + + // A list field with the same response path in two fragments: the list-multiplier + // branch reads stats aggregated over both fragments and charges each node for the + // union of friends. + // 1 Human with 2 friends, 1 Droid with 1 friend => 3 names resolved in total. + t.Run("list field selected in both fragments", runWithoutError( + makeCase( + `{ + heroes { + ...on Human { + friends { name } + } + ...on Droid { + friends { name } + } + } + }`, + &plan.DataSourceCostConfig{ + Types: map[string]int{"Human": 0, "Droid": 0, "Friend": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Friend", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + }, + `{"data":{"heroes":[`+ + `{"__typename":"Human","friends":[{"name":"a"},{"name":"b"}]},`+ + `{"__typename":"Droid","friends":[{"name":"c"}]}]}}`, + `{"data":{"heroes":[`+ + `{"friends":[{"name":"a"},{"name":"b"}]},`+ + `{"friends":[{"name":"c"}]}]}}`, + 3000, // 10 heroes * 10 friends * 30 + 90, // 3 names * 30 + ), + computeCosts(), + )) + + // toy is nested one level deeper inside two fragments: the colliding pet nodes are + // siblings, but the toy nodes under them are not, so each toy node scales its + // children by the AVERAGE toy presence across both fragments instead of its own. + // Human's fragment selects the weighted toy.name; Droid's selects only toy.id (weight 0). + toyNameCostConfig := &plan.DataSourceCostConfig{ + Types: map[string]int{"Human": 0, "Droid": 0, "Pet": 0, "Toy": 0}, + Weights: map[plan.FieldCoordinate]*plan.FieldCost{ + {TypeName: "Toy", FieldName: "name"}: {HasWeight: true, Weight: 30}, + }, + } + descendantQuery := `{ + heroes { + ...on Human { + pet { toy { name } } + } + ...on Droid { + pet { toy { id } } + } + } + }` + + t.Run("descendant of fragments, weighted name never resolved", runWithoutError( + makeCase( + descendantQuery, + toyNameCostConfig, + `{"data":{"heroes":[`+ + `{"__typename":"Human","pet":{"toy":null}},`+ + `{"__typename":"Droid","pet":{"toy":{"id":"t1"}}}]}}`, + `{"data":{"heroes":[`+ + `{"pet":{"toy":null}},`+ + `{"pet":{"toy":{"id":"t1"}}}]}}`, + 300, // 10 * max(30, 0) + 0, // name never resolved + ), + computeCosts(), + )) + + t.Run("descendant of fragments, weighted name resolved once", runWithoutError( + makeCase( + descendantQuery, + toyNameCostConfig, + `{"data":{"heroes":[`+ + `{"__typename":"Human","pet":{"toy":{"name":"ball"}}},`+ + `{"__typename":"Droid","pet":{"toy":null}}]}}`, + `{"data":{"heroes":[`+ + `{"pet":{"toy":{"name":"ball"}}},`+ + `{"pet":{"toy":null}}]}}`, + 300, // 10 * max(30, 0) + 30, // name resolved once + ), + computeCosts(), + )) + }) } diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index 15b652da9d..42b3536d5b 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -104,6 +104,7 @@ func runExecutionTest(testCase ExecutionEngineTestCase, withError bool, expected PropagateFetchReasons: opts.propagateFetchReasons, ValidateRequiredExternalFields: opts.validateRequiredExternalFields, } + resolveOpts.ResolvableOptions.EnableCostControl = opts.computeCosts engine, err := NewExecutionEngine(ctx, abstractlogger.Noop{}, engineConf, resolveOpts) require.NoError(t, err) diff --git a/execution/graphql/request.go b/execution/graphql/request.go index 5f3ee9b10f..d14be5e3bc 100644 --- a/execution/graphql/request.go +++ b/execution/graphql/request.go @@ -195,13 +195,13 @@ func (r *Request) OperationType() (OperationType, error) { } func (r *Request) ComputeEstimatedCost(calc *plan.CostCalculator, vars resolve.VariablesView) { - if calc != nil { - r.estimatedCost = calc.EstimateCost(vars) - // Debugging of cost trees. Uncomment to debug. - // fmt.Println(calc.DebugPrint(vars, nil)) - } else { + if calc == nil { r.estimatedCost = 0 + return } + r.estimatedCost = calc.EstimateCost(vars) + // Debugging of cost trees. Uncomment to debug: + // fmt.Println(calc.DebugPrint(vars, nil)) } func (r *Request) EstimatedCost() int { @@ -209,13 +209,15 @@ func (r *Request) EstimatedCost() int { } func (r *Request) ComputeActualCost(calc *plan.CostCalculator, vars resolve.VariablesView, typeStats map[string]resolve.TypeNameStats) { - if calc != nil { - r.actualCost = calc.ActualCost(vars, typeStats) - // Debugging of cost trees. Uncomment to debug. - // fmt.Println(calc.DebugPrint(vars, typeStats)) - } else { + // typeStats is nil unless the resolver was built with ResolvableOptions.EnableCostControl; + // without runtime stats the actual cost cannot be computed. + if calc == nil || typeStats == nil { r.actualCost = 0 + return } + r.actualCost = calc.ActualCost(vars, typeStats) + // Debugging of cost trees. Uncomment to debug: + // fmt.Println(calc.DebugPrint(vars, typeStats)) } func (r *Request) ActualCost() int { diff --git a/v2/pkg/engine/plan/cost.go b/v2/pkg/engine/plan/cost.go index 3990df4164..484c2acc45 100644 --- a/v2/pkg/engine/plan/cost.go +++ b/v2/pkg/engine/plan/cost.go @@ -331,6 +331,41 @@ func (node *CostTreeNode) maxWeightImplementingField(config *DataSourceCostConfi return maxWeight } +// actualImplementingFieldWeight returns the runtime-weighted average of the per-implementing-type +// weights for an abstract field, based on which concrete types this node actually resolved to. +// +// It returns the estimatedMaxWeight when the response carries no per-type information +// or when no returned implementing type defines an explicit weight. +// +// Returned types whose implementing field has no explicit weight contribute zero to the average. +func (node *CostTreeNode) actualImplementingFieldWeight(config *DataSourceCostConfig, fieldName string, stats resolve.TypeNameStats, estimatedMaxWeight float64) float64 { + if stats.Size == 0 { + return estimatedMaxWeight + } + if len(stats.TypeNames) == 1 { + if _, onlyAbstract := stats.TypeNames[node.fieldTypeName]; onlyAbstract { + return estimatedMaxWeight + } + } + var sum float64 + weighted := false + for _, implTypeName := range node.implementingTypeNames { + count, returned := stats.TypeNames[implTypeName] + if !returned { + continue + } + fieldWeight := config.Weights[FieldCoordinate{implTypeName, fieldName}] + if fieldWeight != nil && fieldWeight.HasWeight { + weighted = true + sum += float64(fieldWeight.Weight * count) + } + } + if !weighted { + return estimatedMaxWeight + } + return sum / float64(stats.Size) +} + func (node *CostTreeNode) maxMultiplierImplementingField(config *DataSourceCostConfig, fieldName string, arguments map[string]ArgumentInfo, vars resolve.VariablesView, defaultListSize int) *FieldListSize { var maxMultiplier int var maxListSize *FieldListSize @@ -472,7 +507,11 @@ func (node *CostTreeNode) cost(input *costInput) float64 { // "A: [Obj] @cost(weight: 5)" means that the cost of the field is 5 for each object in the list. // "type Object @cost(weight: 5) { ... }" does exactly the same thing. // Weight defined on a field has priority over the weight defined on a type. - cost += (childrenCost + nodeCost.field) * nodeCost.multiplier + // + // The field's own weight scales with multiplier, while children scale with + // childMultiplier. These are equal except for a non-list object that resolved to + // null some/all of the time: we still charge the field but not its absent children. + cost += nodeCost.field*nodeCost.multiplier + childrenCost*nodeCost.childMultiplier if cost < 0 { cost = 0 } @@ -535,6 +574,10 @@ type costNodeResult struct { args int directives int multiplier float64 + + // childMultiplier scales the cost of this node's children. It is normally equal to multiplier, + // but can differ for a non-list object field that resolves to null part of the time. + childMultiplier float64 } // setDefaultMultiplier enforces multiplier=1 for non-list fields including the root node. @@ -545,6 +588,10 @@ func (r *costNodeResult) setDefaultMultiplier(node *CostTreeNode) { if r.multiplier == undefinedMultiplier { r.multiplier = 0 } + // By default, children scale exactly like the node itself. + if r.childMultiplier == undefinedMultiplier { + r.childMultiplier = r.multiplier + } } // costsAndMultiplier returns the cost values for a node based on its data sources. @@ -561,13 +608,14 @@ func (r *costNodeResult) setDefaultMultiplier(node *CostTreeNode) { // Also, it picks the maximum field weight of implementing types and then // the maximum among slicing arguments. func (node *CostTreeNode) costsAndMultiplier(input *costInput) (nodeCost costNodeResult) { + nodeCost.multiplier = undefinedMultiplier + nodeCost.childMultiplier = undefinedMultiplier if len(node.dataSourceHashes) == 0 { // no data source is responsible for this field return } parent := node.parent - nodeCost.multiplier = undefinedMultiplier for _, dsHash := range node.dataSourceHashes { dsCostConfig, ok := input.configs[dsHash] @@ -582,16 +630,16 @@ func (node *CostTreeNode) costsAndMultiplier(input *costInput) (nodeCost costNod // the corresponding field on each concrete type implementing that interface, // either directly or indirectly through other interfaces. // - // Composition should not let interface fields have weights, so we assume that - // the enclosing type is concrete. - // Commented condition is a good check for that. Might be needed later: - // fieldWeight != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType + // fromImplementingTypes marks that fieldWeight was resolved from the implementing + // types of the enclosing interface/union rather than from the field itself. + fromImplementingTypes := false if node.isEnclosingTypeAbstract && parent.returnsAbstractType { // This field is part of the enclosing interface/union. // We look into implementing types and find the max-weighted field. // Found fieldWeight can be used for all the calculations. if !input.ignoreImplementingTypeWeights { fieldWeight = parent.maxWeightImplementingField(dsCostConfig, node.fieldCoords.FieldName) + fromImplementingTypes = fieldWeight != nil } // If this field has listSize defined, then do not look into implementing types. if input.isEstimation && listSize == nil && node.returnsListType { @@ -600,7 +648,11 @@ func (node *CostTreeNode) costsAndMultiplier(input *costInput) (nodeCost costNod } if fieldWeight != nil && fieldWeight.HasWeight { - nodeCost.field += float64(fieldWeight.Weight) + weight := float64(fieldWeight.Weight) + if fromImplementingTypes && !input.isEstimation { + weight = parent.actualImplementingFieldWeight(dsCostConfig, node.fieldCoords.FieldName, input.typeStats[parent.jsonPath], weight) + } + nodeCost.field += weight } else { // Use the weight of the type returned by this field switch { @@ -739,73 +791,60 @@ func (node *CostTreeNode) costsAndMultiplier(input *costInput) (nodeCost costNod return } - // This block adjusts the multiplier of this node in actual mode. - // If this node returns a list, then we need to divide its multiplier by the size of the - // ancestor's node returning a list. - // If this node is enclosed by a node returning an abstract type, then we need to downsize - // multiplier too. - var ancestorStats resolve.TypeNameStats - var ancestorNode *CostTreeNode - // Find the nearest enclosing list ancestor. - for p := node.parent; p != nil && p.fieldCoords != costTreeRootNodeCoords; p = p.parent { - if p.returnsListType { - ancestorNode = p - ancestorStats = input.typeStats[p.jsonPath] - break - } + // The block below adjusts the multiplier of the node in ACTUAL mode. + + if node.parent == nil { + return } + parentStats := input.typeStats[node.parent.jsonPath] + if node.parent.fieldCoords == costTreeRootNodeCoords && parentStats.Size == 0 { + parentStats.Size = 1 + } + if node.returnsListType { - // This node's multiplier is its own array size, averaged over the nearest enclosing list - // to avoid double-counting of nested lists. + // This node's multiplier is its own array size, averaged over its immediate parent's + // occurrence count to avoid double-counting. if nodeStats, ok := input.typeStats[node.jsonPath]; ok && nodeStats.Size != 0 { - enclosingSize := ancestorStats.Size - if enclosingSize <= 0 { - enclosingSize = 1 + parentSize := 1.0 + if parentStats.Size > 0 { + parentSize = float64(parentStats.Size) } - nodeCost.multiplier = float64(nodeStats.Size) / float64(enclosingSize) + nodeCost.multiplier = float64(nodeStats.Size) / parentSize } return } - // Non-list field. If it sits directly under an abstract list, narrow - // its multiplier by the share of elements that actually match this - // field's concrete type. The parent list's full-size multiplier will - // multiply this back up to the correct per-type count. - if ancestorNode == nil || node.parent != ancestorNode || !ancestorNode.returnsAbstractType || ancestorStats.Size == 0 { - return + // Non-list field. + + // For a concrete object field, scale its children by how often the object actually + // resolved non-null relative to its parent's occurrences. + // + // Fragment fields under an abstract parent are excluded: runtime stats are keyed by + // response path, so occurrences of the same field selected in other fragments are + // indistinguishable and the ratio would count them all. Their childMultiplier follows + // the type-share multiplier set below instead. + isFragmentField := node.parent.returnsAbstractType && !node.isEnclosingTypeAbstract + if !node.returnsSimpleType && !isFragmentField && parentStats.Size > 0 { + nodeStats := input.typeStats[node.jsonPath] + // The field's own weight is kept via multiplier while its children + // are charged only for the fraction of occurrences where the object was present. + nodeCost.childMultiplier = float64(nodeStats.Size) / float64(parentStats.Size) } - if node.isEnclosingTypeAbstract && nodeCost.field > 0 && !input.ignoreImplementingTypeWeights { - var weightedSum float64 - found := false - for _, implTypeName := range parent.implementingTypeNames { - count, typeNameFound := ancestorStats.TypeNames[implTypeName] - if !typeNameFound { - continue - } - found = true - for _, dsHash := range node.dataSourceHashes { - dsCostConfig, ok := input.configs[dsHash] - if !ok || dsCostConfig == nil { - continue - } - coords := FieldCoordinate{implTypeName, node.fieldCoords.FieldName} - fieldWeight := dsCostConfig.Weights[coords] - if fieldWeight != nil { - weightedSum += float64(fieldWeight.Weight * count) - } - } - } - if found { - nodeCost.multiplier = weightedSum / (nodeCost.field * float64(ancestorStats.Size)) - } + // If the field sits directly under a field resolving an abstract type (a list or a single object), + // narrow its multiplier by the share of parent occurrences that + // actually match this field's concrete type. + if !node.parent.returnsAbstractType || parentStats.Size == 0 { + return } + // Fields selected on the abstract type itself need no multiplier adjustment: their + // weight is already resolved per actual type distribution (see actualImplementingFieldWeight). if !node.isEnclosingTypeAbstract { - count, typeNameFound := ancestorStats.TypeNames[node.fieldCoords.TypeName] - if ancestorStats.Size == 0 || !typeNameFound { + count, typeNameFound := parentStats.TypeNames[node.fieldCoords.TypeName] + if !typeNameFound { nodeCost.multiplier = 0 } else { - nodeCost.multiplier = float64(count) / float64(ancestorStats.Size) + nodeCost.multiplier = float64(count) / float64(parentStats.Size) } } return @@ -1018,7 +1057,7 @@ func (node *CostTreeNode) debugPrint(sb *strings.Builder, input *costInput, dept } indent := strings.Repeat(" ", depth) - fmt.Fprintf(sb, "%s* %s", indent, node.fieldCoords) + fmt.Fprintf(sb, "%s· %s", indent, node.fieldCoords) if node.fieldTypeName != "" { fmt.Fprintf(sb, " : %s", node.fieldTypeName) @@ -1055,10 +1094,11 @@ func (node *CostTreeNode) debugPrint(sb *strings.Builder, input *costInput, dept nodeCost := node.costsAndMultiplier(input) nodeCost.setDefaultMultiplier(node) - fmt.Fprintf(sb, "%s multiplier = %.2f", indent, nodeCost.multiplier) - + fmt.Fprintf(sb, "%s mult = %.2f", indent, nodeCost.multiplier) fmt.Fprintf(sb, ", fieldCost = %.2f", nodeCost.field) - + if nodeCost.childMultiplier != nodeCost.multiplier { + fmt.Fprintf(sb, ", childMult = %.2f", nodeCost.childMultiplier) + } if nodeCost.args > 0 { fmt.Fprintf(sb, ", argsCost = %d", nodeCost.args) } diff --git a/v2/pkg/engine/resolve/resolvable.go b/v2/pkg/engine/resolve/resolvable.go index a8be43d210..44a96ebfd3 100644 --- a/v2/pkg/engine/resolve/resolvable.go +++ b/v2/pkg/engine/resolve/resolvable.go @@ -143,6 +143,7 @@ type Resolvable struct { // typeNameStats maps the JSON path to its accumulated array/object stats in the final response. // Used to compute the actual cost of the operation. + // Only populated when ResolvableOptions.EnableCostControl is true. typeNameStats map[string]TypeNameStats // subgraphExtensions holds the `extensions` objects collected from subgraph @@ -167,9 +168,8 @@ type Resolvable struct { } type TypeNameStats struct { - Size int // the Size of the resolved array/list. It is 1 for non-list objects. - TypeNames map[string]int // distribution of TypeNames in the array - actualListSizes map[string]int + Size int // the Size of the resolved array/list. It is 1 for non-list objects. + TypeNames map[string]int // distribution of TypeNames in the array } type ResolvableOptions struct { @@ -179,6 +179,9 @@ type ResolvableOptions struct { ApolloCompatibilityReplaceInvalidVarError bool AllowedSubgraphExtensions map[string]struct{} ExtensionForwardingAlgorithm ExtensionForwardingAlgorithm + + // EnableCostControl gates whether typeNameStats are computed during the walk. + EnableCostControl bool } type ExtensionForwardingAlgorithm string @@ -213,7 +216,6 @@ func NewResolvable(a arena.Arena, options ResolvableOptions) *Resolvable { authorizationAllow: make(map[uint64]struct{}), authorizationDeny: make(map[uint64]string), astjsonArena: a, - typeNameStats: make(map[string]TypeNameStats), } } @@ -251,10 +253,18 @@ func (r *Resolvable) Reset() { r.deferItemDataNull = false } +// initCostControl prepares typeNameStats collection for this walk when cost control is active. +func (r *Resolvable) initCostControl() { + if r.options.EnableCostControl && r.typeNameStats == nil { + r.typeNameStats = make(map[string]TypeNameStats) + } +} + func (r *Resolvable) Init(ctx *Context, initialData []byte, operationType ast.OperationType) (err error) { r.ctx = ctx r.operationType = operationType r.renameTypeNames = ctx.RenameTypeNames + r.initCostControl() r.data = astjson.ObjectValue(r.astjsonArena) // don't init errors! It will heavily increase memory usage r.errors = nil @@ -275,6 +285,7 @@ func (r *Resolvable) InitSubscription(ctx *Context, initialData []byte, postProc r.ctx = ctx r.operationType = ast.OperationTypeSubscription r.renameTypeNames = ctx.RenameTypeNames + r.initCostControl() // don't init errors! It will heavily increase memory usage r.errors = nil if initialData != nil { @@ -1301,7 +1312,7 @@ func (r *Resolvable) walkObject(obj *Object, parent *astjson.Value) (hasError bo } } - if !r.render() { + if !r.render() && r.options.EnableCostControl { r.recordObjectTypeStats(obj, typeName) // For Cost Control } @@ -1720,7 +1731,7 @@ func (r *Resolvable) walkArray(arr *Array, value *astjson.Value) bool { } values := value.GetArray() - if !r.render() { + if !r.render() && r.options.EnableCostControl { // Record arrays stats for Cost Control. pathKey := r.currentFieldPath() stats := r.typeNameStats[pathKey] @@ -1780,11 +1791,10 @@ func (r *Resolvable) walkArray(arr *Array, value *astjson.Value) bool { return false } -// recordObjectTypeStats records the runtime __typename of a single (non-array) object -// that resolves an abstract (interface/union) field. +// recordObjectTypeStats records the runtime __typename of a single (non-array) object. func (r *Resolvable) recordObjectTypeStats(obj *Object, typeName []byte) { // An array item Object has an empty Path - if len(obj.Path) == 0 || !obj.isAbstract() { + if len(obj.Path) == 0 { return } pathKey := r.currentFieldPath() From 145115f8c26da7ad5dbb8e20db657131dc140b98 Mon Sep 17 00:00:00 2001 From: "wundergraph-bot[bot]" <285992168+wundergraph-bot[bot]@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:16:38 +0300 Subject: [PATCH 07/21] chore(master): release 2.9.1 (#1578) :robot: I have created a release *beep* *boop* --- ## [2.9.1](https://github.com/wundergraph/graphql-go-tools/compare/v2.9.0...v2.9.1) (2026-07-07) ### Bug Fixes * do not charge children of null-parents ([#1574](https://github.com/wundergraph/graphql-go-tools/issues/1574)) ([cf436ec](https://github.com/wundergraph/graphql-go-tools/commit/cf436ec42bd6d5755429d93f8bcd5ea6ccb88be5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com> --- release-please-manifest.json | 2 +- v2/CHANGELOG.md | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/release-please-manifest.json b/release-please-manifest.json index 7f2dac6e89..14f74e3c0b 100644 --- a/release-please-manifest.json +++ b/release-please-manifest.json @@ -1,4 +1,4 @@ { - "v2": "2.9.0", + "v2": "2.9.1", "execution": "1.16.0" } diff --git a/v2/CHANGELOG.md b/v2/CHANGELOG.md index b62e22ab59..73ecce51f2 100644 --- a/v2/CHANGELOG.md +++ b/v2/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## [2.9.1](https://github.com/wundergraph/graphql-go-tools/compare/v2.9.0...v2.9.1) (2026-07-07) + + +### Bug Fixes + +* do not charge children of null-parents ([#1574](https://github.com/wundergraph/graphql-go-tools/issues/1574)) ([cf436ec](https://github.com/wundergraph/graphql-go-tools/commit/cf436ec42bd6d5755429d93f8bcd5ea6ccb88be5)) + ## [2.9.0](https://github.com/wundergraph/graphql-go-tools/compare/v2.8.0...v2.9.0) (2026-07-06) From 67bef6e7cd79e61ee648483d8a3aeda1a4ee03ac Mon Sep 17 00:00:00 2001 From: "wundergraph-bot[bot]" <285992168+wundergraph-bot[bot]@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:19:40 +0300 Subject: [PATCH 08/21] chore(master): release execution 1.17.0 (#1568) :robot: I have created a release *beep* *boop* --- ## [1.17.0](https://github.com/wundergraph/graphql-go-tools/compare/execution/v1.16.0...execution/v1.17.0) (2026-07-07) ### Features * add defer support part 4 ([#1547](https://github.com/wundergraph/graphql-go-tools/issues/1547)) ([8891a0e](https://github.com/wundergraph/graphql-go-tools/commit/8891a0e9e606a3b3055e671a48a9e7bbc5a928ea)) ### Bug Fixes * do not charge children of null-parents ([#1574](https://github.com/wundergraph/graphql-go-tools/issues/1574)) ([cf436ec](https://github.com/wundergraph/graphql-go-tools/commit/cf436ec42bd6d5755429d93f8bcd5ea6ccb88be5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com> --- execution/CHANGELOG.md | 12 ++++++++++++ release-please-manifest.json | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/execution/CHANGELOG.md b/execution/CHANGELOG.md index e823b12148..eb49994134 100644 --- a/execution/CHANGELOG.md +++ b/execution/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## [1.17.0](https://github.com/wundergraph/graphql-go-tools/compare/execution/v1.16.0...execution/v1.17.0) (2026-07-07) + + +### Features + +* add defer support part 4 ([#1547](https://github.com/wundergraph/graphql-go-tools/issues/1547)) ([8891a0e](https://github.com/wundergraph/graphql-go-tools/commit/8891a0e9e606a3b3055e671a48a9e7bbc5a928ea)) + + +### Bug Fixes + +* do not charge children of null-parents ([#1574](https://github.com/wundergraph/graphql-go-tools/issues/1574)) ([cf436ec](https://github.com/wundergraph/graphql-go-tools/commit/cf436ec42bd6d5755429d93f8bcd5ea6ccb88be5)) + ## [1.16.0](https://github.com/wundergraph/graphql-go-tools/compare/execution/v1.15.6...execution/v1.16.0) (2026-06-18) diff --git a/release-please-manifest.json b/release-please-manifest.json index 14f74e3c0b..e957b8364b 100644 --- a/release-please-manifest.json +++ b/release-please-manifest.json @@ -1,4 +1,4 @@ { "v2": "2.9.1", - "execution": "1.16.0" + "execution": "1.17.0" } From c28eb40eab8cb6c11a4a0c39da21a37552d107ba Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 17:44:57 +0530 Subject: [PATCH 09/21] fix: updates --- v2/pkg/engine/resolve/resolvable.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/v2/pkg/engine/resolve/resolvable.go b/v2/pkg/engine/resolve/resolvable.go index 44a96ebfd3..18957b6f42 100644 --- a/v2/pkg/engine/resolve/resolvable.go +++ b/v2/pkg/engine/resolve/resolvable.go @@ -1024,12 +1024,7 @@ func (r *Resolvable) printInlineArgumentsExtension() error { if i > 0 { r.printBytes(comma) } - // json.Marshal yields a correctly-escaped, quoted JSON string. - encoded, err := json.Marshal(name) - if err != nil { - return err - } - r.printBytes(encoded) + r.printBytes(strconv.AppendQuote(nil, name)) } r.printBytes(rBrack) From e4595720829bc08ec5cad743e925eaf10dc68ef4 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 7 Jul 2026 18:16:27 +0530 Subject: [PATCH 10/21] fix: err cleanup --- v2/pkg/engine/resolve/resolvable.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/v2/pkg/engine/resolve/resolvable.go b/v2/pkg/engine/resolve/resolvable.go index 18957b6f42..7e487cebd5 100644 --- a/v2/pkg/engine/resolve/resolvable.go +++ b/v2/pkg/engine/resolve/resolvable.go @@ -882,10 +882,7 @@ func (r *Resolvable) printExtensions(ctx context.Context, fetchTree *FetchTreeNo r.printBytes(comma) } writeComma = true - err := r.printInlineArgumentsExtension() - if err != nil { - return err - } + r.printInlineArgumentsExtension() } if r.ctx.TracingOptions.Enable && r.ctx.TracingOptions.IncludeTraceOutputInResponseExtensions { @@ -998,10 +995,7 @@ func getDefaultReservedExtensions() map[string]struct{} { } } -// printInlineArgumentsExtension renders the non-enforcing disallow-inline-arguments -// findings as `"inlineArguments":{"count":N,"arguments":["field.arg",...]}`. It is -// only called when r.ctx.InlineArguments is non-empty. -func (r *Resolvable) printInlineArgumentsExtension() error { +func (r *Resolvable) printInlineArgumentsExtension() { r.printBytes(quote) r.printBytes(literalInlineArguments) r.printBytes(quote) @@ -1029,7 +1023,6 @@ func (r *Resolvable) printInlineArgumentsExtension() error { r.printBytes(rBrack) r.printBytes(rBrace) - return r.printErr } func (r *Resolvable) hasExtensions() bool { From 8e7cb3f8577f7566312c2d6e56968c5715887e0f Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Mon, 13 Jul 2026 17:11:01 +0530 Subject: [PATCH 11/21] fix: renaming --- v2/pkg/astnormalization/inline_arguments.go | 14 +++++++------- v2/pkg/astnormalization/inline_arguments_test.go | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index 288ce6e572..4cf0a61fc5 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -36,20 +36,20 @@ type InlineArgumentsValidationOptions struct { } type InlineArgumentsValidator struct { - Options InlineArgumentsValidationOptions - Findings []InlineArgument - Disabled bool + Options InlineArgumentsValidationOptions + InlineArguments []InlineArgument + Disabled bool } -func (v *InlineArgumentsValidator) ClearFindings() { +func (v *InlineArgumentsValidator) ClearInlineArguments() { if v == nil { return } - v.Findings = v.Findings[:0] + v.InlineArguments = v.InlineArguments[:0] } func (v *InlineArgumentsValidator) HadInlineArguments() bool { - return len(v.Findings) > 0 + return len(v.InlineArguments) > 0 } // InlineArgumentsRule returns a prevalidation rule that flags every argument @@ -121,5 +121,5 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { } } - v.validator.Findings = append(v.validator.Findings, finding) + v.validator.InlineArguments = append(v.validator.InlineArguments, finding) } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index 5c4c75683a..e0ca03627a 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -139,13 +139,13 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { require.False(t, report.HasErrors(), "log-only mode must never error: %s", report.Error()) if len(tt.expected) == 0 { - assert.Empty(t, validator.Findings) + assert.Empty(t, validator.InlineArguments) return } - require.Len(t, validator.Findings, len(tt.expected)) - got := make([]InlineArgument, len(validator.Findings)) - for i, f := range validator.Findings { + require.Len(t, validator.InlineArguments, len(tt.expected)) + got := make([]InlineArgument, len(validator.InlineArguments)) + for i, f := range validator.InlineArguments { f.Position = tt.expected[i].Position // ignore position in this comparison got[i] = f } @@ -162,9 +162,9 @@ func TestInlineArgumentsRule_Position(t *testing.T) { Enforce: false, }) require.False(t, report.HasErrors()) - require.Len(t, validator.Findings, 1) + require.Len(t, validator.InlineArguments, 1) - pos := validator.Findings[0].Position + pos := validator.InlineArguments[0].Position assert.Equal(t, uint32(1), pos.LineStart) assert.Equal(t, uint32(30), pos.CharStart) } @@ -190,7 +190,7 @@ func TestInlineArgumentsRule_Enforce(t *testing.T) { // Enforce rejects on the first inline argument and stops the walk, so no // findings are collected. - assert.Empty(t, validator.Findings) + assert.Empty(t, validator.InlineArguments) // The rejection is a generic error: no per-argument location is attached. assert.Empty(t, extErr.Locations) From 0d395a0c6b9799453681602c1db8632cb8efa819 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Mon, 13 Jul 2026 17:57:41 +0530 Subject: [PATCH 12/21] fix: review comments --- v2/pkg/astnormalization/astnormalization.go | 41 +++++++++- v2/pkg/astnormalization/inline_arguments.go | 61 ++++++-------- .../astnormalization/inline_arguments_test.go | 79 +++++++++++-------- 3 files changed, 112 insertions(+), 69 deletions(-) diff --git a/v2/pkg/astnormalization/astnormalization.go b/v2/pkg/astnormalization/astnormalization.go index b096c2bdfc..4c816a1c84 100644 --- a/v2/pkg/astnormalization/astnormalization.go +++ b/v2/pkg/astnormalization/astnormalization.go @@ -109,6 +109,7 @@ type OperationNormalizer struct { removeOperationDefinitionsVisitor *removeOperationDefinitionsVisitor inlineDeferVisitor *deferExpandIntoInternalVisitor + inlineArgumentsVisitor *inlineArgumentsVisitor options options definitionNormalizer *DefinitionNormalizer @@ -155,6 +156,7 @@ type options struct { ignoreSkipInclude bool enableDefer bool prevalidationRules []func(walker *astvisitor.Walker) + inlineArgumentsValidation *InlineArgumentsValidationOptions } type Option func(options *options) @@ -213,6 +215,17 @@ func WithPrevalidationRules(rules ...func(walker *astvisitor.Walker)) Option { } } +// WithInlineArgumentsValidation enables detection of arguments whose values are +// supplied inline (as literals) instead of as variables. Findings are returned +// from NormalizeNamedOperationWithResult as a NormalizationResult. When opts.Enforce +// is set, normalization aborts on the first inline argument and surfaces the error +// via the report instead of collecting findings. +func WithInlineArgumentsValidation(opts InlineArgumentsValidationOptions) Option { + return func(options *options) { + options.inlineArgumentsValidation = &opts + } +} + func (o *OperationNormalizer) setupOperationWalkers() { o.operationWalkers = make([]walkerStage, 0, 9) @@ -240,6 +253,10 @@ func (o *OperationNormalizer) setupOperationWalkers() { } } + if o.options.inlineArgumentsValidation != nil { + o.inlineArgumentsVisitor = registerInlineArgumentsValidation(&directivesIncludeSkip, *o.options.inlineArgumentsValidation) + } + cleanup := astvisitor.NewWalkerWithID(8, "Cleanup") deduplicateFields(&cleanup) if o.options.enableDefer { @@ -384,12 +401,25 @@ func (o *OperationNormalizer) NormalizeOperation(operation, definition *ast.Docu } } -// NormalizeNamedOperation applies all registered rules to one specific named operation in the AST func (o *OperationNormalizer) NormalizeNamedOperation(operation, definition *ast.Document, operationName []byte, report *operationreport.Report) { + o.NormalizeNamedOperationWithResult(operation, definition, operationName, report, RunOptions{}) +} + +func (o *OperationNormalizer) NormalizeNamedOperationWithResult( + operation, definition *ast.Document, + operationName []byte, + report *operationreport.Report, + runOpts RunOptions, +) *NormalizationResult { + if o.inlineArgumentsVisitor != nil { + o.inlineArgumentsVisitor.disabled = runOpts.SkipInlineArguments + o.inlineArgumentsVisitor.result.InlineArguments = o.inlineArgumentsVisitor.result.InlineArguments[:0] + } + if o.options.normalizeDefinition { o.prepareDefinition(definition, report) if report.HasErrors() { - return + return nil } } @@ -403,7 +433,7 @@ func (o *OperationNormalizer) NormalizeNamedOperation(operation, definition *ast } o.operationWalkers[i].walker.Walk(operation, definition, report) if report.HasErrors() { - return + return nil } // NOTE: debug code - do not remove @@ -412,6 +442,11 @@ func (o *OperationNormalizer) NormalizeNamedOperation(operation, definition *ast // fmt.Println(printed) // fmt.Println("variables:", string(operation.Input.Variables)) } + + if o.inlineArgumentsVisitor != nil { + return &o.inlineArgumentsVisitor.result + } + return nil } type VariablesNormalizer struct { diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index 4cf0a61fc5..2e6fa86a92 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -35,47 +35,38 @@ type InlineArgumentsValidationOptions struct { StatusCode int } -type InlineArgumentsValidator struct { - Options InlineArgumentsValidationOptions +// NormalizationResult carries per-run outputs of normalization beyond report errors. +type NormalizationResult struct { InlineArguments []InlineArgument - Disabled bool } -func (v *InlineArgumentsValidator) ClearInlineArguments() { - if v == nil { - return - } - v.InlineArguments = v.InlineArguments[:0] +// RunOptions are per-call inputs to a normalization run. +type RunOptions struct { + SkipInlineArguments bool } -func (v *InlineArgumentsValidator) HadInlineArguments() bool { - return len(v.InlineArguments) > 0 -} - -// InlineArgumentsRule returns a prevalidation rule that flags every argument -// whose value is an inline literal instead of a variable, in any context: field -// arguments, directive arguments (@skip/@include and any custom directive), and -// introspection-field arguments. Register it via WithPrevalidationRules; results -// land on the given validator. -// -// Variable-definition default values (e.g. `$x: Int = 5`) are naturally excluded -// — they are not arguments and are never visited as one. -func InlineArgumentsRule(validator *InlineArgumentsValidator) func(walker *astvisitor.Walker) { - return func(walker *astvisitor.Walker) { - visitor := &inlineArgumentsVisitor{ - Walker: walker, - validator: validator, - } - walker.RegisterEnterDocumentVisitor(visitor) - walker.RegisterEnterArgumentVisitor(visitor) +func registerInlineArgumentsValidation(walker *astvisitor.Walker, opts InlineArgumentsValidationOptions) *inlineArgumentsVisitor { + visitor := &inlineArgumentsVisitor{ + Walker: walker, + opts: opts, } + walker.RegisterEnterDocumentVisitor(visitor) + walker.RegisterEnterArgumentVisitor(visitor) + return visitor } type inlineArgumentsVisitor struct { *astvisitor.Walker operation, definition *ast.Document - validator *InlineArgumentsValidator + opts InlineArgumentsValidationOptions + + // disabled is set per run (see RunOptions.SkipInlineArguments) to exempt this + // operation from detection/enforcement. + disabled bool + // result accumulates the findings for the current run. Reset by the normalizer + // at the start of each run. + result NormalizationResult } func (v *inlineArgumentsVisitor) EnterDocument(operation, definition *ast.Document) { @@ -84,7 +75,7 @@ func (v *inlineArgumentsVisitor) EnterDocument(operation, definition *ast.Docume } func (v *inlineArgumentsVisitor) EnterArgument(ref int) { - if v.validator.Disabled { + if v.disabled { return } valueKind := v.operation.Arguments[ref].Value.Kind @@ -92,14 +83,14 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { return } - if v.validator.Options.Enforce { + if v.opts.Enforce { // Reject on the first inline argument and stop the walk. A single generic // error is enough to signal that the operation is non-compliant; we don't // name the argument or point at its location. v.StopWithExternalErr(operationreport.ExternalError{ - Message: v.validator.Options.ErrorMessage, - ExtensionCode: v.validator.Options.ErrorCode, - StatusCode: v.validator.Options.StatusCode, + Message: v.opts.ErrorMessage, + ExtensionCode: v.opts.ErrorCode, + StatusCode: v.opts.StatusCode, }) return } @@ -121,5 +112,5 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { } } - v.validator.InlineArguments = append(v.validator.InlineArguments, finding) + v.result.InlineArguments = append(v.result.InlineArguments, finding) } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index e0ca03627a..50557eb005 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -27,7 +27,12 @@ const inlineArgumentsTestSchema = ` input Filter { a: Int } ` -func runInlineArgumentsRule(t *testing.T, operation string, opts InlineArgumentsValidationOptions) (*InlineArgumentsValidator, *operationreport.Report) { +func runInlineArgumentsRule(t *testing.T, operation string, opts InlineArgumentsValidationOptions) (*NormalizationResult, *operationreport.Report) { + t.Helper() + return runInlineArgumentsRuleWithRunOpts(t, operation, opts, RunOptions{}) +} + +func runInlineArgumentsRuleWithRunOpts(t *testing.T, operation string, opts InlineArgumentsValidationOptions, runOpts RunOptions) (*NormalizationResult, *operationreport.Report) { t.Helper() definitionDocument := unsafeparser.ParseGraphqlDocumentString(inlineArgumentsTestSchema) @@ -36,11 +41,10 @@ func runInlineArgumentsRule(t *testing.T, operation string, opts InlineArguments operationDocument := unsafeparser.ParseGraphqlDocumentString(operation) report := &operationreport.Report{} - validator := &InlineArgumentsValidator{Options: opts} - normalizer := NewWithOpts(WithPrevalidationRules(InlineArgumentsRule(validator))) - normalizer.NormalizeOperation(&operationDocument, &definitionDocument, report) + normalizer := NewWithOpts(WithInlineArgumentsValidation(opts)) + result := normalizer.NormalizeNamedOperationWithResult(&operationDocument, &definitionDocument, nil, report, runOpts) - return validator, report + return result, report } func TestInlineArgumentsRule_Detection(t *testing.T) { @@ -135,17 +139,18 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - validator, report := runInlineArgumentsRule(t, tt.operation, InlineArgumentsValidationOptions{Enforce: false}) + result, report := runInlineArgumentsRule(t, tt.operation, InlineArgumentsValidationOptions{Enforce: false}) require.False(t, report.HasErrors(), "log-only mode must never error: %s", report.Error()) + require.NotNil(t, result) if len(tt.expected) == 0 { - assert.Empty(t, validator.InlineArguments) + assert.Empty(t, result.InlineArguments) return } - require.Len(t, validator.InlineArguments, len(tt.expected)) - got := make([]InlineArgument, len(validator.InlineArguments)) - for i, f := range validator.InlineArguments { + require.Len(t, result.InlineArguments, len(tt.expected)) + got := make([]InlineArgument, len(result.InlineArguments)) + for i, f := range result.InlineArguments { f.Position = tt.expected[i].Position // ignore position in this comparison got[i] = f } @@ -158,20 +163,21 @@ func TestInlineArgumentsRule_Position(t *testing.T) { // The reported position points at the argument in the operation as parsed. // `userId` starts at column 30 on line 1 of the operation below. operation := `query GetUserById { userById(userId: "12345") { loginName } }` - validator, report := runInlineArgumentsRule(t, operation, InlineArgumentsValidationOptions{ + result, report := runInlineArgumentsRule(t, operation, InlineArgumentsValidationOptions{ Enforce: false, }) require.False(t, report.HasErrors()) - require.Len(t, validator.InlineArguments, 1) + require.NotNil(t, result) + require.Len(t, result.InlineArguments, 1) - pos := validator.InlineArguments[0].Position + pos := result.InlineArguments[0].Position assert.Equal(t, uint32(1), pos.LineStart) assert.Equal(t, uint32(30), pos.CharStart) } func TestInlineArgumentsRule_Enforce(t *testing.T) { t.Run("stops at the first inline argument and reports a typed error", func(t *testing.T) { - validator, report := runInlineArgumentsRule(t, + result, report := runInlineArgumentsRule(t, `query { userById(userId: "12345") { loginName } field(order: ASC) }`, InlineArgumentsValidationOptions{ Enforce: true, @@ -188,36 +194,47 @@ func TestInlineArgumentsRule_Enforce(t *testing.T) { assert.Equal(t, "INLINE_ARGUMENT_VALUES_NOT_ALLOWED", extErr.ExtensionCode) assert.Equal(t, 400, extErr.StatusCode) - // Enforce rejects on the first inline argument and stops the walk, so no - // findings are collected. - assert.Empty(t, validator.InlineArguments) + // Enforce rejects on the first inline argument and stops the walk, so + // normalization fails and no result is returned. + assert.Nil(t, result) // The rejection is a generic error: no per-argument location is attached. assert.Empty(t, extErr.Locations) }) t.Run("compliant operation passes enforce mode", func(t *testing.T) { - validator, report := runInlineArgumentsRule(t, + result, report := runInlineArgumentsRule(t, `query q($userId: ID!) { userById(userId: $userId) { loginName } }`, InlineArgumentsValidationOptions{Enforce: true, ErrorMessage: "nope", ErrorCode: "CODE", StatusCode: 400}, ) require.False(t, report.HasErrors(), "compliant operation must not error: %s", report.Error()) - assert.False(t, validator.HadInlineArguments()) + require.NotNil(t, result) + assert.Empty(t, result.InlineArguments) }) - t.Run("disabled validator records nothing", func(t *testing.T) { - definitionDocument := unsafeparser.ParseGraphqlDocumentString(inlineArgumentsTestSchema) - require.NoError(t, asttransform.MergeDefinitionWithBaseSchema(&definitionDocument)) - operationDocument := unsafeparser.ParseGraphqlDocumentString(`query { userById(userId: "12345") { loginName } }`) - report := &operationreport.Report{} - - validator := &InlineArgumentsValidator{Options: InlineArgumentsValidationOptions{Enforce: true, ErrorMessage: "x", ErrorCode: "C", StatusCode: 400}} - validator.Disabled = true - - normalizer := NewWithOpts(WithPrevalidationRules(InlineArgumentsRule(validator))) - normalizer.NormalizeOperation(&operationDocument, &definitionDocument, report) + t.Run("SkipInlineArguments records nothing and does not enforce", func(t *testing.T) { + result, report := runInlineArgumentsRuleWithRunOpts(t, + `query { userById(userId: "12345") { loginName } }`, + InlineArgumentsValidationOptions{Enforce: true, ErrorMessage: "x", ErrorCode: "C", StatusCode: 400}, + RunOptions{SkipInlineArguments: true}, + ) require.False(t, report.HasErrors()) - assert.False(t, validator.HadInlineArguments()) + require.NotNil(t, result) + assert.Empty(t, result.InlineArguments) }) } + +func TestInlineArgumentsRule_OptionOffReturnsNil(t *testing.T) { + definitionDocument := unsafeparser.ParseGraphqlDocumentString(inlineArgumentsTestSchema) + require.NoError(t, asttransform.MergeDefinitionWithBaseSchema(&definitionDocument)) + operationDocument := unsafeparser.ParseGraphqlDocumentString(`query { userById(userId: "12345") { loginName } }`) + report := &operationreport.Report{} + + // No WithInlineArgumentsValidation option: there is no result to produce. + normalizer := NewWithOpts() + result := normalizer.NormalizeNamedOperationWithResult(&operationDocument, &definitionDocument, nil, report, RunOptions{}) + + require.False(t, report.HasErrors()) + assert.Nil(t, result) +} From 137aad6989191cf8443d3ead22409d8d937cf4e7 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 14 Jul 2026 12:55:06 +0530 Subject: [PATCH 13/21] fix: connectrpc resolving --- .../engine/datasource/graphql_datasource/graphql_datasource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 8c3bb3411d..515e52cf1b 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -366,7 +366,7 @@ func (p *Planner[T]) ConfigureFetch() resolve.FetchConfiguration { return resolve.FetchConfiguration{} } - if p.rpcTransport == nil { + if p.rpcTransport == nil && !p.config.grpc.Disabled { p.stopWithError(errors.WithStack(errors.New("grpc / connect configuration requires an rpc transport"))) return resolve.FetchConfiguration{} } From 4db647f0ef4998a66901c1e7f262d6ec7cfa801d Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 14 Jul 2026 15:40:31 +0530 Subject: [PATCH 14/21] fix: make router start --- .../graphql_datasource/graphql_datasource.go | 4 ---- .../datasource/grpc_datasource/grpc_datasource.go | 7 ++++++- .../grpc_datasource/grpc_datasource_test.go | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 515e52cf1b..c7800180c2 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -366,10 +366,6 @@ func (p *Planner[T]) ConfigureFetch() resolve.FetchConfiguration { return resolve.FetchConfiguration{} } - if p.rpcTransport == nil && !p.config.grpc.Disabled { - p.stopWithError(errors.WithStack(errors.New("grpc / connect configuration requires an rpc transport"))) - return resolve.FetchConfiguration{} - } dataSource, err = grpcdatasource.NewDataSource(p.rpcTransport, grpcdatasource.DataSourceConfig{ Operation: &opDocument, diff --git a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go index 276e612092..40cf98b6b8 100644 --- a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go +++ b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go @@ -113,7 +113,12 @@ func (d *DataSource) Load(ctx context.Context, headers http.Header, input []byte builder := newJSONBuilder(item.Arena, d.mapping, variables) if d.disabled { - return builder.writeErrorBytes(fmt.Errorf("gRPC datasource needs to be enabled to be used")), nil + return builder.writeErrorBytes(fmt.Errorf("gRPC / connect datasource needs to be enabled to be used")), nil + } + + // If the transport is nil we will return the following error message instead + if d.transport == nil { + return builder.writeErrorBytes(fmt.Errorf("gRPC / connect configuration requires an rpc transport")), nil } // convert headers to grpc metadata and attach to ctx diff --git a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go index 61c2b3d139..57bb9f43ba 100644 --- a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go +++ b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go @@ -20,6 +20,8 @@ import ( protoref "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/dynamicpb" + "github.com/wundergraph/go-arena" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astparser" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/plan" "github.com/wundergraph/graphql-go-tools/v2/pkg/grpctest" @@ -222,6 +224,18 @@ func Test_DataSource_Load(t *testing.T) { require.NoError(t, err) } +func Test_DataSource_Load_NilTransport(t *testing.T) { + ds := &DataSource{pool: arena.NewArenaPool(), disabled: false} + + out, err := ds.Load(context.Background(), nil, []byte(`{}`)) + require.NoError(t, err) + + require.Equal(t, + `{"errors":[{"message":"gRPC / connect configuration requires an rpc transport","extensions":{"code":"Internal"}}]}`, + string(out), + ) +} + // Test_DataSource_Load_WithMockService tests the datasource.Load method with an actual gRPC server // TODO update this test to not use mappings anc expect no response func Test_DataSource_Load_WithMockService(t *testing.T) { From 12a89e3bb05584533b5c5e53ea99fa7f494aa0a2 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 14 Jul 2026 16:23:40 +0530 Subject: [PATCH 15/21] fix: review comments --- .../datasource/graphql_datasource/graphql_datasource.go | 4 ++++ v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index c7800180c2..8c3bb3411d 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -366,6 +366,10 @@ func (p *Planner[T]) ConfigureFetch() resolve.FetchConfiguration { return resolve.FetchConfiguration{} } + if p.rpcTransport == nil { + p.stopWithError(errors.WithStack(errors.New("grpc / connect configuration requires an rpc transport"))) + return resolve.FetchConfiguration{} + } dataSource, err = grpcdatasource.NewDataSource(p.rpcTransport, grpcdatasource.DataSourceConfig{ Operation: &opDocument, diff --git a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go index 40cf98b6b8..4139904f84 100644 --- a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go +++ b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go @@ -118,7 +118,7 @@ func (d *DataSource) Load(ctx context.Context, headers http.Header, input []byte // If the transport is nil we will return the following error message instead if d.transport == nil { - return builder.writeErrorBytes(fmt.Errorf("gRPC / connect configuration requires an rpc transport")), nil + return nil, fmt.Errorf("gRPC / connect configuration requires an rpc transport") } // convert headers to grpc metadata and attach to ctx From 2d907401dbf4e858dbeb33bcf5d632f2f238b5ff Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 14 Jul 2026 16:32:36 +0530 Subject: [PATCH 16/21] fix: tests --- .../datasource/grpc_datasource/grpc_datasource_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go index 57bb9f43ba..9d2f4e6e4a 100644 --- a/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go +++ b/v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go @@ -228,12 +228,8 @@ func Test_DataSource_Load_NilTransport(t *testing.T) { ds := &DataSource{pool: arena.NewArenaPool(), disabled: false} out, err := ds.Load(context.Background(), nil, []byte(`{}`)) - require.NoError(t, err) - - require.Equal(t, - `{"errors":[{"message":"gRPC / connect configuration requires an rpc transport","extensions":{"code":"Internal"}}]}`, - string(out), - ) + require.EqualError(t, err, "gRPC / connect configuration requires an rpc transport") + require.Nil(t, out) } // Test_DataSource_Load_WithMockService tests the datasource.Load method with an actual gRPC server From ad52548aa2931c0cfa6b8edebe259eb65a76586f Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Tue, 14 Jul 2026 17:06:21 +0530 Subject: [PATCH 17/21] fix: revert --- .../datasource/graphql_datasource/graphql_datasource.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 8c3bb3411d..d8229f3262 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -366,11 +366,6 @@ func (p *Planner[T]) ConfigureFetch() resolve.FetchConfiguration { return resolve.FetchConfiguration{} } - if p.rpcTransport == nil { - p.stopWithError(errors.WithStack(errors.New("grpc / connect configuration requires an rpc transport"))) - return resolve.FetchConfiguration{} - } - dataSource, err = grpcdatasource.NewDataSource(p.rpcTransport, grpcdatasource.DataSourceConfig{ Operation: &opDocument, Definition: p.config.schemaConfiguration.upstreamSchemaAst, From 7517d4899161e8d3971332dc86a72f375f8a5f35 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Wed, 15 Jul 2026 14:49:34 +0530 Subject: [PATCH 18/21] fix: naming --- v2/pkg/astnormalization/inline_arguments.go | 22 +++++++++---------- .../astnormalization/inline_arguments_test.go | 20 ++++++++--------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index 2e6fa86a92..ce69bde159 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -10,19 +10,19 @@ import ( // InlineArgument describes a single argument in an operation whose value was // supplied inline (a literal) instead of as a variable. type InlineArgument struct { - ArgumentName string - EnclosingName string - EnclosingKind ast.NodeKind - ValueKind ast.ValueKind - Position position.Position + ArgumentName string + AncestorName string + AncestorKind ast.NodeKind + ValueKind ast.ValueKind + Position position.Position } func (a InlineArgument) QualifiedName() string { - switch a.EnclosingKind { + switch a.AncestorKind { case ast.NodeKindField: - return a.EnclosingName + "." + a.ArgumentName + return a.AncestorName + "." + a.ArgumentName case ast.NodeKindDirective: - return "@" + a.EnclosingName + "." + a.ArgumentName + return "@" + a.AncestorName + "." + a.ArgumentName default: return a.ArgumentName } @@ -103,12 +103,12 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { if len(v.Ancestors) > 0 { parent := v.Ancestors[len(v.Ancestors)-1] - finding.EnclosingKind = parent.Kind + finding.AncestorKind = parent.Kind switch parent.Kind { case ast.NodeKindField: - finding.EnclosingName = v.operation.FieldNameString(parent.Ref) + finding.ArgumentName = v.operation.FieldNameString(parent.Ref) case ast.NodeKindDirective: - finding.EnclosingName = v.operation.DirectiveNameString(parent.Ref) + finding.AncestorName = v.operation.DirectiveNameString(parent.Ref) } } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index 50557eb005..01a18d75cd 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -57,7 +57,7 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { name: "inline string field argument", operation: `query GetUserById { userById(userId: "12345") { loginName } }`, expected: []InlineArgument{ - {ArgumentName: "userId", EnclosingName: "userById", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, + {ArgumentName: "userId", AncestorName: "userById", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, }, }, { @@ -69,42 +69,42 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { name: "inline enum argument", operation: `query { field(order: ASC) }`, expected: []InlineArgument{ - {ArgumentName: "order", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, + {ArgumentName: "order", AncestorName: "field", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, }, }, { name: "inline null argument", operation: `query { field(flag: null) }`, expected: []InlineArgument{ - {ArgumentName: "flag", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindNull}, + {ArgumentName: "flag", AncestorName: "field", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindNull}, }, }, { name: "inline list argument recorded once", operation: `query { field(by: [1, 2, 3]) }`, expected: []InlineArgument{ - {ArgumentName: "by", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindList}, + {ArgumentName: "by", AncestorName: "field", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindList}, }, }, { name: "inline object argument recorded once", operation: `query { field(obj: { a: 1 }) }`, expected: []InlineArgument{ - {ArgumentName: "obj", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindObject}, + {ArgumentName: "obj", AncestorName: "field", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindObject}, }, }, { name: "mixed variable and literal flags only the literal", operation: `query q($flag: Boolean) { field(flag: $flag, order: DESC) }`, expected: []InlineArgument{ - {ArgumentName: "order", EnclosingName: "field", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, + {ArgumentName: "order", AncestorName: "field", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindEnum}, }, }, { name: "inline directive argument (@include)", operation: `query q($userId: ID!) { userById(userId: $userId) @include(if: true) { loginName } }`, expected: []InlineArgument{ - {ArgumentName: "if", EnclosingName: "include", EnclosingKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, + {ArgumentName: "if", AncestorName: "include", AncestorKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, }, }, { @@ -116,7 +116,7 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { name: "introspection field argument", operation: `query { __type(name: "User") { name } }`, expected: []InlineArgument{ - {ArgumentName: "name", EnclosingName: "__type", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, + {ArgumentName: "name", AncestorName: "__type", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindString}, }, }, { @@ -126,8 +126,8 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { name: "argument under a @skip(if:true)-removed node still flagged", operation: `query q($userId: ID!) { user @skip(if: true) { posts(first: 10) } userById(userId: $userId) { loginName } }`, expected: []InlineArgument{ - {ArgumentName: "if", EnclosingName: "skip", EnclosingKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, - {ArgumentName: "first", EnclosingName: "posts", EnclosingKind: ast.NodeKindField, ValueKind: ast.ValueKindInteger}, + {ArgumentName: "if", AncestorName: "skip", AncestorKind: ast.NodeKindDirective, ValueKind: ast.ValueKindBoolean}, + {ArgumentName: "first", AncestorName: "posts", AncestorKind: ast.NodeKindField, ValueKind: ast.ValueKindInteger}, }, }, { From 7ab70dfb155237221cb0251e4fe3c9016fd39807 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Wed, 15 Jul 2026 15:01:14 +0530 Subject: [PATCH 19/21] fix: updates --- v2/pkg/astnormalization/inline_arguments.go | 8 ++-- .../astnormalization/inline_arguments_test.go | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index ce69bde159..4e195fb236 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -13,6 +13,7 @@ type InlineArgument struct { ArgumentName string AncestorName string AncestorKind ast.NodeKind + Path string ValueKind ast.ValueKind Position position.Position } @@ -20,9 +21,9 @@ type InlineArgument struct { func (a InlineArgument) QualifiedName() string { switch a.AncestorKind { case ast.NodeKindField: - return a.AncestorName + "." + a.ArgumentName + return a.Path + "#" + a.ArgumentName case ast.NodeKindDirective: - return "@" + a.AncestorName + "." + a.ArgumentName + return a.Path + "@" + a.AncestorName + "#" + a.ArgumentName default: return a.ArgumentName } @@ -99,6 +100,7 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { ArgumentName: v.operation.ArgumentNameString(ref), ValueKind: valueKind, Position: v.operation.Arguments[ref].Position, + Path: v.Path.DotDelimitedString(), } if len(v.Ancestors) > 0 { @@ -106,7 +108,7 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { finding.AncestorKind = parent.Kind switch parent.Kind { case ast.NodeKindField: - finding.ArgumentName = v.operation.FieldNameString(parent.Ref) + finding.AncestorName = v.operation.FieldNameString(parent.Ref) case ast.NodeKindDirective: finding.AncestorName = v.operation.DirectiveNameString(parent.Ref) } diff --git a/v2/pkg/astnormalization/inline_arguments_test.go b/v2/pkg/astnormalization/inline_arguments_test.go index 01a18d75cd..81a8e25c67 100644 --- a/v2/pkg/astnormalization/inline_arguments_test.go +++ b/v2/pkg/astnormalization/inline_arguments_test.go @@ -152,6 +152,7 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { got := make([]InlineArgument, len(result.InlineArguments)) for i, f := range result.InlineArguments { f.Position = tt.expected[i].Position // ignore position in this comparison + f.Path = tt.expected[i].Path // path is asserted via QualifiedName instead got[i] = f } assert.Equal(t, tt.expected, got) @@ -159,6 +160,49 @@ func TestInlineArgumentsRule_Detection(t *testing.T) { } } +func TestInlineArgumentsRule_QualifiedName(t *testing.T) { + tests := []struct { + name string + operation string + expected []string + }{ + { + name: "top-level field argument", + operation: `query { userById(userId: "12345") { loginName } }`, + expected: []string{"query.userById#userId"}, + }, + { + name: "nested field argument carries the full query path", + operation: `query { userById(userId: "12345") { posts(first: 10) } }`, + expected: []string{"query.userById#userId", "query.userById.posts#first"}, + }, + { + name: "field alias is used in the path", + operation: `query { u: userById(userId: "12345") { p: posts(first: 10) } }`, + expected: []string{"query.u#userId", "query.u.p#first"}, + }, + { + name: "directive argument names the enclosing field and directive", + operation: `query { userById(userId: "x") @include(if: true) { loginName } }`, + expected: []string{"query.userById#userId", "query.userById@include#if"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, report := runInlineArgumentsRule(t, tt.operation, InlineArgumentsValidationOptions{Enforce: false}) + require.False(t, report.HasErrors(), "log-only mode must never error: %s", report.Error()) + require.NotNil(t, result) + + got := make([]string, len(result.InlineArguments)) + for i, f := range result.InlineArguments { + got[i] = f.QualifiedName() + } + assert.Equal(t, tt.expected, got) + }) + } +} + func TestInlineArgumentsRule_Position(t *testing.T) { // The reported position points at the argument in the operation as parsed. // `userId` starts at column 30 on line 1 of the operation below. From b8b1e26dcca31781003a5e58ce5e509a8eced2ce Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Wed, 15 Jul 2026 15:20:31 +0530 Subject: [PATCH 20/21] fix: ancestor --- v2/pkg/astnormalization/inline_arguments.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/v2/pkg/astnormalization/inline_arguments.go b/v2/pkg/astnormalization/inline_arguments.go index 4e195fb236..6f738dbb09 100644 --- a/v2/pkg/astnormalization/inline_arguments.go +++ b/v2/pkg/astnormalization/inline_arguments.go @@ -103,15 +103,13 @@ func (v *inlineArgumentsVisitor) EnterArgument(ref int) { Path: v.Path.DotDelimitedString(), } - if len(v.Ancestors) > 0 { - parent := v.Ancestors[len(v.Ancestors)-1] - finding.AncestorKind = parent.Kind - switch parent.Kind { - case ast.NodeKindField: - finding.AncestorName = v.operation.FieldNameString(parent.Ref) - case ast.NodeKindDirective: - finding.AncestorName = v.operation.DirectiveNameString(parent.Ref) - } + parent := v.Ancestor() + finding.AncestorKind = parent.Kind + switch parent.Kind { + case ast.NodeKindField: + finding.AncestorName = v.operation.FieldNameString(parent.Ref) + case ast.NodeKindDirective: + finding.AncestorName = v.operation.DirectiveNameString(parent.Ref) } v.result.InlineArguments = append(v.result.InlineArguments, finding) From bfee16b3233201fd65196c525d2573077a03e57b Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Wed, 15 Jul 2026 15:45:55 +0530 Subject: [PATCH 21/21] fix: add clone copy --- v2/pkg/engine/resolve/context.go | 1 + 1 file changed, 1 insertion(+) diff --git a/v2/pkg/engine/resolve/context.go b/v2/pkg/engine/resolve/context.go index 35e1ec723f..55adaa90e8 100644 --- a/v2/pkg/engine/resolve/context.go +++ b/v2/pkg/engine/resolve/context.go @@ -316,6 +316,7 @@ func (c *Context) clone(ctx context.Context) *Context { cpy.Files = append([]*httpclient.FileUpload(nil), c.Files...) cpy.Request.Header = c.Request.Header.Clone() cpy.RenameTypeNames = append([]RenameTypeName(nil), c.RenameTypeNames...) + cpy.InlineArguments = append([]string(nil), c.InlineArguments...) if c.RemapVariables != nil { cpy.RemapVariables = make(map[string]string, len(c.RemapVariables))