Merge scopes when adding a dependency that exists but with different scope

[fabapp2]

Currently, the implementation will add a dependency with compile scope when the same dependency already exists with test scope. It should merge these dependencies and set the state accordingly (or throw exception?)

Originally posted by @fabapp2 in #309 (comment)

[ravig-kant]

Hi @fabapp2

Any takers for this? Otherwise, I will give it a shot.

[fabapp2]

HI @ravig-kant !
Thank you for picking this up. 🚀
The description is rather minimal.
Please don't hesitate to ask

assigned-to: @ravig-kant

[ravig-kant]

Hi @fabapp2

Here are my initial thoughts. OpenRewriteMavenBuildFile OpenRewriteMavenBuildFile logic should be changed as follow.

• If the given Dependency is added to the test scope
  • Lookup in compile scope and no-op if found
  • Lookup in test scope and no-op if found
  • Add dependency to the test scope
• If the Dependency is added to the compile scope
  • Lookup in the test scope. If found, remove from test scope
  • Lookup in the compile scope. If found no-op
  • Add to the compile scope
• If the Dependency is added without specifying the scope
  • Lookup in the test scope. If found, throw an exception that dependency already exists in the test scope
  • Lookup in the compile scope. If found no-op
  • Add to the compile scope

Let me know your thoughts.

[fabapp2]

Hi @fabapp2

Here are my initial thoughts. OpenRewriteMavenBuildFile OpenRewriteMavenBuildFile logic should be changed as follow.

• If the given Dependency is added to the test scope
  • Lookup in compile scope and no-op if found
  • Lookup in test scope and no-op if found
  • Add dependency to the test scope
• If the Dependency is added to the compile scope
  • Lookup in the test scope. If found, remove from test scope (or change scope to compile?)
  • Lookup in the compile scope. If found no-op
  • Add to the compile scope
• If the Dependency is added without specifying the scope

The scope is then set to compile in OpenRewriteMavenBuildFile:437:
dependency.getScope() == null ? "compile" : dependency.getScope(),

• Lookup in the test scope. If found, throw an exception that dependency already exists in the test scope

Why not changing it to compile?

• Lookup in the compile scope. If found no-op
• Add to the compile scope

Let me know your thoughts.

Hi @ravig-kant

thanks for looking into it!
I looked into Baeldung and this would be the change matrix I (think) I understood. It states that system is deprecated and I think we can ignore it if it is unclear or gives us a headache.
I am not completely sure about the implications with transitive dependencies and scope settings in dependencyManagement and would need to consult the latest documentation.
But, if we use the effective dependencies these settings in transitive and managed should be reflected and this shouldn't be an issue (I recall that explicit scope overwrites scope in managedDependencies 🤔).

current / given	provided	compile	test	system	import
provided				system	added with type="pom"
compile	provided			system	added with type="pom"
test	provided	compile		system	added with type="pom"
system	provided				added with type="pom"
import	added without type="pom"	added without type="pom"	added without type="pom"	added without type="pom"	added with type="pom"

Let me know your thoughts. 😃

[ravig-kant]

Hi @fabapp2
Here are my initial thoughts. OpenRewriteMavenBuildFile OpenRewriteMavenBuildFile logic should be changed as follow.

• If the given Dependency is added to the test scope

  • Lookup in compile scope and no-op if found
  • Lookup in test scope and no-op if found
  • Add dependency to the test scope

• If the Dependency is added to the compile scope

  • Lookup in the test scope. If found, remove from test scope (or change scope to compile?)

Agree. I meant to say remove from test scope and add to compile.

• Lookup in the compile scope. If found no-op
• Add to the compile scope
• If the Dependency is added without specifying the scope

The scope is then set to compile in OpenRewriteMavenBuildFile:437: dependency.getScope() == null ? "compile" : dependency.getScope(),

• Lookup in the test scope. If found, throw an exception that dependency already exists in the test scope

Why not changing it to compile?

Sure. Let's do that.

• Lookup in the compile scope. If found no-op
• Add to the compile scope

Let me know your thoughts.

Hi @ravig-kant

thanks for looking into it! I looked into Baeldung and this would be the change matrix I (think) I understood. It states that system is deprecated and I think we can ignore it if it is unclear or gives us a headache. I am not completely sure about the implications with transitive dependencies and scope settings in dependencyManagement and would need to consult the latest documentation. But, if we use the effective dependencies these settings in transitive and managed should be reflected and this shouldn't be an issue (I recall that explicit scope overwrites scope in managedDependencies 🤔).

current / given provided compile test system import
provided system added with type="pom"
compile provided system added with type="pom"
test provided compile system added with type="pom"
system provided added with type="pom"
import added without type="pom" added without type="pom" added without type="pom" added without type="pom" added with type="pom"
Let me know your thoughts. 😃

Hi @fabapp2

If we are promoting a dependency from transitive to direct, it should not be an issue. Direct dependencies take precedence over transitive dependencies.
A dependency in 'dependencyManagement' can be in the same build file, or parent projects build file. In the former case, I think we should update the 'depedencyManagement' section. For the latter, we can override, but the users should be informed about the change.
From the official documentation, it seems the import scope is only for 'dendencyManagement' section. I prefer changing the' dependencies' section in this enhancement. However, this contradicts with my proposal in last paragraph :)
I agree that 'system' scope be left out of the scope. The dependency refers to a jar from the local file system. So I don't see the need to change something to the system scope. However, promoting a system scope dependency to provided/compile/test is possible, although it may be challenging because the artefact and group name may not match.

Let me know your thoughts.

P.S.: My responses will be delayed due to the festivities.

[ravig-kant]

Hi @fabapp2

While implementing the change, I got some doubts about your proposed change matrix :). The first column represents the scope of the dependency in the current state, while the first row represents the proposed change.

For instance, if a dependency is currently in compile scope and it's now being added in provided scope, as per the matrix, the updated pom will have provided scope. Is it correct?

Ravi

[fabapp2]

Hi @ravig-kant

I hope you enjoyed the festivities!
Sorry for my late response, I was on vacation.

Maybe Actually, I overdid the requirement a bit 🤔

• 👍 For ignoring system scope.
• For Spring Boot provided scope will have the same effect as compile, given this comment. Not sure what the current state is but I'd say lets assume it's still the same and I'll clarify with the team? documented here
• 👍 For not modifying import scope. I am not sure (actually I doubt) if there's a valid case to define the same dependency (pom) as import and any other scope.

While implementing the change, I got some doubts about your proposed change matrix

Me too now, I guess we're back to the requirements you initially defined?! 😃

[ravig-kant]

Hi @fabapp2

I think I was too eager to make some progress 😅 . I just put together a tiny algorithm which works as below.

• Look up matching existing dependencies for a proposed new dependency. The match currently is being done based on the groupId and artefact.
• The matches are looked in the effective dependencies. So transitive are also checked. These potential matches are candidates and will decide the action ( upsert/no-op) for the proposed new dependency.
• Maintain a matrix of scopes which tells which other scopes a given scope supersedes. For instance, a compile scope supersedes any existing entries with Test, Provided and Runtime scope.
• If the proposed dependency supersedes the existing dependency, then perform an upsert operation otherwise, its a no-op
• If there are no potential candidates, add the proposed dependency.

You can look at this code which implements this algorithm. I think this algorithm satisfies the asks in this enhancement and can be extended for any future asks.

Thanks
Ravi

[fabapp2]

Ha! I like your attitude 💪

I need to look at your code next few days, as being a bit behind due to my vacation.

[fabapp2]

Hi @ravig-kant

looks nice clean and concise. 🤩
Could you add tests, embed it into a recipe (yaml?), and create a PR?

[ravig-kant]

Hi @fabapp2

Thanks. I have created a PR. I have changed the AddDependency action instead of creating a new recipe.
The assumption is that the recipes will use the AddDependency action. I want to understand more about MultiModuleAwareAction. Can you provide some document pointers to understand it?
There are two APIs to add a dependency. One is, of course, AddDependency action. Another is the BuildFile interface. What is the difference between the two? I wanted to change the OpenRewriteBuildFile.addDependency implementation, but I was unsure about its existence.