[MNG-7344] track dependencyManagement import location in effective Model for MPH-183 #603

jjkester · 2021-11-01T19:46:06Z

The goal of MPH-183 is to print via which BOM(s) a dependencyManagement dependency got in the effective POM in case of dependencyManagement import.
MNG-7344 implements the required tracking in Maven core effective model.

This information was previously not available to the help plugin. This MR:

introduces a change to the InputSource model to keep a hierarchy of sources (which are POM files) by tracking the POM that imported it
tracks dependencies through different dependency management imports, setting the hierarchy of sources on the InputSource objects

For now, this code has been written under the assumption that a BOM hierarchy (at least at the point the dependency management blocks are imported) is tree-shaped, without diamonds. That is, given a project P with dependency D, importing BOMs A and B, BOM C with dependency D, only A or B (not both) can import C.

Check assumption

If such a structure would be possible, the end result may be incorrectly represented as C via B via A, which is not correct, because there is no direct relationship between A and B.

Furthermore, at the moment the original InputSource of the file is mutated. This makes the hierarchy valid for all imported dependencies, and potentially other parts of the effective POM. This may not be desirable. A relatively minor change, replacing InputSource objects that are about to be mutated, would solve this issue. In case updating the original InputSource is desirable, there may be a better place to set it.

Check solution strategy

Any input on the above points is highly appreciated.

A pull request for printing this information in the effective POM: apache/maven-help-plugin#37

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MNG-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

rfscholte · 2021-11-09T09:24:34Z

maven-model/src/main/mdo/maven.mdo

@@ -3137,6 +3137,20 @@
            ]]>
          </description>
        </field>
+        <field>
+          <name>importedBy</name>


Thinking out loud: there are more ways to define a path to a dependency. Via parent is another one, just like maven-tiles by @talios. There are plans to support import for plugins too.
In the end there's only 1 path, so let's use a more generic name.
Not sure if there is a need to keep track of the type of reference (parent, import-dependency), so let's do that if people ask for it.

I will look into that approach.

I did consider pluginManagement as well, but this approach (as-is) would be easy to replicate in the similar DefaultPluginManagementImporter.

In order to keep track of all references, I think it needs to be tracked elsewhere (and I have not found that location yet - but I will have a look).

In order to support keeping the type of reference, I propose we do not reference an InputLocation or InputSource (see also the discussion here) directly, but use a container type that can be extended (for example ReferencedInputLocation).

rfscholte · 2021-11-09T09:34:47Z

...er/src/main/java/org/apache/maven/model/composition/DefaultDependencyManagementImporter.java

+            return;
+        }
+
+        while ( hierarchicalSource.getImportedBy() != null )


Is this loop really necessary? Would love to see an example that needs it. (maybe as an integration test)

I had the impression while testing that in long chains (e.g. pom - bom1 - bom2 - bom3 - bom4) parts of the objects defined in bom4 are copied to bom3, then bom2, etc. This loop ensures that we set a new location at the right place, without overwriting any existing. I will however investigate this. Also, I expect this code to be quite different if we decide to go with InputLocation (see here) so I'll look into that first.

rfscholte · 2021-11-09T09:37:21Z

...er/src/main/java/org/apache/maven/model/composition/DefaultDependencyManagementImporter.java

+            return;
+        }
+
+        InputSource hierarchicalSource = dependencyLocation.getSource();


This is actually very interesting: it looks like it is possible to even use InputLocation instead of InputSource, whcih would give even more details of the path. How about using that instead?

That is interesting. InputLocation is tied to a specific line in the file, so making those hierarchical would only work for single locations. Given that a dependency has many, we either need to decide on what to track or just modify them all (the latter would of course be much more generic). Since the InputSource objects are per-file, that is just a quick way to get some order.

Just to check your suggestion, would your idea be that an InputSource refers to the InputLocation where it is referenced? In that case (keeping in mind a more generic solution), I think there are a few things to investigate:

Is there a place in the code where we have access to the InputLocation of the reference (parent, import, ...) and the InputSource of the referenced POM?

In this place in the code, is the model already a tree or is it a graph? I.e., is there only one path from the root POM to every other (indirectly) referenced POM?

A simple test case for the latter could be a project like this:

pom.xml --imports-> bom1 | | imports imports | | v v parent --imports-> bom2 | imports | v bom3

There are two paths to resolve a dependency defined in bom3, ideally there should be a deterministic way to define the path that Maven takes. And that path should (in my opinion) be accurately reflected in the model. Currently my knowledge is too limited to determine how Maven actually resolves this.

For the effective pom there will be only one route, either bom1 or parent wins. Saying it differently: if something in bom3 has to be changed and I have access to both bom1 and parent, I need to know which of the 2 has to be changed.

jjkester · 2021-11-23T16:25:26Z

Hey @rfscholte, would it be appropriate to create a new ticket for this? The ticket this work is under is focused on maven-help-plugin, but this is turning out a bit more involved. I'm happy to work on it, but I think it makes sense to make the requirements, reasoning and discussion easily findable at a location where it is expected.

If you agree, can you create a ticket and formulate your ideas and expected results?

rfscholte · 2021-11-23T21:37:21Z

I've created https://issues.apache.org/jira/browse/MNG-7344

hboutemy

now I need to check MPH-183

and I'd love to have equivalent update for 3.9.x: I'll dig into it later

hboutemy · 2024-03-31T07:32:59Z

api/maven-api-model/src/main/java/org/apache/maven/api/model/InputLocation.java

@@ -83,6 +95,13 @@ public Map<Object, InputLocation> getLocations() {
        return locations;
    }

+    /**
+     * Gets the input location that caused this model to be read.
+     */


we need an @since here to remember

"that caused this model to be read."?
I need to dig more into code to propose another description, but this one is not clear

hboutemy · 2024-03-31T07:33:45Z

api/maven-api-model/src/main/java/org/apache/maven/api/model/InputLocationTracker.java

@@ -20,4 +20,6 @@

 public interface InputLocationTracker {
    InputLocation getLocation(Object field);
+
+    InputLocation getImportedFrom();


same @since

hboutemy · 2024-03-31T07:36:33Z

api/maven-api-model/src/main/java/org/apache/maven/api/model/InputSource.java

+    }
+
+    public InputSource importedFrom(InputLocation importedFrom) {
+        return new InputSource(modelId, location, importedFrom);


really create a new object and forget about it? strange (i did not really dig in depth, just giving initial feedback)

This is due to the immutable model, but I think we can simply remove this and make the new constructor public. We don't seem to use it (anymore) upon further inspection...

hboutemy · 2024-03-31T07:36:46Z

api/maven-api-model/src/main/java/org/apache/maven/api/model/InputSource.java

@@ -64,6 +74,14 @@ public String getModelId() {
        return this.modelId;
    }

+    public InputLocation getImportedFrom() {


same @since

hboutemy · 2024-03-31T07:38:36Z

maven-model/src/main/java/org/apache/maven/model/InputLocation.java

+    /**
+     * Get the imported from location.
+     *
+     * @return InputLocation


same @since

hboutemy · 2024-04-01T21:03:52Z

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java

+                // And map model v3 to model v4 -> importMgmt(v3).getDelegate() returns a v4 object
+                importMgmts.add(
+                        org.apache.maven.api.model.DependencyManagement.newBuilder(importMgmt.getDelegate(), true)
+                                .importedFrom(dependency.getDelegate().getLocation(""))


importedFrom should not be assigned to DependencyManagement but to each Dependency, isn't it?

@hboutemy you're right. It should only map it from model v3 to model v4. We're going to remove the line .importedFrom(dependency.getDelegate().getLocation(""))

… where it shouldn't

gnodet

The changes to DefaultModelBuilder and DefaultDependencyManagementImporter need to be moved from maven-model-builder to the implementations in maven-api-impl, as the ones from maven-model-builder are kept for compatibility but are not used anymore.

…management-hierarchy-inputsource

gnodet · 2024-07-09T14:21:55Z

The changes to DefaultModelBuilder and DefaultDependencyManagementImporter need to be moved from maven-model-builder to the implementations in maven-api-impl, as the ones from maven-model-builder are kept for compatibility but are not used anymore.

@Giovds I've merged, moved the code and fixed a few things, but I cannot push to your branch.
I've pushed to https://github.com/gnodet/maven/commits/MPH-183_dependency-management-hierarchy-inputsource/ instead.
The build seems to work for Maven, but I cannot find a way to display with MPH-183 PR.

juulhobert · 2024-07-12T10:09:40Z

@gnodet I've merged your changes into this branch. I've also verified that the output is still the same. If you would like to test it yourself, these are the reproduction steps:

Download example project mng-7344.zip
Install dependencies and boms in the same sequence as in validate.sh
mvn install help plugin branch MPH-183_effective-pom-path-to-source
mvn org.apache.maven.plugins:maven-help-plugin:3.4.1-SNAPSHOT:effective-pom -Dverbose (replace mvn with the packaged Maven from this branch)

juulhobert · 2024-08-02T13:12:21Z

@mthmulders can you please run the failed integration tests again? I expect it to give a green check mark this time (see https://github.com/infosupport/maven/actions/runs/10215678351)

gnodet · 2024-08-10T22:39:38Z

@juulhobert I merged another PR which causes a small conflict with this one.
As I can't write to your branch, could you please merge master and push ?

…ncy-management-hierarchy-inputsource

juulhobert · 2024-08-13T06:32:40Z

@gnodet I've merged the changes and pushed it

jjkester mentioned this pull request Nov 1, 2021

[MPH-183] Effective POM path to source apache/maven-help-plugin#37

Open

9 tasks

rfscholte requested changes Nov 9, 2021

View reviewed changes

mthmulders force-pushed the MPH-183_dependency-management-hierarchy-inputsource branch from b814fec to 67465f0 Compare May 31, 2023 14:32

jjkester and others added 4 commits February 9, 2024 15:32

[MPH-183] [WIP] BOM hierarchy in InputSource

2767730

Formatting

550bb4d

WIP and notes

198c581

[MNG-7344] Recursive imported from tracking

38dc034

juulhobert force-pushed the MPH-183_dependency-management-hierarchy-inputsource branch from 67465f0 to 38dc034 Compare February 9, 2024 15:01

juulhobert and others added 4 commits March 1, 2024 16:25

[MNG-7344] Imported from in the v3 model

069ae93

[MPH-183] Fix error in import path and small refactor

3cd4d9d

[MPH-183] Remove indentation for readability

b50cda2

[MPH-183] Add getLocationKeys for help plugin

9881c22

hboutemy force-pushed the master branch from a075afa to 6e50886 Compare March 24, 2024 21:16

juulhobert and others added 2 commits March 29, 2024 10:01

[MNG-7344] Unit tests for updatedWithImportedFrom

941b502

[MPH-183] Rename test to better match behaviour

71c2989

hboutemy changed the title ~~[MPH-183] BOM hierarchy in InputSource~~ [MNG-7344] track dependencyManagement import location in effective Model for MPH-183 Mar 30, 2024

hboutemy requested changes Mar 31, 2024

View reviewed changes

hboutemy reviewed Apr 1, 2024

View reviewed changes

juulhobert and others added 4 commits April 12, 2024 10:29

[MNG-7344] Removed line that changed the importedFrom in the dep mgmt…

21541ed

… where it shouldn't

[MNG-7344] Improve comment on getImportedFrom method

1a9aa52

[MNG-7344] Remove unused method and make constructor public

237de75

[MNG-7344] Add since and docs to methods

73f5ff2

gnodet requested review from rfscholte and hboutemy July 8, 2024 15:41

gnodet requested changes Jul 8, 2024

View reviewed changes

gnodet added this to the 4.0.0-beta-4 milestone Jul 9, 2024

gnodet added 2 commits July 9, 2024 15:41

Merge remote-tracking branch 'origin/master' into MPH-183_dependency-…

b27c457

…management-hierarchy-inputsource

Move code to new implementation

78b912c

gnodet mentioned this pull request Jul 15, 2024

[MNG-7344] Integration test for effective pom with imports apache/maven-integration-testing#333

Open

Merge branch 'master' of github.com:apache/maven into MPH-183_depende…

1918fdc

…ncy-management-hierarchy-inputsource

gnodet approved these changes Aug 15, 2024

View reviewed changes

gnodet merged commit 5b61e95 into apache:master Aug 15, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-7344] track dependencyManagement import location in effective Model for MPH-183 #603

[MNG-7344] track dependencyManagement import location in effective Model for MPH-183 #603

jjkester commented Nov 1, 2021 •

edited by hboutemy

Loading

rfscholte Nov 9, 2021

jjkester Nov 9, 2021

rfscholte Nov 9, 2021

jjkester Nov 9, 2021

rfscholte Nov 9, 2021

jjkester Nov 9, 2021

rfscholte Nov 10, 2021

jjkester commented Nov 23, 2021 •

edited

Loading

rfscholte commented Nov 23, 2021

hboutemy left a comment

hboutemy Mar 31, 2024

hboutemy Apr 1, 2024

hboutemy Mar 31, 2024

hboutemy Mar 31, 2024

Giovds Apr 12, 2024

hboutemy Mar 31, 2024

hboutemy Mar 31, 2024

hboutemy Apr 1, 2024

juulhobert Apr 12, 2024

gnodet left a comment

gnodet commented Jul 9, 2024

juulhobert commented Jul 12, 2024

juulhobert commented Aug 2, 2024

gnodet commented Aug 10, 2024

juulhobert commented Aug 13, 2024

[MNG-7344] track dependencyManagement import location in effective Model for MPH-183 #603

[MNG-7344] track dependencyManagement import location in effective Model for MPH-183 #603

Conversation

jjkester commented Nov 1, 2021 • edited by hboutemy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjkester commented Nov 23, 2021 • edited Loading

rfscholte commented Nov 23, 2021

hboutemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet left a comment

Choose a reason for hiding this comment

gnodet commented Jul 9, 2024

juulhobert commented Jul 12, 2024

juulhobert commented Aug 2, 2024

gnodet commented Aug 10, 2024

juulhobert commented Aug 13, 2024

jjkester commented Nov 1, 2021 •

edited by hboutemy

Loading

jjkester commented Nov 23, 2021 •

edited

Loading