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

Improve Handling of Absolute, Relative, and Non-Literal URLs in URL Migration #678

Merged
merged 11 commits into from
Feb 26, 2025

Conversation

karthikNousher
Copy link
Contributor

@karthikNousher karthikNousher commented Feb 18, 2025

What's changed?

  • Updated the logic for converting new URL(String spec) to URI.create(String).toURL() in the recipe.
  • Added special handling for non Literals in the transformNonLiteralURIToValidURL method, ensuring that they are processed correctly.
  • Used URI.create(spec).toURL() for absolute URLs and transformNonLiteralURIToValidURL(spec) for relative URLs.
  • Enhanced error handling to catch potential IllegalArgumentException when dealing with Literal values.

What's your motivation?

  • The update ensures that arguments of type absolute, relative, and non-literal URLs are correctly handled when migrating new URL(String spec) to URI.create(String).toURL().
  • The fix improves handling for relative paths, which were previously falling back incorrectly.

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

  • Review the handling of relative URLs in the visitNewClass() method, especially how transformNonLiteralURIToValidURL is applied in these cases.
    Ensure that the fallback to convertURI(spec) for relative URLs behaves as expected, and that absolute URLs continue to be handled correctly.

Any additional context

This is part of the migration from new URL(String spec) to URI.create(String).toURL(). By making this change, we ensure consistent handling of URLs in code.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

Sorry, something went wrong.

Unverified

This user has not yet uploaded their public signing key.
@karthikNousher karthikNousher changed the title added new recipe to add a new convert uri method. Improve Handling of Absolute, Relative, and Non-Literal URLs in URL Migration Feb 18, 2025
@karthikNousher karthikNousher marked this pull request as draft February 18, 2025 18:39
@karthikNousher karthikNousher marked this pull request as ready for review February 18, 2025 18:53
@ranuradh ranuradh requested a review from timtebeek February 18, 2025 18:59
@ranuradh ranuradh added the recipe Recipe requested label Feb 18, 2025
@karthikNousher karthikNousher marked this pull request as draft February 18, 2025 21:02
timtebeek and others added 6 commits February 19, 2025 00:36

Unverified

This user has not yet uploaded their public signing key.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@rlsanders4 rlsanders4 marked this pull request as ready for review February 19, 2025 18:41
@rlsanders4
Copy link
Contributor

These changes only modify the behavior of the single argument constructor for new URL(). The three argument and four argument constructors are unaffected by this change. Here are the three use cases that we expect for the single argument constructor.

Case 1: The argument is a string literal of an absolute path
The recipe will migrate the constructor to use URI.create().toURL().
Before:

URL url1 = new URL("https://absolutepath.com");

After:

URL url1 = URI.create("https://absolutepath.com").toURL();

Case 2: The argument is a string literal of a relative path
The recipe will make no changes to the constructor.
Before/after:

URL url1 = new URL("some/relative/path");

Case 3: The argument is a not a string literal (it could be a constant, local variable, method invocation, etc)
We cannot determine the value of the argument with static analysis and confirm that it is an absolute path, so we replace the constructor with a wrapper method that will make the determination for us at runtime.
Before:

URL url1 = new URL(myVariable);

After:

URL url1 = transformNonLiteralURIToValidURL(myVariable);

... (at the end of the parent class declaration)

public URL transformNonLiteralURIToValidURL(String spec) {
    try {
        return URI.create(spec).toURL();
    } catch (Exception e) {
        return new URL(spec);
    }
}

@timtebeek timtebeek requested a review from MBoegers February 23, 2025 07:41
@timtebeek
Copy link
Contributor

I feel case 3 might be quite surprising to see for folks. Would it not be better to hold off on making any change there, and potentially attempt to resolve constants as seen here? Any cases not yet converted we can then remove in a future Java version of the old constructors are removed. Until then a deprecation warning might be preferred.

@rlsanders4
Copy link
Contributor

Sure, we can update case 3 here. If the parameter is a constant, we can attempt to resolve it as you mentioned. If the parameter is not a string literal nor a constant, then we will make no changes.

@rlsanders4 rlsanders4 marked this pull request as draft February 24, 2025 17:38
@rlsanders4 rlsanders4 marked this pull request as ready for review February 25, 2025 22:17
@MBoegers MBoegers force-pushed the enhancement/uriRecipee/6616 branch from 9082240 to e050ff3 Compare February 26, 2025 11:54
@MBoegers
Copy link
Contributor

MBoegers commented Feb 26, 2025

Hi @rlsanders4 I've added a Precondition to prevent executions on CompilationUnits not using the java.net.URL class at all. We try to apply these preconditions wherever possible to prevent obsolete cycles.
I also restructured your checks and used the TypeUtils#isOfType(JavaType, JavaType) method instead of the String checks, this way the recipe can take advantages of future improvements within the util.

From the logical perspective, all was good 👍

@MBoegers MBoegers merged commit 7a21d21 into openrewrite:main Feb 26, 2025
2 checks passed
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.

Update made by URLConstructorsToURI recipe can yield IllegalArgumentException
5 participants