crankydillo commented 4 months ago

It's kind of weird. The setup actually does what we want.. We want to replace an old managed dependency from parent-X.1 with a new managed dependency from parent-X.2. However, the behavior seems weird in at least 2 ways.

First, the parent version change drops the now unmanaged dependency into the project-parent's (the one parented by parent-X) into a dependency management section, presumably because of this code. However, it does not do that for the module, which transitively is parented by parent-X. Second (and ironically), this is actually what we want. We want to force them to deal with the removal of that dep and are having to do that with maven.RemoveManagedDependency. We also want to test our declarative recipe, so we setup the test in the adhoc-test:recipe (link below), but it fails with:

Assertion error

``` expected: " 4.0.0 com.myorg.app profile-test-2 5.0.0-SNAPSHOT module-1 org.apache.commons commons-collections4 " but was: " 4.0.0 com.myorg.app profile-test-2 5.0.0-SNAPSHOT module-1 org.apache.commons commons-collections4 " ```

But when we run this with mvn ... (a 'real' rewrite), we end up with what we want in the child module:

  <dependency>
           <groupId>org.apache.commons</groupId>
           <artifactId>commons-collections4</artifactId>
  </dependency>

However, I still think there's an issue that is exposed by the test, because I used mvnDebug and see this:

4183 may be related.

What version of OpenRewrite are you using?

OpenRewrite v8.28.1
Maven v5.35.0

How are you running OpenRewrite?

Are you using the Maven plugin, Gradle plugin, Moderne CLI, Moderne.io or something else?

Maven plugin

Is your project a single module or a multi-module project?

multi-module

Is your project public? If so, can you share a link to it?

adhoc-test folder in this branch

What is the smallest, simplest way to reproduce the problem?

Steps in readme. Can use debugger + mvnDebug or look at the unit test. It's actually an internal test that first exposed this, but I can't share that. It should be almost identical to this other than not managing the parent poms inside the test.

What did you expect to see?

I expect the test to pass.

What did you see instead?

Test failing with 'No version provided' in the output. (see above)

Are you interested in contributing a fix to OpenRewrite?

Possibly, but this might get a little intensive and seems pretty 'core' to Maven. I tried a few trivial things (e.g. moving withModules to after resolveDependencies in UpdateMavenModel, but somehow I think we have to change the 'resolvedPom' in the Module to have new parentage. I haven't figured out how to do that yet.

timtebeek commented 3 months ago

Hi! Quick bit of answer to give some context, as it's hard to dive deep into each reported issue. The  you're seeing in tests is a marker that's printed there. When running with the Maven plugin such markers are suppressed. You can see the relevant printer here: https://github.com/openrewrite/rewrite-maven-plugin/blob/b8ff7852bf7abe186fe43f3039deede6b61c3acd/src/main/java/org/openrewrite/maven/SanitizedMarkerPrinter.java#L25-L29

It looks like that informational marker is the result of this exception: https://github.com/openrewrite/rewrite/blob/4299aff5c31509124810371e7cc1e7a1dfec6ca2/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java#L849-L857

timtebeek commented 3 months ago

As to what to do: You can update your test expectation to include that marker, and rest assured it's not produced in practice. That's a bit icky I agree. The other/better option is looking to resolve that missing version issue somehow. I haven't yet explored what would be needed for that, but a unit test as you have internally good be a good start on a draft PR.

crankydillo commented 3 months ago

As to what to do: You can update your test expectation to include that marker, and rest assured it's not produced in practice. That's a bit icky I agree.

Believe it or not, but I have this code just sitting there;) We will proceed with this for now.

The other/better option is looking to resolve that missing version issue somehow. I haven't yet explored what would be needed for that, but a unit test as you have internally good be a good start on a draft PR.

Yeah. We're just getting started with rewrite, but having that model correct seems pretty key. As I mentioned, I didn't see any obvious, easy fix. We may be able to look at this, but not sure yet.

openrewrite / rewrite

maven.UpgradeParentVersion and maven.ChangeDependencyGroupIdAndArtifactId not working correctly with Maven modules #4323