-
Notifications
You must be signed in to change notification settings - Fork 0
Share response types across queries in client generator #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Share response types across queries in client generator #11
Conversation
- Add sharedClassNameCache and sharedTypeToSelectionSetMap to enable global deduplication - Update GraphQLClientGeneratorContext to accept shared caches as constructor parameters - Add objectClassToTypeSpecs map to store shared response types - Modify generateTypeName.kt to use shared package '.types' for response types (objects, interfaces, unions) - Response types now generated in com.expediagroup.graphql.generated.types instead of query-specific packages - Migrate test expectations to match new shared package structure - Update all test query files to import from .types package This enables response types to be generated once and reused across multiple queries/mutations, eliminating duplicate classes and reducing the need for manual mapping. Co-Authored-By: Arthur Poon <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Lines 185 to 279 in e791543
| private fun updateImplementationTypeSpecWithSuperInformation( | |
| context: GraphQLClientGeneratorContext, | |
| interfaceName: String, | |
| implementationName: String, | |
| implementationClassName: ClassName, | |
| commonProperties: List<PropertySpec> | |
| ) { | |
| val commonPropertyNames = commonProperties.map { it.name } | |
| val implementationTypeSpec = context.typeSpecs[implementationClassName]!! | |
| val builder = implementationTypeSpec.toBuilder() | |
| val superClassName = ClassName("${context.packageName}.${context.operationName.lowercase()}", interfaceName) | |
| if (context.serializer == GraphQLSerializer.KOTLINX) { | |
| builder.addAnnotation( | |
| AnnotationSpec.builder(SerialName::class) | |
| .addMember("value = %S", implementationName) | |
| .build() | |
| ) | |
| .superclass(superClassName) | |
| } else { | |
| builder.addSuperinterface(superClassName) | |
| } | |
| if (implementationTypeSpec.propertySpecs.isNotEmpty()) { | |
| val constructor = FunSpec.constructorBuilder() | |
| implementationTypeSpec.propertySpecs.forEach { property -> | |
| val updatedProperty = if (commonPropertyNames.contains(property.name)) { | |
| property.toBuilder().addModifiers(KModifier.OVERRIDE).build() | |
| } else { | |
| property | |
| } | |
| // add the property to the type builder only if the property was actually uptaded. | |
| if (updatedProperty != property) { | |
| builder.addProperty(updatedProperty) | |
| } | |
| constructor.addParameter(updatedProperty.name, updatedProperty.type) | |
| } | |
| builder.primaryConstructor(constructor.build()) | |
| } | |
| val updatedType = builder.build() | |
| context.polymorphicTypes[superClassName]?.add(implementationClassName) | |
| context.typeSpecs[implementationClassName] = updatedType | |
| } | |
| private fun generateFallbackImplementation(context: GraphQLClientGeneratorContext, interfaceName: String, commonProperties: List<PropertySpec>): ClassName { | |
| val fallbackTypeName = "Default${interfaceName}Implementation" | |
| val superClassName = ClassName("${context.packageName}.${context.operationName.lowercase()}", interfaceName) | |
| val fallbackClassName = ClassName("${context.packageName}.${context.operationName.lowercase()}", fallbackTypeName) | |
| val fallbackType = TypeSpec.classBuilder(fallbackTypeName) | |
| .addAnnotation(Generated::class) | |
| .addKdoc("Fallback $interfaceName implementation that will be used when unknown/unhandled type is encountered.") | |
| .also { | |
| if (context.serializer == GraphQLSerializer.KOTLINX) { | |
| it.addAnnotation(Serializable::class) | |
| .superclass(superClassName) | |
| .addKdoc("\n\nNOTE: This fallback logic has to be manually registered with the instance of GraphQLClientKotlinxSerializer. See documentation for details.") | |
| } else { | |
| it.addSuperinterface(superClassName) | |
| } | |
| if (commonProperties.isNotEmpty()) { | |
| it.addModifiers(KModifier.DATA) | |
| } | |
| } | |
| .addProperties( | |
| commonProperties.map { abstractProperty -> | |
| abstractProperty.toBuilder() | |
| .initializer(abstractProperty.name) | |
| .addModifiers(KModifier.OVERRIDE) | |
| .also { | |
| it.modifiers.remove(KModifier.ABSTRACT) | |
| } | |
| .build() | |
| } | |
| ) | |
| .primaryConstructor( | |
| FunSpec.constructorBuilder() | |
| .addParameters( | |
| commonProperties.map { propertySpec -> | |
| val constructorParameter = ParameterSpec.builder(propertySpec.name, propertySpec.type) | |
| val className = propertySpec.type as? ClassName | |
| if (propertySpec.type.isNullable) { | |
| constructorParameter.defaultValue("null") | |
| } else if (className != null && context.enumClassToTypeSpecs.keys.contains(className)) { | |
| constructorParameter.defaultValue("%M", className.member(UNKNOWN_VALUE)) | |
| } | |
| constructorParameter.build() | |
| } | |
| ) | |
| .build() | |
| ) | |
| .build() | |
| context.polymorphicTypes[superClassName]?.add(fallbackClassName) |
Object and interface response types are now emitted in com.expediagroup.graphql.generated.types, but generateInterfaceTypeSpec still builds superclass and fallback class names using ${context.packageName}.${context.operationName.lowercase()}. When an interface is encountered, the generated implementations and the Default…Implementation class extend/implement a type in the old query‑specific package that no longer exists, and the implementations are never registered in context.polymorphicTypes because the map is keyed by the shared package. Any client containing an interface now produces Kotlin files that reference missing types and fail to compile. The superclass package should be updated to the shared .types package before this change can work.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tion - Update generateInterfaceTypeSpec to use shared .types package instead of query-specific packages - Ensures superClassName and fallbackClassName match the package used in generateTypeName - Fixes filtering logic to correctly exclude polymorphic types from shared types - Reduces test failures from 14 to 1 (only kotlinx/multiple_queries remaining) Co-Authored-By: Arthur Poon <[email protected]>
- Update FirstQuery.kt with correct import order and remove default value for enumQuery - Add ComplexObject2.kt for FirstQuery's selection set on ComplexObject type - Both files now match generator output exactly Co-Authored-By: Arthur Poon <[email protected]>
📝 Description
This PR implements deduplication and sharing of GraphQL response types across multiple queries/mutations, similar to how input types and enums are already shared.
Problem: Previously, identical response types were generated multiple times in query-specific packages (e.g.,
com.expediagroup.graphql.generated.firstquery.ComplexObjectandcom.expediagroup.graphql.generated.secondquery.ComplexObject), creating duplicate classes that required manual mapping.Solution: Response types are now:
.typespackage (e.g.,com.expediagroup.graphql.generated.types)ComplexObject,ComplexObject2)Key Changes:
sharedClassNameCacheandsharedTypeToSelectionSetMapto share deduplication state across all queriesGraphQLClientGeneratorContextto delegate to shared cache instances.typespackage instead of query-specific packagesobjectClassToTypeSpecsmap to track shared response types.firstquery) to the shared.typespackage.Testing:
🔗 Related Issues
Requested by @arthurkkp-cog in Devin session 601a68caa9db4dc6ac8acc0839282f0f
Human Review Checklist:
kotlinx/multiple_queries/test data whereBasicInterfaceandScalarWrapperare shared betweenFirstQueryandSecondQuery)ComplexObjectvsComplexObject2in multiple_queries test)generateTypeName.ktlines 111-180 for edge cases