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

URLThreeArgumentConstructor does not introduce newly thrown exception #467

Open
timtebeek opened this issue Apr 29, 2024 · 4 comments
Open
Labels
bug Something isn't working

Comments

@timtebeek
Copy link
Contributor

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:

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 timtebeek added the bug Something isn't working label Apr 29, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Apr 29, 2024
@timtebeek
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

koppor commented Sep 24, 2024

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);
         }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

3 participants