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

javax.security.cert, LogRecord.getLongThreadID, and com.sun.net.ssl.internal.ssl.Provider migration recipes #290

Merged
merged 11 commits into from
Sep 15, 2023

Conversation

cjobinabo
Copy link
Contributor

@cjobinabo cjobinabo commented Sep 13, 2023

What's changed?

This pull request includes recipes for the following migration issues:
Screenshot 2023-09-13 at 5 14 40 PM
Screenshot 2023-09-13 at 5 15 41 PM
Screenshot 2023-09-13 at 5 16 35 PM

What's your motivation?

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

The test for these recipes currently fail with the following exception:

io.github.classgraph.ClassGraphException: Uncaught exception during scan
	at io.github.classgraph.ClassGraph.scan(ClassGraph.java:1637)
	at io.github.classgraph.ClassGraph.scan(ClassGraph.java:1654)
	at io.github.classgraph.ClassGraph.scan(ClassGraph.java:1667)
	at org.openrewrite.config.ClasspathScanningLoader.scanClasses(ClasspathScanningLoader.java:143)
	at org.openrewrite.config.ClasspathScanningLoader.<init>(ClasspathScanningLoader.java:60)
	at org.openrewrite.config.Environment$Builder.scanRuntimeClasspath(Environment.java:224)
	at org.openrewrite.java.migrate.UpgradeToJava17Test.defaults(UpgradeToJava17Test.java:34)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:140)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
	at org.openrewrite.java.migrate.UpgradeToJava17Test.upgradeFromJava8ToJava17(UpgradeToJava17Test.java:42)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.OutOfMemoryError: Java heap space
	at java.base/java.util.Arrays.copyOf(Arrays.java:3568)
	at nonapi.io.github.classgraph.fileslice.reader.ClassfileReader.readTo(ClassfileReader.java:206)
	at <unknown class>.<unknown method>(Unknown Source)
	at io.github.classgraph.Classfile.readMethods(Classfile.java:1482)
	at io.github.classgraph.Classfile.<init>(Classfile.java:2027)
	at io.github.classgraph.Scanner$ClassfileScannerWorkUnitProcessor.processWorkUnit(Scanner.java:725)
	at io.github.classgraph.Scanner$ClassfileScannerWorkUnitProcessor.processWorkUnit(Scanner.java:647)
	at nonapi.io.github.classgraph.concurrency.WorkQueue.runWorkLoop(WorkQueue.java:246)
	at nonapi.io.github.classgraph.concurrency.WorkQueue.runWorkQueue(WorkQueue.java:161)
	at io.github.classgraph.Scanner.processWorkUnits(Scanner.java:316)
	at io.github.classgraph.Scanner.performScan(Scanner.java:969)
	at io.github.classgraph.Scanner.openClasspathElementsThenScan(Scanner.java:1114)
	at io.github.classgraph.Scanner.call(Scanner.java:1148)
	at io.github.classgraph.Scanner.call(Scanner.java:82)

My IntelliJ IDE is currently set to use 12GBs of memory. I noticed that other existing tests on this repo also fail so I'm curious to see if the builds show the same failure. If they do I'm hoping to get some guidance on how to resolve these.

A test currently doesn't exist for the org.openrewrite.java.migrate.RemovedLegacySunJSSEProviderName recipe. The core code for that currently lives in the rewrite repo. I'm wondering if I can move org.openrewrite.java.ChangeStringLiteral from the rewrite-liberty repo to the core rewrite repo.

Anyone you would like to review specifically?

@timtebeek @joanvr

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@cjobinabo cjobinabo self-assigned this Sep 13, 2023
@timtebeek timtebeek added the recipe Recipe requested label Sep 14, 2023
@timtebeek timtebeek changed the title 3 new Java 11 migration rules javax.security.cert, com.sun.net.ssl.internal.ssl.Provider and LogRecord.getLongThreadID migration recipes Sep 14, 2023
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these recipes; I've added some comments where I think we can improve, but a good start overall. I think moving (and renaming) ChangeStringLiteral here would be a first step to getting the tests going.

timtebeek added a commit to openrewrite/rewrite-liberty that referenced this pull request Sep 15, 2023
@cjobinabo cjobinabo changed the title javax.security.cert, com.sun.net.ssl.internal.ssl.Provider and LogRecord.getLongThreadID migration recipes javax.security.cert, and com.sun.net.ssl.internal.ssl.Provider migration recipes Sep 15, 2023
timtebeek added a commit to openrewrite/rewrite-liberty that referenced this pull request Sep 15, 2023
@timtebeek
Copy link
Contributor

Thanks again @cjobinabo ; I'm merging these changes early as I think there's enough value in having these already. We can come back to that int getThreadID() to long getThreadID() later with a custom recipe that takes into account the type.

@timtebeek timtebeek merged commit 0c11336 into openrewrite:main Sep 15, 2023
@cjobinabo cjobinabo deleted the Initial-Java-11-rules branch September 15, 2023 15:01
@cjobinabo cjobinabo changed the title javax.security.cert, and com.sun.net.ssl.internal.ssl.Provider migration recipes javax.security.cert, LogRecord.getLongThreadID, and com.sun.net.ssl.internal.ssl.Provider migration recipes Sep 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants