[MNG-6732] - DefaultArtifactDescriptorReader.loadPom to check IGNORE_MISSING policy upon ArtifactTransferException

[suztomo]

This PR is to allow DefaultArtifactDescriptorReader.loadPom to check IGNORE_MISSING policy upon ArtifactTransferException.

Problem description in https://issues.apache.org/jira/browse/MNG-6732

Example project to demonstrate effect of ArtifactTransferException even when a dependency is unnecessary: https://github.com/suztomo/maven-missing-artifact .

---

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

• [x] 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.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] 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.
• [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [x] 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.

• [x] 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.

[suztomo]

@eolivelli Thank you for question.

Isn't ArtifactTransferException a temporary failure?

ArtifactTransferException does not specify that. ArtifactTransferException can be a permanent failure from a retired repository (such as http://repository.codehaus.org/ ) as well as a temporary repository failure.

ArtifactNotFoundException is subclass of ArtifactTransferException.

Will this change add unpredictable behavior to builds?

No, it does not add unpredictable behavior (as far as I know). Thinking of two cases:

• Case 1 When all Maven repositories involved are alive: No error. No unpredictable behavior.
• Case 2 When a Maven repository is temporarily/permanently down: Maven takes the following steps:
  1. DefaultRepositorySystem.collectDependencies builds a partial dependency graph through DefaultArtifactDescriptorReader.loadPom, rather than failing here (new behavior).
  2. The dependency graph is transformed via Maven's dependency mediation (no change) In my example, artifact-to-be-removed:1.0 is removed from graph in favor of artifact-to-be-removed:2.0.
  3. DefaultRepositorySystem.resolveDependencies tries to resolve dependencies. (no change) Maven fails to resolve dependencies if the graph contains an artifact hosted in the unavailable repository. (no change) Maven succeeds if the graph does not contain missing artifact, resulting in the same graph as "Case 1 When all Maven repositories involved are alive". (new behavior)

[suztomo]

Sure, let me think how to test this.

[eolivelli]

Okay. I feel I don't have enough context to sponsor this change, I am not saying I am -1

Any other committer with more insight on this kind of error?

[michael-o]

I need will some time to better understand this issue, so a review won't happen before end of this month. Maybe some other committer knows better.

[suztomo]

@michael-o Did you get a chance to check this?

[michael-o]

@michael-o Did you get a chance to check this?

I did a bit today, but am currently in a bad condition. Will try to pickup Thu or Fri. Still trying to wrap my head around the root problem.

[suztomo]

Sure.

[michael-o]

ITs are runing, I see repeated failures at home on my box and on a server at work. Will post the failures shortly.

[rfscholte]

https://github.com/apache/maven-integration-testing/commit/e284945d8600a63a3cda60e1f90433cc7674e1ed should fix the http issues (master is also back to blue). All branches need to merge in master

[michael-o]

These aren't the issues I see, still analyzing.

[michael-o]

OK, there is some work to be done. IT for MNG-5175 is failing. See http://home.apache.org/~michaelo/issues/MNG-6732/. It needs to be modified or split. @suztomo Do you have a proposal?

@rfscholte What would be the proper approach here? The PR makes sense to me.

[suztomo]

Let me check tomorrow. (I thought I checked “mvn test -Prun-its”, but maybe I forgot)

[michael-o]

Let me check tomorrow. (I thought I checked “mvn test -Prun-its”, but maybe I forgot)

Don't forget, they are here: https://github.com/apache/maven-integration-testing

[rfscholte]

https://issues.apache.org/jira/browse/MNG-5175 was fixed by @olamy so I hope he can provide feedback (but now he's enjoying holidays in Europe)

[michael-o]

@rfscholte The test itself is fine. I took at least two hours to understand why this is failing and it perfectly makes sense. The given IT simply tests for physical unavailability of a server, plus that an JAR POM artifact cannot be downloaded. We have changed the game in a way such that a missing POM can be ignored if the server is not available, it will continue with the JAR. So the breakage is correct. One simply need to adapt the test or copy to a new one.

[rfscholte]

As mentioned by Jason, please don't touch old ITs. Better to copy it and adjust the mavenranges for both.

[michael-o]

As mentioned by Jason, please don't touch old ITs. Better to copy it and adjust the mavenranges for both.

That's probably the way even if it means code duplication.

[michael-o]

I have uploaded a squash branch, will tests again..

[suztomo]

Using https://github.com/apache/maven-integration-testing, I confirmed test failure > testmng5175_ReadTimeOutFromSettings

Better to copy it and adjust the mavenranges for both.

I'll follow that.

[michael-o]

Using https://github.com/apache/maven-integration-testing, I confirmed test failure > testmng5175_ReadTimeOutFromSettings

Better to copy it and adjust the mavenranges for both.

I'll follow that.

I have pushed a branch for the ITs addressing this issue.

[suztomo]

Thank you, then I'll wait for the integration test updated.

[michael-o]

Here is the next problem: IT for MNG-3470 fails, see http://home.apache.org/~michaelo/issues/MNG-6732/IT-3470/. The checksum validation is ignored because the ChecksumFailureException is wrapped inside an ArtifactTransferException. The problem is even if we could determine wether it is IGNORE_MISSING or IGNORE_IVALID in this case and the checksum policy would be fail, but the ArtifactDescriptorPolicy is lax, we have a conflict. The checksum policy needs to beat the artifact descriptor policy. This is a hard nut to crack. Comments? I believe that the current code is broken and your PR simply reveals this bug.

This is our problem in org.apache.maven.repository.internal.MavenRepositorySystemUtils:

session.setArtifactDescriptorPolicy( new SimpleArtifactDescriptorPolicy( true, true ) );

[suztomo]

I believe that the current code is broken and your PR simply reveals this bug.

I agree. The policy for the artifact should not have ArtifactDescriptorPolicy.IGNORE_MISSING. I see dependency setup is very normal. If the artifact is missing, the build should fail. https://github.com/apache/maven-integration-testing/blob/master/core-it-suite/src/test/resources/mng-3470/pom.xml#L37

[michael-o]

I believe that the current code is broken and your PR simply reveals this bug.

I agree. The policy for the artifact should not have ArtifactDescriptorPolicy.IGNORE_MISSING. I see dependency setup is very normal. If the artifact is missing, the build should fail. https://github.com/apache/maven-integration-testing/blob/master/core-it-suite/src/test/resources/mng-3470/pom.xml#L37

The problem is that transfer is fine, the file is there, but checksum is intentionally wrong. So ignore missing does not apply here. It could be IGNORE_INVALID.

[suztomo]

How about adding if statement for IGNORE_INVALID? (I'll do experiment next week)

[michael-o]

How about adding if statement for IGNORE_INVALID? (I'll do experiment next week)

I did that:

diff --git a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java
index 0e9a5745e..da13f363d 100644
--- a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java
+++ b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java
@@ -245,7 +245,7 @@ private Model loadPom( RepositorySystemSession session, ArtifactDescriptorReques
             catch ( ArtifactResolutionException e )
             {
                 Throwable cause = e.getCause();
-                if ( cause instanceof ArtifactTransferException )
+                if ( cause instanceof ArtifactNotFoundException )
                 {
                     missingDescriptor( session, trace, a, (Exception) cause );
                     if ( ( getPolicy( session, a, request ) & ArtifactDescriptorPolicy.IGNORE_MISSING ) != 0 )
@@ -253,6 +253,14 @@ private Model loadPom( RepositorySystemSession session, ArtifactDescriptorReques
                         return null;
                     }
                 }
+                if ( cause instanceof ArtifactTransferException )
+                {
+                    missingDescriptor( session, trace, a, (Exception) cause );
+                    if ( ( getPolicy( session, a, request ) & ArtifactDescriptorPolicy.IGNORE_INVALID ) != 0 )
+                    {
+                        return null;
+                    }
+                }
                 result.addException( e );

Doesn't make a difference.

[suztomo]

Memo for myself: Invoking the test via IntelliJ did not fail the test somehow. Command-line worked:

suztomo-macbookpro44:maven-integration-testing suztomo$ mvn clean install -Prun-its -Dmaven.repo.local=`pwd`/repo -DmavenDistro=$HOME/maven/apache-maven/target/apache-maven-3.7.0-SNAPSHOT-bin.zip -Dtest=MavenITmng3470StrictCheckumVerificationOfDependencyPomTest -DfailIfNoTests=false 
...
[INFO] Running org.apache.maven.it.MavenITmng3470StrictCheckumVerificationOfDependencyPomTest
mng3470StrictCheckumVerificationOfDependencyPom(it).........FAILURE (6.6 s)
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 6.746 s <<< FAILURE! - in org.apache.maven.it.MavenITmng3470StrictCheckumVerificationOfDependencyPomTest
[ERROR] testit(org.apache.maven.it.MavenITmng3470StrictCheckumVerificationOfDependencyPomTest)  Time elapsed: 6.641 s  <<< FAILURE!
junit.framework.AssertionFailedError: Build did not fail despite broken checksum of dependency POM.
	at org.apache.maven.it.MavenITmng3470StrictCheckumVerificationOfDependencyPomTest.testit(MavenITmng3470StrictCheckumVerificationOfDependencyPomTest.java:63)

Maybe try export MAVEN_OPTS='-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005' to debug.

[suztomo]

@michael-o I cannot set breakpoints in IntelliJ to look at the values at line 250 of DefaultArtifactDescriptorReader.java for the test case; the test execution with the MAVEN_OPTS above does not stop there. Would you share how you setup debugger and your IDE/editor?

[michael-o]

Go to the interpolated IT directory in core-it-suite/target, use the args from the IT class and pass them to mvnDebug.

[suztomo]

@michael-o Now I can debug it with IntelliJ. Thank you.

suztomo@suxtomo24:~/maven-integration-testing/core-it-suite/target/test-classes/mng-3470$ /usr/local/google/home/suztomo/app/maven/apache-maven-3.7.0-SNAPSHOT/bin/mvnDebug validate --settings settings.xml