Refactor blueprint to be per service #404

gnawf · 2022-03-22T06:11:38Z

Pending #403 and need to rebase onto master to get Artyom's blueprint changes.

Effectively this change uses the more correct way to collect services types that the validation has been doing i.e. uses getServiceTypes and getReachableTypes to collect all the types a service uses.

This improves the way we handle shared types as blueprints are now per service, which is actually a better view as we only execute the "query" on one service at a time anyway.

Stuff is not final and am happy to discuss this change.

Please make sure you consider the following:

Add tests that use __typename in queries
Does this change work with all nadel transformations (rename, type rename, hydration, etc)? Add tests for this.
Is it worth using hints for this change in order to be able to enable a percentage rollout?
Do we need to add integration tests for this change in the graphql gateway?
Do we need a pollinator check for this?

gnawf · 2022-03-22T11:54:26Z

lib/src/main/java/graphql/nadel/Nadel.kt

+                    underlyingSchema = underlyingSchema,
+                    serviceExecution = serviceExecution,
+                    definitionRegistry = nadelDefinitionRegistry
+                )


Just reformat

gnawf · 2022-03-22T11:55:18Z

lib/src/main/java/graphql/nadel/NextgenEngine.kt

    )
-    private val executionIdProvider = nadel.executionIdProvider


Above is mostly rearranging stuff and changing constructors to take in Service instead of the now removed NadelOverallExecutionBlueprint.

gnawf · 2022-03-22T11:55:43Z

lib/src/main/java/graphql/nadel/NextgenEngine.kt

@@ -180,7 +184,11 @@ class NextgenEngine @JvmOverloads constructor(
        service: Service,
        executionContext: NadelExecutionContext,
    ): ExecutionResult {
-        val executionPlan = executionPlanner.create(executionContext, services, service, topLevelField)
+        if (service.name == IntrospectionService.name) {
+            return executeIntrospection(service, topLevelField, executionContext)


Shortcuts introspection so it never runs transforms.

gnawf · 2022-03-22T11:57:44Z

lib/src/main/java/graphql/nadel/Service.kt

+     * For now, this is the actual overall schema. But in the future this will be a subset
+     * of the overall schema that this [Service] contributed.
+     */
+    lateinit var schema: GraphQLSchema


So I'm thinking, we should really operate on a subset of the overall/engine schema for this specific service. We're always executing a query/top level field in the context of one service. This way we limit the possible types etc to the one service we're operating on, leaving less room for error.

how is this going to work? if service Issues defines a new type

type Issue { id: ID assignee: User }

that references a type declared in another service Users, then that part of the overall schema owned by Issues service is not a valid graphql schema (like it's not self contained)

So it'll work by creating a subset of the schema for each service. We already have a pretty good understanding of which types belong to a service (including shared types) by using all the definitions and collecting all the reachable/referenced types.

Basically it won't just look at the .nadel file but beyond that, and then filter the schema by the types reachable by that service.

gnawf · 2022-03-22T11:58:53Z

lib/src/main/java/graphql/nadel/engine/introspection/IntrospectionService.kt

@@ -1,4 +1,4 @@
-package graphql.nadel.engine.blueprint
+package graphql.nadel.engine.introspection


Moved this to its own package since it didn't make sense in the blueprint package.

gnawf · 2022-03-22T12:00:42Z

lib/src/main/java/graphql/nadel/validation/NadelTypeValidation.kt

@@ -182,107 +168,6 @@ internal class NadelTypeValidation(
        return false
    }

-    private fun getServiceTypes(


Moved this to NadelGetServicesTypes.kt

gnawf · 2022-03-22T12:01:19Z

lib/src/main/java/graphql/nadel/validation/util/NadelGetReachableTypes.kt

            return CONTINUE
        }

        override fun visitGraphQLInterfaceType(
            node: GraphQLInterfaceType,
            context: TraverserContext<GraphQLSchemaElement>,
        ): TraversalControl {
-            add(node.name)
+            add(node)
+            addAll(getImplementations(node))


This now collects implementations of the given interface type which is more correct. We were possibly missing out on some validation beforehand.

gnawf · 2022-03-22T12:02:23Z

lib/src/main/java/graphql/nadel/validation/util/NadelGetServiceTypes.kt

+import graphql.schema.GraphQLSchema
+import graphql.schema.GraphQLUnionType
+
+fun getServiceTypes(


Copied from NadelTypeValidation

gnawf · 2022-03-22T12:03:59Z

lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprintFactory.kt

@@ -1,217 +1,130 @@
 package graphql.nadel.engine.blueprint


I would recommend just reading the new version of this file instead of reading the diff.

The creation of field instructions hasn't changed much. The main change is the process of collecting service types, which now follows the more correct process that validation uses.

Blueprints are also now per service. This will accommodate shared types better and avoids the need to keep passing in the service to blueprint functions.

gnawf · 2022-03-23T22:04:08Z

lib/src/main/java/graphql/nadel/NextgenEngine.kt

@@ -255,7 +269,6 @@ class NextgenEngine @JvmOverloads constructor(
    ): Pair<NadelQueryTransformer.TransformResult, NadelExecutionPlan> {
        val executionPlan = executionPlanner.create(
            executionContext,
-            services,


Moved to constructor

temaEmelyan · 2022-03-24T03:58:30Z

test/src/test/kotlin/graphql/nadel/tests/CentralSchemaTesting.kt

@@ -36,7 +36,7 @@ val File.parents: Sequence<File>
 * Set this to true to run the query. You'll have to modify the [ServiceExecutionFactory] to
 * actually return something e.g. response from Splunk etc.
 */
-const val runQuery = false
+const val runQuery = true


hey, is this deliberate? the doco above says the default is `false

Yup mistake, I'll fix revert it.

gnawf force-pushed the merge/api/engine-nextgen branch from 3d7a2d0 to 2cde2c5 Compare March 22, 2022 22:33

gnawf changed the base branch from merge/api/engine-nextgen to master March 23, 2022 03:42

Refactor blueprint to be per service

5d78eed

gnawf force-pushed the nadel-kt/blueprint-refactor branch from b339d66 to 5d78eed Compare March 23, 2022 09:41

gnawf commented Mar 24, 2022

View reviewed changes

gnawf marked this pull request as ready for review March 24, 2022 03:18

gnawf requested review from temaEmelyan, bbakerman, dondonz, faizan-siddiqui and felipe-gdr March 24, 2022 03:18

temaEmelyan reviewed Mar 24, 2022

View reviewed changes

gnawf mentioned this pull request Mar 25, 2022

Attempt to fix shared types #327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor blueprint to be per service #404

Refactor blueprint to be per service #404

gnawf commented Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

temaEmelyan Mar 24, 2022

gnawf Mar 24, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 22, 2022

gnawf Mar 23, 2022

temaEmelyan Mar 24, 2022

gnawf Mar 24, 2022

		)
		private val executionIdProvider = nadel.executionIdProvider

		@@ -1,4 +1,4 @@
		package graphql.nadel.engine.blueprint
		package graphql.nadel.engine.introspection

Refactor blueprint to be per service #404

Are you sure you want to change the base?

Refactor blueprint to be per service #404

Conversation

gnawf commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment