✨ feat: Include BOM POMs in the lockfile

[brunoapimentel]

Adds a "bom" key to the main project and to each dependency node in case they declare BOM POMs as part of their dependency management section of the "pom.xml" file.

To resolve BOM POMs of dependencies, the algorithm implemented is roughly:

• Traverse the dependency graph:
  • Resolve the POM artifact for a node
  • Build a MavenProject for a node
  • Check the original model's dependency management section
  • Filter out BOM POMs from other dependencies
  • For each BOM POM found:
    • Resolve the POM artifact
    • Build the MavenProject (so we can get the resolved URL)
    • Resolve the potential hierarchy of parent POMs
    • Add it to the "boms" section of the relative node

Resolves #1483

[adambkaplan]

Overall approach looks good, however I'm not as knowledgeable with respect to which "model" to use (getOriginalModel vs. getModel). Lack of Javadoc from the Maven API code does not help!!

[adambkaplan]

Taking a second pass, found a potential bug. Otherwise I think this is in good shape.

[brunoapimentel]

There is a design choice behind this setter here. This way, we can set the boms variable only if there's content to it, which will make it not be rendered in the lockfile for nodes in which it is null.

I'm not sure if that's what we want, since I saw that other keys (such as children) are always present as an empty array.

[brunoapimentel]

@algomaster99 What would be the preferred approach here?

[algomaster99]

I would prefer to render them as null because if you don't render it, there will be two ways in which one can tell that boms is empty: and null. null clearly conveys here that indeed boms are not present.

[brunoapimentel]

But .dependencies[].children is rendered as an empty array. Wouldn't it be better to be consistent in how we render the "not present" attributes?

I thinking adding null will go against what you just mentioned: to have a consistent way to show that something is not present.

[algomaster99]

@brunoapimentel I am sorry I missed this comment.

I didn't see that the type of boms is a collection. So by "empty" in my last comment, I meant that the entire attribute is not present.

But .dependencies[].children is rendered as an empty array. Wouldn't it be better to be consistent in how we render the "not present" attributes?

You are right, and apologies for the confusion. I do agree it should be an empty array.

In short, all possible attributes of lockfile.json should be present.

[algomaster99]

Am I right to mark this PR as superseding #1505?

[brunoapimentel]

@algomaster99 Can you approve the CI to run on this one, please?

[algomaster99]

I haven't looked at the PR thoroughly. I will do it soon.

[algomaster99]

@LogFlames ghasum check is failing. Could you take a look?

[LogFlames]

It's likely due to it being a branch from a fork. We might have to set it up similar to how LockfilePR.yml is setup (with PR/PR from fork) using different tokens. Will investigate more 👍

[LogFlames]

Created #1526 that should resolve the ghasum update.

[algomaster99]

Thank you for this new feature! Comments are as follows.

[algomaster99]

Suggested change

	private static void resolveBomsForDependencies(
	private static void resolveBomsInDependencyManagement(

The function resolveProject only scans the dependency management section. Would this make sense?

[algomaster99]

It would be nice to mention dependency management import (for dependencies of type pom and scope import in the <dependencyManagement> section) source: https://maven.apache.org/ref/3.9.12/maven-model-builder/ as a comment here. credits to @maximeq95 for the idea.

[brunoapimentel]

The function resolveProject only scans the dependency management section. Would this make sense?

Well, my intention with the resolveBomsForDependencies name is to say that "we will resolve BOM POMs for every dependency your project has", regardless of how that is actually written in the pom.xml file.

To illustrate: if my project depends on junit, and junit depends on junit-bom, it will be resolved (that is, the BOM of the dependency was resolved).

But... that's how I see it. I have no strong attachment to the name, I can change it if you prefer.

[algomaster99]

Then I would just call it resolveBoms because resolveBomsForDependencies implies only the boms of the dependencies and not in the current maven project pom file. I infer this from what you describe above. Do you agree?

[brunoapimentel]

We already have a function called resolveBoms, and that one resolves the BOMs for the current project. resolveBomsForDependencies is dedicated only for dependencies.

But as I said, I'm glad to change the names to what you see fit.

[algomaster99]

I am sorry this is taking too long but I just realized about another concern here. resolveBomsForDependencies is not invoked in the call BomResolver graph resolveBoms -> resolveForProject -> resolveBomParents and it is done outside the actual flow. This is slightly confusing in my opinion. You can probably call resolveBomsForDependencies as part of resolveForProject

Also, resolveBomsForDependencies should live where all bom resolution logic is living - BomResolver.

Another thing I thought of here is that resolveForProject has side-effects because it mutates the dependency graph. I think we could change, perhaphs not in this PR, to augment our dependency graph with bom information.

Overall, I think refactoring resolveBomsForDependencies to be instead used by resolveForProject is a nice idea for this PR. Making it free of side effects could be done in a different PR.

[brunoapimentel]

You can probably call resolveBomsForDependencies as part of resolveForProject

The way it currently is, resolveForProject takes a MavenProject as input. The graph is resolved outside of a MavenProject, and has no relation to it, that's why we need to traverse the graph and build MavenProjects for each node, then resolveForProject.

This whole resolveForDependency workflow can indeed be part of the BomResolver, I can move it there if you prefer, but we will still need to keep separate calls for resolveForProject and resolveForDependencies (which is essentially, calling resolveForProject in a loop).

Please let me know if you had a better idea in mind.

[brunoapimentel]

I'd say the cause for this is the way we have the data classes structured... a MavenProject does not contain a DependencyGraph. I wouldn't touch this on this PR though. As we are uncovering more needs for the hermetic builds, I think the lockfile structure will need to go through a change. A good starting point to discuss this is #1530.

[brunoapimentel]

New push:

• Traverse the whole dependency graph when resolving BOM POMs (previously, this implementation was only covering root nodes)
• Always print the "boms" key

I still have some review items to address, so no rush in reviewing this yet.

[algomaster99]

I still have some review items to address, so no rush in reviewing this yet.

@brunoapimentel I am happy to go over again so please re-request review once you are done :)

[brunoapimentel]

I am happy to go over again so please re-request review once you are done :)

@algomaster99 Sorry for the delay, could you please take another look at this one?

This thread is the one that is concerning me the most right now.

[algomaster99]

Nit: also nice to put a comment here that org.sonatype.oss:oss-parent:7 is the parent.

[algomaster99]

@LogFlames it seems GitHub actions updated their maven version. How have you dealt with this before? Did you update versions in all lockfiles?

[algomaster99]

@algomaster99 Sorry for the delay, could you please take another look at this one?

No need to apologize :) We are already very grateful for contributions from you and your team :)

This thread is the one that is concerning me the most right now.

I don't undertstand what help you need with but I have left a comment here.

I also left another comment about boms of dependencies.

[LogFlames]

@LogFlames it seems GitHub actions updated their maven version. How have you dealt with this before? Did you update versions in all lockfiles?

Yes, previously we have updated the version in the lockfiles.

@copilot create a new branch, trigger lockfile regeneration in it and create a PR.

[LogFlames]

It doesn't seem like copilot could be tagged from here 😁 Will create it manually

[algomaster99]

Continuing the conversation of #1516 (comment) here.

When I was writing the review, I had this change in mind. I am a bit confused why resolveBomsForDependencies is invoked. It seems bad smell because it is mutating the graph by adding boms to nodes but I guess we have to do this as you said "a MavenProject does not contain a DependencyGraph".

I would suggest adding a TODO comment on resolveBomsForDependencies which says avoid such a side effect.

Also, now it is clearer to me that there are two independent steps. First one is setting boms for each depedency of the current project. The second step is getting boms for the current project.

Please let me know if you had a better idea in mind.

My ideal goal would be

1. Call resolveBoms on root maven project.
2. It calls resolveForProject (happens right now)
3. It goes through all dependencies of the root project and each dependency is a maven project.
4. So now, resolveBoms could be called on dependency itself.

Step 3 and 4 have been instead implemented separately and called before step 2. But I guess this would require refactoring of MavenProject.

I can move it there if you prefer, but we will still need to keep separate calls for resolveForProject and resolveForDependencies (which is essentially, calling resolveForProject in a loop).

Let's move the bom resolution logic in BomResolver. The LockfileFacade is anyway becoming quite big.

[brunoapimentel]

All right, that makes sense. I will do the changes you asked then.

Going a little ahead of what we need for this PR, though, I have to say that all of this is bound to get yet more complicated 😅 . I was testing building some projects hermetically, and I found we still have quite a few things to resolve.

Check the WIP commits here. Essentially, we will have to resolve BOMs and parents for every single dependency node, regardless of why it is imported in the first place (be it a plugin, direct dependency, transitive dependency, parent POM, BOM POM, etc).

So I'm not sure how we will approach this from a non-mutation point of view. If I understand it correctly, we can use Maven's API to resolve dependencies and plugins, but it won't automatically resolve BOMs and parent POMs, so that seems to be always a secondary effort on top of the already resolved graphs.

I think Adam's idea of having a separate flat list of artifact might help us to have a more organized lockfile, and even avoid wasting time processing the same node more than once. That will require changes to the data model as well, I believe.

[algomaster99]

I have to say that all of this is bound to get yet more complicated 😅 . I was testing building some projects hermetically, and I found we still have quite a few things to resolve.

        resolveParentPomsForDependencies(graph, session, project.getRemoteArtifactRepositories(), checksumCalculator);
        resolveBomsForDependencies(graph, session, project, checksumCalculator);
        resolveBomsForPlugins(plugins, session, project, checksumCalculator);
        var boms = resolveBoms(session, project, checksumCalculator);

+         resolveParentPomsForDependencies(graph, session, project.getRemoteArtifactRepositories(), checksumCalculator);
+         resolveBomsForDependencies(graph, session, project, checksumCalculator);

I guess these two could be combined, but indeed, there are many scenarios. We have tests for two cases right now

1. Project has one singe bom (bomPom)
2. Project has a bom which has a parent (bomPomWithParent)
3. Project has a dependencies which has a bom (classifier-dependency-check-correct)

What you add here is:

1. Project has a dependency which has parent pom and that has a bom.
2. Porject has a plugin that has a bom.

Am I right?

Regardless, let's think about the design of MavenProject and dependency later and get this PR merged. The onlu changes needed are:

1. Refactoring resolveForDependencies into bom resolver.
2. Adding a todo about "I would suggest adding a TODO comment on resolveBomsForDependencies which says avoid such a side effect.".
3. Adding a comment.

[brunoapimentel]

What you add here is:

Project has a dependency which has parent pom and that has a bom.
Porject has a plugin that has a bom.

And:

• Project that has a BOM which has a BOM :)

[brunoapimentel]

All right, so I believe all the changes you asked are now done.

[algomaster99]

Thanks for this huge addition! Looking forward to the follow up PRs :)