search

smallrye / smallrye-graphql

Implementation for MicroProfile GraphQL
Apache License 2.0
160 stars 92 forks source link

Add more info to the "Ignoring non null" message #2181

Open jmartisk opened 2 months ago

jmartisk commented 2 months ago 
    2024-09-04T22:16:39.6005201Z 2024-09-04 22:16:39,512 WARN [io.sma.gra.sch.hel.NonNullHelper] (build-11) Ignoring non null on [java.lang.String] as there is a @DefaultValue

To make the message a bit more useful, we should attach some info saying which argument this is related to, This message is logged by io.smallrye.graphql.schema.helper.NonNullHelper

t1 commented 2 months ago

….and it should not be a warning, should it?

jmartisk commented 2 months ago

Hmm you're not the first person pointing this out, but I disagree, because it means that the framework has decided to override something that the user wrote (the user added NonNull but we still make it nullable in the end)

t1 commented 2 months ago

Hmmm... you're right. Then we should make it an error and fail the deployment, if a @NonNull is combined with a @DefaultValue, shouldn't we?

jmartisk commented 2 months ago

That makes sense. @gsmet would you agree, since you're the first person who brought this up?

gsmet commented 2 months ago

FWIW, if you make it an error, I think the Quarkus CI will fail, given I found out about this message in the Quarkus CI. Well, except we have a bug that won't trigger the codestarts test as the dependencies are not properly declared.

But that might be a good way to find out who the culprit is.

Ideally, do the change, deploy a snapshot of SmallRye GraphQL, update the Quarkus BOM, build the entire Quarkus tree and try running mvn -f integration-tests/devtools clean verify.

This is where I got this thing: one of the codestarts test somehow triggers it but I wasn't able to reproduce it by isolating a specific test.

You have an example of the warning here: https://github.com/quarkusio/quarkus/actions/runs/10710128172/job/29696739970 . Look for NonNull in the logs.

jmartisk commented 2 months ago

Ah crap, it seems a class from the TCK uses this: https://github.com/eclipse/microprofile-graphql/blob/main/server/tck/src/main/java/org/eclipse/microprofile/graphql/tck/apps/superhero/model/Item.java#L156 so it breaks the TCKs completely, because the schema won't build at all.. And I don't think we can easily work around this.

t1 commented 2 months ago

Crap. Maybe we can sneak a bugfix release in? @phillip-kruger ?