Skip to content

Commit 4e4cff3

Browse files
temaEmelyantemaEmelyan
authored
bugfix: don't widen the type conditions in the NadelServiceTypeFilterTransform (#741)
1 parent 22a4ca9 commit 4e4cff3

4 files changed

Lines changed: 382 additions & 3 deletions

File tree

‎lib/src/main/java/graphql/nadel/engine/transform/NadelServiceTypeFilterTransform.kt‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import graphql.introspection.Introspection
44
import graphql.nadel.Service
55
import graphql.nadel.ServiceExecutionHydrationDetails
66
import graphql.nadel.ServiceExecutionResult
7-
import graphql.nadel.definition.hydration.NadelHydrationConditionDefinition.Keyword.result
87
import graphql.nadel.engine.NadelExecutionContext
98
import graphql.nadel.engine.NadelServiceExecutionContext
109
import graphql.nadel.engine.blueprint.IntrospectionService
@@ -176,8 +175,14 @@ class NadelServiceTypeFilterTransform : NadelTransform<State> {
176175
state: State,
177176
transformServiceExecutionContext: NadelTransformServiceExecutionContext?,
178177
): NadelTransformFieldResult {
178+
// A transform on a parent may have narrowed this field after planning. Intersect with the current types
179+
// so service filtering can never reintroduce a type removed by an earlier transform.
180+
val currentFieldObjectTypeNamesOwnedByService = field.objectTypeNames.filter {
181+
it in state.fieldObjectTypeNamesOwnedByService
182+
}
183+
179184
// Nothing to query if there are no fields, we need to add selection
180-
if (state.fieldObjectTypeNamesOwnedByService.isEmpty()) {
185+
if (currentFieldObjectTypeNamesOwnedByService.isEmpty()) {
181186
val objectTypeNames = state.overallField.parent.getFieldDefinitions(executionBlueprint.engineSchema)
182187
.asSequence()
183188
.flatMap { fieldDef ->
@@ -218,7 +223,7 @@ class NadelServiceTypeFilterTransform : NadelTransform<State> {
218223
newField = field
219224
.toBuilder()
220225
.clearObjectTypesNames()
221-
.objectTypeNames(state.fieldObjectTypeNamesOwnedByService)
226+
.objectTypeNames(currentFieldObjectTypeNamesOwnedByService)
222227
.build(),
223228
)
224229
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/execution/servicetypefilter/ServiceTypeFilterDoesNotRestoreRemovedOwnedTypeTestSnapshot.kt‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// @formatter:off
2+
package graphql.nadel.tests.next.fixtures.execution.servicetypefilter
3+
4+
import graphql.nadel.tests.next.ExpectedNadelResult
5+
import graphql.nadel.tests.next.ExpectedServiceCall
6+
import graphql.nadel.tests.next.TestSnapshot
7+
import graphql.nadel.tests.next.listOfJsonStrings
8+
import kotlin.Suppress
9+
import kotlin.collections.List
10+
import kotlin.collections.listOf
11+
12+
private suspend fun main() {
13+
graphql.nadel.tests.next.update<ServiceTypeFilterDoesNotRestoreRemovedOwnedTypeTest>()
14+
}
15+
16+
/**
17+
* This class is generated. Do NOT modify.
18+
*
19+
* Refer to [graphql.nadel.tests.next.UpdateTestSnapshots]
20+
*/
21+
@Suppress("unused")
22+
public class ServiceTypeFilterDoesNotRestoreRemovedOwnedTypeTestSnapshot : TestSnapshot() {
23+
/**
24+
* Query
25+
*
26+
* ```graphql
27+
* query {
28+
* crossServiceItems {
29+
* value
30+
* }
31+
* }
32+
* ```
33+
*
34+
* Variables
35+
*
36+
* ```json
37+
* {}
38+
* ```
39+
*/
40+
override val calls: List<ExpectedServiceCall> = listOf(
41+
ExpectedServiceCall(
42+
service = "issues",
43+
query = """
44+
| {
45+
| crossServiceItems {
46+
| ... on AllowedIssuesItem {
47+
| value
48+
| }
49+
| }
50+
| }
51+
""".trimMargin(),
52+
variables = "{}",
53+
result = """
54+
| {
55+
| "data": {
56+
| "crossServiceItems": [
57+
| {
58+
| "value": "allowed issues data"
59+
| },
60+
| {}
61+
| ]
62+
| }
63+
| }
64+
""".trimMargin(),
65+
delayedResults = listOfJsonStrings(
66+
),
67+
),
68+
)
69+
70+
/**
71+
* ```json
72+
* {
73+
* "data": {
74+
* "crossServiceItems": [
75+
* {
76+
* "value": "allowed issues data"
77+
* },
78+
* {}
79+
* ]
80+
* }
81+
* }
82+
* ```
83+
*/
84+
override val result: ExpectedNadelResult = ExpectedNadelResult(
85+
result = """
86+
| {
87+
| "data": {
88+
| "crossServiceItems": [
89+
| {
90+
| "value": "allowed issues data"
91+
| },
92+
| {}
93+
| ]
94+
| }
95+
| }
96+
""".trimMargin(),
97+
delayedResults = listOfJsonStrings(
98+
),
99+
)
100+
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/execution/servicetypefilter/ServiceTypeFilterDoesNotWidenNarrowedSelectionTest.kt‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
package graphql.nadel.tests.next.fixtures.execution.servicetypefilter
2+
3+
import graphql.nadel.Nadel
4+
import graphql.nadel.Service
5+
import graphql.nadel.ServiceExecutionHydrationDetails
6+
import graphql.nadel.engine.NadelExecutionContext
7+
import graphql.nadel.engine.NadelServiceExecutionContext
8+
import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint
9+
import graphql.nadel.engine.transform.NadelTransformFieldResult
10+
import graphql.nadel.engine.transform.NadelTransformServiceExecutionContext
11+
import graphql.nadel.engine.transform.query.NadelQueryTransformer
12+
import graphql.nadel.tests.next.NadelIntegrationTest
13+
import graphql.nadel.tests.util.NadelTransformAdapter
14+
import graphql.normalized.ExecutableNormalizedField
15+
16+
/**
17+
* Verifies that service type filtering composes monotonically with a transform on an abstract parent field.
18+
*
19+
* Nadel plans every field's transform state before query transformations start. A parent transform can subsequently
20+
* narrow its child fields' [ExecutableNormalizedField.objectTypeNames] before Nadel recursively executes those
21+
* children's already-planned service type filters. The service filter must therefore intersect its planned
22+
* service-owned types with each child's current types. Replacing the current types with the planned set would
23+
* resurrect implementations deliberately removed by the parent and could expose their fields.
24+
*
25+
* The concrete tests cover both outcomes of that intersection: no service-owned types remain, so Nadel substitutes an
26+
* artificial `__typename`, or one service-owned type remains and must not be widened to its removed sibling.
27+
*/
28+
abstract class ServiceTypeFilterNarrowedSelectionTest(
29+
private val narrowedTypeNames: List<String>,
30+
) : NadelIntegrationTest(
31+
query = """
32+
query {
33+
crossServiceItems {
34+
value
35+
}
36+
}
37+
""".trimIndent(),
38+
services = listOf(
39+
Service(
40+
name = "shared",
41+
overallSchema = """
42+
interface CrossServiceItem {
43+
value: String
44+
}
45+
""".trimIndent(),
46+
underlyingSchema = """
47+
type Query {
48+
echo: String
49+
}
50+
""".trimIndent(),
51+
runtimeWiring = {},
52+
),
53+
Service(
54+
name = "comments",
55+
overallSchema = """
56+
type CommentsItem implements CrossServiceItem {
57+
value: String
58+
}
59+
""".trimIndent(),
60+
underlyingSchema = """
61+
type Query {
62+
echo: String
63+
}
64+
interface CrossServiceItem {
65+
value: String
66+
}
67+
type CommentsItem implements CrossServiceItem {
68+
value: String
69+
}
70+
""".trimIndent(),
71+
runtimeWiring = { wiring ->
72+
wiring.type("CrossServiceItem") { type ->
73+
type.typeResolver { env ->
74+
env.schema.getObjectType("CommentsItem")
75+
}
76+
}
77+
},
78+
),
79+
Service(
80+
name = "issues",
81+
overallSchema = """
82+
type Query {
83+
crossServiceItems: [CrossServiceItem]
84+
}
85+
type AllowedIssuesItem implements CrossServiceItem {
86+
value: String
87+
}
88+
type DeniedIssuesItem implements CrossServiceItem {
89+
value: String
90+
}
91+
""".trimIndent(),
92+
underlyingSchema = """
93+
type Query {
94+
crossServiceItems: [CrossServiceItem]
95+
}
96+
interface CrossServiceItem {
97+
value: String
98+
}
99+
type AllowedIssuesItem implements CrossServiceItem {
100+
value: String
101+
}
102+
type DeniedIssuesItem implements CrossServiceItem {
103+
value: String
104+
}
105+
""".trimIndent(),
106+
runtimeWiring = { wiring ->
107+
data class AllowedIssuesItem(val value: String)
108+
data class DeniedIssuesItem(val value: String)
109+
110+
wiring
111+
.type("CrossServiceItem") { type ->
112+
type.typeResolver { env ->
113+
env.schema.getObjectType(env.getObject<Any>().javaClass.simpleName)
114+
}
115+
}
116+
.type("Query") { type ->
117+
type.dataFetcher("crossServiceItems") {
118+
listOf(
119+
AllowedIssuesItem(value = "allowed issues data"),
120+
DeniedIssuesItem(value = "denied issues data"),
121+
)
122+
}
123+
}
124+
},
125+
),
126+
),
127+
) {
128+
override fun makeNadel(): Nadel.Builder {
129+
return super.makeNadel()
130+
.transforms(listOf(NarrowChildSelectionsTransform(narrowedTypeNames)))
131+
}
132+
}
133+
134+
class ServiceTypeFilterDoesNotWidenNarrowedSelectionTest : ServiceTypeFilterNarrowedSelectionTest(
135+
narrowedTypeNames = listOf("CommentsItem"),
136+
)
137+
138+
class ServiceTypeFilterDoesNotRestoreRemovedOwnedTypeTest : ServiceTypeFilterNarrowedSelectionTest(
139+
narrowedTypeNames = listOf("AllowedIssuesItem", "CommentsItem"),
140+
)
141+
142+
/** Simulates a parent transform, such as AGG scope narrowing, restricting its child selections. */
143+
private class NarrowChildSelectionsTransform(
144+
private val narrowedTypeNames: List<String>,
145+
) : NadelTransformAdapter {
146+
override suspend fun isApplicable(
147+
executionContext: NadelExecutionContext,
148+
serviceExecutionContext: NadelServiceExecutionContext,
149+
executionBlueprint: NadelOverallExecutionBlueprint,
150+
services: Map<String, Service>,
151+
service: Service,
152+
overallField: ExecutableNormalizedField,
153+
transformServiceExecutionContext: NadelTransformServiceExecutionContext?,
154+
hydrationDetails: ServiceExecutionHydrationDetails?,
155+
): Unit? {
156+
return Unit.takeIf { overallField.name == "crossServiceItems" }
157+
}
158+
159+
override suspend fun transformField(
160+
executionContext: NadelExecutionContext,
161+
serviceExecutionContext: NadelServiceExecutionContext,
162+
transformer: NadelQueryTransformer,
163+
executionBlueprint: NadelOverallExecutionBlueprint,
164+
service: Service,
165+
field: ExecutableNormalizedField,
166+
state: Unit,
167+
transformServiceExecutionContext: NadelTransformServiceExecutionContext?,
168+
): NadelTransformFieldResult {
169+
field.children.forEach { child ->
170+
check(
171+
child.objectTypeNames.toSet() ==
172+
setOf("AllowedIssuesItem", "CommentsItem", "DeniedIssuesItem")
173+
)
174+
child.setObjectTypeNames(narrowedTypeNames)
175+
}
176+
return NadelTransformFieldResult.unmodified(field)
177+
}
178+
}

0 commit comments

Comments
 (0)