search

openrewrite / rewrite

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

Generalize property handling in Maven recipes? #4527

Open gsmet opened 1 month ago

gsmet commented 1 month ago

Hi,

We have been very happy with the property handling in UpgradeDependencyVersion: when a version is a property, the property is updated instead of the original version tag. Which is awesome as a lot of versions are properties.

All good but... in Quarkus, we are using properties in generated projects also for the groupId/artifactId of the Quarkus BOM (and it's something that is important to us as we have several "Platforms" with different coordinates and we need to enforce the Platform consistently to the BOM and the Quarkus Maven plugin, thus why we use properties).

While working on a PR to handle changing the Platform entirely (i.e. including the groupId/artifactId, until now we were just updating the version), I stumbled upon the fact that this nice property handling is actually not generalized to the Maven recipes.

For instance, there is nothing to handle it in ChangeDependencyGroupIdAndArtifactId or ChangeManagedDependencyGroupIdAndArtifactId (unless I missed something but the code seems in line with the behavior I'm experiencing).

I'm not sure exactly why this is handled only in a very specific case. I'm also not completely sure it should be generalized everywhere but not handling it for the groupId/artifactId is actually quite blocking for us. And to be fair, the version is not handled either in ChangeDependencyGroupIdAndArtifactId and ChangeManagedDependencyGroupIdAndArtifactId so it's not very consistent.

I was wondering if it's something you would consider? If you had some advice on how to fix it? I could probably dedicate some time to it as it's in our way but I definitely need some guidance. Also, I might not be able to generalize it everywhere in the time I would be able to dedicate to it but I could at least put the infrastructure in place and fix the cases that are in our way, opening the gate for more in the future if someone needs it elsewhere.

Thoughts?

timtebeek commented 1 month ago

Hi @gsmet ! Thanks for the additional context! I think the Jenkins folks have similar requirements, as seen on this existing PR:

Haven't had time to review that just yet, and have a few weeks of travel ahead, but property resolve is certainly something that we could support better, but fitting in that work might take us a while. I understand you're similarly busy over there, so for now we'll track support here

gsmet commented 1 month ago

I think it's actually a bit orthogonal. Because AFAICS, OpenRewrite actually resolves my properties as the correct dependencies are adjusted. What it does not is rewrite the value of the property instead of rewriting the <artifactId> tag.

I'll draft something soon.