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

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

Open
jmartisk opened this issue Sep 6, 2024 · 7 comments
Open

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

jmartisk opened this issue Sep 6, 2024 · 7 comments

Comments

@jmartisk
Copy link
Member

jmartisk commented Sep 6, 2024

    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
Copy link
Collaborator

t1 commented Sep 6, 2024

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

@jmartisk
Copy link
Member Author

jmartisk commented Sep 6, 2024

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
Copy link
Collaborator

t1 commented Sep 6, 2024

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
Copy link
Member Author

jmartisk commented Sep 6, 2024

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

@gsmet
Copy link
Contributor

gsmet commented Sep 6, 2024

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
Copy link
Member Author

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
Copy link
Collaborator

t1 commented Sep 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants