search

jooby-project / jooby

The modular web framework for Java and Kotlin
https://jooby.io
Apache License 2.0
1.7k stars 200 forks source link

Reflective Bean Converter and APT (ParamDefinition) have inconsistent Nullable/NonNull logic. #3408

Closed agentgt closed 3 months ago

agentgt commented 4 months ago

We need to check for @Nullable first like param definition:

Reflective Bean Converter nullable/nonnull check:

https://github.com/jooby-project/jooby/blob/1c7c381bc9d4cfbbcbd79ef99e962ab80481e2f7/jooby/src/main/java/io/jooby/internal/converter/ReflectiveBeanConverter.java#L246

ParamDefinition nullable/nonnull check:

https://github.com/jooby-project/jooby/blob/1c7c381bc9d4cfbbcbd79ef99e962ab80481e2f7/modules/jooby-apt/src/main/java/io/jooby/internal/apt/ParamDefinition.java#L95

I thought this was originally my change so I made a big deal on discord but it appears the issue is just that reflective bean converter just has different logic.

This will allow in bean-like to do something like:

record(@jakarta.validation.constraints.NotNull @Nullable UUID someIdToBeFilledLater){}
agentgt commented 4 months ago

@jknack @edgarespinawt let me know if you want me to put in a PR. I think its a pretty straight forward change so I didn't bother.

I made a big stink about ReflectiveBeanConverter on discord because I thought it was my fault (I too got mixed up with ParamDefinition and ReflectiveBeanConverter) and only found out about it being different when trying to convert Springs PetClinic to Jooby.

The ParamDefinition logic of checking Nullable I think is the ideal logic and I think will get us 90% there to handle most use cases.

Anyway feel free to task with me with something to make up for the confusion I caused.

edgarespinawt commented 4 months ago

yea, send a PR and we can move from there. Please don't use my other account.

agentgt commented 4 months ago

Please don't use my other account.

So its @edgarespinawt going forward correct? (hopefully I don't ping you on the wrong one again :) )

Sorry about that. I wasn't clear if one of them was because of some sort of automation and you were accidentally logged in. Does this include handlebars.java as well?

jknack commented 4 months ago

hey the public account its @jknack... just realize I commented with my work account :D

agentgt commented 4 months ago

Yeah kindly ask to try not to do that. The XZ thing has made my company and various others paranoid.

I'm guilty for pinging you on it as well. jknack for now on 😄

Also sorry for the discord overload of walls of paragraphs. I struggle with being pithy/concise.