From f43d0b4f25d6b03c9030d9b44749dcd99d0fc67c Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Fri, 26 Apr 2024 08:23:46 -0500 Subject: [PATCH 1/5] failing test --- .../expected.kt | 29 ++++++++++ .../schema.graphql | 54 +++++++++++++++++++ .../expected.kt | 0 .../schema.graphql | 0 .../expected.kt | 0 .../schema.graphql | 0 .../expected.kt | 0 .../schema.graphql | 0 8 files changed, 83 insertions(+) create mode 100644 test/unit/should_consolidate_input_and_output_types/expected.kt create mode 100644 test/unit/should_consolidate_input_and_output_types/schema.graphql rename test/unit/{should_default_non-nullable_boolean_fields_to_false => should_default_non_nullable_boolean_fields_to_false}/expected.kt (100%) rename test/unit/{should_default_non-nullable_boolean_fields_to_false => should_default_non_nullable_boolean_fields_to_false}/schema.graphql (100%) rename test/unit/{should_generate_multi-union_types_properly => should_generate_multi_union_types_properly}/expected.kt (100%) rename test/unit/{should_generate_multi-union_types_properly => should_generate_multi_union_types_properly}/schema.graphql (100%) rename test/unit/{should_generate_non-nullable_union_list_types_properly => should_generate_non_nullable_union_list_types_properly}/expected.kt (100%) rename test/unit/{should_generate_non-nullable_union_list_types_properly => should_generate_non_nullable_union_list_types_properly}/schema.graphql (100%) diff --git a/test/unit/should_consolidate_input_and_output_types/expected.kt b/test/unit/should_consolidate_input_and_output_types/expected.kt new file mode 100644 index 0000000..541c1be --- /dev/null +++ b/test/unit/should_consolidate_input_and_output_types/expected.kt @@ -0,0 +1,29 @@ +package com.kotlin.generated + +import com.expediagroup.graphql.generator.annotations.* + +data class MyTypeToConsolidate1( + val field: String? = null +) + +@GraphQLDescription("A description for MyTypeToConsolidate2") +data class MyTypeToConsolidate2( + val field: String? = null +) + +data class MyTypeToConsolidate3( + val field: String? = null +) + +@GraphQLDescription("It always uses the type description when consolidating") +data class MyTypeToConsolidate4( + val field: String? = null +) + +data class MyTypeNotToConsolidate( + val field: String? = null +) + +data class MyTypeToNotConsolidateInput( + val field: String? = null +) diff --git a/test/unit/should_consolidate_input_and_output_types/schema.graphql b/test/unit/should_consolidate_input_and_output_types/schema.graphql new file mode 100644 index 0000000..5cc4e03 --- /dev/null +++ b/test/unit/should_consolidate_input_and_output_types/schema.graphql @@ -0,0 +1,54 @@ +# typical case where input and output types are consolidated + +type MyTypeToConsolidate { + field: String +} + +input MyTypeToConsolidateInput { + field: String +} + +# case where type has description but input does not + +"A description for MyTypeToConsolidate2" +type MyTypeToConsolidate2 { + field: String +} + +input MyTypeToConsolidate2Input { + field: String +} + +# case where input has description but type does not + +type MyTypeToConsolidate3 { + field: String +} + +"It ignores the description on the input when consolidating" +input MyTypeToConsolidate3Input { + field: String +} + +# case where both type and input have descriptions + +"It always uses the type description when consolidating" +type MyTypeToConsolidate4 { + field: String +} + +"A description for MyTypeToConsolidateInput4" +input MyTypeToConsolidate4Input { + field: String +} + +# case where type and input names do not match + +type MyTypeNotToConsolidate { + field: String +} + +"The type name must exactly match in order to consolidate" +input MyTypeToNotConsolidateInput { + field: String +} diff --git a/test/unit/should_default_non-nullable_boolean_fields_to_false/expected.kt b/test/unit/should_default_non_nullable_boolean_fields_to_false/expected.kt similarity index 100% rename from test/unit/should_default_non-nullable_boolean_fields_to_false/expected.kt rename to test/unit/should_default_non_nullable_boolean_fields_to_false/expected.kt diff --git a/test/unit/should_default_non-nullable_boolean_fields_to_false/schema.graphql b/test/unit/should_default_non_nullable_boolean_fields_to_false/schema.graphql similarity index 100% rename from test/unit/should_default_non-nullable_boolean_fields_to_false/schema.graphql rename to test/unit/should_default_non_nullable_boolean_fields_to_false/schema.graphql diff --git a/test/unit/should_generate_multi-union_types_properly/expected.kt b/test/unit/should_generate_multi_union_types_properly/expected.kt similarity index 100% rename from test/unit/should_generate_multi-union_types_properly/expected.kt rename to test/unit/should_generate_multi_union_types_properly/expected.kt diff --git a/test/unit/should_generate_multi-union_types_properly/schema.graphql b/test/unit/should_generate_multi_union_types_properly/schema.graphql similarity index 100% rename from test/unit/should_generate_multi-union_types_properly/schema.graphql rename to test/unit/should_generate_multi_union_types_properly/schema.graphql diff --git a/test/unit/should_generate_non-nullable_union_list_types_properly/expected.kt b/test/unit/should_generate_non_nullable_union_list_types_properly/expected.kt similarity index 100% rename from test/unit/should_generate_non-nullable_union_list_types_properly/expected.kt rename to test/unit/should_generate_non_nullable_union_list_types_properly/expected.kt diff --git a/test/unit/should_generate_non-nullable_union_list_types_properly/schema.graphql b/test/unit/should_generate_non_nullable_union_list_types_properly/schema.graphql similarity index 100% rename from test/unit/should_generate_non-nullable_union_list_types_properly/schema.graphql rename to test/unit/should_generate_non_nullable_union_list_types_properly/schema.graphql From aad9208bf150489d22430973257c098d9c8c2f67 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Fri, 26 Apr 2024 08:28:43 -0500 Subject: [PATCH 2/5] pass test --- src/definitions/input.ts | 6 ++++++ src/definitions/object.ts | 8 +++++++- .../should_consolidate_input_and_output_types/expected.kt | 7 ++++++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/definitions/input.ts b/src/definitions/input.ts index ddca970..5807424 100644 --- a/src/definitions/input.ts +++ b/src/definitions/input.ts @@ -27,6 +27,12 @@ export function buildInputObjectDefinition( return ""; } + const typeNameWithoutInput = node.name.value.replace("Input", ""); + const correspondingType = schema.getType(typeNameWithoutInput); + if (correspondingType) { + return ""; + } + const classMembers = (node.fields ?? []) .map((arg) => { const typeToUse = buildTypeMetadata(arg.type, schema, config); diff --git a/src/definitions/object.ts b/src/definitions/object.ts index 29dbcb5..1a39ee8 100644 --- a/src/definitions/object.ts +++ b/src/definitions/object.ts @@ -34,6 +34,12 @@ export function buildObjectTypeDefinition( return ""; } + const typeNameWithInput = `${node.name.value}Input`; + const correspondingInput = schema.getType(typeNameWithInput); + const inputOutputAnnotation = correspondingInput + ? "@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT])\n" + : ""; + const annotations = buildAnnotations({ config, definitionNode: node, @@ -57,7 +63,7 @@ ${getDataClassMembers({ node, schema, config, completableFuture: true })} }`; } - return `${annotations}data class ${name}( + return `${annotations}${inputOutputAnnotation}data class ${name}( ${getDataClassMembers({ node, schema, config })} )${interfaceInheritance}`; } diff --git a/test/unit/should_consolidate_input_and_output_types/expected.kt b/test/unit/should_consolidate_input_and_output_types/expected.kt index 541c1be..0878c5f 100644 --- a/test/unit/should_consolidate_input_and_output_types/expected.kt +++ b/test/unit/should_consolidate_input_and_output_types/expected.kt @@ -2,20 +2,24 @@ package com.kotlin.generated import com.expediagroup.graphql.generator.annotations.* -data class MyTypeToConsolidate1( +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) +data class MyTypeToConsolidate( val field: String? = null ) @GraphQLDescription("A description for MyTypeToConsolidate2") +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate2( val field: String? = null ) +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate3( val field: String? = null ) @GraphQLDescription("It always uses the type description when consolidating") +@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate4( val field: String? = null ) @@ -24,6 +28,7 @@ data class MyTypeNotToConsolidate( val field: String? = null ) +@GraphQLDescription("The type name must exactly match in order to consolidate") data class MyTypeToNotConsolidateInput( val field: String? = null ) From ebd835da9851651371353ba35d0f3f8f5eeb7952 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Fri, 26 Apr 2024 08:46:15 -0500 Subject: [PATCH 3/5] refactor --- src/definitions/input.ts | 4 ++-- src/definitions/object.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/definitions/input.ts b/src/definitions/input.ts index 5807424..26802e7 100644 --- a/src/definitions/input.ts +++ b/src/definitions/input.ts @@ -28,8 +28,8 @@ export function buildInputObjectDefinition( } const typeNameWithoutInput = node.name.value.replace("Input", ""); - const correspondingType = schema.getType(typeNameWithoutInput); - if (correspondingType) { + const matchingType = schema.getType(typeNameWithoutInput); + if (matchingType) { return ""; } diff --git a/src/definitions/object.ts b/src/definitions/object.ts index 1a39ee8..618c89d 100644 --- a/src/definitions/object.ts +++ b/src/definitions/object.ts @@ -35,8 +35,8 @@ export function buildObjectTypeDefinition( } const typeNameWithInput = `${node.name.value}Input`; - const correspondingInput = schema.getType(typeNameWithInput); - const inputOutputAnnotation = correspondingInput + const matchingInput = schema.getType(typeNameWithInput); + const typeConsolidationAnnotation = matchingInput ? "@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT])\n" : ""; @@ -63,7 +63,7 @@ ${getDataClassMembers({ node, schema, config, completableFuture: true })} }`; } - return `${annotations}${inputOutputAnnotation}data class ${name}( + return `${annotations}${typeConsolidationAnnotation}data class ${name}( ${getDataClassMembers({ node, schema, config })} )${interfaceInheritance}`; } From c6f38ab3127749845440e21ba789f7a464e1bf4c Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Fri, 26 Apr 2024 10:25:08 -0500 Subject: [PATCH 4/5] improve test cases --- src/definitions/input.ts | 52 +++++++++++++++++-- src/definitions/object.ts | 8 +-- .../expected.kt | 27 ++++++++-- .../schema.graphql | 34 +++++++++++- 4 files changed, 105 insertions(+), 16 deletions(-) diff --git a/src/definitions/input.ts b/src/definitions/input.ts index 26802e7..ae78bcf 100644 --- a/src/definitions/input.ts +++ b/src/definitions/input.ts @@ -11,7 +11,12 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { GraphQLSchema, InputObjectTypeDefinitionNode } from "graphql"; +import { + GraphQLSchema, + InputObjectTypeDefinitionNode, + Kind, + TypeNode, +} from "graphql"; import { shouldIncludeTypeDefinition } from "../helpers/should-include-type-definition"; import { buildTypeMetadata } from "../helpers/build-type-metadata"; import { buildAnnotations } from "../helpers/build-annotations"; @@ -27,9 +32,21 @@ export function buildInputObjectDefinition( return ""; } - const typeNameWithoutInput = node.name.value.replace("Input", ""); - const matchingType = schema.getType(typeNameWithoutInput); - if (matchingType) { + const typeNameWithoutInput = getTypeNameWithoutInput(node.name.value); + const matchingType = schema.getType(typeNameWithoutInput)?.astNode; + const matchingTypeFields = + matchingType?.kind === Kind.OBJECT_TYPE_DEFINITION + ? matchingType.fields + : []; + const inputFields = node.fields; + const fieldsMatch = matchingTypeFields?.every((field) => { + const matchingInputField = inputFields?.find( + (inputField) => inputField.name.value === field.name.value, + ); + if (!matchingInputField) return false; + return fieldsAreEquivalent(field.type, matchingInputField.type); + }); + if (matchingTypeFields?.length && fieldsMatch) { return ""; } @@ -60,3 +77,30 @@ export function buildInputObjectDefinition( ${classMembers} )`; } + +function getTypeNameWithoutInput(name: string) { + return name.endsWith("Input") ? name.replace("Input", "") : name; +} + +function fieldsAreEquivalent( + typeField: TypeNode, + inputField: TypeNode, +): boolean { + switch (typeField.kind) { + case Kind.NAMED_TYPE: + return ( + inputField.kind === Kind.NAMED_TYPE && + typeField.name.value === getTypeNameWithoutInput(inputField.name.value) + ); + case Kind.LIST_TYPE: + return ( + inputField.kind === Kind.LIST_TYPE && + fieldsAreEquivalent(typeField.type, inputField.type) + ); + case Kind.NON_NULL_TYPE: + return ( + inputField.kind === Kind.NON_NULL_TYPE && + fieldsAreEquivalent(typeField.type, inputField.type) + ); + } +} diff --git a/src/definitions/object.ts b/src/definitions/object.ts index 618c89d..29dbcb5 100644 --- a/src/definitions/object.ts +++ b/src/definitions/object.ts @@ -34,12 +34,6 @@ export function buildObjectTypeDefinition( return ""; } - const typeNameWithInput = `${node.name.value}Input`; - const matchingInput = schema.getType(typeNameWithInput); - const typeConsolidationAnnotation = matchingInput - ? "@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT])\n" - : ""; - const annotations = buildAnnotations({ config, definitionNode: node, @@ -63,7 +57,7 @@ ${getDataClassMembers({ node, schema, config, completableFuture: true })} }`; } - return `${annotations}${typeConsolidationAnnotation}data class ${name}( + return `${annotations}data class ${name}( ${getDataClassMembers({ node, schema, config })} )${interfaceInheritance}`; } diff --git a/test/unit/should_consolidate_input_and_output_types/expected.kt b/test/unit/should_consolidate_input_and_output_types/expected.kt index 0878c5f..5ad0a2a 100644 --- a/test/unit/should_consolidate_input_and_output_types/expected.kt +++ b/test/unit/should_consolidate_input_and_output_types/expected.kt @@ -2,24 +2,25 @@ package com.kotlin.generated import com.expediagroup.graphql.generator.annotations.* -@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate( + val field: List? = null, + val field2: NestedTypeToConsolidate? = null +) + +data class NestedTypeToConsolidate( val field: String? = null ) @GraphQLDescription("A description for MyTypeToConsolidate2") -@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate2( val field: String? = null ) -@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate3( val field: String? = null ) @GraphQLDescription("It always uses the type description when consolidating") -@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT, GraphQLValidObjectLocations.Locations.OBJECT]) data class MyTypeToConsolidate4( val field: String? = null ) @@ -32,3 +33,21 @@ data class MyTypeNotToConsolidate( data class MyTypeToNotConsolidateInput( val field: String? = null ) + +data class MyTypeToNotConsolidate2( + val field: String? = null +) + +data class MyTypeInputToNotConsolidate2( + val field: String? = null +) + +data class MyTypeWhereFieldsDoNotMatch( + val field: String? = null, + val field2: String? = null +) + +data class MyTypeWhereFieldsDoNotMatchInput( + val field: String? = null, + val field2: Int? = null +) diff --git a/test/unit/should_consolidate_input_and_output_types/schema.graphql b/test/unit/should_consolidate_input_and_output_types/schema.graphql index 5cc4e03..2cacc6a 100644 --- a/test/unit/should_consolidate_input_and_output_types/schema.graphql +++ b/test/unit/should_consolidate_input_and_output_types/schema.graphql @@ -1,10 +1,20 @@ # typical case where input and output types are consolidated type MyTypeToConsolidate { - field: String + field: [String!] + field2: NestedTypeToConsolidate } input MyTypeToConsolidateInput { + field: [String!] + field2: NestedTypeToConsolidateInput +} + +type NestedTypeToConsolidate { + field: String +} + +input NestedTypeToConsolidateInput { field: String } @@ -52,3 +62,25 @@ type MyTypeNotToConsolidate { input MyTypeToNotConsolidateInput { field: String } + +# case where type has input in the middle of the name + +type MyTypeToNotConsolidate2 { + field: String +} + +input MyTypeInputToNotConsolidate2 { + field: String +} + +# case where type and input names match but fields do not match + +type MyTypeWhereFieldsDoNotMatch { + field: String + field2: String +} + +input MyTypeWhereFieldsDoNotMatchInput { + field: String + field2: Int +} From ff3d48cbaa2486d95a06fb6210a0e09e4a553337 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Fri, 26 Apr 2024 11:09:38 -0500 Subject: [PATCH 5/5] refactor --- src/definitions/input.ts | 52 ++----------------- .../input-type-has-matching-output-type.ts | 52 +++++++++++++++++++ 2 files changed, 56 insertions(+), 48 deletions(-) create mode 100644 src/helpers/input-type-has-matching-output-type.ts diff --git a/src/definitions/input.ts b/src/definitions/input.ts index ae78bcf..bbfd265 100644 --- a/src/definitions/input.ts +++ b/src/definitions/input.ts @@ -11,17 +11,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { - GraphQLSchema, - InputObjectTypeDefinitionNode, - Kind, - TypeNode, -} from "graphql"; +import { GraphQLSchema, InputObjectTypeDefinitionNode } from "graphql"; import { shouldIncludeTypeDefinition } from "../helpers/should-include-type-definition"; import { buildTypeMetadata } from "../helpers/build-type-metadata"; import { buildAnnotations } from "../helpers/build-annotations"; import { indent } from "@graphql-codegen/visitor-plugin-common"; import { CodegenConfigWithDefaults } from "../helpers/build-config-with-defaults"; +import { inputTypeHasMatchingOutputType } from "../helpers/input-type-has-matching-output-type"; export function buildInputObjectDefinition( node: InputObjectTypeDefinitionNode, @@ -32,21 +28,8 @@ export function buildInputObjectDefinition( return ""; } - const typeNameWithoutInput = getTypeNameWithoutInput(node.name.value); - const matchingType = schema.getType(typeNameWithoutInput)?.astNode; - const matchingTypeFields = - matchingType?.kind === Kind.OBJECT_TYPE_DEFINITION - ? matchingType.fields - : []; - const inputFields = node.fields; - const fieldsMatch = matchingTypeFields?.every((field) => { - const matchingInputField = inputFields?.find( - (inputField) => inputField.name.value === field.name.value, - ); - if (!matchingInputField) return false; - return fieldsAreEquivalent(field.type, matchingInputField.type); - }); - if (matchingTypeFields?.length && fieldsMatch) { + const typeWillBeConsolidated = inputTypeHasMatchingOutputType(node, schema); + if (typeWillBeConsolidated) { return ""; } @@ -77,30 +60,3 @@ export function buildInputObjectDefinition( ${classMembers} )`; } - -function getTypeNameWithoutInput(name: string) { - return name.endsWith("Input") ? name.replace("Input", "") : name; -} - -function fieldsAreEquivalent( - typeField: TypeNode, - inputField: TypeNode, -): boolean { - switch (typeField.kind) { - case Kind.NAMED_TYPE: - return ( - inputField.kind === Kind.NAMED_TYPE && - typeField.name.value === getTypeNameWithoutInput(inputField.name.value) - ); - case Kind.LIST_TYPE: - return ( - inputField.kind === Kind.LIST_TYPE && - fieldsAreEquivalent(typeField.type, inputField.type) - ); - case Kind.NON_NULL_TYPE: - return ( - inputField.kind === Kind.NON_NULL_TYPE && - fieldsAreEquivalent(typeField.type, inputField.type) - ); - } -} diff --git a/src/helpers/input-type-has-matching-output-type.ts b/src/helpers/input-type-has-matching-output-type.ts new file mode 100644 index 0000000..590657e --- /dev/null +++ b/src/helpers/input-type-has-matching-output-type.ts @@ -0,0 +1,52 @@ +import { Kind, TypeNode } from "graphql/index"; +import { GraphQLSchema, InputObjectTypeDefinitionNode } from "graphql"; + +export function inputTypeHasMatchingOutputType( + inputTypeNode: InputObjectTypeDefinitionNode, + schema: GraphQLSchema, +) { + const typeNameWithoutInput = getTypeNameWithoutInput( + inputTypeNode.name.value, + ); + const matchingType = schema.getType(typeNameWithoutInput)?.astNode; + const matchingTypeFields = + matchingType?.kind === Kind.OBJECT_TYPE_DEFINITION + ? matchingType.fields + : []; + const inputFields = inputTypeNode.fields; + const fieldsMatch = matchingTypeFields?.every((field) => { + const matchingInputField = inputFields?.find( + (inputField) => inputField.name.value === field.name.value, + ); + if (!matchingInputField) return false; + return fieldsAreEquivalent(field.type, matchingInputField.type); + }); + return Boolean(matchingTypeFields?.length && fieldsMatch); +} + +function getTypeNameWithoutInput(name: string) { + return name.endsWith("Input") ? name.replace("Input", "") : name; +} + +function fieldsAreEquivalent( + typeField: TypeNode, + inputField: TypeNode, +): boolean { + switch (typeField.kind) { + case Kind.NAMED_TYPE: + return ( + inputField.kind === Kind.NAMED_TYPE && + typeField.name.value === getTypeNameWithoutInput(inputField.name.value) + ); + case Kind.LIST_TYPE: + return ( + inputField.kind === Kind.LIST_TYPE && + fieldsAreEquivalent(typeField.type, inputField.type) + ); + case Kind.NON_NULL_TYPE: + return ( + inputField.kind === Kind.NON_NULL_TYPE && + fieldsAreEquivalent(typeField.type, inputField.type) + ); + } +}