openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.13k stars 317 forks source link

Update gradle wrapper adds wrong wrapper when customDistributionUrl is requested. #4490

Open Jenson3210 opened 1 week ago

Jenson3210 commented 1 week ago

In the event of a gradle wrapper that needs to be added to a project that does not contain one AND the distributionUrl is mentioned, this code will no add the correct wrapper.

If the outside world is reachable, this try block does not fail -> the distribution url is not used and the wrong wrapper gets added.

https://github.com/openrewrite/rewrite/blob/0b6914b82be589bb6924f273c7ee2cb6c873b853/rewrite-gradle/src/main/java/org/openrewrite/gradle/UpdateGradleWrapper.java#L124-L160

The Gradle wrapper will be created using the outside world services gradle and we will never reach the distributionUrl wrapper additions.

https://github.com/openrewrite/rewrite/blob/0b6914b82be589bb6924f273c7ee2cb6c873b853/rewrite-gradle/src/main/java/org/openrewrite/gradle/util/GradleWrapper.java#L60-L90

The recipe from the list:

Screenshot 2024-09-13 at 12 04 19

The output:

Screenshot 2024-09-13 at 12 04 45

I think if we rework the getWrapper method we could fix this.

    private GradleWrapper getGradleWrapper(ExecutionContext ctx) {
        if (gradleWrapper != null) {
            return gradleWrapper;
        }
        if (wrapperUri != null) {
            if (wrapperUri.contains("${version})")) {
                if (version == null) {
                    throw new IllegalArgumentException(
                            "wrapperUri contains a ${version} interpolation specifier but no version parameter was specified.");
                }
                if (!version.matches("[0-9.]+")) {
                    throw new IllegalArgumentException(
                            "Version selectors like \"" + version + "\" are unavailable when custom distribution url is used. " +
                            "Specify an exact, literal version number.");
                }
            }
            String effectiveWrapperUri = wrapperUri
                    .replace("${version}", version == null ? "" : version)
                    .replace("${distribution}", distribution == null ? "bin" : distribution);
            gradleWrapper = GradleWrapper.create(URI.create(effectiveWrapperUri), ctx);
        } else {
            try {
                gradleWrapper = GradleWrapper.create(distribution, version, null, ctx);
            } catch (Exception e) {
                // services.gradle.org is unreachable, possibly because of a firewall
                // But if the user specified a wrapperUri to an internal repository things might still be workable
                // If the user didn't specify a wrapperUri, but they did provide a specific version we assume they know this version
                // is available from whichever distribution url they were previously using and update the version
                if (!StringUtils.isBlank(version) && Semver.validate(version, null).getValue() instanceof ExactVersion) {
                    gradleWrapper = new GradleWrapper(version, new DistributionInfos("", null, null));
                } else {
                    throw new IllegalArgumentException(
                            "Could not reach services.gradle.org, no alternative wrapper URI is provided and no exact version is provided. " +
                            "To use this recipe in environments where services.gradle.org is unavailable specify a wrapperUri or exact version.", e);
                }
            }
        }
        return gradleWrapper;
    }
Jenson3210 commented 1 week ago

I have test case + this code fix almost ready but having dpendency issues in our firewall (timeouts)

Jenson3210 commented 1 week ago

Downloading the other required files like gradlew, gradlew.bat, gradle/wrapper/gradle-wrapper.jar is failing as the goal here is to access a non-publicly available distributionUrl.

Moderne platform does not have the ability to reach this endpoint.

timtebeek commented 1 week ago

Capturing some of the details from chat here as well, in case that's helpful for others.

Some unit tests we run make use of okhttp3's MockWebServer; perhaps that can get you past the hump of testing with a remote server?

Beyond that, if you create an "http tool" in your agent config, recipes can access it through your agent.

Jenson3210 commented 1 week ago

The existing test is only working because the 8.0.x gets retrieved to 8.0.2 by https://services.gradle.org/distributions/ I believe either the version should be translated/resolved by https://company.com/repo/ or should not be resolvable. There is no guarantee that company repo is capable of resolving 8.0.2 (imagine that one was not created yet). Using the central services repo to determine the version and then updating the url feels off to me. What do you think?

What I propose is not allowing wildcard matchers if you specify the distribution url. Very limited value I see in allowing the placeholder resolution even if you are already setting the distribution url with the version and distribution variables. 1 variable vs 2 or 3 in the yaml.

So either provide version & optionally distribution OR provide entire distribution url. What do you think?

Screenshot 2024-09-13 at 16 41 52