DependencyManagement logic bug when versions are set via Maven property

[gjd6640]

What's changed?

Adds tests to UpgradeTransitiveDependencyVersionTest. Two demonstrate current behaviors and are added to prevent regression and demonstrate to the reader what the recipe can do.

The third demonstrates a potential bug:

I think this only happens when all of the following are true:

1. A BOM file in the dependencyManagement section sets the library's version number
2. That BOM file uses a Maven property to determine the library version
3. The parent pom also specifies that property and it sets it to the lower/undesired version

The plugin appears to look thru the list of managed dependency entries, sees the one in the BOM that sets the newer/desired version, and decides that no change is needed. Of course, the Maven engine understands the full context and is choosing the older version number specified via the property in the parent POM so the recipe does need to act to override the version used.

What's your motivation?

We're remediating CVEs across many applications and some projects have this pattern present. We don't intend to continue to refine our parent POMs nor are we ready to rip them out so are trying to remediate CVEs by overriding in the local project's POM file.

Anyone you would like to review specifically?

@timtebeek has been discussing this with me in Slack.

This pull request serves to port these tests over from where they were built in this rewrite-recipe-starter fork.

Checklist

• [ x ] I've added unit tests to cover both positive and negative cases
• [ no, sorry :) ] I've read and applied the recipe conventions and best practices
• [ no, sorry :) ] I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Very clear example @gjd6640 ; thanks for opening the issue here; hope we can see this resolved for you going forward, although it might take a detailed look first.

[github-actions]

Some suggestions could not be made:

• rewrite-gradle/src/main/groovy/RewriteSettings.groovy
  • lines 31-36

[github-actions]

Some suggestions could not be made:

• rewrite-gradle/src/main/groovy/RewriteSettings.groovy
  • lines 31-36

[github-actions]

Some suggestions could not be made:

• rewrite-gradle/src/main/groovy/RewriteSettings.groovy
  • lines 31-36

[github-actions]

Some suggestions could not be made:

• rewrite-gradle/src/main/groovy/RewriteSettings.groovy
  • lines 31-36

[timtebeek]

Thanks a lot for the added tests! I think the ones that pass we could already merge to help explain the cases covered.

Indeed looks like we should be making the change you've logged in the for now failing test. Did you already explore the changes needed there?

[timtebeek]

Hah, while I certainly understand the sentiment, we actually care deeply about the white space. Consider for instance the case where we would add the correct dependency, but it's completely left aligned, lacks newlines between tags or uses tabs instead of spaces. All of those would lead to a developer reviewing the PR to reject the changes, or require that a formatter is applied before a merge. That doesn't scale well. I hope you understand!

Suggested change

// Bonus: If there's a way to modify these assertions to do an XML-based comparison instead of a string compare please point that out. These are unnecessarily brittle as-is due to caring about whitespace.