projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.72k stars 2.36k forks source link

[FEATURE] Make lombok process jakarta.validation.NotNull and other jakarta null-prohibiting annotations as @NonNull #3645

Open Evemose opened 3 months ago

Evemose commented 3 months ago

Make lombok process jakarta.validation.NotNull and other jakarta null-prohibiting annotations as NonNull annotaion

Problem Its common issue for developers working with validation annotations in jakarta/javax packages to have to duplicate nullity annotations, which leads to potential inconsistencies due to unneccesary code duplication.

Solution Treat annotations that prohibit null values by specification same as NonNull annotaion

Target audience Vast majority of web developers deal with jakarta validation annotations. Server side development is a most common use of Java, so feature would be helpful for virtually every Java developer.

Potential issues Neccesity to keep track of jakarta specification updates and custom annotations from validation providers like Hibernate

Context If lombok team would approve such feature, I could implement it myself, but I would like to know if maintainers of project think whether it would be expedient or not.

rulojuka commented 1 month ago

This would be very helpful to me. So, a +1 from my side.

Also, as a suggestion, I figure this should come as a configuration option because there will be projects that already use the jakarta validations and do not want them to be processed by lombok.

Edit: I researched and thought for a while that https://projectlombok.org/api/lombok/ConfigurationKeys#ADD_NULL_ANNOTATIONS did what we wanted but it does the opposite. Anyway, it seems to me that the existence of ADD_NULL_ANNOTATIONS makes the existence of READ_NULL_ANNOTATIONS (or something similar) very natural.

vladimirshefer commented 2 weeks ago

I use the jakarta annotation for documentation purposes and lombok generates the rundtime checks for it, breaking my project. I don't see any option to disable runtime checks. If I would like to add runtime checks, then I will use lombok.NonNull. Please let me avoid runtime checks for jakarta annotations!

Evemose commented 1 week ago

I use the jakarta annotation for documentation purposes and lombok generates the rundtime checks for it, breaking my project. I don't see any option to disable runtime checks. If I would like to add runtime checks, then I will use lombok.NonNull. Please let me avoid runtime checks for jakarta annotations!

This could be a configurable option that is disabled by default. However, I don't see practical example of where @NonNull is not applicable to field that is annotated with @NotNull besides some legacy code backward compatibility. Still, configuration key could be a win-win compromise

Evemose commented 1 week ago

So, I finally spared some time so I will put some work in it. Issue has been out there for quite a bunch of time, so I hope I could take it as a slight green from Lombok team. Check for updates in closest future, I'm going to open PR as soon as possible

vladimirshefer commented 1 week ago

I don't see practical example of where @NonNull is not applicable to field that is annotated with @NotNull

I have a DTO class with @NotNull annotated field. In most cases this field is really never null. But sometimes I need to accept kind of Partial<FooDto> - with only several fields set to non-null. For instance, in PATCH requests, the value null means "not changed".

Runtime checks for @NotNull fail this usecase for me.

Evemose commented 1 week ago

I don't see practical example of where @NonNull is not applicable to field that is annotated with @NotNull

I have a DTO class with @NotNull annotated field. In most cases this field is really never null. But sometimes I need to accept kind of Partial<FooDto> - with only several fields set to non-null. For instance, in PATCH requests, the value null means "not changed".

Runtime checks for @NotNull fail this usecase for me.

That is a fair point. However, there is a few things to it:

  1. As far as I know, any deserialization/binding library will not invoke setters unless value is directly specified in request. Therefore, this is not a problem for setters since: 1) If value isnt present, setters should not be invoked and exception will not be thrown. 2) If field value is directly specified as null in request, this is more like a bug that you accept this request body as valid. Still, this may be required for the sake of backwards compatibility.

  2. More serious issue is null checks in constructor, especially with records, as for classes you could just declare protected no-args constructor and most providers would use it as default. I am not sure how to work this around, as there is no erticular way to pass metadata about current validation group to constructor. It is possible to just let people disable generating null checks on constructors by annotating params with something like @Nullable. It is a shaky workaround, however, still provides more security than just leaving things as it is. It might a good idea to think on separate behaviour (or just disable it) for records though