`AddManagedDependency` should allow version placeholder properties about rewrite HOT 7 CLOSED

Hey @dpozinen, #3992 should have fixed that, but the following still does not work out of the box:

type: specs.openrewrite.org/v1beta/recipe
name: org.ammachado.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.group-id
      newValue: io.quarkus.platform
      addIfMissing: "True"
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.artifact-id
      newValue: quarkus-bom
      addIfMissing: "True"
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.version
      newValue: 3.2.3.Final
      addIfMissing: "True"
  - org.openrewrite.maven.AddManagedDependency:
      groupId: ${quarkus.platform.group-id}
      artifactId: ${quarkus.platform.artifact-id}
      version: ${quarkus.platform.version}
      scope: import
      type: pom
      addToRootPom: "True"

The properties will be added, but the dependency will fail to resolve, because the placeholders were not resolved.

I think that ChangePropertyValue is just missing a call to maybeUpdateModel() on visitDocument. I wrote a recipe to force the maven model to be updated. I use it before my org.openrewrite.maven.AddManagedDependency recipes, and it works as expected.

type: specs.openrewrite.org/v1beta/recipe
name: org.ammachado.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.group-id
      newValue: io.quarkus.platform
      addIfMissing: "True"
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.artifact-id
      newValue: quarkus-bom
      addIfMissing: "True"
  - org.openrewrite.maven.ChangePropertyValue:
      key: quarkus.platform.version
      newValue: 3.2.3.Final
      addIfMissing: "True"
  - org.ammachado.ForceMavenModelUpdate
  - org.openrewrite.maven.AddManagedDependency:
      groupId: ${quarkus.platform.group-id}
      artifactId: ${quarkus.platform.artifact-id}
      version: ${quarkus.platform.version}
      scope: import
      type: pom
      addToRootPom: "True"

public class ForceMavenModelUpdate extends Recipe {

    @Override
    public String getDisplayName() {
        return "Force Maven Model Update";
    }

    @Override
    public String getDescription() {
        return "This recipe will force the Maven model to be updated.";
    }

    @Override
    public TreeVisitor<?, ExecutionContext> getVisitor() {
        return new UpdateMavenModel<>();
    }
}

@timtebeek, the Yaml from the example was just created using https://app.moderne.io. Booleans are showing "True", instead of true. Is that a problem as well?

from rewrite.

Thanks for the suggestion @dpozinen ; how would you envision this to work both in terms of configuration, and in terms of when the recipe runs? Because I have quite a few questions when I think through how to implement this, and I'll list a few below.

Right now the recipe already has quite an array of options: https://docs.openrewrite.org/recipes/maven/addmanageddependency

• Were we to allow version: ${something.version} then we'd need to also know the actual version. How would you pass that in?
• Should the property already exist, or should it be created?
• How to handle conflicts if we create the property?
• Should we honor any parent properties or override?

I'm thinking if we add option to indicate that the version should be placed in a property, then the naming of that property is going to be very much specific to one's particular taste. Some projects prefer <version.abc> whereas others prefer <abc.version>, and for instance any Spring migration recipes should ideally not force one over the other.

We're constantly trying to balance feature requests with usability and maintainability by thinking through some of these above edge cases. Since versions can be updated through recipes, we tend to favor just having them where they are used rather than add indirection.

If you do want to keep managed dependency versions in properties consistently then perhaps a dedicated recipe could make sense. That also has the benefit of not further complicating the logic for AddManagedDependency, so it's something to consider.

from rewrite.

oh nice, #3992 indeed added this possibility, things move fast lol; apologies for not checking first.

And indeed what you're describing @ammachado is what we're trying to do (add property and then use it), will try your workaround for maven model update, thanks!

from rewrite.

Thanks for chiming in @ammachado ! Indeed that org.ammachado.ForceMavenModelUpdate utility would solve this right after adding the properties. I'm wondering if we should call that maybeUpdateModel() from ChangePropertyValue, and what any performance impact of that would be. Could be nice to have things "just work" rather than require such a workaround. 🤔

As for the "True" in the recipe builder output, I think that should be fine based on how that field is a nullable java.lang.Boolean, not a boolean. We load those values in here, and I've not heard any issues about their values so far 🤞🏻

from rewrite.