Make UpgradeDependencyVersion recipe runnable with defined repositories in the buildscript only

[jevanlingen]

What's changed?

• The UpgradeDependencyVersion recipe can run with defined repositories in the buildscript only
• Refactoring of the UpdateVariable class in the UpgradeDependencyVersion recipe
• Update of the DefaultImportsCustomizer

What's your motivation?

The UpgradeDependencyVersion recipe did not work with a setup like:

buildscript {
    ext {
        guavaVersion = "29.0-jre"
    }
    repositories {
        mavenCentral()
    }
    dependencies {
        classpath("com.google.guava:guava:${guavaVersion}")
    }
}

Why: click.

[shanman190]

Overall, I'm a little puzzled with this PR... In the description it seems like an extension variable defined in the buildscript block fails to update the version contained within the variable, but there is explicitly the UpgradeDependencyVersionTest#upgradesVariablesDefinedInExtraProperties test that verifies extension properties both on the buildscript as well as the project are updated (notice that two different variable names were used explicitly to guarantee one of them was not accidentally updated as an inadvertent side effect).

[jevanlingen]

Overall, I'm a little puzzled with this PR... In the description it seems like an extension variable defined in the buildscript block fails to update the version contained within the variable, but there is explicitly the UpgradeDependencyVersionTest#upgradesVariablesDefinedInExtraProperties test that verifies extension properties both on the buildscript as well as the project are updated (notice that two different variable names were used explicitly to guarantee one of them was not accidentally updated as an inadvertent side effect).

Ah, I should have given a little more context in the motivation part of this PR. Well, let's work it out here. Yes there was a test, but it should have been two tests. So the original test looked like:

@Test
void upgradesVariablesDefinedInExtraProperties() {
    rewriteRun(
      buildGradle(
          """
          buildscript {
              ext { guavaVersion = "29.0-jre" }
              repositories { mavenCentral() } // <-- repositories in extension of buildscript
              dependencies { classpath("com.google.guava:guava:${guavaVersion}") }
          }

          plugins { id "java" }
          ext { guavaVersion2 = "29.0-jre" }
          repositories { mavenCentral() } // <-- repositories in root
          dependencies { implementation "com.google.guava:guava:${guavaVersion2}" }
          """,
          """
          <upgraded gradle script>
          """
      )
    );
}

The problem here is the buildscript extension is not totally tested. The repositories part is taken from the root. If you would define another test (which I did in this very PR) with a Gradle file containing only a buildscript:

@Test
void upgradesVariablesDefinedInExtraPropertiesWithBuildscript() {
    rewriteRun(
      buildGradle(
          """
          buildscript {
              ext { guavaVersion = "29.0-jre" }
              repositories { mavenCentral() } // <-- repositories in extension of build script
              dependencies { classpath("com.google.guava:guava:${guavaVersion}") }
          }
          """,
          """
          <upgraded gradle script>
          """
      )
    );
}

You would end up with following error:

"buildscript {
    ext { /*~~(com.google.guava:guava failed. Unable to download metadata.)~~>*/guavaVersion = "29.0-jre" }
    repositories { mavenCentral() } // <-- repositories in extension of build script
    dependencies { classpath("com.google.guava:guava:${guavaVersion}") }
}"

The reason this happens is because the retrieval of the repositories was done in two steps:

try {
  // ..
  String selectedVersion = Optional.ofNullable(selector.select(gav, null, newVersion, versionPattern, ctx))
    .orElse(selector.select(gav, "classpath", newVersion, versionPattern, ctx));
  // ..
  return newVersion ?: oldVersion;
} catch (MavenDownloadingException e) {
  // error: Unable to download metadata.
  return e.warn(a);
}

The idea behind it: first use a null configuration, if that lacks any results, try it with the "classpath" configuration. This does not work though, because the first select will already throw a MavenDownloadingException, so the next select will never be run.

That why I moved that idea to the DependencyVersionSelector:

return repos.isEmpty() ? gradleProject.getBuildscript().getMavenRepositories() : repos;

But as you mentioned a couple hours ago, this is invalid from a Gradle standpoint. So I'll revert my changes to the DependencyVersionSelector and change the recipe a little more (putting back the select(null) => select("classpath"), but without the bug). Edit: That's done now, new version of the PR could be reviewed if you want to.

Thanks for the review, my Gradle knowledge is not that big. So really nice to have a review from someone that knows Gradle very well!

[shanman190]

Aha! That makes a lot more sense as to the where the bug in behavior comes from. Thanks for clarifying that context!

[sambsnyd]

So long as Shannon's comment is addressed this looks OK to me