JSpecifyBestPractices ignores my annotation and rewrites the code

[beeet]

Setup

        <plugin>
            <groupId>org.openrewrite.maven</groupId>
            <artifactId>rewrite-maven-plugin</artifactId>
            <version>6.38.0</version>
            <executions>
                <execution>
                    <id>run-rewrite</id>
                    <phase>process-sources</phase>
                    <goals>
                        <goal>run</goal>
                    </goals>
                </execution>
            </executions>
            <configuration>
                <exportDatatables>false</exportDatatables>
                <activeRecipes>
                    <recipe>org.openrewrite.java.jspecify.JSpecifyBestPractices</recipe>
                </activeRecipes>
            </configuration>
            <dependencies>
                <dependency>
                    <groupId>org.openrewrite.recipe</groupId>
                    <artifactId>rewrite-migrate-java</artifactId>
                    <version>3.34.0</version>
                </dependency>
        </plugin>

My Code BEFORE running rewrite (without JSpecify):

   public byte[] generate(final PrintMode printMode, final PdfFragebogenUiStateDto uiStateDto) throws PdfGenerationException {
        if (printMode == null) {
            throw new IllegalArgumentException("my concrete business exception");
        }
        if (uiStateDto == null) {
            throw new IllegalArgumentException("my concrete business exception");
        }
        //... below the logic
}

My Code AFTER running rewrite:

   public byte[] generate(@NonNull final PrintMode printMode, @NonNull final PdfFragebogenUiStateDto uiStateDto) throws PdfGenerationException {
        // the throwing of the IllegalArgumentExceptions is removed
        //... below the logic
}

I understand this behaviour of removing the exceptions, if NO JSpecify-Annotation is configured.

But if I want to throw an explicit business-related exception (we handle it in the ExceptionHandler), then that shouldn't be rewritten. In such cases, we want to display business-related messages.

So, the following code shouldn't IMHO not rewritten:

   public byte[] generate(@Nullable final PrintMode printMode, @Nullable final PdfFragebogenUiStateDto uiStateDto) throws PdfGenerationException {
        if (printMode == null) {
            throw new IllegalArgumentException("my concrete business exception");
        }
        if (uiStateDto == null) {
            throw new IllegalArgumentException("my concrete business exception");
        }
        //... below the logic
}

[timtebeek]

hi! I'm wondering a bit how to interpret your request, since you're using @Nullable in your title and final example, but also mention looking at messages and custom exceptions. Should we:

1. retain any checks when parameters are already annotated?
2. retain any checks when custom messages are provided?
3. retain any checks when custom exception are used?
4. retain any checks when requireNonNull has a message argument?

[beeet]

Maybe I don't fully understand the framework's philosophy.

In my opinion, the throwing of exceptions should generally not be expanded, because this alters the business logic in the code.
For example, error messages displayed to the user in the UI would be lost.

If an annotation has been explicitly defined, it is therefore questionable why it is being changed. In my case, from @nullable to @nonnull.

The resulting problem is that the recipe keeps correcting itself over and over again (since there is no @IgnoreRewrite annotation), so I have to exclude the entire class, which is too broad a scope (other recipes then stop working as well)

Conclusion:

• If @nullable or @nonnull is already annotated, do not change anything
• Generally, do not remove business exceptions (whether with or without a custom message) and, if necessary, choose the weaker annotation @nullable