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

suztomo · 2019-08-12T20:10:18Z

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:

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.

eolivelli

Isn't ArtifactTransferException a temporary failure?
ArtifactNotFoundException sounds like the artifact is definitely not available.

Will this change add unpredictable behavior to builds?

suztomo · 2019-08-15T17:54:33Z

@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)

elharo

Is it plausible to add a test for this?

...ider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java

suztomo · 2019-08-16T21:23:56Z

Sure, let me think how to test this.

suztomo

@elharo Added test.

eolivelli · 2019-08-18T18:45:00Z

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 · 2019-08-18T18:46:20Z

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 @elharo (Revisiting this) Would you review this PR?

...ider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java

elharo

Nice discovery

elharo · 2020-01-10T12:56:08Z

.../src/test/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReaderTest.java

 import org.eclipse.aether.resolution.ArtifactDescriptorRequest;
 import org.mockito.ArgumentCaptor;

 public class DefaultArtifactDescriptorReaderTest
    extends AbstractRepositoryTestCase
 {

-    public void testMng5459()
+    DefaultArtifactDescriptorReader reader;


These can be private

These can be private

I agree, make this and the subsequent ones private

Marked them as private.

suztomo · 2020-01-14T16:17:01Z

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

michael-o · 2020-01-14T18:51:02Z

@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 · 2020-01-14T18:55:24Z

Sure.

michael-o

The change looks fine at first glance and makes sense. Please modify the required spots. I will run unit tests and ITs.

michael-o · 2020-01-16T15:39:38Z

.../src/test/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReaderTest.java

 import org.eclipse.aether.resolution.ArtifactDescriptorRequest;
 import org.mockito.ArgumentCaptor;

 public class DefaultArtifactDescriptorReaderTest
    extends AbstractRepositoryTestCase
 {

-    public void testMng5459()
+    DefaultArtifactDescriptorReader reader;


These can be private

I agree, make this and the subsequent ones private

.../src/test/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReaderTest.java

suztomo

@michael-o Thank you for review. Updated them.

suztomo · 2020-01-16T15:46:38Z

.../src/test/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReaderTest.java

 import org.eclipse.aether.resolution.ArtifactDescriptorRequest;
 import org.mockito.ArgumentCaptor;

 public class DefaultArtifactDescriptorReaderTest
    extends AbstractRepositoryTestCase
 {

-    public void testMng5459()
+    DefaultArtifactDescriptorReader reader;


Marked them as private.

.../src/test/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReaderTest.java

michael-o · 2020-01-16T19:53:42Z

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

rfscholte · 2020-01-16T20:49:12Z

apache/maven-integration-testing@e284945 should fix the http issues (master is also back to blue). All branches need to merge in master

michael-o · 2020-01-16T20:51:04Z

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

michael-o · 2020-01-16T22:42:30Z

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 · 2020-01-17T05:11:02Z

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

michael-o · 2020-01-17T07:52:56Z

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 · 2020-01-17T08:42:36Z

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 · 2020-01-17T10:24:34Z

@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 · 2020-01-17T13:11:40Z

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

michael-o · 2020-01-17T13:20:44Z

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.

…SSING policy upon ArtifactTransferException This closes #277

michael-o · 2020-01-17T13:47:15Z

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

suztomo · 2020-01-17T17:28:24Z

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 · 2020-01-17T19:44:26Z

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 · 2020-01-17T19:47:10Z

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

michael-o · 2020-01-17T20:05:26Z

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 · 2020-01-17T21:16:58Z

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 · 2020-01-17T21:23:51Z

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 · 2020-01-17T22:21:11Z

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

michael-o · 2020-01-17T22:31:57Z

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 · 2020-01-21T15:30:21Z

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 · 2020-01-21T20:25:47Z

@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 · 2020-01-21T21:31:23Z

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

suztomo · 2020-01-21T21:50:51Z

@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

suztomo · 2020-01-21T22:03:57Z

Memo for myself:

Non-existent artifact (guava:30.0 below) causes mvn command failure but checksum verification failure doesn't. They both seem to go the same code path around this ArtifactResolutionException now with this PR; IGNORE_MISSING was true. I guess the latter (checksum verification failure) did not go return null before this PR. Isn't the code in this PR supposed to throw ArtifactDescriptorException upon the missing artifact case? Isn't the IGNORE_MISSING flag controlling whether the build should fail or not?

  <dependencies>
    <dependency>
      <groupId>com.google.guava</groupId>
      <artifactId>guava</artifactId>
      <version>30.0</version> <!-- does not exist -->
    </dependency>
    <!--
    <dependency>
      <groupId>org.apache.maven.its.mng3470</groupId>
      <artifactId>dep</artifactId>
      <version>0.1</version>
    </dependency>
    -->
  </dependencies>

suztomo · 2020-01-22T19:55:35Z

@michael-o I think it's safer to special-case UnknownHostException. I confirmed that the integration tests pass. Would you take a look at this change?

michael-o · 2020-01-22T20:44:30Z

Will look at this tomorrow.

…MISSING policy upon ArtifactTransferException

This was referenced Aug 12, 2019

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

Closed

Enforcer rule: Paths to the missing artifact: (empty) GoogleCloudPlatform/cloud-opensource-java#834

Closed

eolivelli requested changes Aug 15, 2019

View reviewed changes

elharo reviewed Aug 16, 2019

View reviewed changes

...ider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java Outdated Show resolved Hide resolved

suztomo commented Aug 17, 2019

View reviewed changes

suztomo commented Jan 10, 2020

View reviewed changes

...ider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java Outdated Show resolved Hide resolved

elharo reviewed Jan 10, 2020

View reviewed changes

elharo approved these changes Jan 10, 2020

View reviewed changes

michael-o requested changes Jan 16, 2020

View reviewed changes

suztomo commented Jan 16, 2020

View reviewed changes

asfgit pushed a commit that referenced this pull request Jan 17, 2020

[MNG-6732] DefaultArtifactDescriptorReader.loadPom to check IGNORE_MI…

c08d92a

…SSING policy upon ArtifactTransferException This closes #277

[MNG-6732] - DefaultArtifactDescriptorReader.loadPom to check IGNORE_…

acc53b7

…MISSING policy upon ArtifactTransferException

gnodet force-pushed the artifacttransferexception2 branch from fbf6c42 to acc53b7 Compare July 3, 2023 23:29

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

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

Are you sure you want to change the base?

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

Conversation

suztomo commented Aug 12, 2019 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

suztomo commented Aug 15, 2019 • edited Loading

elharo left a comment

Choose a reason for hiding this comment

suztomo commented Aug 16, 2019

suztomo left a comment

Choose a reason for hiding this comment

eolivelli commented Aug 18, 2019

michael-o commented Aug 18, 2019 • edited Loading

suztomo left a comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

elharo Jan 10, 2020

Choose a reason for hiding this comment

michael-o Jan 16, 2020

Choose a reason for hiding this comment

suztomo Jan 16, 2020

Choose a reason for hiding this comment

suztomo commented Jan 14, 2020

michael-o commented Jan 14, 2020

suztomo commented Jan 14, 2020

michael-o left a comment

Choose a reason for hiding this comment

michael-o Jan 16, 2020

Choose a reason for hiding this comment

suztomo left a comment

Choose a reason for hiding this comment

suztomo Jan 16, 2020

Choose a reason for hiding this comment

michael-o commented Jan 16, 2020

rfscholte commented Jan 16, 2020

michael-o commented Jan 16, 2020

michael-o commented Jan 16, 2020 • edited Loading

suztomo commented Jan 17, 2020

michael-o commented Jan 17, 2020

rfscholte commented Jan 17, 2020

michael-o commented Jan 17, 2020

rfscholte commented Jan 17, 2020

michael-o commented Jan 17, 2020

michael-o commented Jan 17, 2020 • edited Loading

suztomo commented Jan 17, 2020

michael-o commented Jan 17, 2020

suztomo commented Jan 17, 2020

michael-o commented Jan 17, 2020 • edited Loading

suztomo commented Jan 17, 2020

michael-o commented Jan 17, 2020

suztomo commented Jan 17, 2020

michael-o commented Jan 17, 2020 • edited Loading

suztomo commented Jan 21, 2020

suztomo commented Jan 21, 2020 • edited Loading

michael-o commented Jan 21, 2020

suztomo commented Jan 21, 2020

suztomo commented Jan 21, 2020

suztomo commented Jan 22, 2020

michael-o commented Jan 22, 2020

suztomo commented Aug 12, 2019 •

edited

Loading

suztomo commented Aug 15, 2019 •

edited

Loading

michael-o commented Aug 18, 2019 •

edited

Loading

michael-o commented Jan 16, 2020 •

edited

Loading

michael-o commented Jan 17, 2020 •

edited

Loading

michael-o commented Jan 17, 2020 •

edited

Loading

michael-o commented Jan 17, 2020 •

edited

Loading

suztomo commented Jan 21, 2020 •

edited

Loading