Proposed API changes for SHRINKRES while working with pom.xml

[kpiwko]

Hello,

I have a fruitful discussion about some real life usage of ShrinkWrap Maven Resolver with Ron Smeral. Basically it was based on top of SHRINKRES-112 issue.

We have figured out following facts:

Strategy, as it is implemented, is not really useful. Problems:

• Not many people understand what is the different in between PreResolution and Resolution filtering.
• Strategy should be more general. It should not have two different phases
• Same strategy has different meaning/effect when used on different place
• We don't have a strategy builder

What is preresolution filtering?

Resolver internally keep a list of dependencies to be set for dependency collection request. Prefilter phase allows you to filter those before they are actually send to be resolved.

This means preresolution saves resources. However, it is not always possible to achieve that.

Motivation to have such possibility mainly comes from working with pom files. For other use cases you don't actually need it, because you can simply omit dependency you don't want to resolve.

What is resolution filtering?

Maven constructs a dependency graph, that is resolves all the dependencies. Resolution filtering simply rejects some of the resolved dependencies from resolved results.

OK, got that. So, can you give me some examples?

Sure. See following fluent API calls:

Maven.resolver().resolve("a:b:v1", "c:d:v2").using(new RejectDependenciesStrategy("e:f:v")).asFile();

This resolves two trees (note, Maven by default skips test and provided scopes and optional dependencie, we have TransitiveExclusionPolicy to change that behavior) including transitive dependencies of a:b and c:d. Later on, this tree is traversed and all occurences of e:f:v are removed.

Maven.resolver().resolve("a:b:v1", "c:d:v2").using(new AcceptScopesStrategy(ScopeType.TEST)).asFile();

Again, this resolves two trees. Later on, all dependencies with different scope then test are removed. (Note, unless you change TransitiveExclusionPolicy, you'll get an empty set)

I don't see nothing wrong there.

You are correct, this makes perfect sense. However, what happens here?

Maven.resolver().loadPomFromFile("/path/to/pom").importRuntimeAndTestDependencies(new AcceptScopesStategy(ScopeType.TEST).asFile();

Now, dependencies specified in POM file have their own scope defined.

So, we apply a prefiltering and put all dependencies from POM with scope TEST to resolution. Nope, this does not happen.

The problem is that AcceptScopesStrategy has not pre resolution filters. Because it would yield unexpected results for previous calls (it would wipe all dependencies for resolution).

So, what happens instead is that importRuntimeAndTestDependencies will put all dependencies from POM with scope COMPILE, RUNTIME, SYSTEM, TEST, IMPORT, PROVIDED (all) to resolution. When graph is constructed, it is filtered for dependencies with scope test. This is completely different than you would expect when you read the call sequence.

What is the proposal?

We are currently in Beta cycle, so we should avoid doing significant API changes. I'm proposing to split preresolution filtering and filtering in more visible manner:

1/ Remove or at least deprecate preResolutionFiltering. RejectDependenciesStrategy is the only one that actually uses it. We can live with a single filtering.

Update: RejectDependencies use preResolutionFiltering.

2/ Make prefiltering syntax sugar based on scope only

3/ Align usage with and without pom

So,

Maven.resolver().loadPomFromFile("/path/to/file").importTestDependencies().importCompileRuntimeDependencies().resolve().using(Strategy s).asFile();
Maven.resolver().loadPomFromFile("/path/to/file").importTestDependencies().importCompileAndRuntimeDependencies().resolve().withTransitivity().asFile();
Maven.resolver().loadPomFromFile("/path/to/file").importDependencies(ScopeType...scopes).resolve().withTransitivity().asFile();

Update: How to include all test dependencies from pom.xml but A:B for resolution?

This is not possible with new API proposal. The workaround is to reject all dependencies that origin from A:B in resolution filtering phase.

Maven.resolver().loadPomFromFile("/path/to/file").importTestDependencies().resolve().using(new RejectDependencies(true, "A:B")).asFile

Attaching discussion image:

Implementation details:

https://github.com/kpiwko/resolver/tree/SHRINKRES-112

[alrubinger]

Work in progress in:

  https://github.com/shrinkwrap/resolver/tree/SHRINKRES-112

I've added some commits to:

1) Limit the impact of the API changes

2) Remove some formatting-only changes in the tests to make more clear exactly what's impacted from a user perspective

Ultimately I think this appraoch outlined by Karel is the correct course of action for maintainability once CRs and Final is issued.  There *will be* some limited API changes, breaking a design criteria of our Beta release process.  In this case, I believe the changing the design here is the lesser of two evils when compared with keeping a strict API lock.

With a +1 I'll squash these down and put upstream.

S,

ALR

[kpiwko]

+1, definitely greatly polished.

[rsmeral]

I know this is a bit late, after the beta-4 release, but still:

{code}Maven.resolver().loadPomFromFile("pom.xml").importCompileAndRuntimeDependencies().resolve().withTransitivity().asFile();{code}

reads OK,

{code}Maven.resolver().resolve("a:b:c").withTransitivity().asFile();{code}

reads OK too, but

{code}Maven.resolver().loadPomFromFile("pom.xml").importCompileAndRuntimeDependencies().resolve("a:b").withTransitivity().asFile();{code}

looks quite strange and it's not immediately clear what it does.

Does it resolve only "a:b" from the imported dependencies, acting as a filter? Does it resolve "a:b" additionally to the imported dependencies?

The second option is currently true, but it is a bit confusing, IMHO.

Also, are both: addDependency(..) and resolve(..) necessary? Wouldn't it make sense if there was only addDependency(MavenDependency) and addDependency(String) and only argumentless resolve()?