From 884884b3e3c4d0f99d34671ff34c2acb8d2e1635 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Tue, 28 May 2024 11:19:02 -0500 Subject: [PATCH] feat: improve top-level Query and Mutation type handling (#72) * unit test * refactor * pass test * add mutation tests * fix integration test * refactor * reorganize * separate field definition functions * pull out boolean arg * pull out other boolean arg * refactor * refactor * bun version * set up docs workspace * docs --- codegen.ts | 13 +- docs/docs/inheritance.md | 129 ++++++++++++++++++ docs/docs/recommended-usage.md | 33 ++--- src/config/build-config-with-defaults.ts | 1 + ...find-type-in-resolver-interfaces-config.ts | 2 +- src/definitions/object.ts | 21 +++ test/integration/expected.graphql | 2 + .../codegen.config.ts | 5 + .../expected.kt | 35 +++++ .../schema.graphql | 9 ++ 10 files changed, 219 insertions(+), 31 deletions(-) create mode 100644 docs/docs/inheritance.md create mode 100644 test/unit/should_handle_top_level_types_properly/codegen.config.ts create mode 100644 test/unit/should_handle_top_level_types_properly/expected.kt create mode 100644 test/unit/should_handle_top_level_types_properly/schema.graphql diff --git a/codegen.ts b/codegen.ts index 063df63..1beb087 100644 --- a/codegen.ts +++ b/codegen.ts @@ -1,4 +1,3 @@ -import { GraphQLKotlinCodegenConfig } from "./src/plugin"; import { CodegenConfig } from "@graphql-codegen/cli"; export default { @@ -9,17 +8,7 @@ export default { }, generates: { "test/integration/Types.kt": { - plugins: [ - { - "dist/plugin.cjs": { - resolverInterfaces: [ - { - typeName: "Query", - }, - ], - } satisfies GraphQLKotlinCodegenConfig, - }, - ], + plugins: ["dist/plugin.cjs"], }, }, } satisfies CodegenConfig; diff --git a/docs/docs/inheritance.md b/docs/docs/inheritance.md new file mode 100644 index 0000000..1d96dea --- /dev/null +++ b/docs/docs/inheritance.md @@ -0,0 +1,129 @@ +--- +sidebar_position: 6 +--- + +# Inheritance + +When dealing with GraphQL schema containing [field arguments](https://graphql.com/learn/arguments/), +generating Kotlin code can be a bit tricky. This is because the generated code cannot in itself be the implementation +in a resolver. + +Here's an example: + +```graphql +type Query { + resolveMyType: MyType! +} + +type MyType { + resolveMe(input: String!): String! +} +``` + +Generated Kotlin: + +```kotlin +package com.types.generated + +open class Query { + open fun resolveMyType(): MyType = throw NotImplementedError("Query.resolveMyType must be implemented.") +} + +open class MyType { + open fun resolveMe(input: String): String = throw NotImplementedError("MyType.resolveMe must be implemented.") +} +``` + +Source code: + +```kotlin +import com.expediagroup.graphql.server.operations.Query +import com.types.generated.MyType as MyTypeInterface +import com.types.generated.Query as QueryInterface + +class MyQuery : Query, QueryInterface() { + override fun resolveMyType(): MyTypeInterface = MyType() +} + +class MyType : MyTypeInterface() { + override fun resolveMe(input: String): String = "Hello world!" +} +``` + +As you can see, the generated code is not part of the implementation. Rather, it becomes an interface to inherit from in your implementation. +This enforces a type contract between the schema and your resolver code. + +Note that GraphQL Kotlin will use the implementation classes to generate the schema, not the generated interfaces. +This means that all `@GraphQLDescription` and `@Deprecated` annotations have to be added to implementation classes +in order to be propagated to the resulting schema. + +## Top Level Types + +When dealing with top-level types, i.e. `Query` and `Mutation`, you can inherit from the corresponding generated class +to enforce the type contract. This is fine as long as all of your resolvers are contained in the same Query or Mutation class. + +```graphql +type Query { + foo: String! + bar: String! +} +``` + +```kotlin +import com.expediagroup.graphql.server.operations.Query +import com.types.generated.Query as QueryInterface + +class MyQuery : Query, QueryInterface() { + override fun foo(): String = "Hello" + override fun bar(): String = "World" +} +``` + +However, you might want to separate the implementation into multiple classes like so: + +```kotlin +import com.expediagroup.graphql.server.operations.Query + +class FooQuery : Query { + override fun foo(): String = "Hello" +} + +class BarQuery : Query { + override fun bar(): String = "World" +} +``` + +If you try to inherit from the generated `Query` class, you will get an error during schema generation. + +```kotlin +import com.expediagroup.graphql.server.operations.Query +import com.types.generated.Query as QueryInterface + +class FooQuery : Query, QueryInterface() { + override fun foo(): String = "Hello" +} + +class BarQuery : Query, QueryInterface() { + override fun bar(): String = "World" +} +``` + +This is because the generated `Query` class contains both `foo` and `bar` fields, which creates a conflict when inherited by multiple implementation classes. + +Instead, you should inherit from the field-level generated `Query` classes like so: + +```kotlin +import com.expediagroup.graphql.server.operations.Query +import com.types.generated.FooQuery as FooQueryInterface +import com.types.generated.BarQuery as BarQueryInterface + +class FooQuery : Query, FooQueryInterface() { + override fun foo(): String = "Hello" +} + +class BarQuery : Query, BarQueryInterface() { + override fun bar(): String = "World" +} +``` + +This way, schema generation can complete without conflict, and you can separate your implementation into multiple classes! diff --git a/docs/docs/recommended-usage.md b/docs/docs/recommended-usage.md index 3459037..12f7db1 100644 --- a/docs/docs/recommended-usage.md +++ b/docs/docs/recommended-usage.md @@ -21,8 +21,8 @@ type Query { } type MyType { - field1: String! - field2: String + foo: String! + bar: String } ``` @@ -38,8 +38,8 @@ open class Query { } data class MyType( - val field1: String, - val field2: String? = null + val foo: String, + val bar: String? = null ) ``` @@ -53,16 +53,15 @@ import com.types.generated.Query as QueryInterface class MyQuery : Query, QueryInterface() { override fun resolveMyType(input: String): MyType = MyType( - field1 = myExpensiveCall1(), - field2 = myExpensiveCall2() + foo = myExpensiveCall1(), + bar = myExpensiveCall2() ) } - ``` The resulting source code is extremely unperformant. The `MyType` class is a data class, which means -that the `field1` and `field2` properties are both initialized when the `MyType` object is created, and -`myExpensiveCall1()` and `myExpensiveCall2()` will both be called in sequence! Even if I only query for `field1`, not +that the `foo` and `bar` properties are both initialized when the `MyType` object is created, and +`myExpensiveCall1()` and `myExpensiveCall2()` will both be called in sequence! Even if I only query for `foo`, not only will `myExpensiveCall2()` still run, but it will also wait until `myExpensiveCall1()` is totally finished. ### Instead, use the `resolverInterfaces` config! @@ -91,8 +90,8 @@ open class Query { } open class MyType { - open fun field1(): String = throw NotImplementedError("MyType.field1 must be implemented.") - open fun field2(): String? = throw NotImplementedError("MyType.field2 must be implemented.") + open fun foo(): String = throw NotImplementedError("MyType.foo must be implemented.") + open fun bar(): String? = throw NotImplementedError("MyType.bar must be implemented.") } ``` @@ -100,21 +99,19 @@ Source code: ```kotlin import com.types.generated.MyType as MyTypeInterface -import com.expediagroup.graphql.generator.annotations.GraphQLIgnore class MyQuery : Query, QueryInterface() { override fun resolveMyType(input: String): MyType = MyType() } -@GraphQLIgnore class MyType : MyTypeInterface() { - override fun field1(): String = myExpensiveCall1() - override fun field2(): String? = myExpensiveCall2() + override fun foo(): String = myExpensiveCall1() + override fun bar(): String? = myExpensiveCall2() } ``` -This code is much more performant! The `MyType` class is no longer a data class, so the `field1` and `field2` properties -can now be resolved independently of each other. If I query for only `field1`, only `myExpensiveCall1()` will be called, and -if I query for only `field2`, only `myExpensiveCall2()` will be called. +This code is much more performant! The `MyType` class is no longer a data class, so the `foo` and `bar` properties +can now be resolved independently of each other. If I query for only `foo`, only `myExpensiveCall1()` will be called, and +if I query for only `bar`, only `myExpensiveCall2()` will be called. Check out the [related GraphQL Kotlin docs](https://opensource.expediagroup.com/graphql-kotlin/docs/schema-generator/execution/fetching-data/) for more information on this topic. diff --git a/src/config/build-config-with-defaults.ts b/src/config/build-config-with-defaults.ts index a293b9d..2e696c1 100644 --- a/src/config/build-config-with-defaults.ts +++ b/src/config/build-config-with-defaults.ts @@ -25,6 +25,7 @@ export function buildConfigWithDefaults( includeDependentTypes: true, unionGeneration: "MARKER_INTERFACE", extraImports: ["com.expediagroup.graphql.generator.annotations.*"], + resolverInterfaces: [{ typeName: "Query" }, { typeName: "Mutation" }], } as const satisfies GraphQLKotlinCodegenConfig; return merge(defaultConfig, config) as GraphQLKotlinCodegenConfig & diff --git a/src/config/find-type-in-resolver-interfaces-config.ts b/src/config/find-type-in-resolver-interfaces-config.ts index e5dd2bb..9b58458 100644 --- a/src/config/find-type-in-resolver-interfaces-config.ts +++ b/src/config/find-type-in-resolver-interfaces-config.ts @@ -18,7 +18,7 @@ export function findTypeInResolverInterfacesConfig( node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode, config: CodegenConfigWithDefaults, ) { - return config.resolverInterfaces?.findLast( + return config.resolverInterfaces.findLast( (resolverInterface) => resolverInterface.typeName === node.name.value, ); } diff --git a/src/definitions/object.ts b/src/definitions/object.ts index 54279bb..28679cb 100644 --- a/src/definitions/object.ts +++ b/src/definitions/object.ts @@ -73,6 +73,23 @@ export function buildObjectTypeDefinition( ? node.fields : fieldsWithArguments; + const isTopLevelType = + node.name.value === "Query" || node.name.value === "Mutation"; + if (isTopLevelType) { + const individualQueryClasses = node.fields?.map((fieldNode) => { + const className = `${titleCase(fieldNode.name.value)}${node.name.value}`; + return `${annotations}${outputRestrictionAnnotation}open class ${className}${interfaceInheritance} { +${getClassMembers({ node, fieldNodes: [fieldNode], schema, config })} +}`; + }); + const consolidatedQueryClass = `${annotations}${outputRestrictionAnnotation}open class ${name}${interfaceInheritance} { +${getClassMembers({ node, fieldNodes, schema, config })} +}`; + return [consolidatedQueryClass, ...(individualQueryClasses ?? [])].join( + "\n\n", + ); + } + const shouldGenerateFunctions = shouldGenerateFunctionsInClass( node, typeInResolverInterfacesConfig, @@ -139,3 +156,7 @@ export function shouldGenerateFunctionsInClass( node.fields?.some((fieldNode) => fieldNode.arguments?.length), ); } + +function titleCase(str: string) { + return str.charAt(0).toUpperCase() + str.slice(1); +} diff --git a/test/integration/expected.graphql b/test/integration/expected.graphql index 9b0072d..b501b84 100644 --- a/test/integration/expected.graphql +++ b/test/integration/expected.graphql @@ -30,6 +30,8 @@ directive @specifiedBy( ) on SCALAR type Query { + getStuff: String! + getStuffWithInput(input: String!): String! testQuery1: SomeType! testQuery2: SomeHybridType! } diff --git a/test/unit/should_handle_top_level_types_properly/codegen.config.ts b/test/unit/should_handle_top_level_types_properly/codegen.config.ts new file mode 100644 index 0000000..44cf068 --- /dev/null +++ b/test/unit/should_handle_top_level_types_properly/codegen.config.ts @@ -0,0 +1,5 @@ +import { GraphQLKotlinCodegenConfig } from "../../../src/plugin"; + +export default { + resolverInterfaces: [{ typeName: "Mutation", classMethods: "SUSPEND" }], +} satisfies GraphQLKotlinCodegenConfig; diff --git a/test/unit/should_handle_top_level_types_properly/expected.kt b/test/unit/should_handle_top_level_types_properly/expected.kt new file mode 100644 index 0000000..91dfa0c --- /dev/null +++ b/test/unit/should_handle_top_level_types_properly/expected.kt @@ -0,0 +1,35 @@ +package com.kotlin.generated + +import com.expediagroup.graphql.generator.annotations.* + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class Query { + open fun getStuff(dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Query.getStuff must be implemented.") + open fun getStuffWithInput(input: String, dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Query.getStuffWithInput must be implemented.") +} + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class GetStuffQuery { + open fun getStuff(dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Query.getStuff must be implemented.") +} + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class GetStuffWithInputQuery { + open fun getStuffWithInput(input: String, dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Query.getStuffWithInput must be implemented.") +} + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class Mutation { + open suspend fun mutateStuff(dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Mutation.mutateStuff must be implemented.") + open suspend fun mutateStuffWithInput(input: String, dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Mutation.mutateStuffWithInput must be implemented.") +} + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class MutateStuffMutation { + open suspend fun mutateStuff(dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Mutation.mutateStuff must be implemented.") +} + +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT]) +open class MutateStuffWithInputMutation { + open suspend fun mutateStuffWithInput(input: String, dataFetchingEnvironment: graphql.schema.DataFetchingEnvironment): String = throw NotImplementedError("Mutation.mutateStuffWithInput must be implemented.") +} diff --git a/test/unit/should_handle_top_level_types_properly/schema.graphql b/test/unit/should_handle_top_level_types_properly/schema.graphql new file mode 100644 index 0000000..cbe576e --- /dev/null +++ b/test/unit/should_handle_top_level_types_properly/schema.graphql @@ -0,0 +1,9 @@ +type Query { + getStuff: String! + getStuffWithInput(input: String!): String! +} + +type Mutation { + mutateStuff: String! + mutateStuffWithInput(input: String!): String! +}