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!

[shanman190]

We should probably check that the configurations list contain "classpath" prior to running this for all failing cases.

[jevanlingen]

Good one, will work on this a little more tomorrow 😄.

[shanman190]

Another little comment here would be wouldn't we want to continue adding an error marker when things truly fail to resolve? That way users can see and know what went wrong?

[jevanlingen]

Another little comment here would be wouldn't we want to continue adding an error marker when things truly fail to resolve? That way users can see and know what went wrong?

The new logic continues the error handling as it worked before 😉. The first select is catched, so the second select can be called. The second select is not catched, so in the end it works the same. I even made sure this works, by adding the new test cannotDownloadMetaDataWhenNoRepositoriesAreDefined:

@Test
void cannotDownloadMetaDataWhenNoRepositoriesAreDefined() {
  rewriteRun(
    buildGradle(
      """
        plugins { id 'java-library' }
        dependencies { implementation "com.google.guava:guava:29.0-jre" }
      """,
      """
        plugins { id 'java-library' }
        dependencies { /*~~(com.google.guava:guava failed. Unable to download metadata.)~~>*/implementation "com.google.guava:guava:29.0-jre" }
      """
  ));
}

There is no date table, we could opt to add that as well. If we want that, I'll suppose we should just create a separate PR.

[jevanlingen]

Ok I made a new implementation, with an extra check

String selectedVersion;
try {
  selectedVersion = dependencyVersionSelector.select(gav, null, newVersion, versionPattern, ctx);
} catch (MavenDownloadingException e) {
+ if (!gaWithConfigurations.getValue().contains("classpath")) {
+    throw e;
+ }
  // try again with "classpath" configuration; if this one fails as well, bubble error up so it can be handled
  selectedVersion = dependencyVersionSelector.select(gav, "classpath", newVersion, versionPattern, ctx);
}

Which works of course to fix unneeded calls. But now I am wondering, if a Gradle file has indeed defined both repositories in the root and in the buildscript (like the original unit test), would first looking at the root and if that fails trying to load it from buildscript Gradle wise a logic decision? Or should each scope only use its own repositories function. With other words, if you look at following Gradle script:

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

plugins { id "java" }
repositories {
  mavenCentral() // repositories within the root
}
ext { guavaVersion2 = "29.0-jre" }
dependencies { classpath("com.google.guava:guava:${guavaVersion2}") }

Is it correct that the mavenCentral() may "own" both the root and the buildscript repositories. That's the current behaviour (as it first tries to do just that, if it fails it tries to use the gradlePluginPortal()). Or should every configuration only apply to its own scope? Or even harder, should the root repositories also apply to the buildscript unless the buildscript uses its own repositories?

I am probably using wrong wording here, as I as before my Gradle knowledge is rather slim. But I hope you understand what I am getting at.

[shanman190]

So to talk about some infrastructure setup for a moment, the Gradle default plugin repository gradlePluginPortal() will accept a request for an artifact, if it's found it'll return it. If it's not, then it forwards that request to mavenCentral behind the scenes.

When we talk about artifact resolution, you're well on the right path.

• Plugins
  • Plugin requests only use the buildscript.repositories (pluginManagement.repositories from the settings.gradle is pushed into this list by Gradle automatically for us as well).
• Project dependencies
  • External dependency requests only use the repositories (dependencyResolutionManagement.repositories from the settings.gradle is pushed into this list by Gradle automatically for us as well).

Why this little section is confusing us because we're updating variables outside of the scope of exactly where the dependency is declared or where the variable is declared. Part of this is extension properties (ext, buildscript.ext) and part of it is project properties (gradle.properties) -- and here in the near-term future even libs.versions.toml. When we are updating the dependency as a Literal or G.MapEntry, we always do make sure to take into account the call site, but using the "classpath" configuration as the indicator.

[sambsnyd]

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