Include the classifier when creating an ArtifactKey from a org.apachemaven.artifact.Artifact

[TheHound]

Include the classifier when creating an ArtifactKey from a org.apachemaven.artifact.Artifact to avoid issues with same dependency but different classifier & scope combinations.

[laeubi]

@TheHound can you explain the specific problem this solves? Could it probably even be demonstrated as a junit test (e.g. in org.eclipse.m2e.core.tests?

[github-actions]

Test Results

   210 files  +2     210 suites  +2   31m 24s ⏱️ + 4m 54s
   656 tests +1     647 ✔️ +1    9 💤 ±0  0 ❌ ±0 
1 312 runs  +2  1 294 ✔️ +2  18 💤 ±0  0 ❌ ±0 

Results for commit c7643f1. ± Comparison against base commit 172f6be.

♻️ This comment has been updated with latest results.

[TheHound]

Sure I'll have a go tomorrow at adding a unit test but in the meantime a basic explanation of the issue is imagine the following dependencies :

<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>slf4j-api</artifactId>
    <version>2.0.7</version>
    <scope>compile</scope>
</dependency>
<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>slf4j-api</artifactId>
    <version>2.0.7</version>
    <classifier>tests<classifier>
    <scope>test</scope>
</dependency>

The dependency doesn't matter just the concept of having the same dependency but using different classifier with different scope.
Now in MavenProjectFacade the artifact refs are created as

void setMavenProjectArtifacts(MavenProject mavenProject) {
  Set<ArtifactRef> collect = mavenProject.getArtifacts().stream()
      .map(a -> new ArtifactRef(new ArtifactKey(a), a.getScope()))
      .collect(Collectors.toCollection(LinkedHashSet::new));
  this.artifacts = Collections.unmodifiableSet(collect);
}

https://github.com/eclipse-m2e/m2e-core/blob/master/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java#L343
Because the ArtifactKey doesn't contain the classifier we end up with a set with two items with identical keys org.slf4:slf4j-api:2.0.7:: and different scopes.

This can cause issues in other plugins using m2e (for example I am pretty sure the eclipse Jetty plugin doesn't handle maven projects using classifiers because of this)

[TheHound]

Have updated the commit with a test case demonstrating the expected change in artifact portable strings

[laeubi]

Looks good to me but as its is a quite central change will request a review from some other commiters.

[HannesWell]

This looks like a reasonable change for me.

@mickaelistria what do you think?

[HannesWell]

@mickaelistria, @akurtakov what do you think about this?
We plan do perform a release next week and I think it would be good to have this included.

[mickaelistria]

I don't see a reason against this change; unless test do show some related error.

[laeubi]

It seems a versionbump is required, beside this I think it should be included in this release. FYI @fbricon

[fbricon]

I think the classifier was not considered initially due to workspace project resolution. You need to ensure a workspace project A depending on workspace B and B:tests won't break for instance.
https://github.com/tesla/m2e-core-tests/blob/0652381e7da3c6dfa5bb5e864e215da786bb2efd/org.eclipse.m2e.tests/src/org/eclipse/m2e/tests/BuildPathManagerTest.java#L740-L764 might be relevant

[HannesWell]

I think the classifier was not considered initially due to workspace project resolution. You need to ensure a workspace project A depending on workspace B and B:tests won't break for instance. https://github.com/tesla/m2e-core-tests/blob/0652381e7da3c6dfa5bb5e864e215da786bb2efd/org.eclipse.m2e.tests/src/org/eclipse/m2e/tests/BuildPathManagerTest.java#L740-L764 might be relevant

The test passes in the CI. So should we still consider this or better have another solution?

[fbricon]

I haven't tested that change. If you're confident it's safe, then go ahead with it.

[HannesWell]

I haven't tested that change. If you're confident it's safe, then go ahead with it.

Me neither and since I and nobody else considered the case you mentioned I'm not that confident.
Nevertheless in order to make progress here. but in a safer way I suggest to merge this after the upcoming release is out (and we are sure we don't contribute a follow up to Sim-Rel). Then we have some time to test the change via snapshots and get more confidence for the next release.

[HannesWell]

Let's see how this works IRL.
Thanks @TheHound!