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

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

Open
Helmsdown opened this issue May 22, 2024 · 6 comments
Assignees
Labels
type: enhancement A general enhancement

Comments

@Helmsdown
Copy link

Helmsdown commented May 22, 2024

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

@Helmsdown Helmsdown changed the title NulllableUtils only considers javax.annotation nullability annotations, should it also consider jakarta.annoation nullability annotations? NulllableUtils only considers javax.annotation nullability annotations, should it also consider jakarta.annotation nullability annotations? May 22, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 22, 2024
@mp911de
Copy link
Member

mp911de commented May 23, 2024

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

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

mp911de commented May 24, 2024

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

@Helmsdown
Copy link
Author

Helmsdown commented May 26, 2024

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 mp911de added for: team-attention An issue we need to discuss as a team to make progress type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-attention An issue we need to discuss as a team to make progress labels Jun 24, 2024
@mp911de
Copy link
Member

mp911de commented Jun 26, 2024

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

@mp911de mp911de self-assigned this Jun 26, 2024
@sdeleuze
Copy link
Contributor

FYI JSpecify 1.0.0 has been released.

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

No branches or pull requests

4 participants