search

openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.27k stars 338 forks source link

Support Lombok Annotations #1297

Open hetzijzo opened 2 years ago

hetzijzo commented 2 years ago

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.

pway99 commented 2 years ago

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

pway99 commented 2 years ago

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

josemariavillar commented 2 years ago

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

pway99 commented 2 years ago

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

hetzijzo commented 2 years ago

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 (#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 commented 2 years ago

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 commented 2 years ago

reverted accidental commit to the wrong branch

hetzijzo commented 2 years ago

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
pway99 commented 2 years ago

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. https://github.com/openrewrite/rewrite-testing-frameworks/pull/228

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

timtebeek commented 2 years ago

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 commented 2 years ago

Re-opening since https://github.com/openrewrite/rewrite-migrate-java/pull/124 does not actually provide rewrite lombok annotation support.

hetzijzo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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", "tester@fictive.nl");

    // Act
    client.notifyByMail(event);

    // Assert
    assertEquals(1, memoryAppender.countEventsForLogger("NotificatiebeheerServiceClient"));
    Assert.assertTrue(
        memoryAppender.contains(
            "Something went wrong while sending e-mail notification to tester@fictive.nl",
            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", "vobankfinancieeladvies@fictive.nl");

    // Act
    client.notifyByMail(event);

    // Assert
    assertEquals(1, memoryAppender.countEventsForLogger("NotificatiebeheerServiceClient"));
    Assert.assertTrue(
        memoryAppender.contains(
            "Something went wrong while sending e-mail notification to vobankfinancieeladvies@fictive.nl",
            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 commented 1 year ago

@timtebeek I hope this helps a little

timtebeek commented 1 year ago

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

hetzijzo commented 1 year ago

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

knutwannheden commented 1 year ago

@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 commented 1 month ago

It's been a long time coming but I think I have an approach that will work for this: https://github.com/openrewrite/rewrite/compare/main...lombok-spike

sambsnyd commented 2 weeks ago

After further experimentation we've identified a number of shortcomings in the current lombok support. Some of it pretty substantial, so I'm going to re-open this while I work through it.

Here's roughly what still needs to happen: