MigrateAuditorAwareToOptional should not rewrite already Optional return type

[tubbynl]

Versions / config

• OpenRewrite Maven v6.2.3
• rewrite-spring 6.2.1

I am using the Maven plugin, and my project is a multi module project but inherits the same parent pom build plugin:

<plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>6.2.3</version>
                <configuration>
                    <activeRecipes>
                        <recipe>org.openrewrite.java.migrate.UpgradeToJava21</recipe>
                        <recipe>org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2</recipe>
                        <recipe>org.openrewrite.java.testing.junit5.JUnit5BestPractices</recipe>
                    </activeRecipes>
                    <failOnDryRunResults>true</failOnDryRunResults>
                    <!-- below entries are to work around known issues in above recipes, see readme) -->
                    <exclusions>
                        <!-- UpgradeSpringBoot_3_2 adds virtual.threads flag in wrong file -->
                        <exclusion>**/application.properties</exclusion>
                    </exclusions>
                    <!-- UpgradeToJava21 adds junit4 exclusion to testcontainers; breaking the build -->
                    <skipMavenParsing>true</skipMavenParsing>
                </configuration>
                <dependencies>
                    <dependency>
                        <groupId>org.openrewrite.recipe</groupId>
                        <artifactId>rewrite-spring</artifactId>
                        <version>6.2.1</version>
                    </dependency>
                </dependencies>
            </plugin>

Reproduce snippet

import org.springframework.data.domain.AuditorAware;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.data.jpa.repository.JpaRepository;

public class UserConfiguration {
     
    @Bean
    public UserService userDetailsService(UserRepository users) {
        return username -> {
            try {
                return users.findByEmail(username).orElseThrow().toUserDetails();
                // it rewrites to 
                // return Optional.ofNullable(users.findByEmail(username).orElseThrow().toUserDetails());
            } catch (Throwable t)) {
                throw new UsernameNotFoundException("o gosh",t);
            }
     }
}

public interface UserService extends AuditorAware<String>, UserDetailsService {
}

public interface UserRepository extends JpaRepository<User> {
    Optional<User> findByEmail(String email);
}

Expected

To retain the original line because it already has a Optional type

Are you interested in contributing a fix to OpenRewrite?

• Yes, if my issue is valid i'd like to contribute a fix

[bsmahi]

Hi @tubbynl,

In this context return users.findByEmail(username).orElseThrow().toUserDetails(); is correct one because

• users.findByEmail(username) returns an Optional<User>.
• .orElseThrow() ensures that if no user is found, an exception is thrown.
• .toUserDetails() is then called on the User object.

However, the rewritten version return Optional.ofNullable(users.findByEmail(username).orElseThrow().toUserDetails());

• Wraps the UserService inside an Optional<UserService>, which changes the return type to Optional<UserService>.
• This is incorrect if your method expects UserService as the return type.

So we can stick to first version unless the method is supposed to return Optional<UserService>

Thanks, Mahi

[tubbynl]

somewhat;

Optional<User> findByEmail(String email);

the original code is functional

return users.findByEmail(username);
// returns Optional<User>

the rewritten code is not

return Optional.ofNullable(users.findByEmail(username);
// returns Optional<Optional<User>>

so; the recipe should not alter function invocations which already return a Optional (yes; it should retain the original code)

[tubbynl]

this issue has somewhat degraded using maven-rewrite 6.7.0 and spring recipe 6.6.0

return Optional.of(Optional.ofNullable(users.findByEmail(username));
// returns Optional<Optional<Optional<User>>>

[tubbynl]

extra side-note; if i promote the anonymous class the recipe does work correctly

[timtebeek]

Only seeing this issue now; thanks for the report @tubbynl ! A fix would be appreciated indeed, perhaps starting with a unit test for the corresponding recipe, as seen here: https://github.com/openrewrite/rewrite-spring/blob/3cd7f61670d4013c11f83913275ad01b434747ff/src/test/java/org/openrewrite/java/spring/data/MigrateAuditorAwareToOptionalTest.java#L38-L68

A draft PR with just a test is usually a good way to kick this off, then from there tag folks for help as needed.

[tubbynl]

seems fixed in maven-rewrite 6.9.0 and spring recipe 6.7.0