spring-projects / spring-data-commons

Spring Data Commons. Interfaces and code shared between the various datastore specific implementations.
https://spring.io/projects/spring-data
Apache License 2.0
778 stars 675 forks source link

NulllableUtils only considers javax.annotation nullability annotations, should it also consider jakarta.annotation nullability annotations? #3100

Open Helmsdown opened 5 months ago

Helmsdown commented 5 months ago

Greetings. I am upgrading from Spring Boot 2.7 to Spring Boot 3. That means I'm going from spring-data-commons:2.7.18 to spring-data-commons:3.2.5. Additionally this upgrade is bringing along a Hibernate 5.4 to 6.4.4 migration. Hibernate 6+ is all in on jakarta.annotation stuff.

In fact the automation my company wrote to help with the upgrade generously refactored all usages in my repo of javax.annotation to jakarta.annotation.

However, now a JPA query class I have is making org.springframework.data.repository.core.support.MethodInvocationValidator sad because the method is annotated with jakarta.annotation.Nullable but NullableUtils does evaluate that annotation when deciding whether something is or is not documented as "nullable".

Here are some questions to guide this discussion:

  1. It seems that this strategy of "if this one annotation class is on the classpath, then that must be the right one to use" is a bit...myopic. Especially given the code falls back to org.springframework.lang.Nullable, reactor.util.lang.NonNullApi, and rg.springframework.lang.NonNullApi if it can't find javax.annotation.Nullable. Is it fair to say "this strategy could be improved upon"?
  2. Do you agree that the current state of NullabileUtils is at best a paper cut when upgrading to SB3 and Spring 6 (and at worst a blocker if you use spring-data-jpa)?
  3. Would you support an annotation resolution strategy that is as follows? a. Evaluate what nullability annotations are on the classpath (javax, jakarta, spring, reactor, spotbugs, findbugs, intellij, etc) b. When attempting to evaluate if a given parameter or return type are nullable, consult all annotations that are present on the method or parameter in a deterministic order, if any are present, use them. c. bonus points: if there is a disagreement in the annotations (e.g. both Nullable and Nonnull) throw an exception

I look forward to hearing back from you

mp911de commented 5 months ago

TL;DR: Supporting JSR-305 annotations (avax.annotation.Nullable) instead of the Jakarta packages is a consequence of Spring Framework's support for these annotations. It is not only the meta-annotation org.springframework.lang.Nullable but also runtime support for annotation discovery.

Annotated elements in Java do not cause class loading issues if annotations aren't found, instead, annotations not available during runtime are just invisible to the reflection API. That is the source of why we fall back to Spring's annotations if JSR-305 isn't on the class path. We do not intend adding class-file parsing as such an approach would resort in many other issues that we would have to solve and added complexity.

If you want to evaluate nullability rules for bare JSR-305-annotated elements, then JSR-305 classes must be present during runtime.

I do not agree that our nullable utils are a harm. Instead, the entire package migration from javax to jakarta has been a harm to the entire Java eco-system. Consequences follow from there by migrating more packages than necessary into the Jakarta package name space.

We do not intend to be a holistic facility for all possible null annotations. We merely take advantage from some annotations being present in a natural consequence. Each framework comes with its own, slightly adjusted semantics, there are efforts to create yet-another-non-null annotation framework.

That being said, this is a problem to be solved in a different place while we're dogfooding based on Reactor and Spring annotations.

Helmsdown commented 5 months ago

I want to make sure I understand your position. Is it fair to rephrase your position as the following?

"Even if you upgrade your project to SB 3, Spring6, and Hibernate 6, and want to lean into the jakarta nullability annotations, if you happen to have the javax.annotation.Nonnull on your class path you must use that to document nullability otherwise you will run afoul of this validation"

mp911de commented 5 months ago

Yeah, that is fair to say. Going forward, did you file a ticket against Spring Framework to support Jakarta nullability annotations?

Helmsdown commented 5 months ago

Forgive me, but hasn't that ship already sailed?

in your application source code: e.g. the javax to jakarta namespace change in Jakarta EE 9 wherever you're touching the Servlet API, JPA, Bean Validation, etc.

Doesn't the "namespace change" cover this?

Someone playing devil's advocate would point to this.

Spring annotations are meta-annotated with JSR 305 annotations (a dormant but widespread JSR). JSR-305 meta-annotations let tooling vendors like IDEA or Kotlin provide null-safety support in a generic way, without having to hard-code support for Spring annotations.

It is neither necessary nor recommended to add a JSR-305 dependency to the project classpath to take advantage of Spring’s null-safe APIs. Only projects such as Spring-based libraries that use null-safety annotations in their codebase should add com.google.code.findbugs:jsr305:3.0.2 with compileOnly Gradle configuration or Maven provided scope to avoid compiler warnings.

But I would argue the meta-annotations are to support findbugs/spotbugs tooling. The meta-annotations do not mean that JSR305 javax.annotations are sacrosanct, and the only option if they appear on the classpath.

Am I making any sense?

mp911de commented 4 months ago

We discussed nullability annotation coverage in the team. jakarta.annotation-api introduced with version 2.1 a slim variant of nullability annotations that are a shallow copy of JSR305 (without when or @TypeQualifierDefault) so they do not correspond with JSR305 in any way.

With JSpecify on the horizon, it makes sense to revisit our nullability arrangement. The growth in nullability annotations requires on one hand a more flexible model, on the other hand, we cannot support each and every nullability annotation.

A nuanced path forward is to support:

  1. Our Spring annotations, regardless the presence of JSR305
  2. Support JSR305 annotations directly and through meta-annotations (i.e. if a library uses its own annotations that are JSR305 meta-annotated) if JSR305 is on the class path
  3. Support JSpecify annotations if JSpecify is on the class path
sdeleuze commented 4 months ago

FYI JSpecify 1.0.0 has been released.