openrewrite / rewrite-spring

OpenRewrite recipes for Spring projects.
Apache License 2.0
237 stars 64 forks source link

ReplaceStringLiteralsWithMediaTypeConstants does not work in test source code. #546

Open mgvinuesa opened 5 days ago

mgvinuesa commented 5 days ago

What version of OpenRewrite are you using?

I am using:

How are you running OpenRewrite?

I'm using maven plugin on my own project.

You can check the project here: https://github.com/mgvinuesa/spring-petclinic-openrewrite

It is a fork of spring-petclinic and you might use the branch with the name migrate_with_open_rewrite, I started with the TAG 1.5.x to migrate boot Spring Boot and Java using OpenRewrite.

What is the smallest, simplest way to reproduce the problem?

Execute the mvn command

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.activeRecipes=org.openrewrite.java.spring.http.ReplaceStringLiteralsWithMediaTypeConstants

What did you expect to see?

There is a test VetControllerTestswith the following code

@Test
    public void testShowResourcesVetList() throws Exception {
        ResultActions actions = mockMvc.perform(get("/vets.json").accept(MediaType.APPLICATION_JSON))
            .andExpect(status().isOk());
        actions.andExpect(content().contentType("application/json;charset=UTF-8"))
            .andExpect(jsonPath("$.vetList[0].id").value(1));
    }

The string "application/json;charset=UTF-8" should be changed to the MediaType enum. I know that this string is deprecated, so maybe could be that problem, but if I change it to "application/json" nothing happends again.

Apart from that the documentation has a mistake, the links in the menu are swapped:

ReplaceStringLiteralsWithMediaTypeConstants refers to the http header and ReplaceStringLiteralsWithHttpHeadersConstants to MediaTypes.

image

What did you see instead?

Nothing happens

[INFO] Using active recipe(s) [org.openrewrite.java.spring.http.ReplaceStringLiteralsWithMediaTypeConstants]
[INFO] Using active styles(s) []
[INFO] Validating active recipes...
[INFO] Project [petclinic] Resolving Poms...
[INFO] Project [petclinic] Parsing source files
[INFO] Running recipe(s)...
[INFO] Printing Available Datatables to: target\rewrite\datatables\2024-06-28_11-26-14-151

What is the full stack trace of any errors you encountered?

No errors, only the result is not the proper one.

timtebeek commented 5 days ago

Thanks for the sharp eye there spotting that the class names were swapped. I've fixed that now in https://github.com/openrewrite/rewrite-spring/commit/d49e38ed41300f6234cabc9794fce3959912f894

I've also had a brief look at testing the recipes themselves, with a new test class.

package org.openrewrite.java.spring.http;

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.InMemoryExecutionContext;
import org.openrewrite.java.JavaParser;
import org.openrewrite.java.ReplaceStringLiteralWithConstant;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;

class ReplaceStringLiteralsWithMediaTypeConstantsTest implements RewriteTest {

    @Override
    public void defaults(RecipeSpec spec) {
        spec
          .recipe(new ReplaceStringLiteralWithConstant("org.springframework.http.MediaType.APPLICATION_JSON_VALUE"))
          .parser(JavaParser.fromJavaVersion().classpathFromResources(new InMemoryExecutionContext(), "spring-web-6.+", "spring-core-6.+"));
    }

    @DocumentExample
    @Test
    void replaceHttpHeaderLiteralWithConstant() {
        rewriteRun(
          //language=java
          java(
            """
              class Test {
                  void foo() {
                      String httpHeader = "application/json";
                  }
              }
              """,
            """
              import org.springframework.http.MediaType;

              class Test {
                  void foo() {
                      String httpHeader = MediaType.APPLICATION_JSON_VALUE;
                  }
              }
              """
          )
        );
    }
}

This test currently fails to find the class constant, possibly because we're not passing in a classloader in the recipe itself: https://github.com/openrewrite/rewrite/blob/6b659111e2683d5958e8eef331aff53ddca559ea/rewrite-java/src/main/java/org/openrewrite/java/ReplaceStringLiteralWithConstant.java#L158

Seems like this is an oversight that might have failed the recipe for functioning properly. We'd need to explore this in more detail. Thanks for pointing it out! Any help with further debugging appreciated as well. :)

mgvinuesa commented 5 days ago

Hello @timtebeek in fact, I was going to add more information before you replied me. To solve this issue at the moment I was going to use the recipe that you commented (ReplaceStringLiteralWithConstant):

---
type: specs.openrewrite.org/v1beta/recipe
name: com.example.ReplaceStringLiteralWithConstantMediaType
displayName: Replace String literal with constant example
recipeList:
  - org.openrewrite.java.ReplaceStringLiteralWithConstant:
      literalValue: application/json;charset=UTF-8
      fullyQualifiedConstantName: org.springframework.http.MediaType.APPLICATION_JSON_VALUE

But this only works if I add the spring-web dependency in the maven plugin configuration:

    <dependencies>
            <dependency>
                <groupId>org.openrewrite.recipe</groupId>
                <artifactId>rewrite-spring</artifactId>
                <version>5.14.0-SNAPSHOT</version>
            </dependency>
            <dependency>
                <groupId>org.springframework</groupId>
                <artifactId>spring-web</artifactId>
                <version>6.1.0</version>
            </dependency>

        </dependencies>

If not, the execution returned this error:

Recipe validation error in fullyQualifiedConstantName: No class for specified name was found.

I don´t know if this helps, but anyway I think that this point maybe have to be reviewed o documented. Thanks for your support.

timtebeek commented 5 days ago

Thanks! It's indeed that second plugin dependency where we need to be smarter and use the project classpath, not the plugin classpath when looking for the Spring classes and constants defined on those. That'll be something to work out still.