Resolved stackoverflow exception for circular dependencies.

[Jenson3210]

Not able to (unit) test due to no known circular dependencies in OSS packages.

Fixes #4299

[timtebeek]

Great to see you worked this out! I'll tag Sam for a review once he gets back on Tuesday; but we can likely merge already given the small scope of the change. Did you run this against your troublesome project LSTs from your IDE classpath already?

[github-actions]

Some suggestions could not be made:

• rewrite-core/src/main/java/org/openrewrite/internal/lang/NullFields.java
  • lines 36-36

[Jenson3210]

@timtebeek , no.
I have a question raised here on how to get my new snapshot to run without sonatype download to override my snapshot:
https://rewriteoss.slack.com/archives/C01A843MWG5/p1720125622258949?thread_ts=1720125232.837419&cid=C01A843MWG5

[Jenson3210]

Tested locally and recipe ran fine!

[Jenson3210]

FYI:
This PR will besides fixing circular dependency stackoverflow exception also help in performance of the bump. If 4 libraries pull the same dependency, it's not worth it doing the nesting 4 times.

[shanman190]

Personally, I'm more suspicious of a deep dependency graph resulting in the stack overflow rather than circular dependencies. Gradle and Maven will typically bawk about circular dependencies rather than trying to resolve them.

The change as is seems like it would fail to update the new dependency replacement throughout the configuration graph which could make it look like there are old and new versions coming in when that's not strictly true.

[Jenson3210]

@shanman190 , thanks for kicking in here!
You mean to say the lst would contain faulty resolvedDependency versions after this change as we're returning the existing dep if we already covered this dependency?

I do think circular dependencies are possible in the following setup:

Lib B builds a version B.1 without dependency on Lib A
Lib A builds with dependency on B.1 publishing A.1
Lib B now needs a dependency on Lib A consuming A.1 publishing B.2

Now my endproduct/consumer needs both libs and pulls in A and B. At the moment of resolution, gradle seems to take highest version of A and B in some cases without bawking.
We would've loved it to crash, but as it is already there for quite some time, we can now not resolve this issue in the consumer.

Attached is the screenshot of a circular one we're seeing here (with masked values as circular "mess" is inhouse code) We have an enginedmn lib and relmgmtdmn lib which are always resolved to the same version as the consumer can only have one of each.

[Jenson3210]

[shanman190]

I see. That's certainly interesting.

But yeah, each resolved dependency would be unique. So if libA A.1 appears multiple times in the graph it was meant to be replaced with libA A.2 throughout the graph.

With this change presently, if we started with (where libC is the direct dependency):

+ libC
|-- libB
|---- libA V.1
|-- libA V.1

Then we would only be updating the libA that is a dependency of libB resulting in the following:

+ libC
|-- libB
|---- libA V.2
|-- libA V.1

This part in particular doesn't seem right to me given what the goal of that particular code block was attempting to do. What we may have to do to stay correct is follow each recursive path and do a replacement on the first one of that path that we encounter (which would break the cycle), but we would want to continue down the other paths to make sure that we perform the update throughout the graph.

[Jenson3210]

do a replacement on the first one of that path that we encounter
In this current case we're facing it the dependency that needs an update would not be part of these libraries, so we will not encounter it.
So I will still see the stackoverflow error, no?

I might be misunderstanding you :)

[shanman190]

I'm just meaning from the dependency graph standpoint where libA, libB, and libC are all remote resolved dependencies. There is no guarantees about the dependency graph shape or how many times a single dependency can appear within the tree of resolved dependencies. A particular libB dependency could appear multiple times throughout the tree at differing levels all of which would have their own libA copy. All of the libA's need to be updated not just the first one encountered.

What I think we need to do is keep a memo of our current traversal path and back check where we have come from, so that if we identify a point in our current depth first path where we are now encountering a dependency that we have already passed through previously (positive identification of the cycle itself), then we should no longer continue going down this immediate path any further and instead move on to the next closest depth first dependency path to continue scanning for possible updates to apply.

A paths in my head is using the simple dependency graph from earlier would be:

1) libC -> libB -> libA
2) libC -> libA

We would want to ensure that if we have updated libA, that both copies in libC's dependency graph got updated to the new version of libA.

[Jenson3210]

This is becoming a advent of code kinda challenge 🤣

I get it now!
Will try to get something created as such

thanks for you patience! ;)

[Jenson3210]

@shanman190 I think this is what you meant. This is also working.

I checked if keeping a Map<ResolvedDependencyGroupArtifactVersion, ResolvedDependency> accross all configurations would be beneficial to performance (spring libs are often being traversed multiple times, so if we could get from map the already bumped ResolvedDependency, would be possible improvement, BUT no performance gain in my tree found. And the project is not the smallest in dependency tree... So did not implement it.

[shanman190]

@Jenson3210, yep! That should fix your issue while not introducing a new issue like I was describing. I'll let Sam or Tim give it a once over as well, but it looks good to me. Thanks for the fix here!

[timtebeek]

Looks good indeed! Strikes the right balance between preventing circular resolve issues without halting an update early. Thanks!