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

Adopt TypeTable #676

Merged
merged 32 commits into from
Feb 14, 2025
Merged

Adopt TypeTable #676

merged 32 commits into from
Feb 14, 2025

Conversation

timtebeek
Copy link
Contributor

@timtebeek timtebeek commented Feb 12, 2025

What's changed?

  • Remove packaged resource jars and replace those with TypeTable

Needs a new

What's your motivation?

Packaged resource jars could trip up scanners, even if the jars are not the classpath.

Anything in particular you'd like reviewers to focus on?

Over time we have also added test resource jars; these are typically used only to compile the before-test-sources, and not needed on the runtime classpath, because there JavaTemplate only needs the after-jar-versions. I think I'd prefer to keep that this way, and not unnecessarily inflate the .tsv file further; especially seeing how we also have to deflate that on startup, which could cause a bit of an IO spike.

Any additional context

@timtebeek timtebeek added the enhancement New feature or request label Feb 12, 2025
@timtebeek timtebeek self-assigned this Feb 12, 2025
@timtebeek timtebeek requested a review from sambsnyd February 12, 2025 22:54
@timtebeek timtebeek marked this pull request as ready for review February 13, 2025 14:32
@timtebeek
Copy link
Contributor Author

timtebeek commented Feb 13, 2025

Ok, next fun edge case discovered on a failing test here: meta annotations.

void renamesClassBeanUsagesInOtherFiles() {

~/.rewrite/classpath/.tt/org/springframework/spring-context/6.+/org/springframework/stereotype$ javap Service.class

public interface org.springframework.stereotype.Service extends java.lang.annotation.Annotation {
  public abstract java.lang.String value();
}

This of course differs quite a bit from what we see in Spring:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Component
public @interface Service {

	/**
	 * Alias for {@link Component#value}.
	 */
	@AliasFor(annotation = Component.class)
	String value() default "";

}

So the recipe here relies on the Service annotation itself being a component, as per the matchers here.

Preconditions.or(
new FindAnnotations("@" + FQN_QUALIFIER, false).getVisitor(),
new FindAnnotations("@" + FQN_BEAN, false).getVisitor(),
new FindAnnotations("@" + FQN_COMPONENT, true).getVisitor()) :
Preconditions.or(new UsesType<>(type, false), new DeclaresType<>(type));

Looks like we need to capture yet more about the Service.class in openrewrite/rewrite` before we can replace the .jars here.

@timtebeek
Copy link
Contributor Author

timtebeek commented Feb 13, 2025

Not seeing the same (or any) failure locally; Ci reports this:

MigrateResponseStatusExceptionTest > migrateResponseStatusExceptionGetRawStatusCodeMethod() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    MethodInvocation->NamedVariable->VariableDeclarations->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
    /*~~(MethodInvocation type is missing or malformed)~~>*/e.getRawStatusCode()
    https://docs.openrewrite.org/reference/faq#im-seeing-lst-contains-missing-or-invalid-type-information-in-my-recipe-unit-tests-how-to-resolve
        at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:124)
        at org.openrewrite.java.Assertions.validateTypes(Assertions.java:61)
        at org.openrewrite.java.Assertions$$Lambda$1022/0x00007fba946d8830.accept(Unknown Source)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:301)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:129)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:124)
        at org.openrewrite.java.spring.framework.MigrateResponseStatusExceptionTest.migrateResponseStatusExceptionGetRawStatusCodeMethod(MigrateResponseStatusExceptionTest.java:71)

MigrateResponseStatusExceptionTest > migrateResponseStatusExceptionGetStatusMethod() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    MethodInvocation->NamedVariable->VariableDeclarations->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
    /*~~(MethodInvocation type is missing or malformed)~~>*/e.getStatus()
    https://docs.openrewrite.org/reference/faq#im-seeing-lst-contains-missing-or-invalid-type-information-in-my-recipe-unit-tests-how-to-resolve
        at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:124)
        at org.openrewrite.java.Assertions.validateTypes(Assertions.java:61)
        at org.openrewrite.java.Assertions$$Lambda$1022/0x00007fba946d8830.accept(Unknown Source)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:301)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:129)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:124)
        at org.openrewrite.java.spring.framework.MigrateResponseStatusExceptionTest.migrateResponseStatusExceptionGetStatusMethod(MigrateResponseStatusExceptionTest.java:41)

This is the decompiled class as extracted locally

~/.rewrite/classpath/.tt/org/springframework/spring-web/5.+/org/springframework/web/server$ javap ResponseStatusException.class 
public class org.springframework.web.server.ResponseStatusException extends org.springframework.core.NestedRuntimeException {
  public org.springframework.web.server.ResponseStatusException(org.springframework.http.HttpStatus);
  public org.springframework.web.server.ResponseStatusException(org.springframework.http.HttpStatus, java.lang.String);
  public org.springframework.web.server.ResponseStatusException(org.springframework.http.HttpStatus, java.lang.String, java.lang.Throwable);
  public org.springframework.web.server.ResponseStatusException(int, java.lang.String, java.lang.Throwable);
  public org.springframework.http.HttpStatus getStatus();
  public int getRawStatusCode();
  public java.util.Map<java.lang.String, java.lang.String> getHeaders();
  public org.springframework.http.HttpHeaders getResponseHeaders();
  public java.lang.String getReason();
  public java.lang.String getMessage();
}

Where I don't immediately see differences with the orignal at
https://github.com/spring-projects/spring-framework/blob/8a44eaa6c503fcc458713498f12eac2144338a4d/spring-web/src/main/java/org/springframework/web/server/ResponseStatusException.java#L113

Also not spotting any obvious issues with the NestedRuntimeException from spring-core-5.

~/.rewrite/classpath/.tt/org/springframework/spring-core/5.+/org/springframework/core$ javap NestedRuntimeException.class 
public abstract class org.springframework.core.NestedRuntimeException extends java.lang.RuntimeException {
  public org.springframework.core.NestedRuntimeException(java.lang.String);
  public org.springframework.core.NestedRuntimeException(java.lang.String, java.lang.Throwable);
  public java.lang.String getMessage();
  public java.lang.Throwable getRootCause();
  public java.lang.Throwable getMostSpecificCause();
  public boolean contains(java.lang.Class<?>);
  static {};
}

@timtebeek
Copy link
Contributor Author

@sambsnyd Given the limitations seen here and the potentially continued use of (some) packaged resource jars, how would you feel about swapping the order of these two lines such that packaged jars take precedent? That would make it a lot easier to add a type table for all, while still keeping around the odd jar where needed. Now it's a bit awkward having to download recipe jars for a select few only by commenting out the rest, and then creating a type table with the inverse such to the jars are not ignored because of fault entries in type tables.

@timtebeek
Copy link
Contributor Author

Endless fun to be had! TypeTable uses getResourceAsStream, which is documented as unstable ordering when multiple such resources exist.

Where several modules are defined to the same class loader, and where more than one module contains a resource with the given name, then the ordering that modules are searched is not specified and may be very unpredictable.

rewrite-migrate-java contains a META-INF/rewrite/classpath.tsv.zip, as does rewrite-spring itself, and both are in the same classloader. We're seeing here that the tsv from rewrite-migrate-java is picked up in RestTemplateBuilderRequestFactoryVisitor from rewrite-spring.

.javaParser(JavaParser.fromJavaVersion()
.classpathFromResources(ctx, "spring-boot-2"));

@timtebeek
Copy link
Contributor Author

Next issue: we're storing in the .tsv.zip and inside .tt the requested spring-context-5.+, which then fails to match say spring-context-5.3.*. We likely should adjust TypeTable to use the actual version, not the pattern that was requested.

@timtebeek timtebeek requested a review from sambsnyd February 14, 2025 20:00
github-actions[bot]

This comment was marked as outdated.

@timtebeek
Copy link
Contributor Author

Perfect, just what I was hoping for: a quick apply the plugin, run the task, and off we go!

@timtebeek timtebeek merged commit 66c4b5b into main Feb 14, 2025
2 checks passed
@timtebeek timtebeek deleted the adopt-type-table branch February 14, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants