Give explicit property definitions higher precedence than Maven's implicit ones.

[nguyenhoan]

What's changed?

Exclude Maven built-in property from circular placeholder reference check

What's your motivation?

Error Circular placeholder reference 'XYZ' in property definitions is thrown when a built-in property, such as project.version, is redefined in the root POM and used in a child in multi-module projects.
I would not expect running recipes to fail given that mvn clean verify succeeds.

Attached is a minimum project to reproduce the issue.
Circular-placeholder-reference.zip
You can run any recipe to trigger it. I ran the one for SpringBoot

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-spring:RELEASE -Drewrite.activeRecipes=org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2 

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

Any additional context

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

[github-actions]

Some suggestions could not be made:

• rewrite-core/src/test/java/org/openrewrite/RecipeListTest.java
  • lines 35-35
  • lines 66-69
• rewrite-maven/src/test/java/org/openrewrite/maven/trait/MavenPluginTest.java
  • lines 26-26

[timtebeek]

Thanks for the quick work here! I'd expected a unit test using RewriteTest that shows we can now handle a parent and child that use such properties, perhaps very similar to what you already have in your zip file. I can explore that if you don't get to it for a more thorough review later.

[timtebeek]

Suggested change

import org.openrewrite.internal.lang.NonNull;

[timtebeek]

We've already annotated the package; that means we likely don't have to repeat ourselves here.

rewrite/rewrite-core/src/main/java/org/openrewrite/internal/package-info.java

Lines 16 to 19 in 86ae0f1

	@NonNullApi
	package org.openrewrite.internal;
	import org.openrewrite.internal.lang.NonNullApi;

Suggested change

	private static boolean isMavenBuiltInProperty(@NonNull final String originalPlaceholder) {
	private static boolean isMavenBuiltInProperty(final String originalPlaceholder) {

[nguyenhoan]

That does not sound a like unit test for the changed method. If I wanted to add a unit test, which test class would I add it to?

[timtebeek]

I'd say MavenParserTest is the best place to guarantee that your issue is fixed, and remains fixed with a test similar to this one.

rewrite/rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

Lines 3042 to 3109 in ad91222

	@Test
	@Issue("https://github.com/openrewrite/rewrite/issues/4093")
	void circularImportDependency() {
	rewriteRun(
	mavenProject("root",
	pomXml(
	"""
	<project>
	<groupId>com.example</groupId>
	<artifactId>circular-example-parent</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<modules>
	<module>circular-example-child</module>
	</modules>
	<dependencyManagement>
	<dependencies>
	<dependency>
	<groupId>com.example</groupId>
	<artifactId>circular-example-child</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<scope>import</scope>
	</dependency>
	</dependencies>
	</dependencyManagement>
	</project>
	""",
	spec -> spec.afterRecipe(pomXml -> {
	MavenResolutionResult resolution = pomXml.getMarkers().findFirst(MavenResolutionResult.class).orElseThrow();
	GroupArtifactVersion gav = resolution.getPom().getDependencyManagement().get(0).getGav();
	assertThat(gav.getGroupId()).isEqualTo("junit");
	assertThat(gav.getArtifactId()).isEqualTo("junit");
	assertThat(gav.getVersion()).isEqualTo("4.13.2");
	})
	),
	mavenProject("circular-example-child",
	pomXml(
	"""
	<project>
	<parent>
	<groupId>com.example</groupId>
	<artifactId>circular-example-parent</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	</parent>
	<artifactId>circular-example-child</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<dependencyManagement>
	<dependencies>
	<dependency>
	<groupId>junit</groupId>
	<artifactId>junit</artifactId>
	<version>4.13.2</version>
	</dependency>
	</dependencies>
	</dependencyManagement>
	</project>
	""",
	spec -> spec.afterRecipe(pomXml -> {
	MavenResolutionResult resolution = pomXml.getMarkers().findFirst(MavenResolutionResult.class).orElseThrow();
	GroupArtifactVersion gav = resolution.getPom().getDependencyManagement().get(0).getGav();
	assertThat(gav.getGroupId()).isEqualTo("junit");
	assertThat(gav.getArtifactId()).isEqualTo("junit");
	assertThat(gav.getVersion()).isEqualTo("4.13.2");
	})
	)
	)
	)
	);
	}

[github-actions]

Some suggestions could not be made:

• rewrite-core/src/test/java/org/openrewrite/RecipeListTest.java
  • lines 35-35
  • lines 66-69
• rewrite-maven/src/test/java/org/openrewrite/maven/trait/MavenPluginTest.java
  • lines 26-26

[nguyenhoan]

Thank for the suggestion! Working on adding a test there helped me find the correct strategy.

[timtebeek]

Great to see the added test & fix @nguyenhoan !

And I even got the review done in time on the train. ;)

[github-actions]

Some suggestions could not be made:

• rewrite-core/src/test/java/org/openrewrite/RecipeListTest.java
  • lines 35-35
  • lines 66-69
• rewrite-maven/src/test/java/org/openrewrite/maven/trait/MavenPluginTest.java
  • lines 26-26

[nguyenhoan]

Awesome! Thank you!