don't double add classpath entries (#1578)

* don't double add classpath entries

* code review

* don't run unnecessary runContainerResolverJob

* fix monitor size

Author: elharo

plugins/com.google.cloud.tools.eclipse.appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/BuildPathTest.java

@@ -21,6 +21,7 @@
 
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.jdt.core.IClasspathEntry;
 import org.eclipse.jdt.core.IJavaProject;
@@ -31,38 +32,53 @@
 import com.google.cloud.tools.eclipse.appengine.libraries.model.Library;
 
 public class BuildPathTest {
+  
+  private final List<Library> libraries = new ArrayList<>();
+  private final IJavaProject project = Mockito.mock(IJavaProject.class);
 
   @Test
   public void testAddLibraries_emptyList() throws CoreException {
     IProject project = null;
-    List<Library> libraries = new ArrayList<>();
     BuildPath.addLibraries(project, libraries, new NullProgressMonitor());
   }
 
   @Test
   public void testAddLibraries() throws CoreException {
-    IJavaProject project = Mockito.mock(IJavaProject.class);
     IClasspathEntry[] rawClasspath = new IClasspathEntry[0];
     Mockito.when(project.getRawClasspath()).thenReturn(rawClasspath);
 
-    List<Library> libraries = new ArrayList<>();
     Library library = new Library("libraryId");
     libraries.add(library);
     IClasspathEntry[] result =
         BuildPath.addLibraries(project, libraries, new NullProgressMonitor());
     Assert.assertEquals(1, result.length);
+    
+    Mockito.verify(project)
+        .setRawClasspath(Mockito.any(IClasspathEntry[].class), Mockito.any(IProgressMonitor.class));
+  }
+  
+  @Test
+  public void testListAdditionalLibraries() throws CoreException {
+    IClasspathEntry[] rawClasspath = new IClasspathEntry[0];
+    Mockito.when(project.getRawClasspath()).thenReturn(rawClasspath);
+    
+    Library library = new Library("libraryId");
+    libraries.add(library);
+    IClasspathEntry[] result =
+        BuildPath.listAdditionalLibraries(project, libraries, new NullProgressMonitor());
+    Assert.assertEquals(1, result.length);
+ 
+    Mockito.verify(project, Mockito.never())
+        .setRawClasspath(Mockito.any(IClasspathEntry[].class), Mockito.any(IProgressMonitor.class));
   }
 
   @Test
   public void testAddLibraries_noDuplicates() throws CoreException {
     Library library = new Library("libraryId");
-    IClasspathEntry entry = BuildPath.makeClasspathEntry(library);
-    
-    IJavaProject project = Mockito.mock(IJavaProject.class);
+    IClasspathEntry entry = BuildPath.makeClasspathEntry(library);    
     IClasspathEntry[] rawClasspath = {entry};
     Mockito.when(project.getRawClasspath()).thenReturn(rawClasspath);
 
-    List<Library> libraries = new ArrayList<>();
     libraries.add(library);
     IClasspathEntry[] result =
         BuildPath.addLibraries(project, libraries, new NullProgressMonitor());
@@ -75,11 +91,9 @@ public void testAddLibraries_withDuplicates() throws CoreException {
     Library library2 = new Library("library2");
     IClasspathEntry entry = BuildPath.makeClasspathEntry(library1);
 
-    IJavaProject project = Mockito.mock(IJavaProject.class);
     IClasspathEntry[] rawClasspath = {entry};
     Mockito.when(project.getRawClasspath()).thenReturn(rawClasspath);
 
-    List<Library> libraries = new ArrayList<>();
     libraries.add(library1);
     libraries.add(library2);
     IClasspathEntry[] result =

plugins/com.google.cloud.tools.eclipse.appengine.libraries.ui/src/com/google/cloud/tools/eclipse/appengine/libraries/ui/AppEngineLibrariesPage.java

@@ -102,7 +102,7 @@ public IClasspathEntry[] getNewContainers() {
         return new IClasspathEntry[0];
       } else {
         IClasspathEntry[] added =
-            BuildPath.addLibraries(project, libraries, new NullProgressMonitor());
+            BuildPath.listAdditionalLibraries(project, libraries, new NullProgressMonitor());
         return added;
       }
     } catch (CoreException ex) {

plugins/com.google.cloud.tools.eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/BuildPath.java

@@ -89,10 +89,18 @@ public static IClasspathEntry[] addLibraries(
       IJavaProject javaProject, List<Library> libraries, IProgressMonitor monitor)
           throws JavaModelException, CoreException {
 
+    return prepareLibraries(javaProject, libraries, monitor, true);
+  }
+
+  private static IClasspathEntry[] prepareLibraries(IJavaProject javaProject,
+      List<Library> libraries, IProgressMonitor monitor, boolean addToClasspath)
+          throws JavaModelException, CoreException {
     SubMonitor subMonitor = SubMonitor.convert(monitor,
-        Messages.getString("adding.app.engine.libraries"), libraries.size()); //$NON-NLS-1$
+        Messages.getString("adding.app.engine.libraries"), //$NON-NLS-1$
+        libraries.size() + 1); // + 1 because we pass the submonitor along below
 
     IClasspathEntry[] rawClasspath = javaProject.getRawClasspath();
+    
     List<IClasspathEntry> newRawClasspath = new ArrayList<>(rawClasspath.length + libraries.size());
     newRawClasspath.addAll(Arrays.asList(rawClasspath));
     List<IClasspathEntry> newEntries = new ArrayList<>();
@@ -104,12 +112,23 @@ public static IClasspathEntry[] addLibraries(
       }
       subMonitor.worked(1);
     }
-    javaProject.setRawClasspath(newRawClasspath.toArray(new IClasspathEntry[0]), subMonitor);
 
-    runContainerResolverJob(javaProject);
+    if (addToClasspath) {
+      javaProject.setRawClasspath(newRawClasspath.toArray(new IClasspathEntry[0]), subMonitor);
+      runContainerResolverJob(javaProject);
+    }
 
     return newEntries.toArray(new IClasspathEntry[0]);
   }
+  
+  /**
+   * @return the entries to be added to the classpath. Does not add them to the classpath.
+   */
+  public static IClasspathEntry[] listAdditionalLibraries(
+      IJavaProject javaProject, List<Library> libraries, IProgressMonitor monitor)
+          throws JavaModelException, CoreException {
+    return prepareLibraries(javaProject, libraries, monitor, false);
+  }
 
   @VisibleForTesting
   static IClasspathEntry makeClasspathEntry(Library library) throws CoreException {

plugins/com.google.cloud.tools.eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/ILibraryClasspathContainerResolverService.java

@@ -56,7 +56,7 @@ enum AppEngineRuntime {
 
 
   /**
-   * Verifies that dependencies of a given runtime are available either locally or can be downloaded
+   * Verifies that dependencies of a given runtime are available locally or can be downloaded.
    *
    * @return {@link Status#OK_STATUS} if all dependencies are available,
    * {@link Status#CANCEL_STATUS} if the operation was cancelled via the <code>monitor</code>, or a