Fixup IU location update handling and refresh

Currently the IU location is updated in-place that has several
drawbacks:
- we need to synchronize a to not mess up things
- cached items in the object can getting stale and drip into new state
- equals and hashCode can become unstable over time

to overcome this the code is changed in a way that an update is
performed on a copy of the current state and a new object is returned
and used to perform the update.

Beside that there was an issue with refreshing a repository that do not
exits, this is now fixed by first adding it (and possibly remove it
afterwards).

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java

@@ -157,36 +157,10 @@ public class IUBundleContainer extends AbstractBundleContainer {
 	 * @param repositories metadata repositories used to search for IU's or <code>null</code> for default set
 	 * @param resolutionFlags bitmask of flags to control IU resolution, possible flags are {@link IUBundleContainer#INCLUDE_ALL_ENVIRONMENTS}, {@link IUBundleContainer#INCLUDE_REQUIRED}, {@link IUBundleContainer#INCLUDE_SOURCE}, {@link IUBundleContainer#INCLUDE_CONFIGURE_PHASE}
 	 */
-	IUBundleContainer(String[] ids, String[] versions, URI[] repositories, int resolutionFlags) {
+	IUBundleContainer(String[] ids, Version[] versions, URI[] repositories, int resolutionFlags) {
 		fIds = ids;
 		fFlags = resolutionFlags;
-		fVersions = new Version[versions.length];
-		for (int i = 0; i < versions.length; i++) {
-			fVersions[i] = Version.create(versions[i]);
-
-		}
-		if (repositories == null || repositories.length == 0) {
-			fRepos = null;
-		} else {
-			fRepos = repositories;
-		}
-	}
-
-	/**
-	 * Constructs a installable unit bundle container for the specified units.
-	 *
-	 * @param units IU's
-	 * @param repositories metadata repositories used to search for IU's or <code>null</code> for default set
-	 * @param resolutionFlags bitmask of flags to control IU resolution, possible flags are {@link IUBundleContainer#INCLUDE_ALL_ENVIRONMENTS}, {@link IUBundleContainer#INCLUDE_REQUIRED}, {@link IUBundleContainer#INCLUDE_SOURCE}, {@link IUBundleContainer#INCLUDE_CONFIGURE_PHASE}
-	 */
-	IUBundleContainer(IInstallableUnit[] units, URI[] repositories, int resolutionFlags) {
-		fIds = new String[units.length];
-		fFlags = resolutionFlags;
-		fVersions = new Version[units.length];
-		for (int i = 0; i < units.length; i++) {
-			fIds[i] = units[i].getId();
-			fVersions[i] = units[i].getVersion();
-		}
+		fVersions = versions;
 		if (repositories == null || repositories.length == 0) {
 			fRepos = null;
 		} else {
@@ -378,51 +352,52 @@ void synchronizerChanged(ITargetDefinition target) {
 	}
 
 	/**
-	 * Update the root IUs to the latest available in the repos associated with this container.
+	 * Update the root IUs to the latest available in the repos associated with
+	 * this container.
 	 *
-	 * @param toUpdate the set of IU ids in this container to consider updating.  If empty
-	 * then update everything
-	 * @param monitor progress monitor or <code>null</code>
-	 * @return whether this container was changed as part of this update and must be resolved
-	 * @exception CoreException if unable to retrieve IU's
-	 */
-	public synchronized boolean update(Set<String> toUpdate, IProgressMonitor monitor) throws CoreException {
+	 * @param toUpdate
+	 *            the set of IU ids in this container to consider updating. If
+	 *            empty then update everything
+	 * @param monitor
+	 *            progress monitor or <code>null</code>
+	 * @return a new instance with updated versions or <code>null</code> if
+	 *         nothing was changed as result of the update
+	 * @exception CoreException
+	 *                if unable to retrieve IU's
+	 */
+	public synchronized IUBundleContainer update(Set<String> toUpdate, IProgressMonitor monitor) throws CoreException {
 		SubMonitor progress = SubMonitor.convert(monitor, 100);
-		IQueryable<IInstallableUnit> source = P2TargetUtils.getQueryableMetadata(fRepos, progress.split(30));
+		URI[] updateRepos = fRepos == null ? null : fRepos.clone();
+		IQueryable<IInstallableUnit> source = P2TargetUtils.getQueryableMetadata(updateRepos, progress.split(30));
 		boolean updated = false;
-		SubMonitor loopProgress = progress.split(70).setWorkRemaining(fIds.length);
-		for (int i = 0; i < fIds.length; i++) {
-			if (!toUpdate.isEmpty() && !toUpdate.contains(fIds[i])) {
+		String[] updateIDs = fIds.clone();
+		Version[] updateVersions = fVersions.clone();
+		SubMonitor loopProgress = progress.split(70).setWorkRemaining(updateIDs.length);
+		for (int i = 0; i < updateIDs.length; i++) {
+			if (!toUpdate.isEmpty() && !toUpdate.contains(updateIDs[i])) {
 				continue;
 			}
-			IQuery<IInstallableUnit> query = QueryUtil.createLatestQuery(QueryUtil.createIUQuery(fIds[i]));
+			IQuery<IInstallableUnit> query = QueryUtil.createLatestQuery(QueryUtil.createIUQuery(updateIDs[i]));
 			IQueryResult<IInstallableUnit> queryResult = source.query(query, loopProgress.split(1));
 			Iterator<IInstallableUnit> it = queryResult.iterator();
 			// bail if the feature is no longer available.
 			if (!it.hasNext()) {
-				throw new CoreException(Status.error(NLS.bind(Messages.IUBundleContainer_1, fIds[i])));
+				throw new CoreException(Status.error(NLS.bind(Messages.IUBundleContainer_1, updateIDs[i])));
 			}
 			IInstallableUnit iu = it.next();
 			// if the version is different from the spec (up or down), record the change.
-			if (!iu.getVersion().equals(fVersions[i])) {
+			if (!iu.getVersion().equals(updateVersions[i])) {
 				updated = true;
 				// if the spec was not specific (e.g., 0.0.0) the target def itself has changed.
-				if (!fVersions[i].equals(Version.emptyVersion)) {
-					fVersions[i] = iu.getVersion();
+				if (!updateVersions[i].equals(Version.emptyVersion)) {
+					updateVersions[i] = iu.getVersion();
 				}
 			}
 		}
 		if (updated) {
-			// Increase the sequence number to reload the p2 locations
-			if (fTarget instanceof TargetDefinition) {
-				((TargetDefinition) fTarget).incrementSequenceNumber();
-			}
+			return new IUBundleContainer(updateIDs, updateVersions, updateRepos, fFlags);
 		}
-		if (!updated) {
-			// Things have changed so mark the container as unresolved
-			clearResolutionStatus();
-		}
-		return updated;
+		return null;
 	}
 
 	/**
@@ -807,4 +782,5 @@ public String toString() {
 		sb.append(']');
 		return sb.toString();
 	}
+
 }

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IULocationFactory.java

@@ -101,8 +101,8 @@ public ITargetLocation getTargetLocation(String type, String serializedXML) thro
 			flags |= Boolean.parseBoolean(includeAllPlatforms) ? IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS : 0;
 			flags |= Boolean.parseBoolean(includeSource) ? IUBundleContainer.INCLUDE_SOURCE : 0;
 			flags |= Boolean.parseBoolean(includeConfigurePhase) ? IUBundleContainer.INCLUDE_CONFIGURE_PHASE : 0;
-			IUBundleContainer targetLocation = new IUBundleContainer(iuIDs, iuVer, uris, flags);
-			return targetLocation;
+			return TargetPlatformService.getDefault().newIULocation(iuIDs, iuVer, uris,
+					flags);
 		}
 		return null;
 	}

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java

@@ -293,6 +293,7 @@ public static void forceCheckTarget(final ITargetDefinition target) {
 		if (result.fProfile instanceof org.eclipse.equinox.internal.p2.engine.Profile) {
 			((org.eclipse.equinox.internal.p2.engine.Profile) result.fProfile).setProperty(PROP_SEQUENCE_NUMBER, "-1"); //$NON-NLS-1$
 		}
+		result.fProfile = null;
 	}
 
 	/**

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence35Helper.java

@@ -247,7 +247,7 @@ private static void deserializeBundleContainer(ITargetDefinition definition, Ele
 				}
 			}
 			flags |= Boolean.parseBoolean(includeAllPlatforms) ? IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS : 0;
-			container = new IUBundleContainer(iuIDs, iuVer, uris, flags);
+			container = TargetPlatformService.getDefault().newIULocation(iuIDs, iuVer, uris, flags);
 			break;
 		default:
 			break;

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPersistence36Helper.java

@@ -254,7 +254,7 @@ private static ITargetLocation deserializeBundleContainer(Element location) thro
 			}
 			flags |= Boolean.parseBoolean(includeAllPlatforms) ? IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS : 0;
 			flags |= Boolean.parseBoolean(includeSource) ? IUBundleContainer.INCLUDE_SOURCE : 0;
-			container = new IUBundleContainer(iuIDs, iuVer, uris, flags);
+			container = TargetPlatformService.getDefault().newIULocation(iuIDs, iuVer, uris, flags);
 			break;
 		default:
 			break;

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java

@@ -59,6 +59,7 @@
 import org.eclipse.e4.core.services.events.IEventBroker;
 import org.eclipse.equinox.frameworkadmin.BundleInfo;
 import org.eclipse.equinox.p2.metadata.IInstallableUnit;
+import org.eclipse.equinox.p2.metadata.Version;
 import org.eclipse.osgi.service.datalocation.Location;
 import org.eclipse.osgi.util.NLS;
 import org.eclipse.pde.core.plugin.IPluginModelBase;
@@ -640,12 +641,22 @@ public IStatus compareWithTargetPlatform(ITargetDefinition target) throws CoreEx
 
 	@Override
 	public ITargetLocation newIULocation(IInstallableUnit[] units, URI[] repositories, int resolutionFlags) {
-		return new IUBundleContainer(units, repositories, resolutionFlags);
+		String[] fIds = new String[units.length];
+		Version[] fVersions = new Version[units.length];
+		for (int i = 0; i < units.length; i++) {
+			fIds[i] = units[i].getId();
+			fVersions[i] = units[i].getVersion();
+		}
+		return new IUBundleContainer(fIds, fVersions, repositories, resolutionFlags);
 	}
 
 	@Override
 	public ITargetLocation newIULocation(String[] unitIds, String[] versions, URI[] repositories, int resolutionFlags) {
-		return new IUBundleContainer(unitIds, versions, repositories, resolutionFlags);
+		Version[] fVersions = new Version[versions.length];
+		for (int i = 0; i < versions.length; i++) {
+			fVersions[i] = Version.create(versions[i]);
+		}
+		return new IUBundleContainer(unitIds, fVersions, repositories, resolutionFlags);
 	}
 
 }

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/IUFactory.java

@@ -114,28 +114,41 @@ public IStatus update(ITargetDefinition target, TreePath[] treePaths, IProgressM
 				wrappersMap.computeIfAbsent(wrapper.getParent(), k -> new HashSet<>()).add(wrapper.getIU().getId());
 			}
 		}
-		boolean changed = false;
+		Map<IUBundleContainer, IUBundleContainer> updatedContainer = new HashMap<>();
 		SubMonitor subMonitor = SubMonitor.convert(monitor, (containers.size() + wrappersMap.size()) * 100);
 		for (IUBundleContainer container : containers) {
 			try {
-				changed |= container.update(Collections.emptySet(), subMonitor.split(100));
+				IUBundleContainer update = container.update(Collections.emptySet(), subMonitor.split(100));
+				updatedContainer.put(container, update);
 			} catch (CoreException e) {
 				return e.getStatus();
 			}
 		}
 		for (Entry<IUBundleContainer, Set<String>> entry : wrappersMap.entrySet()) {
-			SubMonitor split = subMonitor.split(100);
 			IUBundleContainer container = entry.getKey();
 			if (containers.contains(container)) {
 				continue;
 			}
 			try {
-				changed |= container.update(entry.getValue(), split);
+				IUBundleContainer update = container.update(entry.getValue(), subMonitor.split(100));
+				updatedContainer.put(container, update);
 			} catch (CoreException e) {
 				return e.getStatus();
 			}
 		}
-		return changed ? Status.OK_STATUS : STATUS_NO_CHANGE;
+		if (updatedContainer.isEmpty()) {
+			return Status.OK_STATUS;
+		}
+		// update changed location
+		ITargetLocation[] targetLocations = target.getTargetLocations();
+		for (int i = 0; i < targetLocations.length; i++) {
+			IUBundleContainer updated = updatedContainer.remove(targetLocations[i]);
+			if (updated != null) {
+				targetLocations[i] = updated;
+			}
+		}
+		target.setTargetLocations(targetLocations);
+		return STATUS_NO_CHANGE;
 	}
 
 	@Override
@@ -199,10 +212,30 @@ public IStatus reload(ITargetDefinition target, ITargetLocation[] targetLocation
 								repositoryTracker.clearRepositoryNotFound(repositoryUri);
 								try {
 									if (artifactRepositoryManager != null) {
-										artifactRepositoryManager.refreshRepository(repositoryUri, convert.split(1));
+										boolean contains = artifactRepositoryManager.contains(repositoryUri);
+										if (!contains) {
+											artifactRepositoryManager.addRepository(repositoryUri);
+										}
+										try {
+											artifactRepositoryManager.refreshRepository(repositoryUri,
+													convert.split(1));
+										} finally {
+											if (!contains) {
+												artifactRepositoryManager.removeRepository(repositoryUri);
+											}
+										}
 									}
 									if (metadataRepositoryManager != null) {
-										metadataRepositoryManager.refreshRepository(repositoryUri, convert.split(1));
+										boolean contains = metadataRepositoryManager.contains(repositoryUri);
+										if (!contains) {
+											metadataRepositoryManager.addRepository(repositoryUri);
+										}
+										try {
+											metadataRepositoryManager.refreshRepository(repositoryUri,
+													convert.split(1));
+										} finally {
+											metadataRepositoryManager.removeRepository(repositoryUri);
+										}
 									}
 								} catch (CoreException e) {
 									IStatus error = e.getStatus();