Skip to content

Commit 5db8c5a

Browse files
NorothNoroth
authored
feat: allow empty arguments for field resolvers (#1350)
# Introduction This PR introduces support to call field resolvers without any arguments. Currently we only check if field resolvers provide arguments with parentheses. Field resolvers were determined whether fields have arguments. This is technically correct but instead of checking field arguments on the operation side, they have to be determined from the schema definition. ## Prerequisites The following problem statement refer to this example schema ```graphql type Query { categories: [Category!]! } type Category { id: ID! # The argument is nullable and can be omitted relevantTopic(orderBy: String): Topic! } type Topic { id: ID! description: String! } ``` ## Problem Statement This will correctly call the field resolver ```graphql query { categories { id relevantTopic("") { id description } } } ``` This did not call the field resolver ```graphql query { categories { id relevantTopic { id description } } } ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced error handling and validation for field resolution * Improved support for optional arguments in field resolvers * Better error messages when field definitions cannot be resolved * **Tests** * Expanded test coverage for field resolver edge cases <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Checklist - [ ] I have discussed my proposed changes in an issue and have received approval to proceed. - [ ] I have followed the coding standards of the project. - [ ] Tests or benchmarks have been added or updated. <!-- Please add any additional information or context regarding your changes here. -->
1 parent 701ea8d commit 5db8c5a

File tree

5 files changed

+204
-35
lines changed

5 files changed

+204
-35
lines changed

‎v2/pkg/engine/datasource/grpc_datasource/execution_plan.go‎

Lines changed: 37 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -779,11 +779,16 @@ func (r *rpcPlanningContext) buildFieldMessage(fieldTypeNode ast.Node, fieldRef
779779
}
780780

781781
for _, fieldRef := range fieldRefs {
782-
if r.isFieldResolver(fieldRef, false) {
782+
fieldDefRef, found := r.definition.NodeFieldDefinitionByName(fieldTypeNode, r.operation.FieldNameBytes(fieldRef))
783+
if !found {
784+
return nil, fmt.Errorf("unable to build required field: field definition not found for field %s", r.operation.FieldNameString(fieldRef))
785+
}
786+
787+
if r.isFieldResolver(fieldDefRef, false) {
783788
continue
784789
}
785790

786-
field, err := r.buildRequiredField(fieldTypeNode, fieldRef)
791+
field, err := r.buildRequiredField(fieldTypeNode, fieldRef, fieldDefRef)
787792
if err != nil {
788793
return nil, err
789794
}
@@ -853,12 +858,12 @@ func (r *rpcPlanningContext) enterResolverCompositeSelectionSet(oneOfType OneOfT
853858
}
854859

855860
// isFieldResolver checks if a field is a field resolver.
856-
func (r *rpcPlanningContext) isFieldResolver(fieldRef int, isRootField bool) bool {
857-
if isRootField {
861+
func (r *rpcPlanningContext) isFieldResolver(fieldDefRef int, isRootField bool) bool {
862+
if isRootField || fieldDefRef == ast.InvalidRef {
858863
return false
859864
}
860865

861-
return len(r.operation.FieldArguments(fieldRef)) > 0
866+
return r.definition.FieldDefinitionHasArgumentsDefinitions(fieldDefRef)
862867
}
863868

864869
// getCompositeType checks whether the node is an interface or union type.
@@ -1097,15 +1102,20 @@ func (r *rpcPlanningContext) buildFieldResolverTypeMessage(typeName string, reso
10971102
message.Fields = make(RPCFields, 0, len(fieldRefs))
10981103

10991104
for _, fieldRef := range fieldRefs {
1100-
if r.isFieldResolver(fieldRef, false) {
1105+
fieldDefRef, found := r.definition.NodeFieldDefinitionByName(parentTypeNode, r.operation.FieldNameBytes(fieldRef))
1106+
if !found {
1107+
return nil, fmt.Errorf("unable to build required field: field definition not found for field %s", r.operation.FieldNameString(fieldRef))
1108+
}
1109+
1110+
if r.isFieldResolver(fieldDefRef, false) {
11011111
continue
11021112
}
11031113

11041114
if message.Fields.Exists(r.operation.FieldNameString(fieldRef), "") {
11051115
continue
11061116
}
11071117

1108-
field, err := r.buildRequiredField(parentTypeNode, fieldRef)
1118+
field, err := r.buildRequiredField(parentTypeNode, fieldRef, fieldDefRef)
11091119
if err != nil {
11101120
return nil, err
11111121
}
@@ -1117,23 +1127,17 @@ func (r *rpcPlanningContext) buildFieldResolverTypeMessage(typeName string, reso
11171127
return message, nil
11181128
}
11191129

1120-
func (r *rpcPlanningContext) buildRequiredField(typeNode ast.Node, fieldRef int) (RPCField, error) {
1121-
fieldName := r.operation.FieldNameString(fieldRef)
1122-
fieldDef, found := r.definition.NodeFieldDefinitionByName(typeNode, r.operation.FieldNameBytes(fieldRef))
1123-
if !found {
1124-
return RPCField{}, fmt.Errorf("unable to build required field: field definition not found for field %s", fieldName)
1125-
}
1126-
1127-
field, err := r.buildField(typeNode, fieldDef, r.operation.FieldNameString(fieldRef), r.operation.FieldAliasString(fieldRef))
1130+
func (r *rpcPlanningContext) buildRequiredField(typeNode ast.Node, fieldRef, fieldDefinitionRef int) (RPCField, error) {
1131+
field, err := r.buildField(typeNode, fieldDefinitionRef, r.operation.FieldNameString(fieldRef), r.operation.FieldAliasString(fieldRef))
11281132
if err != nil {
11291133
return RPCField{}, err
11301134
}
11311135

11321136
// If the field is a message type and has selections, we need to build a nested message.
11331137
if field.ProtoTypeName == DataTypeMessage && r.operation.FieldHasSelections(fieldRef) {
1134-
fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDef))
1138+
fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDefinitionRef))
11351139
if !found {
1136-
return RPCField{}, fmt.Errorf("unable to build required field: unable to resolve field type node for field %s", fieldName)
1140+
return RPCField{}, fmt.Errorf("unable to build required field: unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef))
11371141
}
11381142

11391143
message, err := r.buildFieldMessage(fieldTypeNode, fieldRef)
@@ -1154,22 +1158,22 @@ func (r *rpcPlanningContext) buildCompositeFields(inlineFragmentNode ast.Node, f
11541158
result := make([]RPCField, 0, len(fieldRefs))
11551159

11561160
for _, fieldRef := range fieldRefs {
1157-
if r.isFieldResolver(fieldRef, false) {
1158-
continue
1161+
fieldDefRef := r.fieldDefinitionRefForType(r.operation.FieldNameString(fieldRef), fragmentSelection.typeName)
1162+
if fieldDefRef == ast.InvalidRef {
1163+
return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", r.operation.FieldNameString(fieldRef))
11591164
}
11601165

1161-
fieldDef := r.fieldDefinitionRefForType(r.operation.FieldNameString(fieldRef), fragmentSelection.typeName)
1162-
if fieldDef == ast.InvalidRef {
1163-
return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", r.operation.FieldNameString(fieldRef))
1166+
if r.isFieldResolver(fieldDefRef, false) {
1167+
continue
11641168
}
11651169

1166-
field, err := r.buildField(inlineFragmentNode, fieldDef, r.operation.FieldNameString(fieldRef), r.operation.FieldAliasString(fieldRef))
1170+
field, err := r.buildField(inlineFragmentNode, fieldDefRef, r.operation.FieldNameString(fieldRef), r.operation.FieldAliasString(fieldRef))
11671171
if err != nil {
11681172
return nil, err
11691173
}
11701174

11711175
if field.ProtoTypeName == DataTypeMessage && r.operation.FieldHasSelections(fieldRef) {
1172-
fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDef))
1176+
fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDefRef))
11731177
if !found {
11741178
return nil, fmt.Errorf("unable to build composite field: unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef))
11751179
}
@@ -1256,15 +1260,17 @@ func (r *rpcPlanningContext) createResolverRPCCalls(subgraphName string, resolve
12561260
contextMessage.Fields[i] = field
12571261
}
12581262

1259-
fieldArgsMessage.Fields = make(RPCFields, len(resolvedField.fieldArguments))
1260-
for i := range resolvedField.fieldArguments {
1261-
field, err := r.createRPCFieldFromFieldArgument(resolvedField.fieldArguments[i])
1263+
if argLen := len(resolvedField.fieldArguments); argLen > 0 {
1264+
fieldArgsMessage.Fields = make(RPCFields, argLen)
1265+
for i := range resolvedField.fieldArguments {
1266+
field, err := r.createRPCFieldFromFieldArgument(resolvedField.fieldArguments[i])
12621267

1263-
if err != nil {
1264-
return nil, err
1265-
}
1268+
if err != nil {
1269+
return nil, err
1270+
}
12661271

1267-
fieldArgsMessage.Fields[i] = field
1272+
fieldArgsMessage.Fields[i] = field
1273+
}
12681274
}
12691275

12701276
calls = append(calls, call)

‎v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2618,6 +2618,114 @@ func TestExecutionPlanFieldResolvers_CustomSchemas(t *testing.T) {
26182618
},
26192619
},
26202620
},
2621+
{
2622+
name: "Should correctly create a resolve call for a resolver with an optional argument",
2623+
subgraphName: "Foo",
2624+
operation: `
2625+
query FooQuery {
2626+
foo {
2627+
fooResolverOptionalArgument {
2628+
__typename
2629+
}
2630+
}
2631+
}`,
2632+
schema: schemaWithNestedResolverAndCompositeType(t),
2633+
mapping: mappingWithNestedResolverAndCompositeType(t),
2634+
expectedPlan: &RPCExecutionPlan{
2635+
Calls: []RPCCall{
2636+
{
2637+
ServiceName: "Foo",
2638+
MethodName: "QueryFoo",
2639+
Request: RPCMessage{
2640+
Name: "QueryFooRequest",
2641+
},
2642+
Response: RPCMessage{
2643+
Name: "QueryFooResponse",
2644+
Fields: []RPCField{
2645+
{
2646+
Name: "foo",
2647+
ProtoTypeName: DataTypeMessage,
2648+
JSONPath: "foo",
2649+
Message: &RPCMessage{
2650+
Name: "Foo",
2651+
Fields: RPCFields{},
2652+
},
2653+
},
2654+
},
2655+
},
2656+
},
2657+
{
2658+
Kind: CallKindResolve,
2659+
DependentCalls: []int{0},
2660+
ResponsePath: buildPath("foo.fooResolverOptionalArgument"),
2661+
ServiceName: "Foo",
2662+
MethodName: "ResolveFooFooResolverOptionalArgument",
2663+
Request: RPCMessage{
2664+
Name: "ResolveFooFooResolverOptionalArgumentRequest",
2665+
Fields: []RPCField{
2666+
{
2667+
Name: "context",
2668+
ProtoTypeName: DataTypeMessage,
2669+
Repeated: true,
2670+
Message: &RPCMessage{
2671+
Name: "ResolveFooFooResolverOptionalArgumentContext",
2672+
Fields: []RPCField{
2673+
{
2674+
Name: "id",
2675+
ProtoTypeName: DataTypeString,
2676+
JSONPath: "id",
2677+
ResolvePath: buildPath("foo.id"),
2678+
},
2679+
},
2680+
},
2681+
},
2682+
{
2683+
Name: "field_args",
2684+
ProtoTypeName: DataTypeMessage,
2685+
Message: &RPCMessage{
2686+
Name: "ResolveFooFooResolverOptionalArgumentArgs",
2687+
},
2688+
},
2689+
},
2690+
},
2691+
Response: RPCMessage{
2692+
Name: "ResolveFooFooResolverOptionalArgumentResponse",
2693+
Fields: []RPCField{
2694+
{
2695+
Name: "result",
2696+
ProtoTypeName: DataTypeMessage,
2697+
Repeated: true,
2698+
JSONPath: "result",
2699+
Message: &RPCMessage{
2700+
Name: "ResolveFooFooResolverOptionalArgumentResult",
2701+
Fields: RPCFields{
2702+
{
2703+
Name: "foo_resolver_optional_argument",
2704+
ProtoTypeName: DataTypeMessage,
2705+
JSONPath: "fooResolverOptionalArgument",
2706+
Message: &RPCMessage{
2707+
Name: "Bar",
2708+
OneOfType: OneOfTypeInterface,
2709+
MemberTypes: []string{"Baz"},
2710+
Fields: RPCFields{
2711+
{
2712+
Name: "__typename",
2713+
ProtoTypeName: DataTypeString,
2714+
JSONPath: "__typename",
2715+
StaticValue: "Bar",
2716+
},
2717+
},
2718+
},
2719+
},
2720+
},
2721+
},
2722+
},
2723+
},
2724+
},
2725+
},
2726+
},
2727+
},
2728+
},
26212729
}
26222730

26232731
for _, tt := range tests {
@@ -2651,6 +2759,7 @@ schema {
26512759
type Foo {
26522760
id: ID!
26532761
fooResolver(foo: String!): Bar! @connect__fieldResolver(context: "id")
2762+
fooResolverOptionalArgument(foo: String): Bar! @connect__fieldResolver(context: "id")
26542763
}
26552764
26562765
interface Bar {
@@ -2727,6 +2836,17 @@ func mappingWithNestedResolverAndCompositeType(_ *testing.T) *GRPCMapping {
27272836
Request: "ResolveFooFooResolverRequest",
27282837
Response: "ResolveFooFooResolverResponse",
27292838
},
2839+
"fooResolverOptionalArgument": {
2840+
FieldMappingData: FieldMapData{
2841+
TargetName: "foo_resolver_optional_argument",
2842+
ArgumentMappings: FieldArgumentMap{
2843+
"foo": "foo",
2844+
},
2845+
},
2846+
RPC: "ResolveFooFooResolverOptionalArgument",
2847+
Request: "ResolveFooFooResolverOptionalArgumentRequest",
2848+
Response: "ResolveFooFooResolverOptionalArgumentResponse",
2849+
},
27302850
},
27312851
"Baz": {
27322852
"bazResolver": {

‎v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ func (r *rpcPlanVisitor) EnterField(ref int) {
358358

359359
// If the field is a field resolver, we need to handle it later in a separate resolver call.
360360
// We only store the information about the field and create the call later.
361-
if r.planCtx.isFieldResolver(ref, inRootField) {
361+
if r.planCtx.isFieldResolver(fieldDefRef, inRootField) {
362362
r.enterFieldResolver(ref, fieldDefRef)
363363
return
364364
}
@@ -418,7 +418,15 @@ func (r *rpcPlanVisitor) LeaveField(ref int) {
418418
return
419419
}
420420

421-
if r.planCtx.isFieldResolver(ref, inRootField) {
421+
fieldDefRef, ok := r.walker.FieldDefinition(ref)
422+
if !ok {
423+
r.walker.Report.AddExternalError(operationreport.ExternalError{
424+
Message: fmt.Sprintf("Field %s not found in definition %s", r.operation.FieldNameString(ref), r.walker.EnclosingTypeDefinition.NameString(r.definition)),
425+
})
426+
return
427+
}
428+
429+
if r.planCtx.isFieldResolver(fieldDefRef, inRootField) {
422430
// Pop the field resolver ancestor only when leaving a field resolver field.
423431
r.fieldResolverAncestors.pop()
424432

‎v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ func (r *rpcPlanVisitorFederation) EnterField(ref int) {
331331

332332
// If the field is a field resolver, we need to handle it later in a separate resolver call.
333333
// We only store the information about the field and create the call later.
334-
if r.planCtx.isFieldResolver(ref, inRootField) {
334+
if r.planCtx.isFieldResolver(fieldDefRef, inRootField) {
335335
r.enterFieldResolver(ref, fieldDefRef)
336336
return
337337
}
@@ -387,7 +387,15 @@ func (r *rpcPlanVisitorFederation) LeaveField(ref int) {
387387
return
388388
}
389389

390-
if r.planCtx.isFieldResolver(ref, inRootField) {
390+
fieldDefRef, ok := r.walker.FieldDefinition(ref)
391+
if !ok {
392+
r.walker.Report.AddExternalError(operationreport.ExternalError{
393+
Message: fmt.Sprintf("Field %s not found in definition %s", r.operation.FieldNameString(ref), r.walker.EnclosingTypeDefinition.NameString(r.definition)),
394+
})
395+
return
396+
}
397+
398+
if r.planCtx.isFieldResolver(fieldDefRef, inRootField) {
391399
// Pop the field resolver ancestor only when leaving a field resolver field.
392400
r.fieldResolverAncestors.pop()
393401

‎v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4539,6 +4539,33 @@ func Test_Datasource_Load_WithFieldResolvers(t *testing.T) {
45394539
require.Empty(t, errData)
45404540
},
45414541
},
4542+
{
4543+
name: "Query with field resolver without parentheses (nullable parameter)",
4544+
query: "query CategoriesWithFieldResolverNoParens { categories { id name popularityScore } }",
4545+
vars: `{"variables":{}}`,
4546+
validate: func(t *testing.T, data map[string]interface{}) {
4547+
require.NotEmpty(t, data)
4548+
4549+
categories, ok := data["categories"].([]interface{})
4550+
require.True(t, ok, "categories should be an array")
4551+
require.NotEmpty(t, categories, "categories should not be empty")
4552+
require.Len(t, categories, 4, "Should return 4 categories")
4553+
4554+
for _, category := range categories {
4555+
category, ok := category.(map[string]interface{})
4556+
require.True(t, ok, "category should be an object")
4557+
require.NotEmpty(t, category["id"])
4558+
require.NotEmpty(t, category["name"])
4559+
// popularityScore should return 50 when called without threshold
4560+
// Based on mockservice implementation: threshold defaults to 0, which is <= 50, so returns baseScore of 50
4561+
require.NotEmpty(t, category["popularityScore"], "popularityScore should not be empty when called without parameters")
4562+
require.Equal(t, float64(50), category["popularityScore"], "popularityScore should be 50 when threshold is not provided")
4563+
}
4564+
},
4565+
validateError: func(t *testing.T, errData []graphqlError) {
4566+
require.Empty(t, errData)
4567+
},
4568+
},
45424569
}
45434570

45444571
for _, tc := range testCases {

0 commit comments

Comments
 (0)