Skip to content

Commit d6cceac

Browse files
endigmaendigma
authored
feat: improve abstract field validation (#1588)
Also incorporates a fix to one of the execution test runners, we cannot run `t.Parallel()` inside because the whole thing is slightly flawed where the wrong `t` is captured for `must…(t, …)` type invocations on fields of the TC struct. This is normally not an issue as the `must…` helpers are usually just ceremony for dropping the error value, but when we might actually expect them to fail they will panic by calling `require.XXX(t, …)` with the parent `t` instead of the subtest `t`. This can be resolved by in these cases evaluating it and all of its arguments within the subtest closure `func(t) { … }` but then the `tparallel` linter complains about missing parallel, which cannot be fixed because the inner function also tried to call it. The proper way to fix this would be to have `dataSources` etc fields on the TC be evaluated with subtest `t` via like `dataSources: func (t testing.T) []*DataSource` instead of directly providing it, but this would be a very intrusive change.
1 parent 962ece3 commit d6cceac

12 files changed

Lines changed: 474 additions & 80 deletions

‎execution/engine/abstract_type_validation_test.go‎

Lines changed: 365 additions & 0 deletions
Large diffs are not rendered by default.

‎execution/engine/execution_engine_cost_test.go‎

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
6464
Weights: map[plan.FieldCoordinate]*plan.FieldCost{
6565
{TypeName: "Droid", FieldName: "name"}: {HasWeight: true, Weight: 17},
6666
},
67-
}},
67+
},
68+
},
6869
customConfig,
6970
),
7071
},
@@ -120,7 +121,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
120121
},
121122
{TypeName: "Droid", FieldName: "name"}: {HasWeight: true, Weight: 17},
122123
},
123-
}},
124+
},
125+
},
124126
customConfig,
125127
),
126128
},
@@ -181,7 +183,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
181183
Types: map[string]int{
182184
"Droid": -1, // Negative type weight
183185
},
184-
}},
186+
},
187+
},
185188
customConfig,
186189
),
187190
},
@@ -292,11 +295,7 @@ func TestExecutionEngine_Cost(t *testing.T) {
292295
computeCosts(),
293296
))
294297

295-
// Regression test for the abstract field without __typename bug recordObjectTypeStats).
296-
// When the subgraph resolves a single (non-list) abstract field and does NOT return __typename,
297-
// we must still record one occurrence for that field's path, falling back to the declared
298-
// abstract type name in actual costs.
299-
t.Run("single abstract field without __typename takes into account implementing types", runWithoutError(
298+
t.Run("single abstract field without __typename is rejected and bills nothing", runWithoutError(
300299
ExecutionEngineTestCase{
301300
schema: graphql.StarwarsSchema(t),
302301
operation: func(t *testing.T) graphql.Request {
@@ -325,7 +324,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
325324
),
326325
},
327326
expectedEstimatedCost: intPtr(13), // Query.hero(13)
328-
expectedActualCost: intPtr(13), // Query.hero(13)
327+
// the abstract hero value is rejected and nulled, so nothing is billed
328+
expectedActualCost: intPtr(0),
329329
},
330330
computeCosts(),
331331
))
@@ -778,7 +778,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
778778
customConfig,
779779
),
780780
},
781-
expectedResponse: `{"data":{"hero":{"name":"Luke","friends":[{"name":"Leia"}]}}}`,
781+
// friends items carry no __typename, so they are rejected and nulled
782+
expectedResponse: `{"errors":[{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["hero","friends",0],"extensions":{"code":"INVALID_GRAPHQL"}}],"data":{"hero":{"name":"Luke","friends":[null]}}}`,
782783
// Cost calculation:
783784
// Query.hero: 2
784785
// Character.name: max(Human.name=3, Droid.name=5) = 5
@@ -787,8 +788,9 @@ func TestExecutionEngine_Cost(t *testing.T) {
787788
// name: max(Human.name=3, Droid.name=5) = 5
788789
expectedEstimatedCost: intPtr(55), // 2 + 1*(5 + 6*(3 + 1*5))
789790
// hero returned __typename Human, so its name is billed at Human.name (3).
790-
// friends items carry no __typename, so their type weight and name keep the max (3, 5).
791-
expectedActualCost: intPtr(13), // 2 + 1*(3 + 1*(3 + 1*5))
791+
// The rejected friends element is nulled: its max type weight is still
792+
// counted for the returned element, but no field weights are billed.
793+
expectedActualCost: intPtr(8), // 2 + 1*(3 + 1*3)
792794
},
793795
computeCosts(),
794796
))
@@ -1043,7 +1045,8 @@ func TestExecutionEngine_Cost(t *testing.T) {
10431045
Weights: map[plan.FieldCoordinate]*plan.FieldCost{
10441046
{TypeName: "Droid", FieldName: "primaryFunction"}: {HasWeight: true, Weight: 17},
10451047
},
1046-
}},
1048+
},
1049+
},
10471050
customConfig,
10481051
),
10491052
},
@@ -1313,9 +1316,10 @@ func TestExecutionEngine_Cost(t *testing.T) {
13131316
customConfig,
13141317
),
13151318
},
1316-
expectedResponse: `{"data":{"hero":{"name":"Luke","friends":[{"name":"Leia"}]}}}`,
1319+
// friends items carry no __typename, so they are rejected and nulled
1320+
expectedResponse: `{"errors":[{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["hero","friends",0],"extensions":{"code":"INVALID_GRAPHQL"}}],"data":{"hero":{"name":"Luke","friends":[null]}}}`,
13171321
expectedEstimatedCost: intPtr(20), // 2 + 1*(0 + 6*(3 + 1*0))
1318-
expectedActualCost: intPtr(5), // 2 + 1*(0 + 1*(3 + 1*0))
1322+
expectedActualCost: intPtr(5), // 2 + 1*(0 + 1*3)
13191323
},
13201324
computeCosts(),
13211325
costsIgnoreImplementingTypeWeights(),
@@ -1376,7 +1380,6 @@ func TestExecutionEngine_Cost(t *testing.T) {
13761380
costsIgnoreImplementingTypeWeights(),
13771381
))
13781382
})
1379-
13801383
})
13811384

13821385
t.Run("union types", func(t *testing.T) {
@@ -2511,7 +2514,6 @@ func TestExecutionEngine_Cost(t *testing.T) {
25112514
computeCosts(),
25122515
))
25132516
})
2514-
25152517
})
25162518

25172519
t.Run("nested lists with compounding multipliers", func(t *testing.T) {
@@ -5271,7 +5273,6 @@ func TestExecutionEngine_Cost(t *testing.T) {
52715273
computeCosts(),
52725274
))
52735275
})
5274-
52755276
})
52765277

52775278
t.Run("validate requireOneSlicingArgument on concrete types", func(t *testing.T) {
@@ -6168,7 +6169,6 @@ func TestExecutionEngine_Cost(t *testing.T) {
61686169
"external: field 'Paginated.items' requires exactly one slicing argument, but 2 were provided, locations: [], path: [search,items]",
61696170
computeCosts(),
61706171
))
6171-
61726172
})
61736173

61746174
t.Run("input object cost", func(t *testing.T) {
@@ -7473,7 +7473,7 @@ func TestExecutionEngine_Cost(t *testing.T) {
74737473
computeCosts(),
74747474
))
74757475

7476-
t.Run("without typenames keeps max weight", runWithoutError(
7476+
t.Run("without typenames rejects the values and bills nothing for them", runWithoutError(
74777477
ExecutionEngineTestCase{
74787478
schema: schema,
74797479
operation: func(t *testing.T) graphql.Request {
@@ -7498,17 +7498,18 @@ func TestExecutionEngine_Cost(t *testing.T) {
74987498
),
74997499
},
75007500
fields: []plan.FieldConfiguration{},
7501-
expectedResponse: `{"data":{"items":[` +
7502-
`{"hero":{"name":"Luke"}},` +
7503-
`{"hero":{"name":"Han"}},` +
7504-
`{"hero":{"name":"R2D2"}}]}}`,
7501+
// Abstract values without a __typename are rejected and nulled,
7502+
// so the actual cost bills nothing for them.
7503+
expectedResponse: `{"errors":[` +
7504+
`{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["items",0,"hero"],"extensions":{"code":"INVALID_GRAPHQL"}},` +
7505+
`{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["items",1,"hero"],"extensions":{"code":"INVALID_GRAPHQL"}},` +
7506+
`{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["items",2,"hero"],"extensions":{"code":"INVALID_GRAPHQL"}}],` +
7507+
`"data":{"items":[{"hero":null},{"hero":null},{"hero":null}]}}`,
75057508
expectedEstimatedCost: intPtr(170), // 10 * (0 + (0 + max(7, 17)))
7506-
// Subgraph returned no __typename for hero: no per-type info, keep the max.
7507-
expectedActualCost: intPtr(51), // 3 * 17
7509+
expectedActualCost: intPtr(0),
75087510
},
75097511
computeCosts(),
75107512
))
7511-
75127513
})
75137514

75147515
t.Run("fragment fields sharing a response path under an abstract list", func(t *testing.T) {

‎execution/engine/execution_engine_helpers_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func createTestRoundTripper(t *testing.T, testCase roundTripperTestCase) testRou
110110
receivedBodyBytes, err = io.ReadAll(req.Body)
111111
require.NoError(t, err)
112112
}
113-
require.Equal(t, testCase.expectedBody, string(receivedBodyBytes), "roundTripperTestCase received unexpected body")
113+
assert.Equal(t, testCase.expectedBody, string(receivedBodyBytes), "roundTripperTestCase received unexpected body")
114114
}
115115

116116
body := bytes.NewBuffer([]byte(testCase.sendResponseBody))

‎execution/engine/execution_engine_test.go‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ func mustFactory(t testing.TB, httpClient *http.Client) plan.PlannerFactory[grap
6363

6464
func runExecutionTest(testCase ExecutionEngineTestCase, withError bool, expectedErrorMessage string, options ...executionTestOptions) func(t *testing.T) {
6565
return func(t *testing.T) {
66-
t.Parallel()
6766
t.Helper()
6867

6968
if testCase.skipReason != "" {
@@ -805,7 +804,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
805804
expectedHost: "example.com",
806805
expectedPath: "/",
807806
expectedBody: "",
808-
sendResponseBody: `{"data":{"hero":{"name":"Luke Skywalker"}}}`,
807+
sendResponseBody: `{"data":{"hero":{"__typename":"Human","name":"Luke Skywalker"}}}`,
809808
sendStatusCode: 200,
810809
}),
811810
),
@@ -910,7 +909,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
910909
expectedHost: "example.com",
911910
expectedPath: "/",
912911
expectedBody: "",
913-
sendResponseBody: `{"data":{"hero":{"name":"Luke Skywalker"}}}`,
912+
sendResponseBody: `{"data":{"hero":{"__typename":"Human","name":"Luke Skywalker"}}}`,
914913
sendStatusCode: 200,
915914
}),
916915
),
@@ -957,8 +956,8 @@ func TestExecutionEngine_Execute(t *testing.T) {
957956
testNetHttpClient(t, roundTripperTestCase{
958957
expectedHost: "example.com",
959958
expectedPath: "/",
960-
expectedBody: `{"query":"{hero {name}}","extensions":{"fetch_reasons":[{"typename":"Character","field":"name","by_user":true},{"typename":"Droid","field":"name","by_user":true},{"typename":"Human","field":"name","by_user":true}]}}`,
961-
sendResponseBody: `{"data":{"hero":{"name":"Luke Skywalker"}}}`,
959+
expectedBody: `{"query":"{hero {__typename name}}","extensions":{"fetch_reasons":[{"typename":"Character","field":"name","by_user":true},{"typename":"Droid","field":"name","by_user":true},{"typename":"Human","field":"name","by_user":true}]}}`,
960+
sendResponseBody: `{"data":{"hero":{"__typename":"Human","name":"Luke Skywalker"}}}`,
962961
sendStatusCode: 200,
963962
}),
964963
),
@@ -1016,8 +1015,8 @@ func TestExecutionEngine_Execute(t *testing.T) {
10161015
testNetHttpClient(t, roundTripperTestCase{
10171016
expectedHost: "example.com",
10181017
expectedPath: "/",
1019-
expectedBody: `{"query":"{hero {name}}","extensions":{"fetch_reasons":[{"typename":"Droid","field":"name","by_user":true}]}}`,
1020-
sendResponseBody: `{"data":{"hero":{"name":"Droid Number 6"}}}`,
1018+
expectedBody: `{"query":"{hero {__typename name}}","extensions":{"fetch_reasons":[{"typename":"Droid","field":"name","by_user":true}]}}`,
1019+
sendResponseBody: `{"data":{"hero":{"__typename":"Droid","name":"Droid Number 6"}}}`,
10211020
sendStatusCode: 200,
10221021
}),
10231022
),
@@ -1076,8 +1075,8 @@ func TestExecutionEngine_Execute(t *testing.T) {
10761075
testNetHttpClient(t, roundTripperTestCase{
10771076
expectedHost: "example.com",
10781077
expectedPath: "/",
1079-
expectedBody: `{"query":"{hero {name}}","extensions":{"fetch_reasons":[{"typename":"Character","field":"name","by_user":true},{"typename":"Droid","field":"name","by_user":true},{"typename":"Human","field":"name","by_user":true}]}}`,
1080-
sendResponseBody: `{"data":{"hero":{"name":"Droid Number 6"}}}`,
1078+
expectedBody: `{"query":"{hero {__typename name}}","extensions":{"fetch_reasons":[{"typename":"Character","field":"name","by_user":true},{"typename":"Droid","field":"name","by_user":true},{"typename":"Human","field":"name","by_user":true}]}}`,
1079+
sendResponseBody: `{"data":{"hero":{"__typename":"Droid","name":"Droid Number 6"}}}`,
10811080
sendStatusCode: 200,
10821081
}),
10831082
),
@@ -1280,7 +1279,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
12801279
expectedHost: "example.com",
12811280
expectedPath: "/",
12821281
expectedBody: "",
1283-
sendResponseBody: `{"data":{"hero":{"name":"Luke Skywalker"}}, "errors": []}`,
1282+
sendResponseBody: `{"data":{"hero":{"__typename":"Human","name":"Luke Skywalker"}}, "errors": []}`,
12841283
sendStatusCode: 200,
12851284
}),
12861285
),
@@ -1332,7 +1331,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
13321331
expectedHost: "example.com",
13331332
expectedPath: "/",
13341333
expectedBody: "",
1335-
sendResponseBody: `{"data":{"hero":{"name":"Luke Skywalker"}}}`,
1334+
sendResponseBody: `{"data":{"hero":{"__typename":"Human","name":"Luke Skywalker"}}}`,
13361335
sendStatusCode: 200,
13371336
}),
13381337
),
@@ -2305,7 +2304,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
23052304
testNetHttpClient(t, roundTripperTestCase{
23062305
expectedHost: "example.com",
23072306
expectedPath: "/",
2308-
expectedBody: `{"query":"{codeType {code __typename ... on Country {name}}}"}`,
2307+
expectedBody: `{"query":"{codeType {__typename code ... on Country {name}}}"}`,
23092308
sendResponseBody: `{"data":{"codeType":{"__typename":"Country","code":"de","name":"Germany"}}}`,
23102309
sendStatusCode: 200,
23112310
}),
@@ -2436,7 +2435,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
24362435
expectedHost: "example.com",
24372436
expectedPath: "/",
24382437
expectedBody: "",
2439-
sendResponseBody: `{"data":{"searchResults":[{"name":"Luke Skywalker"},{"length":13.37}]}}`,
2438+
sendResponseBody: `{"data":{"searchResults":[{"__typename":"Human","name":"Luke Skywalker"},{"__typename":"Starship","length":13.37}]}}`,
24402439
sendStatusCode: 200,
24412440
}),
24422441
),
@@ -2484,7 +2483,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
24842483
),
24852484
},
24862485
fields: []plan.FieldConfiguration{},
2487-
expectedResponse: `{"data":{"searchResults":[{},{}]}}`,
2486+
expectedResponse: `{"data":{"searchResults":[{"name":"Luke Skywalker"},{"length":13.37}]}}`,
24882487
},
24892488
))
24902489

‎execution/engine/testdata/complex_nesting_query_with_art.json‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@
9696
"raw_input_data": {},
9797
"input": {
9898
"body": {
99-
"query": "{me {id username history {__typename ... on Purchase {wallet {currency}} ... on Sale {location product {upc __typename}}} __typename}}"
99+
"query": "{me {id username history {__typename ... on Purchase {wallet {__typename currency}} ... on Sale {location product {upc __typename}}} __typename}}"
100100
},
101101
"header": {},
102102
"method": "POST",
@@ -111,6 +111,7 @@
111111
{
112112
"__typename": "Purchase",
113113
"wallet": {
114+
"__typename": "WalletType1",
114115
"currency": "USD"
115116
}
116117
},
@@ -125,6 +126,7 @@
125126
{
126127
"__typename": "Purchase",
127128
"wallet": {
129+
"__typename": "WalletType2",
128130
"currency": "USD"
129131
}
130132
}
@@ -155,13 +157,13 @@
155157
"status": "200 OK",
156158
"headers": {
157159
"Content-Length": [
158-
"277"
160+
"331"
159161
],
160162
"Content-Type": [
161163
"application/json"
162164
]
163165
},
164-
"body_size": 277
166+
"body_size": 331
165167
}
166168
}
167169
}
@@ -387,6 +389,7 @@
387389
{
388390
"__typename": "Purchase",
389391
"wallet": {
392+
"__typename": "WalletType1",
390393
"currency": "USD"
391394
}
392395
},
@@ -401,6 +404,7 @@
401404
{
402405
"__typename": "Purchase",
403406
"wallet": {
407+
"__typename": "WalletType2",
404408
"currency": "USD"
405409
}
406410
}

‎v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -578,19 +578,12 @@ func (p *Planner[T]) EnterSelectionSet(ref int) {
578578
p.addRepresentationsQuery()
579579
}
580580

581-
if p.visitor.Walker.EnclosingTypeDefinition.Kind != ast.NodeKindInterfaceTypeDefinition {
582-
return
583-
}
584-
585-
// handle adding typename for the InterfaceObject
586-
// In case we are inside selection set which returns an interface object
587-
// we need to add __typename field to the selection set to get an initial typename value
588-
typeName := p.visitor.Walker.EnclosingTypeDefinition.NameString(p.visitor.Definition)
589-
for _, interfaceObjectCfg := range p.dataSourceConfig.FederationConfiguration().InterfaceObjects {
590-
if interfaceObjectCfg.InterfaceTypeName == typeName {
591-
p.addTypenameToSelectionSet(set.Ref)
592-
return
593-
}
581+
// abstract values are validated against the contract by their runtime type,
582+
// so the upstream must always report it. This also covers the InterfaceObject
583+
// case, which needs __typename for an initial typename value.
584+
switch p.visitor.Walker.EnclosingTypeDefinition.Kind {
585+
case ast.NodeKindInterfaceTypeDefinition, ast.NodeKindUnionTypeDefinition:
586+
p.addTypenameToSelectionSet(set.Ref)
594587
}
595588
}
596589

‎v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_interface_provides_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func interfaceProvidesPlan() *plan.SynchronousResponsePlan {
4444
Response: &resolve.GraphQLResponse{
4545
Fetches: resolve.Sequence(resolve.Single(&resolve.SingleFetch{
4646
FetchConfiguration: resolve.FetchConfiguration{
47-
Input: `{"method":"POST","url":"http://localhost:4250/provides-on-interface/b","body":{"query":"{media {__typename ... on Book {id animals {id name}}}}"}}`,
47+
Input: `{"method":"POST","url":"http://localhost:4250/provides-on-interface/b","body":{"query":"{media {__typename ... on Book {id animals {__typename id name}}}}"}}`,
4848
DataSource: &Source{},
4949
PostProcessing: DefaultPostProcessingConfiguration,
5050
},

‎v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11328,7 +11328,7 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
1132811328
Fetches: resolve.Sequence(
1132911329
resolve.Single(&resolve.SingleFetch{
1133011330
FetchConfiguration: resolve.FetchConfiguration{
11331-
Input: `{"method":"POST","url":"http://first.service","body":{"query":"{account {some {__typename id}}}"}}`,
11331+
Input: `{"method":"POST","url":"http://first.service","body":{"query":"{account {__typename some {__typename id}}}"}}`,
1133211332
PostProcessing: DefaultPostProcessingConfiguration,
1133311333
DataSource: &Source{},
1133411334
},

0 commit comments

Comments
 (0)