smallrye / smallrye-graphql

Implementation for MicroProfile GraphQL
Apache License 2.0
155 stars 89 forks source link

Fixed wrapper nonnull types #2141

Closed mskacelik closed 3 weeks ago

mskacelik commented 3 weeks ago

fixes:#1366 NonNull annotation is only applied now where it needs to be (in wrapper hierarchy) There are some maybe weird behaviors when it comes to arrays: @NonNull String[] => [String!]! even tho it should be [String]!.

some other changes:

/cc @jmartisk

jmartisk commented 3 weeks ago

cc @robp94 if you want to help us verify this fix

robp94 commented 3 weeks ago

I will try to test it, since our services are still on quarkus 3.8.x I am not sure if I can test it with them.

jmartisk commented 3 weeks ago

It should be possible to cherry-pick this to the 2.8.x branch (which is used by Quarkus 3.8.x).

Or use the branch of this PR, and build Quarkus from Marek's branch: https://github.com/mskacelik/quarkus/tree/srgql-2.9.0 - that one should be compatible with the smallrye-graphql main (you'll need to set the smallrye-graphql.version in bom/application/pom.xml to 2.9.0-SNAPSHOT, build a Quarkus snapshot, and then build an application against the correct Quarkus snapshot version (which will be 999-SNAPSHOT)

robp94 commented 3 weeks ago

I updated my old reproducers: code-with-quarkus-nonnull.zip code-with-quarkus-nonnull-kotlin.zip

I had some errors building quarkus on Marek's branch, but probably not relevant for the reproducers. (Formatings on some kotlin modules)

The java reproducer now looks good, the kotlin one still fails. Should the kotlin one be fixed?

mskacelik commented 3 weeks ago

Dont think so. I only fixed Java variation.

jmartisk commented 3 weeks ago

The code for handling Kotlin nullability is a different story, let's treat it separately. Could you file a separate issue please? (Maybe just copy the Kotlin-related stuff from the original one, and we will close that one) Thanks for verifying!

robp94 commented 3 weeks ago

Ok, I make a new one.