391400 Include repository URL in status if getArtifacts fails

Bug: 391400 Status returned by getArtifacts does not include repository URL of problematic repository in case of an error

7 files changed, 74 insertions, 6 deletions

diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/CompositeArtifactRepository.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/CompositeArtifactRepository.java
index 4a126c046..2a8701242 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/CompositeArtifactRepository.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/CompositeArtifactRepository.java
@@ -334,7 +334,7 @@ public class CompositeArtifactRepository extends AbstractArtifactRepository impl
 
 	public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monitor) {
 		SubMonitor subMonitor = SubMonitor.convert(monitor, requests.length);
-		MultiStatus multiStatus = new MultiStatus(Activator.ID, IStatus.OK, Messages.message_childrenRepos, null);
+		MultiStatus multiStatus = new MultiStatus(Activator.ID, IStatus.OK, Messages.message_artifactsFromChildRepos, null);
 		for (ChildInfo childInfo : loadedRepos) {
 			if (requests.length == 0)
 				break;
@@ -347,7 +347,7 @@ public class CompositeArtifactRepository extends AbstractArtifactRepository impl
 			requests = filterUnfetched(requests);
 			subMonitor.setWorkRemaining(requests.length);
 
-			if (monitor.isCanceled())
+			if (subMonitor.isCanceled())
 				return Status.CANCEL_STATUS;
 		}
 		return multiStatus;
diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java
index eae97fc3a..4588cec90 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java
@@ -40,7 +40,9 @@ public class Messages extends NLS {
 	public static String sar_reportStatus;
 
 	public static String mirror_alreadyExists;
-	public static String message_childrenRepos;
+	public static String message_artifactsFromChildRepos;
+	public static String message_problemReadingArtifact;
+
 	public static String exception_comparatorNotFound;
 	public static String exception_noComparators;
 	public static String exception_unsupportedAddToComposite;
diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties
index 68453f70a..33689fcb8 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties
@@ -37,7 +37,8 @@ sar_reportStatus=Problems downloading artifact: {0}.
 
 mirror_alreadyExists=Artifact: {0} already exists in repository: {1}.
 
-message_childrenRepos = Messages while trying children repositories.
+message_artifactsFromChildRepos=Messages while reading artifacts from child repositories
+message_problemReadingArtifact=Problems while reading artifacts from {0}
 
 exception_comparatorNotFound = The Artifact Comparator {0} was not found.
 exception_noComparators = No Artifact Comparators are available.
diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
index f72ae6847..0953cdea9 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
@@ -765,7 +765,7 @@ public class SimpleArtifactRepository extends AbstractArtifactRepository impleme
 		if (monitor.isCanceled())
 			return Status.CANCEL_STATUS;
 
-		final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK, null, null);
+		final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK, NLS.bind(Messages.message_problemReadingArtifact, getLocation()), null);
 		LinkedList<IArtifactRequest> requestsPending = new LinkedList<IArtifactRequest>(Arrays.asList(requests));
 
 		int numberOfJobs = Math.min(requests.length, getMaximumThreads());
@@ -802,7 +802,13 @@ public class SimpleArtifactRepository extends AbstractArtifactRepository impleme
 				monitor.done();
 			}
 		}
-		return (monitor.isCanceled() ? Status.CANCEL_STATUS : overallStatus);
+
+		if (monitor.isCanceled())
+			return Status.CANCEL_STATUS;
+		else if (overallStatus.isOK())
+			return Status.OK_STATUS;
+		else
+			return overallStatus;
 	}
 
 	public synchronized IArtifactDescriptor getCompleteArtifactDescriptor(IArtifactKey key) {
diff --git a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java
index b7fe6efa0..23769f6d9 100644
--- a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java
+++ b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java
@@ -10,6 +10,13 @@
  *******************************************************************************/
 package org.eclipse.equinox.p2.tests.artifact.repository;
 
+import static org.eclipse.equinox.p2.tests.publisher.actions.StatusMatchers.errorStatus;
+import static org.eclipse.equinox.p2.tests.publisher.actions.StatusMatchers.statusWithMessageWhich;
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+import static org.junit.matchers.JUnitMatchers.containsString;
+import static org.junit.matchers.JUnitMatchers.hasItem;
+
 import java.io.*;
 import java.lang.reflect.Field;
 import java.net.URI;
@@ -573,6 +580,27 @@ public class CompositeArtifactRepositoryTest extends AbstractProvisioningTest {
 		assertTrue(repo2File.length() == destFile2.length());
 	}
 
+	public void testGetArtifactsWithErrorInChild() throws Exception {
+		repositoryURI = getTestData("1", "/testData/artifactRepo/composite/errorInChild").toURI();
+		IArtifactRepository repo = getArtifactRepositoryManager().loadRepository(repositoryURI, null);
+
+		IArtifactRequest[] requests = new IArtifactRequest[] {new ArtifactRequest(new ArtifactKey("osgi.bundle", "plugin", Version.parseVersion("1.0.0")), null) {
+			@Override
+			public void perform(IArtifactRepository sourceRepository, IProgressMonitor monitor) {
+				setResult(sourceRepository.getArtifact(sourceRepository.getArtifactDescriptors(getArtifactKey())[0], new ByteArrayOutputStream(), monitor));
+			}
+		}};
+
+		IStatus status = repo.getArtifacts(requests, null);
+
+		assertThat(status, is(errorStatus()));
+		assertThat(status, is(statusWithMessageWhich(containsString("while reading artifacts from child repositories"))));
+
+		// bug 391400: status should point to repository with problem
+		String brokenChildURI = repositoryURI.toString() + "child";
+		assertThat(Arrays.asList(status.getChildren()), hasItem(statusWithMessageWhich(containsString(brokenChildURI))));
+	}
+
 	public void testLoadingRepositoryRemote() {
 		File knownGoodRepoLocation = getTestData("0.1", "/testData/artifactRepo/composite/good.remote");
 
diff --git a/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/child/artifacts.xml b/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/child/artifacts.xml
new file mode 100644
index 000000000..ff284a36a
--- /dev/null
+++ b/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/child/artifacts.xml
@@ -0,0 +1,20 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<?artifactRepository class='org.eclipse.equinox.internal.p2.artifact.repository.simple.SimpleArtifactRepository' version='1.0.0'?>
+<repository name='Repository with missing artifact file' type="org.eclipse.equinox.p2.artifact.repository.simpleRepository" version="1.0.0">
+  <properties size='3'>
+    <property name='p2.compressed' value='false'/>
+    <property name='p2.timestamp' value='1218734853468'/>
+  </properties>
+  <mappings size='3'>
+    <rule filter='(&amp; (classifier=osgi.bundle))' output='${repoUrl}/plugins/${id}_${version}.jar'/>
+    <rule filter='(&amp; (classifier=binary))' output='${repoUrl}/binary/${id}_${version}'/>
+    <rule filter='(&amp; (classifier=org.eclipse.update.feature))' output='${repoUrl}/features/${id}_${version}.jar'/>
+  </mappings>
+  <artifacts size='2'>
+    <artifact classifier='osgi.bundle' id='plugin' version='1.0.0'>
+      <properties size='3'>
+        <property name='download.size' value='123'/>
+      </properties>
+    </artifact>
+  </artifacts>
+</repository>
diff --git a/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/compositeArtifacts.xml b/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/compositeArtifacts.xml
new file mode 100644
index 000000000..1c555d869
--- /dev/null
+++ b/bundles/org.eclipse.equinox.p2.tests/testData/artifactRepo/composite/errorInChild/compositeArtifacts.xml
@@ -0,0 +1,11 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<?compositeArtifactRepository version='1.0.0'?>
+<repository name='artifact name' type='org.eclipse.equinox.internal.p2.artifact.repository.CompositeArtifactRepository' version='1.0.0'>
+  <properties size='2'>
+    <property name='p2.compressed' value='false'/>
+    <property name='p2.timestamp' value='1234'/>
+  </properties>
+  <children size='1'>
+    <child location='child'/>
+  </children>
+</repository>