Skip to content

Conversation

@Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Aug 27, 2022

Overview

Closes #539.

This cleans up the usage of @NotNull/@Nonnull/@Nullable and improves the situation by adding @ParametersAreNonnullByDefault to all packages.

The changes are:

  • adding @ParametersAreNonnullByDefault to all existing package-info.java (in application and database)
  • removing all @NotNull from all method parameters (its the default now)
  • replacing all @NotNull on return types and similar by @Nonnull to consistently use javax.annotations
  • replacing all @Nullable from Jetbrains by @Nullable from javax.annotations
  • adding some missing @Nonnull/@Nullable on some return types
  • removed some suppressions that do not seem to be important for Sonar and only cause confusion for people who do not have the inspections enabled

Fun fact, with these changes, Sonar even found a missing @Nullable in TopHelperCommand.

Review

This is a huge PR, but with very minor and simple changes. It will also confuse a lot of merge conflicts (but simple conflicts). Ideally we merge this PR as soon as possible, as it will get conflicts with each merge while it is open.

For reviewing this, it is only really important to make sure that I neither removed a @Nullable, nor replaced one with @Nonnull or similar.

It is not important to actually really read the code. Skimming over the changes should be sufficient.

It is possible that some formatting went weird due to the heavy annotation removal + spotless. If you found any odd looking spot, tell in review and I will correct it.

@Zabuzard Zabuzard added enhancement New feature or request priority: low labels Aug 27, 2022
@Zabuzard Zabuzard self-assigned this Aug 27, 2022
@Zabuzard Zabuzard requested review from a team as code owners August 27, 2022 14:20
@Tais993
Copy link
Member

Tais993 commented Aug 27, 2022

Add a way so sonarcloud flags notnull/nullable from Jetbrains?

@Zabuzard
Copy link
Member Author

Add a way so sonarcloud flags notnull/nullable from Jetbrains?

Good idea. I just checked but didnt find any rule. Unfortunately, we can not add custom rules with Sonarcloud (need -qube for that).

So we would need a self-written test in the code using reflection. Not a big deal, but welp...

@Mom0aut
Copy link
Contributor

Mom0aut commented Aug 27, 2022

Does the @ParametersAreNonnullByDefault work with public and private methods?

@Zabuzard
Copy link
Member Author

It works with all methods in the whole package with the annotation. The way this PR sets it up, all code in the applications and database gradle modules has not-null as default for all method parameters now.

@Mom0aut
Copy link
Contributor

Mom0aut commented Aug 27, 2022

It works with all methods in the whole package with the annotation. The way this PR sets it up, all code in the applications and database gradle modules has not-null as default for all method parameters now.

Rly nice change, just as curiosity is the possible to add for example @nullable to private methods or it is now impossible to have null parameters in private methods?

@Zabuzard
Copy link
Member Author

Yes. It is just about the implicit default, which is now @Nonnull. If you want a nullable parameter, just annotate with @Nullable.

Note that its still necessary to always annotate return types - no default for that.

As a side note, these annotations actually dont add checks in the code. You can still pass null into something annotated with @NotNull. However, clever IDEs like Intellij and tools like Sonar will just heavily shout around - so usually its hard to mess up like that.

@Zabuzard Zabuzard force-pushed the feature/parameters_are_nonnull_by_default branch from 7d598ad to beb8f9b Compare August 28, 2022 08:17
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
1.0% 1.0% Duplication

@Zabuzard
Copy link
Member Author

Merging this, seems to be fine - if not, we can act afterwards. Want to accelerate development for this, need to get out a release for all the major improvements. Also, its weekened and I want to code a bit 😄

@Zabuzard Zabuzard enabled auto-merge (squash) August 28, 2022 08:31
@Zabuzard Zabuzard disabled auto-merge August 28, 2022 08:31
@Zabuzard Zabuzard merged commit 1b178ba into develop Aug 28, 2022
@Zabuzard Zabuzard deleted the feature/parameters_are_nonnull_by_default branch August 28, 2022 08:31
@Mom0aut
Copy link
Contributor

Mom0aut commented Sep 1, 2022

I tried to use the @ParametersAreNonnullByDefault in my other project and if i wanna call a method with a null parameter the ide warns me about this null paramater. But in the end the method could be executed with null, is this the expected behavior or should the annotions create a null check and forbid the method call with null?

@Zabuzard
Copy link
Member Author

Zabuzard commented Sep 1, 2022

Its expected. The annotations only add IDE warnings and nothing else. They do not actually manipulate the code at all, nor add real security in the code.

An alternative for that would be Lombok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce @ParametersAreNonnullByDefault

3 participants