leangen / graphql-spqr

Build a GraphQL service in seconds
Apache License 2.0
1.09k stars 179 forks source link

Issue where the generated field of my output type is not the same as my POKO entity? #462

Open wojcik-marcin opened 1 year ago

wojcik-marcin commented 1 year ago

Discussed in https://github.com/leangen/graphql-spqr/discussions/461

Originally posted by **wojcik-marcin** June 5, 2023 I would like to request some guidance with an issue I am experiencing. I am making use of the SPQR Spring Boot Starter v0.0.7 within my Kotlin Spring Boot application. I have a Person data class defined as follows: ``` kotlin data class Person( val name: String, val isMarried: Boolean ) ``` I have a @GraphQLMutation endpoint to update the Person ``` kotlin @GraphQLMutation fun updatePerson(@GraphQLArgument(name = "person") person: Person): Person { person.isMarried = true return Person } ``` The GraphQL schema types generated are as follows: ``` graphql type Person { name: String! married: Boolean! } type PersonInput { name: String! isMarried: Boolean! } ``` As you can see, the input type generates the property name for `isMarried` correctly, however, I expected the same for the Person type that is being returned as the output type. Instead, it is returning as `married`. I am using Jackson as my JSON serializer, and adding `@JsonProperty("isMarried")` to the field in the data class doesn't seem to work. How can I enforce that the output type's `isMarried` property generates correctly to be the same as that of what is defined in the Person data class? Any assistance in this matter will be greatly appreciated.
kaqqao commented 1 year ago

I'm not sure what names Kotlin ends up generating for the getter and setter, but it seems like it does not generate JavaBeans compliant names (which in this case would be isIsMarried or getIsMarried, and setIsMarried). Side note: this is why it's normally preferred in Java to simply name boolean fields x and not isX, as the related getter will be named isX(). I guess Kotlin generated isMarried() and setIsMarried, which would explain what SPQR ends up seeing.

Anyway, the simplest would be to append @GraphQLQuery(name = "isMarried") on the field, or the getter (maybe via @get:GraphQLQuery?). I'm unfortunately not familiar with Kotlin, so can't say exactly. Please report back what happened once you figure it out, as I'd like to learn what Kotlin does here.

The bottom line is that SPQR by default expects the JavaBeans spec to be respected (field, getField or isField, and setField), or explicitly overridden via annotations (@GraphQLQuery for output types, and @GraphQLInputField for input types, with a fallback to @GraphQLQuery if @GraphQLInputField isn't specified). So you can use these annotations to explicitly set the names.

Advanced usage

You almost certainly don't need to do this, unless you're doing some deep customization, like supporting a new set of annotations, or a JVM language with strange naming conventions. But, you can always provide a custom OperationInfoGenerator that implements any naming logic you desire, or maybe even use one of the existing ones, and set it to BeanResolverBuilder.

E.g. to replace the naming logic the default BeanResolverBuilder uses with the one that simply leave the method name untouched if no annotation is present:

generator.withNestedResolverBuilders((config, defaultBuilders) -> defaultBuilders
       // Customize the built-in BeanResolverBuilder
      .replace(BeanResolverBuilder.class, def -> def.withOperationInfoGenerator(new MemberOperationInfoGenerator())))

Or to provide a fully custom naming logic:

private class CustomOperationInfoGenerator extends AnnotatedOperationInfoGenerator {

        @Override
        public String name(OperationInfoGeneratorParams params) {
            // `elements` contains the field, the getter and the setter, is they exist
            List<AnnotatedElement> elements = params.getElement().getElements();
            // Find the field, if it exists
            Optional<String> field = Utils.extractInstances(elements, Field.class).findFirst().map(Field::getName);
            // Return the explicit name from the annotation, if it exists. If not, the name of field.
            // If that is also missing, return null and let the default logic take over
            return Utils.coalesce(super.name(params), field.orElse(null));
        }
}

// Delegates to your custom impl, and falls back to the defaults if yours returns `null`
OperationInfoGenerator customNaming = new DefaultOperationInfoGenerator()
        .withDelegate(new CustomOperationInfoGenerator());

generator.withNestedResolverBuilders((config, defaultBuilders) -> defaultBuilders
      // Customize the built-in BeanResolverBuilder
      .replace(BeanResolverBuilder.class, def -> def.withOperationInfoGenerator(customNaming)))
wojcik-marcin commented 1 year ago

Thanks for your response.

I added the @GraphQLQuery annotation to the isMarried property as follows:

@GraphQLType(name = "Person")
data class Person(
    val name: String,
    @GraphQLQuery(name = "isMarried")
    var isMarried: Boolean? = null
)

That did not seem to work. I am still seeing the generated GraphQL type returning the property as married.

I will try with a CustomOperationInfoGenerator and give feedback.

wojcik-marcin commented 1 year ago

When I changed the name of the isMarried field in to married, the @GraphQLQuery(name = "isMarried") annotation successfully renamed the field in the GraphQLType to isMarried.

@GraphQLType(name = "Person")
data class Person(
    val name: String,
    @GraphQLQuery(name = "isMarried")
    var married: Boolean? = null
)

However, if I leave the name of the field in my data class as isMarried, then it seems as though the annotation is being ignored

Could it possibly because SPQR does not detect it as a ElementType.FIELD when the name of the field starts with get, set or is?

IceBlizz6 commented 12 months ago

I think i may have figure out something. The codebase is using jackson to resolve and name fields. Try to add jackson-module-kotlin dependency to your project.

implementation("com.fasterxml.jackson.module:jackson-module-kotlin:2.9.8") Let me know if it works for you. I just tried it and it looks very promising.

IceBlizz6 commented 12 months ago

Ignore that last post (although you may still want jackson-module-kotlin to solve other issues) I believe the solution here is to just write a SchemaTransformer. I made an attempt at this earlier as i had the same problem.

This has not been properly tested, but it may be a starting point for you:

class PreserveBooleanFieldName : SchemaTransformer {
    override fun transformField(
        field: GraphQLFieldDefinition,
        operation: Operation,
        operationMapper: OperationMapper,
        buildContext: BuildContext
    ): GraphQLFieldDefinition {
        return if (isBoolean(field.type)) {
            GraphQLFieldDefinition
                .newFieldDefinition(field)
                .name(getOriginalName(operation.typedElement))
                .build()
        } else {
            field
        }
    }

    override fun transformInputField(
        field: GraphQLInputObjectField,
        inputField: InputField,
        operationMapper: OperationMapper,
        buildContext: BuildContext
    ): GraphQLInputObjectField {
        return if (isBoolean(field.type)) {
            GraphQLInputObjectField
                .newInputObjectField(field)
                .name(getOriginalName(inputField.typedElement))
                .build()
        } else {
            field
        }
    }

    private fun isBoolean(type: GraphQLType): Boolean {
        val typeName = type.toString()
        return typeName == "Boolean" || typeName == "Boolean!"
    }

    private fun getOriginalName(typedElement: TypedElement): String {
        val elements = typedElement.elements
        val method = elements.filterIsInstance<Method>().singleOrNull()
        val field = elements.filterIsInstance<Field>().singleOrNull()
        if (method != null) {
            return method.name
        } else if (field != null) {
            return field.name
        } else {
            error("No elements found")
        }
    }
}

@kaqqao Do you have a better suggestion for the isBoolean implementation? that string equals check doesn't look great.