Skip to content
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

customTypeMapping, generateExtensionFieldsResolvers and Extension Fields produce invalid parameter names #964

Closed
martinlau opened this issue Mar 4, 2022 · 4 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@martinlau
Copy link

Issue Description

When using customTypeMapping and generateExtensionFieldsResolvers to reference objects defined outside of the GraphQL Schema, for example:

  "customTypesMapping": {
    "External": "com.github.martinlau.external.model.External"
  },
  "generateExtensionFieldsResolvers": true

Combined with extension fields on existing types:

type External {
    name: String!
}

extend type External {
    age: Int!
}

Resolver classes are generated with parameter names based on the mapped value, rather than the original (non-qualified) value:

    fun age(com.github.martinlau.external.model.External: com.github.martinlau.external.model.External): Int

(Kotlin above - same issue arises with Java)

Steps to Reproduce

Refer to https://github.com/martinlau/graphql-codegen

Expected Result

Resolvers should be generated with a name based on the original type, rather than the fully qualified mapped type

    fun age(external: com.github.martinlau.external.model.External): Int

Actual Result

Resolvers are generated with a name based on the fully qualified mapped type.

    fun age(com.github.martinlau.external.model.External: com.github.martinlau.external.model.External): Int

See https://github.com/martinlau/graphql-codegen/blob/main/target/generated-sources/graphql/com/github/martinlau/graphql/resolver/ExternalResolver.kt for the full example

Your Environment and Setup

  • graphql-java-codegen version: 5.4.0
  • Build tool: Maven
  • Mapping Config:
<configuration>
  <graphqlSchemas>
    <rootDir>${project.basedir}/src/main/resources/graphql</rootDir>
    <excludedFiles>
      <excludedFile>models.graphqls</excludedFile>
    </excludedFiles>
  </graphqlSchemas>
  <configurationFiles>
    <configurationFile>${project.basedir}/src/build/graphql-codegen.json</configurationFile>
  </configurationFiles>
  <outputDir>${project.build.directory}/generated-sources/graphql</outputDir>
</configuration>

And

{
  "apiPackageName": "com.github.martinlau.graphql.resolver",
  "apiRootInterfaceStrategy": "DO_NOT_GENERATE",
  "customTypesMapping": {
    "External": "com.github.martinlau.external.model.External"
  },
  "generateApisWithThrowsException": false,
  "generateBuilder": false,
  "generateDataFetchingEnvironmentArgumentInApis": false,
  "generateExtensionFieldsResolvers": true,
  "generateImmutableModels": true,
  "generateModelsForRootTypes": false,
  "generatedLanguage": "KOTLIN",
  "modelPackageName": "com.github.martinlau.graphql.model",
  "packageName": "com.github.martinlau.graphql"
}
@martinlau martinlau added the bug Something isn't working label Mar 4, 2022
martinlau pushed a commit to martinlau/graphql-java-codegen that referenced this issue Mar 5, 2022
@martinlau
Copy link
Author

martinlau commented Mar 5, 2022

This commit shows the broken broken test case.

I believe the fix is in com.kobylynskyi.graphql.codegen.mapper.FieldDefinitionsToResolverDataModelMapper#getOperationParameters:

        // 1. First parameter is the parent object for which we are resolving fields (unless it's the root Query)
        if (!Utils.isGraphqlOperation(parentTypeName)) {
            String parentObjectParamType = graphQLTypeMapper
                    .getLanguageType(mappingContext, new TypeName(parentTypeName));
            String parentObjectParamName = dataModelMapper

// The below is where the parameter type name is generated
// * parentObjectParamType = "com.example.Something"
// * parentTypeName = "Something"

//                  .capitalizeIfRestricted(mappingContext, Utils.unCapitalize(parentObjectParamType));
                    .capitalizeIfRestricted(mappingContext, Utils.unCapitalize(parentTypeName));
            ParameterDefinition parameterDefinition = new ParameterDefinition();
            parameterDefinition.setType(parentObjectParamType);
            parameterDefinition.setName(parentObjectParamName);
            parameterDefinition.setOriginalName(parentObjectParamName);
            parameterDefinition.setDeprecated(DeprecatedDefinitionBuilder.build(mappingContext, resolvedField));
            parameters.add(parameterDefinition);
        }

parentObjectParamType is the fully qualified, mapped type. Couple of options:

  1. Use parent type name as defined in the original schema, rather than the fully qualified mapped type, which I think makes sense and what is included in the above; or
  2. Trim the package of the parentObjectParamType (something like parentObjectParamType.substringAfterLast(".")) - this has the benefit of being consistent with current behaviour (so existing users won't start getting warnings for overridden parameter names being different)

@jxnu-liguobin
Copy link
Collaborator

jxnu-liguobin commented Mar 6, 2022

Exactly, It seems that all langs have the same problem.

This seems feasible.

2. Trim the package of the parentObjectParamType (something like parentObjectParamType.substringAfterLast(".")) - this has the benefit of being consistent with current behaviour (so existing users won't start getting warnings for overridden parameter names being different)

@kobylynskyi
Copy link
Owner

@martinlau thanks a lot for the detailed explanation and the proposed fix.
I've created a PR with the change: #979

@kobylynskyi
Copy link
Owner

Will be released as part of version 5.4.1 #980

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants