openrewrite
Generalize property handling in Maven recipes?
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?
Hi @gsmet ! Thanks for the additional context! I think the Jenkins folks have similar requirements, as seen on this existing PR:
- https://github.com/openrewrite/rewrite/pull/4404
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
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.
For reference: This can be easily reproduce with the following test case. Here, the version attribute references a property. Instead of change the value of the property, the version attribute uses the desired version value, however, the now unused property is still left behind.
@Test
void changeDependencyGroupIdAndArtifactIdWithVersionProperty() {
rewriteRun(
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
"javax.activation",
"javax.activation-api",
"jakarta.activation",
"jakarta.activation-api",
"1.2.x",
null
)),
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<activation.api>1.2.0</activation.api>
</properties>
<dependencies>
<dependency>
<groupId>javax.activation</groupId>
<artifactId>javax.activation-api</artifactId>
<version>${activation.api}</version>
</dependency>
</dependencies>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<activation.api>1.2.2</activation.api>
</properties>
<dependencies>
<dependency>
<groupId>jakarta.activation</groupId>
<artifactId>jakarta.activation-api</artifactId>
<version>${activation.api}</version>
</dependency>
</dependencies>
</project>
"""
)
);
}
The actual outcome is following.
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<activation.api>1.2.0</activation.api>
</properties>
<dependencies>
<dependency>
<groupId>jakarta.activation</groupId>
<artifactId>jakarta.activation-api</artifactId>
<version>1.2.2</version>
</dependency>
</dependencies>
</project>
While I agree we do not have consistency in these recipes on when to impact a property and when not to, we need to think of the edge cases during implementation such as:
What do we do when multiple dependencies share the same variable to be updated? Can we safely change/alter the dependency group variable? Does that mean that suddenly all other variables using this group variable also moved to a new group? Not necessarily does it? So we could introduce a new variable in that case. But does that really help?
Interesting questions you bring up @Jenson3210. For now I would just follow the same principle we use in UpgradeDependencyVersion, that is: "just update the version tag":
https://github.com/openrewrite/rewrite/blob/ebd193a098f73d6253f906d0066fe47c36b458ce/rewrite-maven/src/test/java/org/openrewrite/maven/UpgradeDependencyVersionTest.java#L334-L389
So I'll start fixing ChangeDependencyGroupIdAndArtifactId and ChangeManagedDependencyGroupIdAndArtifactId in the same way. Maybe we should ponder about a more general way to do this instead of going through the different recipes... Edit: I just did that, moved that property resolvement one layer up.
