Adapted ChangeDependency to keep existing version variables when null version as input

[Jenson3210]

What's changed?

If the ChangeDependency has to work on a GString or a KStringTemplate, the version is most likely a variable. The code was returning a J.Literal always in this case. When the version input is null however, this would indicate the user wants to keep the current version there.
This resulted in wrong dependency declarations: group:artifact:
By returning a new StringTemplate iso a JLiteral in the cases where newVersion is null, we can keep the group:artifact:${variable} AND the parameters LST object remains a StringTemplate for future recipes.

What's your motivation?

Fix a hole in our input/behavior coverage.

Anyone you would like to review specifically?

@shanman190 @timtebeek @sambsnyd

Have you considered any alternatives or workarounds?

putting the variable in the Literal -> future recipes might not correctly recognize the variable as a variable as a J.Literal is found iso a G.String/K.StringTemplate.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[shanman190]

@Jenson3210 , I'm assuming that the intent here is that the end user would then follow up with an UpgradeDependencyVersion? I think originally, the intent was that the before version could either be any value (not present, a literal, or a variable), then the result could only be not present or a literal. The idea between Sam and I was that it's most likely that the variable can no longer be trusted across its usages since this recipe is only acting upon a single dependency instance.

In the current example, it would result in a failing build because no GAV for org.apache.commons:commons-lang3:2.6 exists.

Otherwise, I think the change suggested here looks good.

[Jenson3210]

@shanman190 The idea would be that if they would want to set a new version, the code still behaves as you mention. However, not necessarily they would have to change the version. It depends on the input. But if null is the new version, the version should not be trimmed off leaving an ending group:artifact: which is invalid code.

We could either go for:

• group:artifact -> In this case converting to a Literal would be fine, but how to resolve the version for the markers etc...?
• group:artifact:old_version -> In this case converting from a GString to Literal is not correct imo, but we have all info from a dependency viewpoint.

I decided to go for the latter. Not only because that's what we would need atm (though that plays a role 😄). When a MapNotation is used, the recipe also does not trim the version entry if null is specified. So if anything, I think this brings consistency to the recipes not depending on what syntax was used.

And indeed, if we would like to set a new version, we could follow up with an UpgradeDependencyVersion that then updates the variable. In my case here, they are shifting between different dependencies, but the version remains the same across them. (Consider something like switching from spring-boot-starter-web to spring-boot-starter-webflux where the versions are the same, but the dependency itself is different)

[jkschneider]

Is this solution overfit to the problem? What if the artifactId is a variable? Are we having to introduce the below logic because toStringNotation strips the variable?

[Jenson3210]

Are we having to introduce the below logic because toStringNotation strips the variable?

We have to introduce something like this because DependencyStringNotationConverter.parse((String) requireNonNull(literal.getValue())) only receives the literal part from the G.String.
In theory, this could be group only if artifact is a variable also, in which the recipe would even give a NPE earlier in the code (as artifactId is not nullable for dependency/Gav). But as far as I remember, we mostly do not have support for setups like that as also in other recipes the assumption is always made: if it is a GString, the variable part is the version

This is also the case in our GradleDependency Trait, which uses the same DependencyStringConverter

Is this solution overfit to the problem?

Not sure what you mean with overfit, but I made it like this so that our G.String remains a G.String if the version is a variable so that later recipes that touch dependencies/versions receive the correct lst element instead of a literal that contains a variable reference. Other recipes that would do an update/replace by variable lookup would expect a G.String and would think a J.Literal contains the exact version.

[shanman190]

Just to chime in on the "overfit" part is that there is existing precedent in many of the other Gradle recipes to this respect. I agree that only expecting for only the version to be a template variable is a little assumptive, but it is the most prevalent form and very rarely do I see either the group or artifact being parameterized in real life builds.

To take this a step further, even using parameters in the way described no longer aligns with Gradle best practices and the advice there is to utilize a version catalog instead.

[Jenson3210]

@jkschneider I noticed this one was moved to need additional context.
What additional context would be needed here? Or was @shanman190 and my input sufficient to take this further?