graphql-java-generator / graphql-maven-plugin-project

graphql-maven-plugin is a Maven Plugin for GraphQL, based on graphql-java. It accelerates the development for both the client and the server, by generating the Java code. It allows a quicker development when in contract-first approach, by avoiding to code the boilerplate code.
https://graphql-maven-plugin-project.graphql-java-generator.com
MIT License
115 stars 47 forks source link

JsonProperty annotation on generated class properties causes duplicate properties in generated OpenAPI/Swagger schema #175

Closed amalfatti closed 1 year ago

amalfatti commented 1 year ago

I'm using the GraphQL client generator (1.17.3) to generate a client for use in my REST application. This application generates an OpenAPI schema (via Swagger) which it uses to generate clients for my application in other languages ​​(Java, Javascript, etc...). The generated OpenAPI schema contains duplicate properties in the classes generated by the GraphQL ones, which for example cause problems in generating a Java client.

The problem seems to be the @JsonProperty annotation on the property declaration. Indeed, Swagger produces a property for both the annotation and the getter of the property itself.

In my opinion the solution would be to remove the @JsonProperty annotation or at least move it to the property getter.

Not being able to do this even by modifying the templates, I found a solution by modifying the "object_type.vm.java" template where the json autodetect was disabled by annotation:

...
@JsonAutoDetect(
fieldVisibility = JsonAutoDetect.Visibility.NONE,
setterVisibility = JsonAutoDetect.Visibility.NONE,
getterVisibility = JsonAutoDetect.Visibility.NONE,
isGetterVisibility = JsonAutoDetect.Visibility.NONE,
creatorVisibility = JsonAutoDetect.Visibility.NONE
)
${object.annotation}
public class ${targetFileName}
...

I don't really like this solution because I had to include in the template also the content of the "object_content.vm.java" template, which being imported was no longer seen by my custom template.

etienne-sf commented 1 year ago

Not being able to do this even by modifying the templates

Hum, yes, this is true (and bad). The @JsonProperty is added in the Java code, not in the template. This mix prevent you to correct it with the templates. I'd look to move this from the java code of the plugin, to the template.

Back to this issue:

The @JsonProperty is mandatory to properly manage GraphQL attributes that are java keywords (like break or while). But it would be possible to move it to the getter. Then, adding it to the setter, would probably be necessary.

But I don't to change that, unless you're sure it will solve this issue.

Can you provide a small project that would allow to repeat it ?

Etienne

amalfatti commented 1 year ago

Not being able to do this even by modifying the templates

Hum, yes, this is true (and bad). The @JsonProperty is added in the Java code, not in the template. This mix prevent you to correct it with the templates. I'd look to move this from the java code of the plugin, to the template.

it would be very useful for other customizations as well!

Back to this issue:

The @JsonProperty is mandatory to properly manage GraphQL attributes that are java keywords (like break or while). But it would be possible to move it to the getter. Then, adding it to the setter, would probably be necessary.

But I don't to change that, unless you're sure it will solve this issue.

Can you provide a small project that would allow to repeat it ?

Sure, I attach a project (demo.zip) created with spring boot initializr, where you can generate from a GraphQL schema a client and then use its models as output of a REST API (Swagger+Jersey). In the generated OpenAPI schema (you can do it with the "resolve" task) you can see that the Status property of the HelloWorldResult class is duplicated.

image

I'll add as a further detail that I didn't include in the first post, that the problem is highlighted on properties that start with a capital letter (I know it's not good practice to do this but unfortunately I find myself in this situation).

Also the response of the API, as you can see, reports the duplicate property, with initial capital and small letters

2023_02_06_11_45_42_

demo.zip

etienne-sf commented 1 year ago

Hello,

Thanks to the provided zip, I was able to confirm that there are two status fields.

But...

It seems to be a case issue.

If you change:

type HelloWorldResult {
    result: String
    Status: Int
}

by

type HelloWorldResult {
    result: String
    status: Int
}

Then the issue is solved.

The issue is this:

So there are two solutions here:

What do you think here ?

Etienne

amalfatti commented 1 year ago
  • My recommandation is to correct Status to status, and be compliant with usual naming rules

I know, but unfortunately I currently cannot edit the schema (it's a third-party schema).

  • Move the @jackson property to the getter (and setter, perhaps), you mask this difference.

This solution would effectively cause jackson to consider only the getters (I don't think the annotation is also needed on the setters) of the property and not its declaration (avoiding generating the duplicate).

Perhaps the @JsonAutoDetect option should also be considered, in my case it's working and doesn't require to move current jackson annotations.

etienne-sf commented 1 year ago

Actually, moving the annotation from the field to the getter would have impact on the plugin code. But thanks to your test project, I confirm that doubling the @JsonProperty("Status") on both the field and the getters works Ok for your use case.

I'll add that for the next release, which should comme soon.

etienne-sf commented 1 year ago

BTW, this could be done by using custom templates ;)

But it's easier for you to have it embed into the core package, and it could be useful for other such usages.

amalfatti commented 1 year ago

Actually, moving the annotation from the field to the getter would have impact on the plugin code. But thanks to your test project, I confirm that doubling the @JsonProperty("Status") on both the field and the getters works Ok for your use case.

I'll add that for the next release, which should comme soon.

Are you telling me then that adding the @JsonProperty(xxx) annotation on getters/setters as well without removing it from the field declaration generates a single property (without its duplicate property status) in the schema?

If so, then I'd say that's fine.

etienne-sf commented 1 year ago

Yes, it's actually exactly what I've done in the test project you provided. And it solves the problem, for me.

It will be available in the next version, but you can try to add this in a custom templates in the meanwhile.

amalfatti commented 1 year ago

Perfect! I also tried to modify the template as you say and it actually works. I will still wait for the next release. Thank you!

etienne-sf commented 1 year ago

Released in the 1.18.10 version