feat: support the oneOf directive #1308
Conversation
It means validation of operations and values being sent with operations.
ysmolski
requested review from
devsergiy,
Noroth,
StarpTech,
jensneuse and
a team
as code owners
WalkthroughAdds a built-in GraphQL directive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
v2/pkg/astvalidation/operation_rule_values.go (3)
472-538: Consider extracting the variable nullable check logic.The nullable variable validation logic (lines 508-535) within
objectValueViolatesOneOfis quite lengthy and could benefit from being extracted into a separate helper method for better readability and testability.Consider refactoring the nullable variable checking into a helper method:
func (v *valuesVisitor) objectValueViolatesOneOf(objectValue ast.Value, defRef int) bool { def := v.definition.InputObjectTypeDefinitions[defRef] // Check if the input object type has @oneOf directive if !def.HasDirectives { return false } hasOneOfDirective := def.Directives.HasDirectiveByName(v.definition, "oneOf") if !hasOneOfDirective { return false } fieldRefs := v.operation.ObjectValues[objectValue.Ref].Refs if len(fieldRefs) != 1 { objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) v.Report.AddExternalError(operationreport.ErrOneOfInputObjectFieldCount(objName, len(fieldRefs), objectValue.Position)) return true } - type nullableVar struct { - fieldRef int - fieldValue ast.Value - variableDefinitionRef int - } - var nullableVars []nullableVar - - for _, fieldRef := range fieldRefs { - fieldValue := v.operation.ObjectFieldValue(fieldRef) - - if fieldValue.Kind == ast.ValueKindNull { - objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) - fieldName := v.operation.ObjectFieldNameBytes(fieldRef) - v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position)) - return true - } - - if fieldValue.Kind == ast.ValueKindVariable { - // For variables, check if the variable type is nullable - variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref) - if !ok { - continue - } - - // Collect nullable variables - if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull { - nullableVars = append(nullableVars, nullableVar{ - fieldRef, - fieldValue, - variableDefinitionRef}) - } - } - } - - // If exactly one field, but it's a nullable variable, report that error - if len(nullableVars) > 0 { - violation := nullableVars[0] - objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) - fieldName := v.operation.ObjectFieldNameBytes(violation.fieldRef) - variableName := v.operation.VariableValueNameBytes(violation.fieldValue.Ref) - v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable( - objName, fieldName, variableName, violation.fieldValue.Position, - v.operation.VariableDefinitions[violation.variableDefinitionRef].VariableValue.Position)) - return true - } + // Check single field value + fieldRef := fieldRefs[0] + fieldValue := v.operation.ObjectFieldValue(fieldRef) + + if fieldValue.Kind == ast.ValueKindNull { + objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) + fieldName := v.operation.ObjectFieldNameBytes(fieldRef) + v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position)) + return true + } + + if v.isNullableVariableViolation(fieldRef, fieldValue, defRef) { + return true + } return false } + +func (v *valuesVisitor) isNullableVariableViolation(fieldRef int, fieldValue ast.Value, defRef int) bool { + if fieldValue.Kind != ast.ValueKindVariable { + return false + } + + // For variables, check if the variable type is nullable + variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref) + if !ok { + return false + } + + // Check if nullable + if v.operation.Types[variableTypeRef].TypeKind == ast.TypeKindNonNull { + return false + } + + // Report nullable variable violation + objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) + fieldName := v.operation.ObjectFieldNameBytes(fieldRef) + variableName := v.operation.VariableValueNameBytes(fieldValue.Ref) + v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable( + objName, fieldName, variableName, fieldValue.Position, + v.operation.VariableDefinitions[variableDefinitionRef].VariableValue.Position)) + return true +}
433-436: Hooking @OneOf into input-object validation looks correct.The placement after duplicate-field checks is sensible. Note: in variables validation the @OneOf check runs before the “unknown field” check, while here it runs after. If you want consistent error precedence across both paths, consider moving this block before “unknown field” checks.
472-538: Early-return on first nullable-variable found to avoid allocations.You don’t need to collect all nullable variables; returning on the first violation is enough and avoids slice allocation.
Apply this diff:
func (v *valuesVisitor) objectValueViolatesOneOf(objectValue ast.Value, defRef int) bool { @@ - type nullableVar struct { - fieldRef int - fieldValue ast.Value - variableDefinitionRef int - } - var nullableVars []nullableVar - for _, fieldRef := range fieldRefs { fieldValue := v.operation.ObjectFieldValue(fieldRef) if fieldValue.Kind == ast.ValueKindNull { objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) fieldName := v.operation.ObjectFieldNameBytes(fieldRef) v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position)) return true } if fieldValue.Kind == ast.ValueKindVariable { // For variables, check if the variable type is nullable variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref) if !ok { continue } - // Collect nullable variables - if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull { - nullableVars = append(nullableVars, nullableVar{ - fieldRef, - fieldValue, - variableDefinitionRef}) - } + if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull { + objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) + fieldName := v.operation.ObjectFieldNameBytes(fieldRef) + variableName := v.operation.VariableValueNameBytes(fieldValue.Ref) + v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable( + objName, fieldName, variableName, fieldValue.Position, + v.operation.VariableDefinitions[variableDefinitionRef].VariableValue.Position)) + return true + } } } - // If exactly one field, but it's a nullable variable, report that error - if len(nullableVars) > 0 { - violation := nullableVars[0] - objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef) - fieldName := v.operation.ObjectFieldNameBytes(violation.fieldRef) - variableName := v.operation.VariableValueNameBytes(violation.fieldValue.Ref) - v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable( - objName, fieldName, variableName, violation.fieldValue.Position, - v.operation.VariableDefinitions[violation.variableDefinitionRef].VariableValue.Position)) - return true - } - return false }v2/pkg/variablesvalidation/variablesvalidation.go (2)
375-421: Consider improving variable naming and field counting logic.While the implementation is functionally correct, there are a few areas that could be improved:
- The variable name
firstNullFieldcould be more descriptive (e.g.,nullFieldName)- The
obj.Visititeration could be optimized to early exit once a null field is found- The field counting could be simplified since
obj.Len()already provides the total countConsider these improvements for better clarity and efficiency:
func (v *variablesVisitor) violatesOneOfConstraint(inputObjectDefRef int, jsonValue *astjson.Value, typeName []byte) bool { def := v.definition.InputObjectTypeDefinitions[inputObjectDefRef] // Check if the input object type has @oneOf directive if !def.HasDirectives { return false } hasOneOfDirective := def.Directives.HasDirectiveByName(v.definition, "oneOf") if !hasOneOfDirective { return false } // Count all fields in the JSON object obj := jsonValue.GetObject() totalFieldCount := obj.Len() // Prioritize the count error if totalFieldCount != 1 { variableContent := string(jsonValue.MarshalTo(nil)) v.err = v.newInvalidVariableError( fmt.Sprintf(`%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`, v.invalidValueMessage(string(v.currentVariableName), variableContent), string(typeName), totalFieldCount)) return true } - // Try to find at least one null field. - var firstNullField []byte + // Check if the single field has a null value + var nullFieldName []byte obj.Visit(func(key []byte, val *astjson.Value) { - if firstNullField == nil && val.Type() == astjson.TypeNull { - firstNullField = key + if val.Type() == astjson.TypeNull { + nullFieldName = key } }) - // Check if we have exactly one field, and it's non-null - if firstNullField == nil { + // If the field is non-null, the constraint is satisfied + if nullFieldName == nil { return false } variableContent := string(jsonValue.MarshalTo(nil)) v.err = v.newInvalidVariableError( fmt.Sprintf(`%s; OneOf input object "%s" field "%s" value must be non-null.`, v.invalidValueMessage(string(v.currentVariableName), variableContent), - string(typeName), string(firstNullField))) + string(typeName), string(nullFieldName))) return true }
375-421: Include the JSON path in @OneOf error messages for better DX.Other errors include the variable path (at "..."). Adding it here improves consistency and debuggability, especially for lists.
Apply this diff:
func (v *variablesVisitor) violatesOneOfConstraint(inputObjectDefRef int, jsonValue *astjson.Value, typeName []byte) bool { @@ - if totalFieldCount != 1 { - variableContent := string(jsonValue.MarshalTo(nil)) - v.err = v.newInvalidVariableError( - fmt.Sprintf(`%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`, - v.invalidValueMessage(string(v.currentVariableName), variableContent), - string(typeName), totalFieldCount)) + if totalFieldCount != 1 { + variableContent := string(jsonValue.MarshalTo(nil)) + var path string + if len(v.path) > 1 { + path = fmt.Sprintf(` at "%s"`, v.renderPath()) + } + v.err = v.newInvalidVariableError( + fmt.Sprintf(`%s%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`, + v.invalidValueMessage(string(v.currentVariableName), variableContent), + path, string(typeName), totalFieldCount)) return true } @@ - variableContent := string(jsonValue.MarshalTo(nil)) - v.err = v.newInvalidVariableError( - fmt.Sprintf(`%s; OneOf input object "%s" field "%s" value must be non-null.`, - v.invalidValueMessage(string(v.currentVariableName), variableContent), - string(typeName), string(firstNullField))) + variableContent := string(jsonValue.MarshalTo(nil)) + var path string + if len(v.path) > 1 { + path = fmt.Sprintf(` at "%s"`, v.renderPath()) + } + v.err = v.newInvalidVariableError( + fmt.Sprintf(`%s%s; OneOf input object "%s" field "%s" value must be non-null.`, + v.invalidValueMessage(string(v.currentVariableName), variableContent), + path, string(typeName), string(firstNullField))) return true }v2/pkg/astvalidation/operation_validation_test.go (2)
5255-5260: Clarify the @OneOf directive description.Consider stating the behavioral contract in the docstring to aid readers/tools that surface descriptions via introspection.
Apply this diff:
-""" -The @oneOf built-in directive is used within the type system definition language -to indicate an Input Object is a OneOf Input Object. -""" +""" +The @oneOf built-in directive marks an input object as a OneOf Input Object. +Exactly one field must be provided and its value must be non-null at runtime. +All fields defined within a @oneOf input must be nullable in the schema. +""" directive @oneOf on INPUT_OBJECT
3108-3171: Good coverage; rename duplicate test and relax brittle assertions.
- Two tests share the name “oneOf with null field”; rename the second for clarity.
- The “oneOf with one null field” and “oneOf with two fields” cases assert the exact count suffix (“but 2 fields were provided”), which may be brittle if validation ordering changes. Matching only the invariant prefix is safer.
Apply this diff:
- t.Run("oneOf with null field", func(t *testing.T) { + t.Run("oneOf with both fields null", func(t *testing.T) { run(t, ` mutation oneOfWithNoFields { addPet(pet: { cat: null, dog: null }) { name } }`, Values(), Invalid, withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`)) })- }`, Values(), Invalid, - withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`)) + }`, Values(), Invalid, + withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided`)) })- }`, Values(), Invalid, - withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`)) + }`, Values(), Invalid, + withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided`)) })Optionally, add a positive list case to complement the negative one:
t.Run("list of oneOf with nullable variable", func(t *testing.T) { run(t, ` mutation listOfOneOfWithNullableVariable($dog: DogInput) { addPets(pets: [{ dog: $dog }]) { name } }`, Values(), Invalid, withValidationErrors(`OneOf input object "PetInput" field "dog" cannot use nullable variable "$dog". Variables used in oneOf fields must be non-nullable`)) }) + t.Run("list of oneOf with non-null variable", func(t *testing.T) { + run(t, ` + mutation listOfOneOfWithNonNullVariable($dog: DogInput!) { + addPets(pets: [{ dog: $dog }]) { + name + } + }`, Values(), Valid) + })v2/pkg/variablesvalidation/variablesvalidation_test.go (1)
1494-1712: Great coverage for @OneOf; consider a couple more edge cases.
- Looks solid: valid/invalid, nesting, lists, and variable-content-enabled cases are well covered.
- Optional: Add a case where a list element is an empty object {} (field count = 0) to ensure the per-element error is surfaced with list context.
- Optional: Add a case with an unknown field alongside a valid field on a @OneOf object to document precedence vs. other validations (unknown field vs. oneOf count). This can help align with operation value validation precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
execution/engine/engine_config_test.go(1 hunks)execution/engine/testdata/full_introspection.json(1 hunks)execution/engine/testdata/full_introspection_with_deprecated.json(1 hunks)execution/engine/testdata/full_introspection_with_typenames.json(1 hunks)v2/pkg/asttransform/baseschema.go(1 hunks)v2/pkg/asttransform/fixtures/complete.golden(1 hunks)v2/pkg/asttransform/fixtures/custom_query_name.golden(1 hunks)v2/pkg/asttransform/fixtures/mutation_only.golden(1 hunks)v2/pkg/asttransform/fixtures/schema_missing.golden(1 hunks)v2/pkg/asttransform/fixtures/simple.golden(1 hunks)v2/pkg/asttransform/fixtures/subscription_only.golden(1 hunks)v2/pkg/asttransform/fixtures/subscription_renamed.golden(1 hunks)v2/pkg/asttransform/fixtures/with_mutation_subscription.golden(1 hunks)v2/pkg/astvalidation/operation_rule_values.go(2 hunks)v2/pkg/astvalidation/operation_validation_test.go(4 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden(1 hunks)v2/pkg/federation/fixtures/federated_schema.golden(1 hunks)v2/pkg/operationreport/externalerror.go(2 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(2 hunks)v2/pkg/variablesvalidation/variablesvalidation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/astvalidation/operation_rule_values.go (2)
v2/pkg/operationreport/externalerror.go (3)
ErrOneOfInputObjectFieldCount(221-226)ErrOneOfInputObjectNullValue(228-238)ErrOneOfInputObjectNullableVariable(239-253)pkg/ast/ast_val_variable_value.go (1)
VariableValue(13-16)
v2/pkg/astvalidation/operation_validation_test.go (3)
v2/pkg/astvalidation/operation_rule_values.go (1)
Values(15-24)pkg/astvalidation/operation_rule_values.go (1)
Values(14-23)pkg/astvalidation/validation_state.go (2)
Valid(10-10)Invalid(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (27)
v2/pkg/asttransform/fixtures/schema_missing.golden (1)
60-64: Schema fixture picks up @OneOf correctlyThanks for keeping the fixture in lock‑step with the spec-aligned directive docstring and location.
v2/pkg/asttransform/baseschema.go (1)
171-175: Base schema now exposes @OneOfNice to see the built-in directive wired into the canonical SDL; this keeps downstream merges consistent.
v2/pkg/asttransform/fixtures/subscription_only.golden (1)
51-55: Subscription-only fixture matches the base schemaDirective and description look consistent here as well.
v2/pkg/asttransform/fixtures/mutation_only.golden (1)
52-56: Mutation fixture updated cleanlyYep, mirrors the shared SDL addition.
execution/engine/testdata/full_introspection_with_typenames.json (1)
834-843: Introspection output now surfaces @OneOfThe directive entry lines up with the schema definition; introspection clients will see it.
v2/pkg/asttransform/fixtures/simple.golden (1)
60-64: Simple fixture synchronizedEverything stays consistent with the base schema.
v2/pkg/asttransform/fixtures/subscription_renamed.golden (1)
51-55: Renamed subscription fixture includes @OneOfLooks good—helps keep every golden in sync.
v2/pkg/federation/fixtures/federated_schema.golden (1)
100-104: Federated schema exposes @OneOfAppreciate the consistency across federation fixtures, too.
execution/engine/testdata/full_introspection.json (1)
738-746: Introspection wiring for@oneOflooks consistentThe new directive entry mirrors the base SDL definition—no args, scoped to
INPUT_OBJECT, and the description matches the spec wording. Fixtures stay aligned.v2/pkg/asttransform/fixtures/complete.golden (1)
60-64: Base SDL now exposes@oneOfAdding the documented directive right after
@specifiedBykeeps the generated schema in sync with the GraphQL update.execution/engine/engine_config_test.go (1)
402-406: Engine test schema updated appropriatelyThe test schema now contains the
@oneOfdirective with the canonical description and location, matching the other fixtures.v2/pkg/asttransform/fixtures/with_mutation_subscription.golden (1)
71-75: Fixture correctly includes@oneOfDirective definition and placement align with the broader schema fixtures, so introspection and transforms will remain coherent.
v2/pkg/asttransform/fixtures/custom_query_name.golden (1)
60-64: Custom-query fixture stays in syncNice to see the one-of directive propagated here too; no mismatches with the base schema.
execution/engine/testdata/full_introspection_with_deprecated.json (1)
772-779: Deprecated-introspection fixture updatedDirective entry mirrors the others (description, locations, empty args), so the introspection snapshot remains accurate.
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden (1)
500-510: Custom-root introspection fixture aligned
oneOfdirective appears with the expected metadata (isRepeatable: false,INPUT_OBJECTlocation). Looks good.v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden (1)
352-362: Primary introspection fixture reflects new directiveEntry structure matches the rest—no args, not repeatable, scoped to
INPUT_OBJECT. Everything lines up.v2/pkg/variablesvalidation/variablesvalidation_test.go (1)
1495-1713: LGTM! Comprehensive test coverage for the oneOf directive.The test suite thoroughly covers the oneOf constraint validation including:
- Valid single-field cases with different field types
- Invalid cases (no fields, multiple fields, null values)
- Nested oneOf structures
- Lists of oneOf input objects
- Edge cases like empty lists and nullable variables
The error messages are clear and descriptive, properly indicating the specific constraint violations.
v2/pkg/astvalidation/operation_rule_values.go (2)
433-438: LGTM! Well-integrated oneOf validation in input object processing.The validation is properly placed after duplicate field checks and before returning success, ensuring the oneOf constraint is enforced at the appropriate stage of value validation.
472-538: Confirm spec intent for defaulted nullable variables.Current logic flags a violation when the single provided field is a nullable variable type, even if the variable has a non-null default. Please confirm this matches the intended @OneOf spec semantics in this repo (i.e., “variable must be Non-Null” vs. “effective non-null via default is acceptable”), and consider adding a test covering a nullable variable with a non-null default.
v2/pkg/variablesvalidation/variablesvalidation.go (2)
449-452: LGTM! Clean integration point for oneOf validation.The oneOf constraint check is properly integrated into the input object traversal, with early return on violation to prevent further processing.
449-451: Integration point is correct.Running the @OneOf check after field-type traversal ensures type errors are caught first, then structural @OneOf violations are reported.
v2/pkg/astvalidation/operation_validation_test.go (2)
5044-5063: SDL additions for @OneOf look correct.
- PetInput carries nullable fields (cat, dog) as required by oneOf.
- New mutations return the interface Pet, which is valid in SDL-only validation tests.
4079-4107: Approve oneOf nullability tests; non-null vars may have defaults
DefaultValue on non-null variable types is legal per GraphQL spec’s VariableDefinition grammar and coercion rules.v2/pkg/operationreport/externalerror.go (4)
28-30: New oneOf error messages read cleanlyThanks for providing clear, specific templates for each oneOf validation failure; the language matches the rest of the file and keeps the constraint unambiguous.
221-226: Field-count validation helper looks solidThe helper mirrors existing patterns (message constant + location capture) and keeps the off-by-one messaging precise. No issues spotted.
228-238: Null-value constructor aligns with expectationsCapturing the field location explicitly gives clients enough context when a null literal sneaks in. Implementation is consistent with nearby helpers.
239-253: Nullable-variable helper provides great diagnosticsIncluding both the field and variable definition locations will make debugging much easier, and the message text is spot on. 👍
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v2/pkg/variablesvalidation/variablesvalidation.go (1)
375-433: Variables-side @OneOf enforcement looks correct; minor polish suggested.
- Extract directive name "oneOf" to a shared constant for reuse and to avoid typos.
- Tiny nit: rename “Count all fields in the JSON object” comment to “Count provided fields” for clarity.
v2/pkg/variablesvalidation/variablesvalidation_test.go (1)
1495-1711: Add operation-validation test for @OneOf with nullable variable
No existing coverage forpet(input:{cat: $cat})where$cat: String; add a test in v2/pkg/astvalidation/operation_validation_test.go exercising ErrOneOfInputObjectNullableVariable.v2/pkg/astvalidation/operation_validation_test.go (1)
5068-5071: LGTM: PetInput annotated with @OneOfFields are nullable as required by the oneOf contract.
Consider adding a negative test (elsewhere) that asserts schema-level rejection if a @OneOf input defines any Non‑Null field, to guard the schema rule too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
execution/engine/engine_config_test.go(1 hunks)v2/pkg/asttransform/baseschema.go(1 hunks)v2/pkg/asttransform/fixtures/complete.golden(1 hunks)v2/pkg/asttransform/fixtures/custom_query_name.golden(1 hunks)v2/pkg/asttransform/fixtures/mutation_only.golden(1 hunks)v2/pkg/asttransform/fixtures/schema_missing.golden(1 hunks)v2/pkg/asttransform/fixtures/simple.golden(1 hunks)v2/pkg/asttransform/fixtures/subscription_only.golden(1 hunks)v2/pkg/asttransform/fixtures/subscription_renamed.golden(1 hunks)v2/pkg/asttransform/fixtures/with_mutation_subscription.golden(1 hunks)v2/pkg/astvalidation/operation_rule_values.go(2 hunks)v2/pkg/astvalidation/operation_validation_test.go(4 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden(1 hunks)v2/pkg/federation/fixtures/federated_schema.golden(1 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(2 hunks)v2/pkg/variablesvalidation/variablesvalidation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- v2/pkg/asttransform/fixtures/with_mutation_subscription.golden
- v2/pkg/asttransform/fixtures/subscription_renamed.golden
- v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden
- v2/pkg/asttransform/fixtures/custom_query_name.golden
- v2/pkg/asttransform/fixtures/complete.golden
- v2/pkg/asttransform/fixtures/schema_missing.golden
- v2/pkg/asttransform/baseschema.go
- execution/engine/engine_config_test.go
- v2/pkg/asttransform/fixtures/simple.golden
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/astvalidation/operation_rule_values.go (2)
v2/pkg/operationreport/externalerror.go (3)
ErrOneOfInputObjectFieldCount(221-226)ErrOneOfInputObjectNullValue(228-238)ErrOneOfInputObjectNullableVariable(239-253)pkg/ast/ast_val_variable_value.go (1)
VariableValue(13-16)
v2/pkg/astvalidation/operation_validation_test.go (3)
v2/pkg/astvalidation/operation_rule_values.go (1)
Values(15-24)pkg/astvalidation/operation_rule_values.go (1)
Values(14-23)pkg/astvalidation/validation_state.go (2)
Valid(10-10)Invalid(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (14)
v2/pkg/astvalidation/operation_rule_values.go (2)
433-436: Good integration point for @OneOf enforcement.Running the @OneOf check after field existence/duplicates and short‑circuiting on violation is correct.
472-523: Extract constants, clarify comment, add nullable-variable test
- Extract “oneOf” to a shared constant to avoid typos.
- Rename the “Collect nullable variables” comment to reflect that it reports an error on nullable variables.
- (Optional) Add a schema rule enforcing that all fields in a @OneOf input are nullable.
- No tests cover ErrOneOfInputObjectNullableVariable; add a case for “object literal with a variable value of nullable type.”
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden (1)
501-511: Introspection updated correctly to include @OneOf.Directive shape (name, description, locations, empty args, isRepeatable) looks consistent with other built‑ins.
v2/pkg/asttransform/fixtures/subscription_only.golden (1)
51-57: SDL fixture correctly declares @OneOf.Docstring and location align with spec expectations.
v2/pkg/asttransform/fixtures/mutation_only.golden (1)
52-58: SDL fixture correctly declares @OneOf.Placement and description are consistent with other fixtures.
v2/pkg/federation/fixtures/federated_schema.golden (1)
100-106: Federation schema fixture updated appropriately.@OneOf is added alongside existing built‑ins with a clear docstring.
v2/pkg/variablesvalidation/variablesvalidation.go (1)
477-479: Correct placement and short-circuit on @OneOf violation.Runs after field definition checks and exits early on error.
v2/pkg/astvalidation/operation_validation_test.go (7)
3170-3178: LGTM: correct rejection for nullable variable in oneOf listAccurately enforces “variables used in oneOf fields must be non‑nullable”.
4106-4114: LGTM: nullable variable correctly rejected for oneOf fieldMatches the new oneOf validation contract and message format.
5052-5054: LGTM: mutation entry points for oneOf tests
addPetandaddPetssignatures are coherent with the introduced inputs and existingPetinterface.
5056-5066: LGTM: supporting CatInput/DogInput for oneOfShapes align with usage; required
namefields enable meaningful validation.
5263-5269: LGTM: @OneOf directive definition and docsAccurate location (INPUT_OBJECT) and helpful description.
4098-4105: Remove suggested change: default values on Non-Null GraphQL variables are allowed
Current GraphQL spec (June 2018+) permits default values on Non-Null variable definitions, so$cat: CatInput! = { name: "black" }is valid and requires no change.Likely an incorrect or invalid review comment.
3109-3116: No change needed: defaults on Non-Null variables are allowed The GraphQL spec permits a Non-Null variable to include a default value (e.g.query Q($n: Int! = 3)). The existing test cases using$pet: PetInput! = {...}are spec-valid.Likely an incorrect or invalid review comment.
It means validation of operations and values being sent with operations.
Summary by CodeRabbit
New Features
Tests