URLThreeArgumentConstructor does not introduce newly thrown exception

[timtebeek]

What version of OpenRewrite are you using?

• OpenRewrite v8.24.0
• rewrite-migrate-java v2.12.0

How are you running OpenRewrite?

I'm running org.openrewrite.java.migrate.net.URLConstructorsToURI.URLThreeArgumentConstructorRecipe, via:

rewrite-migrate-java/src/main/java/org/openrewrite/java/migrate/net/URLConstructorsToURI.java

Lines 44 to 58 in f399d72

	description = "Converts `new URL(String, String, String)` constructors to `new URI(...).toURL()`."
	)
	public static class URLThreeArgumentConstructor {
	@BeforeTemplate
	URL urlConstructor(String protocol, String host, String file) throws Exception {
	return new URL(protocol, host, file);
	}
	@AfterTemplate
	URL newUriToUrl(String protocol, String host, String file) throws Exception {
	return new URI(protocol, null, host, -1, file, null, null).toURL();
	}
	}
	@RecipeDescriptor(

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

class A {
    URL urlConstructor(String protocol, String host, String file) throws IOException {
        return new URL(protocol, host, file);
    }
}

What did you expect to see?

class A {
    URL urlConstructor(String protocol, String host, String file) throws URISyntaxException, IOException {
            return new URI(protocol, null, host, -1, file, null, null).toURL();
        }
}

What did you see instead?

class A {
    URL urlConstructor(String protocol, String host, String file) throws IOException {
            return new URI(protocol, null, host, -1, file, null, null).toURL();
        }
}

Note the lacking throws URISyntaxException.

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

Compiler error, as URISyntaxException does not extend IOException, in contrast to MalformedURLException thrown before. As reported by @IanDarwin

[timtebeek]

This recipe is implemented as a Refaster recipe, and as such it's well possible the fix should land in https://github.com/openrewrite/rewrite-templating to introduce the new exception there from what's thrown in the after template method. /cc @knutwannheden

[Riyazul555]

Hey @timtebeek I have a suggestion on this issue
It seems like in your URLThreeArgumentConstructor class, you are converting new URL(String, String, String) constructors to new URI(...).toURL() without adding the throws URISyntaxException declaration in the AfterTemplate method. To fix this issue, you should include throws URISyntaxException in your AfterTemplate method like this:

public static class URLThreeArgumentConstructor {
        @BeforeTemplate
        URL urlConstructor(String protocol, String host, String file) throws Exception {
            return new URL(protocol, host, file);
        }

        @AfterTemplate
        URL newUriToUrl(String protocol, String host, String file) throws URISyntaxException {
            return new URI(protocol, null, host, -1, file, null, null).toURL();
        }
    }

This ensures that the throws URISyntaxException is correctly propagated in the converted code, addressing the issue you encountered.

What are your thoughts @timtebeek over this ?

[timtebeek]

Hi! Thanks for the suggestion; the way that our rewrite-templating converts Refaster templates to OpenRewrite recipes does not yet include adding a throws declaration where necessary. It might be more complicated to add that there, but would indeed be helpful here.

Note that our implementation of Refaster is different from ErrorProne itself, if you're familiar with how ErrorProne works.

[koppor]

Similar issue here

 package org.jabref.logic.importer.fetcher;
 
 import java.io.IOException;
+import java.net.URI;
 import java.net.URL;
 import java.util.Objects;
 import java.util.Optional;
 
 import org.jabref.logic.importer.FulltextFetcher;
@@ -60,11 +61,11 @@ public class SpringerLink implements FulltextFetcher, CustomizableKeyFetcher {
                 JSONObject json = jsonResponse.getBody().getObject();
                 int results = json.getJSONArray("result").getJSONObject(0).getInt("total");
 
                 if (results > 0) {
                     LOGGER.info("Fulltext PDF found @ Springer.");
-                    return Optional.of(new URL("http", CONTENT_HOST, "/content/pdf/%s.pdf".formatted(doi.get().getDOI())));
+                    return Optional.of(new URI("http", null, CONTENT_HOST, -1, "/content/pdf/%s.pdf".formatted(doi.get().getDOI()), null, null).toURL());
                 }
             }
         } catch (UnirestException e) {
             LOGGER.warn("SpringerLink API request failed", e);
         }

[ddsharpe]

Because org.openrewrite.java.migrate.UpgradeToJava21 includes the recipe org.openrewrite.java.migrate.net.URLConstructorsToURI, we have been unable to use UpgradeToJava21 in its current form. Everywhere that had new URL code is broken due to the lack of exception handling.

[timtebeek]

Sorry to hear about your issues applying this recipe @ddsharpe ; if you want you can remove that single recipe through the recipe builder for now.

We've recently revised this recipe, which makes it easier to add more custom logic now if needed:

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

The question is a bit what folks expect: through "do no harm" we could remove the recipe from Java 21 upgrade for now, and/or look at adding the throws to any surrounding blocks / catch clauses, although might quickly become complex. Could you describe what behavior you'd expect in various cases? Preferably closely resembling our unit tests?

[ddsharpe]

@timtebeek I don't know if this would be the "perfect" answer, but given this is "converted code" I think this is what I would expect:
Given the original URL constructors can throw MalformedURLException, I think it would be okay for the replacement to do the same thing. Convert new URL(...) to new URI(...) and add a try/catch(URISyntaxException) that wraps the URISyntaxException with a MalformedURLException and rethrows the new MalformedURLException. Unfortunately, MalformedURLException does not have a constructor for that, but it is an extension of IOException that does have that constructor. new IOException(theURISyntaxException). If the user's code was catching MalformedURLException, it would be better to be an extension of that class and not IOException. Unfortunately, nothing is presenting it self as the perfect solution right now. Open to ideas.

[timtebeek]

Thanks for researching and laying out the options here @ddsharpe ; what I've done for now is split the recipe we had in two: one part that's also included with the Java 21 upgrade, and a second part that can be optionally run but then lacks introducing the necessary error handling. /cc @rlsanders4 & @karthikNousher for your earlier involvement on #678

I hope that's satisfying for all: an out of the box experience that just works, as well as a dedicated recipe that can be run for additional changes, but might then result in a compilation issue regarding exception handling.

[ddsharpe]

Satisfied, thank you. Confirmed 3.4.0 did not change URL to URI.