fix: class consolidation bugs (#48)

* fix one case and write unit test for another * pass tests * refactor * refactor * refactor * refactor
ExpediaGroup · Apr 28, 2024 · 55f7101 · 55f7101
1 parent 4bd0da4
commit 55f7101
Show file tree

Hide file tree

Showing 10 changed files with 151 additions and 64 deletions.
diff --git a/src/definitions/input.ts b/src/definitions/input.ts
@@ -28,7 +28,7 @@ export function buildInputObjectDefinition(
     return "";
   }
 
-  const typeWillBeConsolidated = inputTypeHasMatchingOutputType(schema, node);
+  const typeWillBeConsolidated = inputTypeHasMatchingOutputType(node, schema);
   if (typeWillBeConsolidated) {
     return "";
   }

diff --git a/src/definitions/object.ts b/src/definitions/object.ts
@@ -11,7 +11,12 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { GraphQLSchema, Kind, ObjectTypeDefinitionNode } from "graphql";
+import {
+  GraphQLSchema,
+  isInputObjectType,
+  isInterfaceType,
+  ObjectTypeDefinitionNode,
+} from "graphql";
 import { buildAnnotations } from "../helpers/build-annotations";
 import { indent } from "@graphql-codegen/visitor-plugin-common";
 import { buildTypeMetadata } from "../helpers/build-type-metadata";
@@ -58,11 +63,11 @@ ${getDataClassMembers({ node, schema, config, completableFuture: true })}
 }`;
   }
 
-  const potentialMatchingInputType = schema.getType(`${name}Input`)?.astNode;
-  const typeWillBeConsolidated = inputTypeHasMatchingOutputType(
-    schema,
-    potentialMatchingInputType,
-  );
+  const potentialMatchingInputType = schema.getType(`${name}Input`);
+  const typeWillBeConsolidated =
+    isInputObjectType(potentialMatchingInputType) &&
+    potentialMatchingInputType.astNode &&
+    inputTypeHasMatchingOutputType(potentialMatchingInputType.astNode, schema);
   const outputRestrictionAnnotation = typeWillBeConsolidated
     ? ""
     : "@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT])\n";
@@ -89,11 +94,13 @@ function getDataClassMembers({
       const typeMetadata = buildTypeMetadata(fieldNode.type, schema, config);
       const shouldOverrideField =
         !completableFuture &&
-        node.interfaces?.some((i) => {
-          const typeNode = schema.getType(i.name.value)?.astNode;
+        node.interfaces?.some((interfaceNode) => {
+          const typeNode = schema.getType(interfaceNode.name.value);
           return (
-            typeNode?.kind === Kind.INTERFACE_TYPE_DEFINITION &&
-            typeNode.fields?.some((f) => f.name.value === fieldNode.name.value)
+            isInterfaceType(typeNode) &&
+            typeNode.astNode?.fields?.some(
+              (field) => field.name.value === fieldNode.name.value,
+            )
           );
         });
       const fieldDefinition = buildFieldDefinition(

diff --git a/src/helpers/build-annotations.ts b/src/helpers/build-annotations.ts
@@ -16,6 +16,7 @@ import {
   EnumValueDefinitionNode,
   FieldDefinitionNode,
   InputValueDefinitionNode,
+  Kind,
   TypeDefinitionNode,
 } from "graphql";
 import { buildDescriptionAnnotation } from "./build-description-annotation";
@@ -60,8 +61,8 @@ export function buildAnnotations({
   ];
 
   const shouldIndent =
-    definitionNode?.kind === "FieldDefinition" ||
-    definitionNode?.kind === "InputValueDefinition";
+    definitionNode?.kind === Kind.FIELD_DEFINITION ||
+    definitionNode?.kind === Kind.INPUT_VALUE_DEFINITION;
   return (
     annotations
       .map((annotation) =>

diff --git a/src/helpers/build-description-annotation.ts b/src/helpers/build-description-annotation.ts
@@ -31,7 +31,7 @@ export function buildDescriptionAnnotation(
     (arg) => arg.name.value === "reason",
   )?.value;
   const deprecatedReason =
-    deprecatedReasonNode?.kind === "StringValue"
+    deprecatedReasonNode?.kind === Kind.STRING
       ? deprecatedReasonNode.value
       : "";
   const trimmedDeprecatedReason = trimDescription(deprecatedReason);

diff --git a/src/helpers/build-directive-annotations.ts b/src/helpers/build-directive-annotations.ts
@@ -15,6 +15,7 @@ import { CodegenConfigWithDefaults } from "./build-config-with-defaults";
 import { DefinitionNode } from "./build-annotations";
 import { getFederationDirectiveReplacement } from "./get-federation-directive-replacement";
 import { ConstDirectiveNode } from "graphql/language";
+import { Kind } from "graphql";
 
 export function buildDirectiveAnnotations(
   definitionNode: DefinitionNode,
@@ -65,7 +66,7 @@ function buildKotlinAnnotations(
             `Directive argument ${argumentToRetain} in directive ${directive.name.value} has an unsupported type. Only INT, FLOAT, STRING, BOOLEAN, and ENUM are supported.`,
           );
         const argumentValue =
-          argumentValueNode.kind === "StringValue"
+          argumentValueNode.kind === Kind.STRING
             ? `"${argumentValueNode.value}"`
             : argumentValueNode.value;
         return `${argumentToRetain} = ${argumentValue}`;

diff --git a/src/helpers/build-type-metadata.ts b/src/helpers/build-type-metadata.ts
@@ -18,6 +18,7 @@ import {
   TypeNode,
   isScalarType,
   isUnionType,
+  isInputObjectType,
 } from "graphql";
 import { getBaseTypeNode } from "@graphql-codegen/visitor-plugin-common";
 import { wrapTypeWithModifiers } from "@graphql-codegen/java-common";
@@ -76,10 +77,10 @@ export function buildTypeMetadata(
         shouldTreatUnionAsInterface ? schemaType.name : "Any",
       ),
     };
-  } else {
+  } else if (isInputObjectType(schemaType) && schemaType.astNode) {
     const typeWillBeConsolidated = inputTypeHasMatchingOutputType(
-      schema,
       schemaType.astNode,
+      schema,
     );
     const typeName = typeWillBeConsolidated
       ? getTypeNameWithoutInput(schemaType.name)
@@ -88,6 +89,11 @@ export function buildTypeMetadata(
       ...commonMetadata,
       typeName: buildListType(typeNode, typeName),
     };
+  } else {
+    return {
+      ...commonMetadata,
+      typeName: buildListType(typeNode, schemaType.name),
+    };
   }
 }
 

diff --git a/src/helpers/dependent-type-utils.ts b/src/helpers/dependent-type-utils.ts
@@ -14,6 +14,7 @@ limitations under the License.
 import {
   GraphQLSchema,
   GraphQLUnionType,
+  isUnionType,
   Kind,
   TypeDefinitionNode,
   TypeNode,
@@ -42,7 +43,7 @@ function getFieldTypeName(fieldType: TypeNode) {
 
 export function getDependentInterfaceNames(node: TypeDefinitionNode) {
   return node.kind === Kind.OBJECT_TYPE_DEFINITION
-    ? node.interfaces?.map((i) => i.name.value) ?? []
+    ? node.interfaces?.map((interfaceNode) => interfaceNode.name.value) ?? []
     : [];
 }
 
@@ -57,11 +58,9 @@ export function getDependentUnionsForType(
   node: TypeDefinitionNode,
 ) {
   const typeMap = schema.getTypeMap();
-  const unions = Object.keys(typeMap)
-    .filter(
-      (type) => typeMap[type]?.astNode?.kind === Kind.UNION_TYPE_DEFINITION,
-    )
-    .map((type) => typeMap[type] as GraphQLUnionType);
+  const unions = Object.values(typeMap).filter((type) =>
+    isUnionType(type),
+  ) as GraphQLUnionType[];
   return unions
     .filter((union) =>
       union.getTypes().some((type) => type.name === node.name.value),

diff --git a/src/helpers/input-type-has-matching-output-type.ts b/src/helpers/input-type-has-matching-output-type.ts
@@ -1,54 +1,61 @@
-import { Kind, TypeNode } from "graphql/index";
-import { GraphQLSchema, TypeDefinitionNode } from "graphql";
+import {
+  GraphQLSchema,
+  InputObjectTypeDefinitionNode,
+  isInputObjectType,
+  isObjectType,
+} from "graphql";
+import { getBaseTypeNode } from "@graphql-codegen/visitor-plugin-common";
 
 export function inputTypeHasMatchingOutputType(
+  inputNode: InputObjectTypeDefinitionNode,
   schema: GraphQLSchema,
-  typeNode?: TypeDefinitionNode | null,
 ) {
-  if (typeNode?.kind !== Kind.INPUT_OBJECT_TYPE_DEFINITION) {
-    return false;
-  }
-
-  const typeNameWithoutInput = getTypeNameWithoutInput(typeNode.name.value);
-  const matchingType = schema.getType(typeNameWithoutInput)?.astNode;
-  const matchingTypeFields =
-    matchingType?.kind === Kind.OBJECT_TYPE_DEFINITION
-      ? matchingType.fields
-      : [];
-  const inputFields = typeNode.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);
+  const inputName = inputNode.name.value;
+  const typeNameWithoutInput = getTypeNameWithoutInput(inputName);
+  const matchingTypeName =
+    schema.getType(typeNameWithoutInput)?.astNode?.name.value;
+  return Boolean(
+    matchingTypeName && typesAreEquivalent(matchingTypeName, inputName, schema),
+  );
 }
 
 export function getTypeNameWithoutInput(name: string) {
   return name.endsWith("Input") ? name.replace("Input", "") : name;
 }
 
-function fieldsAreEquivalent(
-  typeField: TypeNode,
-  inputField: TypeNode,
+function typesAreEquivalent(
+  typeName: string,
+  inputName: string,
+  schema: GraphQLSchema,
 ): 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)
-      );
+  const typeNode = schema.getType(typeName);
+  const inputNode = schema.getType(inputName);
+  if (!typeNode?.astNode && !inputNode?.astNode) {
+    return true;
   }
+  if (
+    !isObjectType(typeNode) ||
+    !isInputObjectType(inputNode) ||
+    !typeNode.astNode?.fields ||
+    !inputNode.astNode?.fields ||
+    typeNode.astNode.fields.length !== inputNode.astNode.fields.length
+  ) {
+    return false;
+  }
+
+  return typeNode.astNode.fields.every((typeField) => {
+    const matchingInputField = inputNode.astNode?.fields?.find(
+      (inputField) => inputField.name.value === typeField.name.value,
+    );
+    if (!matchingInputField?.type) return false;
+    const baseTypeName = getBaseTypeNode(typeField.type).name.value;
+    const baseInputTypeName = getBaseTypeNode(matchingInputField.type).name
+      .value;
+    const typeNamesAreEquivalent =
+      baseTypeName == getTypeNameWithoutInput(baseInputTypeName);
+    if (!typeNamesAreEquivalent) {
+      return false;
+    }
+    return typesAreEquivalent(baseTypeName, baseInputTypeName, schema);
+  });
 }
diff --git a/test/unit/should_consolidate_input_and_output_types/expected.kt b/test/unit/should_consolidate_input_and_output_types/expected.kt
@@ -77,3 +77,37 @@ interface MyTypeToConsolidateParent2 {
 interface MyTypeToConsolidateParent2CompletableFuture {
     fun field(input: MyTypeToConsolidate): java.util.concurrent.CompletableFuture<String?>
 }
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT])
+data class MyTypeNotToConsolidateParent(
+    val field: MyTypeNotToConsolidate2? = null
+)
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT])
+data class MyTypeNotToConsolidateParentInput(
+    val field: MyTypeNotToConsolidate2Input? = null
+)
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT])
+data class MyTypeNotToConsolidate2(
+    val field1: String? = null,
+    val field2: String? = null
+)
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT])
+data class MyTypeNotToConsolidate2Input(
+    val field1: String? = null
+)
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.OBJECT])
+data class MySuperSetType(
+    val field: String? = null,
+    val field2: String? = null
+)
+
+@GraphQLValidObjectLocations(locations = [GraphQLValidObjectLocations.Locations.INPUT_OBJECT])
+data class MySuperSetTypeInput(
+    val field: String? = null,
+    val field2: String? = null,
+    val field3: 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
@@ -98,3 +98,35 @@ input MyTypeToConsolidateInputParent {
 type MyTypeToConsolidateParent2 {
   field(input: MyTypeToConsolidateInput!): String
 }
+
+# case where parent types reference types that should not be consolidated
+
+type MyTypeNotToConsolidateParent {
+  field: MyTypeNotToConsolidate2
+}
+
+input MyTypeNotToConsolidateParentInput {
+  field: MyTypeNotToConsolidate2Input
+}
+
+type MyTypeNotToConsolidate2 {
+  field1: String
+  field2: String
+}
+
+input MyTypeNotToConsolidate2Input {
+  field1: String
+}
+
+# case where input fields are a superset of type fields
+
+type MySuperSetType {
+  field: String
+  field2: String
+}
+
+input MySuperSetTypeInput {
+  field: String
+  field2: String
+  field3: Int
+}