Conversation
It means validation of operations and values being sent with operations.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAdds a built-in GraphQL directive Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Parser
participant Validator
participant Errors
Client->>Parser: submit operation (literal or variables)
Parser->>Validator: AST + values
alt InputObject not annotated
Validator->>Validator: normal input validation
else InputObject has @oneOf
Validator->>Validator: count provided fields
opt count != 1
Validator->>Errors: create ErrOneOfInputObjectFieldCount
Errors-->>Client: return validation error (short-circuit)
end
opt single field is null
Validator->>Errors: create ErrOneOfInputObjectNullValue
Errors-->>Client: return validation error (short-circuit)
end
opt single field uses nullable variable
Validator->>Errors: create ErrOneOfInputObjectNullableVariable
Errors-->>Client: return validation error (short-circuit)
end
Validator->>Validator: continue deeper validation if no violations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/astvalidation/operation_rule_values.go (1)
484-522: Consider simplifying the field iteration.Since you've already verified
len(fieldRefs) == 1at line 485, the loop on lines 491-519 will only execute once. You could simplify by accessingfieldRefs[0]directly: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 } -for _, fieldRef := range fieldRefs { +// Exactly one field is present (verified above) +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 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 { 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 } } } return falseHowever, the current loop-based approach is defensive and consistent with patterns used elsewhere in this file, so this is purely optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/astvalidation/operation_rule_values.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ 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 (2)
v2/pkg/astvalidation/operation_rule_values.go (2)
433-436: LGTM! Integration point is well-placed.The oneOf validation is correctly positioned after all field-level validations and duplicate checks, ensuring structural constraints are enforced last. The short-circuit on violation is appropriate.
472-482: LGTM! Directive detection is correct.The function properly checks for the
@oneOfdirective and returns early if not present, avoiding unnecessary validation overhead for regular input objects. Good adherence to the past review feedback on variable naming.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.231](v2.0.0-rc.230...v2.0.0-rc.231) (2025-10-20) ### Features * support the oneOf directive ([#1308](#1308)) ([251cb02](251cb02)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for the oneOf directive in v2.0.0-rc.231 * **Chores** * Version bump to v2.0.0-rc.231 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [1.6.0](execution/v1.5.0...execution/v1.6.0) (2025-10-21) ### Features * support the oneOf directive ([#1308](#1308)) ([251cb02](251cb02)) * validate presence of optional [@requires](https://github.com/requires) dependencies ([#1297](#1297)) ([ba75e25](ba75e25)) ### Bug Fixes * bump engine to v2.0.0-rc.231 for execution ([#1329](#1329)) ([ebddb25](ebddb25)) * propagate fetch reasons for interface-related fields ([#1312](#1312)) ([5ee3014](5ee3014)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added oneOf directive support * Added validation of optional @requires dependencies * **Bug Fixes** * Fixed fetch reason propagation for interface-related fields <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
It means validation of operations and values being sent with operations.
Fixed related bug. For input objects, an undefined variable values was producing wrong kind of error before, and sometime it would even pass as valid for non-null fields. The fix makes error message consistent. Some tests were added too.
Summary by CodeRabbit
New Features
Tests