Also exclude omitted dependencies

[ckcd]

• Try to complete Also exclude omitted dependencies #4463

The root cause should be when parse resolved pom, we ignore the omitted dependencies so that these information is lost.

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java

Line 913 in ed13f4a

continue;

So I try to add a new omitDependencies list for ResolvedDependency to record these omitted information. And uses this omitDependencies when search for potential relation in ExcludeDependency. To avoid affecting other logic, I added a new method findDependencyWithOmit.

What's changed?

ExcludeDependency also exclude omitted dependencies.

What's your motivation?

When conflicts occur dependencies are omitted; when we then exclude from the conflict, the omitted dependency can resurface. The goal of ExcludeDependency is to fully exclude a dependency, not to see the omitted conflict being used. We should then also add <exclusion> where dependencies are omitted.

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

Anyone you would like to review specifically?

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

[timtebeek]

Thanks for the work here @ckcd ! Great to see you've found a way around the problem of omitted dependencies. The only thing giving me pause here is that we try to limit change to the ..tree.* classes, as those are serialized and thus require some more consideration before changing things there in a backwards compatible manner. Let us think over the options here a bit before committing to a particular solution. Much like Boolean optional I'm considering an Boolean omitted as one of the possible solutions, and then alter findDependency to exclude omitted dependencies by default. But perhaps there's more options still.

[ckcd]

@timtebeek Thanks for your reply!

Sure for the boolean related I can refine the logic to make it more concise.

And just confirm, do you mean we can't add new field to ResolvedDependency? Generally add a field does not affect backward compatibility I think.

[timtebeek]

Sure for the boolean related I can refine the logic to make it more concise.

Thanks! I think that's preferred over adding a collection field to ResolvedDependency that's only used for the very narrow use case of exclusions.

And just confirm, do you mean we can't add new field to ResolvedDependency? Generally add a field does not affect backward compatibility I think.

Some addition is possible, but indeed backwards compatible and ideally with minimal surface API changes if we can. That way there's less to review and maintain going forward.

[ckcd]

Add @SiBorea

[SiBorea]

@timtebeek I suggest we create new classes that extend ResolvedDependency and ResolvedPom, then create a new recipe ExcludeAllDepedency using them.

[timtebeek]

One thing to note here is that omitDependencies is technically @Nullable, since we serialize LSTs at Moderne, which mean folks might deserialize an older ResolvedDependency class without any omitDepedencies field, which can then lead to a null pointer exception further down. Let's make that explicit by adding that annotation here to be safe, and then handle the usage site below to guard against a null value.

[timtebeek]

This is where we'd have to handle a possible null omitDependencies field.

[timtebeek]

Also here the generated getter might return null; as an alternative we could create an explicit getter that always returns a collection.