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

Development: Migrate nullness annotations to JSpecify #9219

Closed
wants to merge 3 commits into from

Conversation

Strohgelaender
Copy link
Contributor

@Strohgelaender Strohgelaender commented Aug 15, 2024

Checklist

General

Motivation and Context

In #6872 I added a test to consistently use the Jakarta-Nullness Annotations in Java. This choice was not made since the annotation provides some kind of Advantage, but just because it was used previously in most places.

JSpecify is an Attempt to unify nullness annotations in Java. See https://jspecify.dev/docs/start-here/ for more details. The project sounds promising, let's hope it does not end like this:

standards

Description

This PR is mostly search-and-replace of imports. The only change is done in context of Arrays. @Nullable Object[] means that the array can contain null values, where Object @Nullable [] means that the Array itself can be null. I adopted the places where this is relevant.

Steps for Testing

skim over the code.

Code Review

  • Code Review 1
  • Code Review 2

@Strohgelaender Strohgelaender requested a review from a team as a code owner August 15, 2024 11:27
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) documentation labels Aug 15, 2024
Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of using the jspecify Nullable instead of the jakarta Nullable?

@Strohgelaender
Copy link
Contributor Author

What's the benefit of using the jspecify Nullable instead of the jakarta Nullable?

First there is no downside, the JSpecify annotations are already supported by all relevant tools. In my short experience just while creating this PR there seems to be even better support since IntelliJ reported to cases of primitive types annotated with such annotations which does not make sense, and this warning was not shown with the old ones.

JSpecify additionally contains the Annotation @NullMarked to infer null-safety (You can annotate a class with that which then means that only @Nullable fields are nullable and the rest is non-null by default). We could adapt classes to this behaviour in the future.

JSpecify also contains generics support for these annotations which is not the case for the other ones (obviously this is currently unused in Artemis since this is just a migration but could be useful in the future).

And lastly I think the project's aim is great and should get support, which is only possible if pojects use these annotations.

Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are consistent and tried basic functionalities of artemis on TS1, nothing seems to be broken trough the nullcheck changes (would also be weird but just wanted to be sure)

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some tests are failing as a result of these changes. Can you have a look?

Comment on lines +15 to 17
public record KnowledgeAreaRequestDTO(@NonNull @Size(min = 1, max = KnowledgeArea.MAX_TITLE_LENGTH) String title,
@NonNull @Size(min = 1, max = KnowledgeArea.MAX_SHORT_TITLE_LENGTH) String shortTitle, @Size(max = KnowledgeArea.MAX_DESCRIPTION_LENGTH) String description,
Long parentId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to no longer reject null as title / shortTitle values, causing a 500 instead of a 400 in the tests.

MockHttpServletResponse:
           Status = 500
    Error message = null
          Headers = [Vary:"Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers", Content-Version:"1.3.3-beta7", Content-Type:"application/problem+json", X-Content-Type-Options:"nosniff", X-XSS-Protection:"0", Cache-Control:"no-cache, no-store, max-age=0, must-revalidate", Pragma:"no-cache", Expires:"0", Content-Security-Policy:"script-src 'self' 'unsafe-inline' 'unsafe-eval'", Referrer-Policy:"strict-origin-when-cross-origin", Permissions-Policy:"camera=(), fullscreen=(*), geolocation=(), gyroscope=(), magnetometer=(), microphone=(), midi=(), payment=(), sync-xhr=()"]
     Content type = application/problem+json
             Body = {
  "type" : "https://www.jhipster.tech/problem/problem-with-message",
  "title" : "Internal Server Error",
  "status" : 500,
  "detail" : "could not execute statement [NULL not allowed for column \"TITLE\"; SQL statement:\ninsert into knowledge_area (description,parent_id,short_title,title,id) values (?,?,?,?,default) [23502-224]] [insert into knowledge_area (description,parent_id,short_title,title,id) values (?,?,?,?,default)]; SQL [insert into knowledge_area (description,parent_id,short_title,title,id) values (?,?,?,?,default)]; constraint [null]",
  "path" : "/api/admin/standardized-competencies/knowledge-areas",
  "message" : "error.http.500"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess JSpecify is not directly supported by the checker that is run before endpoints are called. If there is no way to fix this, this would be a huge downside to using them IMO, as these automatic checks are rather beneficial.

Comment on lines +16 to +17
public record StandardizedCompetencyRequestDTO(@NonNull @Size(min = 1, max = StandardizedCompetency.MAX_TITLE_LENGTH) String title,
@Size(max = StandardizedCompetency.MAX_DESCRIPTION_LENGTH) String description, CompetencyTaxonomy taxonomy, @NonNull Long knowledgeAreaId, Long sourceId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, title and knowledgeAreaId being annotated with NonNull seems to cause problems in the tests (the corresponding tests pass once I revert the annotations here and in KnowledgeAreaRequestDTO)

@Strohgelaender
Copy link
Contributor Author

We need to wait for Spring-support, see spring-projects/spring-framework#28797.

Closing this, I'll re-open this once Spring support is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants