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

Support Lombok Annotations #1297

Open
hetzijzo opened this issue Dec 28, 2021 · 19 comments · Fixed by openrewrite/rewrite-migrate-java#124 · May be fixed by #4602
Open

Support Lombok Annotations #1297

hetzijzo opened this issue Dec 28, 2021 · 19 comments · Fixed by openrewrite/rewrite-migrate-java#124 · May be fixed by #4602
Assignees
Labels
bug Something isn't working enhancement New feature or request parser-java

Comments

@hetzijzo
Copy link
Contributor

hetzijzo commented Dec 28, 2021

org.openrewrite.java.cleanup.ExplicitInitialization and org.openrewrite.java.cleanup.UseDiamondOperator removes the argument type when it's a lombok.val variable.

val products = new ArrayList<Product>();

val products = new ArrayList<>();

which is incorrect and doesn't compile anymore.

@hetzijzo hetzijzo changed the title UnnecessaryExplicitTypeArguments does not take lombok val into account org.openrewrite.java.cleanup.ExplicitInitialization does not take lombok val into account Dec 28, 2021
@pway99 pway99 added the bug Something isn't working label Dec 28, 2021
@pway99
Copy link
Contributor

pway99 commented Dec 28, 2021

Hi @hetzijzo, Thanks for reporting the issue we'll take a look.

@pway99
Copy link
Contributor

pway99 commented Dec 28, 2021

Unfortunately, we cannot fix this issue until support for Lombok annotations and types are added to the rewrite framework.

@tkvangorder tkvangorder changed the title org.openrewrite.java.cleanup.ExplicitInitialization does not take lombok val into account Support Lombok Annotations Feb 9, 2022
@josemariavillar
Copy link

Good morning @pway99, is there any estimate to have the problem with the Lombok library solved?

@pway99
Copy link
Contributor

pway99 commented Mar 16, 2022

Good Morning @josemariavillar, It's a tricky problem that will require a significant effort; at this time, we do not have an estimate.

@hetzijzo
Copy link
Contributor Author

hetzijzo commented May 27, 2022

I'm trying to figure out to find a workaround. The case is the following tree of recipes:

Changes have been made to .....java by: org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4 org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration org.openrewrite.java.testing.junit5.JUnit4to5Migration org.openrewrite.java.testing.junit5.UseMockitoExtension org.openrewrite.java.testing.mockito.Mockito1to3Migration org.openrewrite.java.testing.mockito.CleanupMockitoImports

I have tried the following workarounds with little success

  1. Copy the complete rewrite recipe tree in my own recipe.yml and remove the not working CleanupMockitoImports. This is time-consuming, also for future rules that use the same tree structure. A printer (Add a printer to unfold and print declarative recipes. #1731 ) would come in very handy to support this workaround.
  2. Ignoring the Java file that is hit by the recipe excludes the complete file from being run by ALL recipes. This is not a correct working workaround.
  3. Not using lombok at all. This is a project risk since so many files will be changed. We also have a lot of templates for code generation that need to be changed as well.

Are there any other options?

@pway99
Copy link
Contributor

pway99 commented May 27, 2022

Hello @hetzijzo, That's a good question, I am not aware of a good workaround for Lombok issues at this time. That said in the spirit of do no harm each recipe should do its best to guard against making a breaking change due to invalid type information.

I took another look at the UseDiamondOperators recipe and was able to guard it against NewClass initializers associated with VariableDeclarations having a null or unknown type.

Are you able to provide some more information or create an issue for the CleanupMockitoImports problems your encountering?

pway99 added a commit that referenced this issue May 27, 2022
pway99 added a commit that referenced this issue May 27, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
@pway99
Copy link
Contributor

pway99 commented May 27, 2022

reverted accidental commit to the wrong branch

@hetzijzo
Copy link
Contributor Author

I have the following two Java files in a project

import lombok.data;

@Data
class MyObject {
  String someField;
}
import static org.mockito.Mockito.when;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class MyObjectTest {
  @Mock
  MyObject myObject;

  @Test
  void test() {
    when(myObject.getSomeField()).thenReturn("testValue");
    ......
  }
}

Now when I run the following command it will lead to a unwanted deleted static import of import static org.mockito.Mockito.when. The project will no longer compile.
The reason for the delete is that the Lombok annotation processor will normally add the getter & setter of the someField which is not detected or taken care of by the AST of openrewrite.

mvn org.openrewrite.maven:rewrite-maven-plugin:4.24.0:run -Dorg.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6 -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-java-security:1.11.0,org.openrewrite.recipe:rewrite-kubernetes:1.17.0,org.openrewrite.recipe:rewrite-logging-frameworks:1.7.0,org.openrewrite.recipe:rewrite-migrate-java:1.6.0,org.openrewrite.recipe:rewrite-spring:4.21.0,org.openrewrite.recipe:rewrite-testing-frameworks:1.22.0

The execution of openrewrite leads to the following change:

 Changes have been made to MyObjectTest .java by:
     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
                 org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration
                     org.openrewrite.java.testing.junit5.JUnit4to5Migration
                         org.openrewrite.java.testing.junit5.UseMockitoExtension
                             org.openrewrite.java.testing.mockito.Mockito1to3Migration
                                 org.openrewrite.java.testing.mockito.CleanupMockitoImports

PatrickViry pushed a commit that referenced this issue May 30, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 1, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
@pway99
Copy link
Contributor

pway99 commented Jun 1, 2022

Hi @hetzijzo, Thanks for the additional info on the CleanupMockitoImports recipe, I have updated it to be more defensive against removing imports for potential Mockito method invocations having null or missing type information. openrewrite/rewrite-testing-frameworks#228

Please don't hesitate to share any other issues you may find :)

PatrickViry pushed a commit that referenced this issue Jun 3, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 6, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 8, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 10, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 14, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 15, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 17, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
PatrickViry pushed a commit that referenced this issue Jun 30, 2022
… variables having a null or unknown type. (issue #1297)"

This reverts commit 3592bfc.
@timtebeek
Copy link
Contributor

Think I might have a start at replacing lombok.val, although there might be some limitations that I'm unaware about. Feel free to critique my approach. I figured since lombok.val is "just an interface" when parsing without Lombok support, we might be able to cross off that one already.

@pway99
Copy link
Contributor

pway99 commented Sep 9, 2022

Re-opening since openrewrite/rewrite-migrate-java#124 does not actually provide rewrite lombok annotation support.

@hetzijzo
Copy link
Contributor Author

I still run into a lot issues with Lombok & OpenRewrite when one of the rules start upgrading JUnit 4 to JUnit 5 or JUnit asserts to AssertJ asserts. The lombok generated methods are not recognized and therefore imports are removed all over the place.

I guess many more teams would have trouble with running OpenRewrite on a project that uses lombok. What are the plans for teams using lombok?

@timtebeek
Copy link
Contributor

Hi @hetzijzo ! I guess full support for Lombok is some way off, but perhaps there's something we can do already to prevent the imports from being removed; do you have a minimal example where you see imports being removed? That way we can try to capture that in a JUnit test and iterate more quickly.

@hetzijzo
Copy link
Contributor Author

Running the following command

mvn org.openrewrite.maven:rewrite-maven-plugin:4.39.0:run -Drewrite.activeRecipes=nl.fictive.bsp.UpgradeBspCoreLatest -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-java-security:1.21.0,org.openrewrite.recipe:rewrite-kubernetes:1.27.0,org.openrewrite.recipe:rewrite-logging-frameworks:1.17.0,org.openrewrite.recipe:rewrite-migrate-java:1.16.0,org.openrewrite.recipe:rewrite-spring:4.32.0,org.openrewrite.recipe:rewrite-testing-frameworks:1.33.0,nl.fictive.bsp:bsp-core-openrewrite-rules:7.7.1 -Drewrite.exclusions=**api**.yaml

On the folliwing Java file:

package nl.fictive.bsp.capability.serviceinzicht.service.client.notificatiebeheer;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;

import ch.qos.logback.classic.Level;
import java.util.Date;
import java.util.UUID;
import nl.fictive.bsp.capability.serviceinzicht.service.application.mortgageinquiry.cleaning.CleanupInquiryEvent;
import nl.fictive.bsp.capability.serviceinzicht.util.logging.MemoryAppender;
import nl.fictive.bsp.capability.notificatiebeheer.api.NotificatiesVersturenApi;
import nl.fictive.bsp.capability.notificatiebeheer.api.model.Notificatie;
import nl.fictive.bsp.core.identity.Bank;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.test.util.ReflectionTestUtils;
import org.springframework.web.client.RestClientException;
import org.wildfly.common.Assert;

@ExtendWith(MockitoExtension.class)
public class NotificatiebeheerServiceClientTest {

  @InjectMocks private NotificatiebeheerServiceClient client;
  @Mock private NotificatiesVersturenApi notificatiesVersturenApi;

  @Captor ArgumentCaptor<Notificatie> notificatie;

  @Test
  public void notifyByMail_ExceptionThrown() {
    // Arrange
    final MemoryAppender memoryAppender = new MemoryAppender(NotificatiebeheerServiceClient.class);
    UUID uuid = UUID.randomUUID();
    CleanupInquiryEvent event =
        CleanupInquiryEvent.builder()
            .batchRun(new Date())
            .message("TestInquiryEvent")
            .successful(true)
            .inquiryId(uuid)
            .build();
    doThrow(new RestClientException("test error message"))
        .when(notificatiesVersturenApi)
        .addNotificatie(eq(Bank.SOME.name().toLowerCase()), any(Notificatie.class));

    ReflectionTestUtils.setField(client, "emailAddress", "[email protected]");

    // Act
    client.notifyByMail(event);

    // Assert
    assertEquals(1, memoryAppender.countEventsForLogger("NotificatiebeheerServiceClient"));
    Assert.assertTrue(
        memoryAppender.contains(
            "Something went wrong while sending e-mail notification to [email protected]",
            Level.ERROR));
  }

  @Test
  public void notifyByMail() {
    // Arrange
    UUID uuid = UUID.randomUUID();
    CleanupInquiryEvent event =
        CleanupInquiryEvent.builder()
            .batchRun(new Date())
            .message("TestInquiryEvent")
            .successful(true)
            .inquiryId(uuid)
            .build();

    // Act
    client.notifyByMail(event);

    // Assert
    verify(notificatiesVersturenApi).addNotificatie(eq("SOME"), notificatie.capture());

    assertTrue(notificatie.getValue().getNotificatieTypes().contains("EMAIL"));
    assertTrue(
        notificatie.getValue().getOnderwerp().equals("Mortgage Inquiry Cleanup Scheduler Warning"));
    assertTrue(
        notificatie
            .getValue()
            .getInhoud()
            .startsWith("Voor beheer van"));
    assertTrue(notificatie.getValue().getInhoud().contains(uuid.toString()));
    assertTrue(notificatie.getValue().getInhoud().contains("SCHEDULED_CLEANUP_INQUIRY"));
    assertTrue(notificatie.getValue().getInhoud().contains("TestInquiryEvent"));
  }
}

Results with the following in the log:

[WARNING] Changes have been made to src\test\java\nl\fictive\bsp\capability\serviceinzicht\service\client\notificatiebeheer\NotificatiebeheerServiceClientTest.java by:
[WARNING]     nl.fictive.bsp.UpgradeBspCoreLatest
[WARNING]         nl.fictive.bsp.UpgradeBspCore7
[WARNING]             nl.fictive.bsp.UpgradeBspCore6
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                             org.openrewrite.java.testing.junit5.JUnit5BestPractices
[WARNING]                                 org.openrewrite.java.testing.junit5.StaticImports
[WARNING]                                     org.openrewrite.java.UseStaticImport: {methodPattern=org.junit.jupiter.api.Assertions *(..)}
[WARNING]                                 org.openrewrite.java.testing.cleanup.TestsShouldNotBePublic

And the file is changed like this:

package nl.fictive.bsp.capability.serviceinzicht.service.client.notificatiebeheer;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;

import ch.qos.logback.classic.Level;
import java.util.Date;
import java.util.UUID;
import nl.fictive.bsp.capability.serviceinzicht.service.application.mortgageinquiry.cleaning.CleanupInquiryEvent;
import nl.fictive.bsp.capability.serviceinzicht.util.logging.MemoryAppender;
import nl.fictive.bsp.capability.notificatiebeheer.api.NotificatiesVersturenApi;
import nl.fictive.bsp.capability.notificatiebeheer.api.model.Notificatie;
import nl.fictive.bsp.core.identity.Bank;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.test.util.ReflectionTestUtils;
import org.springframework.web.client.RestClientException;
import org.wildfly.common.Assert;

@ExtendWith(MockitoExtension.class)
class NotificatiebeheerServiceClientTest {

  @InjectMocks
  private NotificatiebeheerServiceClient client;
  @Mock
  private NotificatiesVersturenApi notificatiesVersturenApi;

  @Captor
  ArgumentCaptor<Notificatie> notificatie;

  @Test
  void notifyByMail_ExceptionThrown() {
    // Arrange
    final MemoryAppender memoryAppender = new MemoryAppender(NotificatiebeheerServiceClient.class);
    UUID uuid = UUID.randomUUID();
    CleanupInquiryEvent event =
        CleanupInquiryEvent.builder()
            .batchRun(new Date())
            .message("TestInquiryEvent")
            .successful(true)
            .inquiryId(uuid)
            .build();
    doThrow(new RestClientException("test error message"))
        .when(notificatiesVersturenApi)
        .addNotificatie(eq(Bank.SOME.name().toLowerCase()), any(Notificatie.class));

    ReflectionTestUtils.setField(client, "emailAddress", "[email protected]");

    // Act
    client.notifyByMail(event);

    // Assert
    assertEquals(1, memoryAppender.countEventsForLogger("NotificatiebeheerServiceClient"));
    Assert.assertTrue(
        memoryAppender.contains(
            "Something went wrong while sending e-mail notification to [email protected]",
            Level.ERROR));
  }

  @Test
  void notifyByMail() {
    // Arrange
    UUID uuid = UUID.randomUUID();
    CleanupInquiryEvent event =
        CleanupInquiryEvent.builder()
            .batchRun(new Date())
            .message("TestInquiryEvent")
            .successful(true)
            .inquiryId(uuid)
            .build();

    // Act
    client.notifyByMail(event);

    // Assert
    verify(notificatiesVersturenApi).addNotificatie(eq("SOME"), notificatie.capture());

    assertTrue(notificatie.getValue().getNotificatieTypes().contains("EMAIL"));
    assertTrue(
        notificatie.getValue().getOnderwerp().equals("Mortgage Inquiry Cleanup Scheduler Warning"));
    assertTrue(
        notificatie
            .getValue()
            .getInhoud()
            .startsWith("Voor beheer van some-service-inzicht"));
    assertTrue(notificatie.getValue().getInhoud().contains(uuid.toString()));
    assertTrue(notificatie.getValue().getInhoud().contains("SCHEDULED_CLEANUP_INQUIRY"));
    assertTrue(notificatie.getValue().getInhoud().contains("TestInquiryEvent"));
  }
}

It misses the import of import static org.junit.jupiter.api.Assertions.assertTrue;

@hetzijzo
Copy link
Contributor Author

@timtebeek I hope this helps a little

@timtebeek
Copy link
Contributor

And just to be sure; the release this morning hasn't helped right?

@hetzijzo
Copy link
Contributor Author

I tried the command with all the latest components released this morning with the same result.

@knutwannheden
Copy link
Contributor

@hetzijzo I wonder if this example has anything to do with Lombok. In the provided example class there are no Lombok annotations (or imports). The only unusual thing I can see is that org.wildfly.common.Assert is being imported and then used in a Assert.assertTrue() statement. I assume this is accidental. I tried to reproduce the problem with this lead but failed.

Can you check if the problem is due to this import? Is the full source code (with all dependencies) available on GitHub or can you provide a reproducer? That would really help to get this issue fixed.

@sambsnyd
Copy link
Member

It's been a long time coming but I think I have an approach that will work for this:
main...lombok-spike

@sambsnyd sambsnyd self-assigned this Oct 22, 2024
@sambsnyd sambsnyd added enhancement New feature or request parser-java labels Oct 23, 2024
@sambsnyd sambsnyd linked a pull request Oct 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request parser-java
Projects
Status: In Progress
7 participants