Merge branch 'master' into whitelist-JENKINS-47736

cli/pom.xml

@@ -5,7 +5,7 @@
   <parent>
     <groupId>org.jenkins-ci.main</groupId>
     <artifactId>pom</artifactId>
-    <version>2.93-SNAPSHOT</version>
+    <version>2.94-SNAPSHOT</version>
   </parent>
 
   <artifactId>cli</artifactId>

core/pom.xml

@@ -29,7 +29,7 @@ THE SOFTWARE.
   <parent>
     <groupId>org.jenkins-ci.main</groupId>
     <artifactId>pom</artifactId>
-    <version>2.93-SNAPSHOT</version>
+    <version>2.94-SNAPSHOT</version>
   </parent>
 
   <artifactId>jenkins-core</artifactId>

core/src/main/java/hudson/XmlFile.java

@@ -24,10 +24,8 @@
 package hudson;
 
 import com.thoughtworks.xstream.XStream;
-import com.thoughtworks.xstream.XStreamException;
 import com.thoughtworks.xstream.converters.Converter;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
-import com.thoughtworks.xstream.io.StreamException;
 import com.thoughtworks.xstream.io.xml.Xpp3Driver;
 import hudson.diagnosis.OldDataMonitor;
 import hudson.model.Descriptor;
@@ -148,7 +146,7 @@ public final class XmlFile {
         }
         try (InputStream in = new BufferedInputStream(Files.newInputStream(file.toPath()))) {
             return xs.fromXML(in);
-        } catch (XStreamException | Error | InvalidPathException e) {
+        } catch (RuntimeException | Error e) {
             throw new IOException("Unable to read "+file,e);
         }
     }
@@ -165,7 +163,7 @@ public final class XmlFile {
         try (InputStream in = new BufferedInputStream(Files.newInputStream(file.toPath()))) {
             // TODO: expose XStream the driver from XStream
             return xs.unmarshal(DEFAULT_DRIVER.createReader(in), o);
-        } catch (XStreamException | Error | InvalidPathException e) {
+        } catch (RuntimeException | Error e) {
             throw new IOException("Unable to read "+file,e);
         }
     }
@@ -184,7 +182,7 @@ public final class XmlFile {
                 writing.set(null);
             }
             w.commit();
-        } catch(StreamException e) {
+        } catch(RuntimeException e) {
             throw new IOException(e);
         } finally {
             w.abort();

core/src/main/java/hudson/model/User.java

@@ -405,23 +405,16 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
      *      An existing or created user. May be {@code null} if a user does not exist and
      *      {@code create} is false.
      */
-    public static @Nullable User get(String idOrFullName, boolean create, Map context) {
+    public static @Nullable User get(String idOrFullName, boolean create, @Nonnull Map context) {
 
         if(idOrFullName==null)
             return null;
 
-        // sort resolvers by priority
-        List<CanonicalIdResolver> resolvers = new ArrayList<CanonicalIdResolver>(ExtensionList.lookup(CanonicalIdResolver.class));
-        Collections.sort(resolvers);
+        // TODO: In many cases the method should receive the canonical ID.
+        // Maybe it makes sense to try AllUsers.byName().get(idkey) before invoking all resolvers and other stuff
+        // oleg-nenashev: FullNameResolver with User.getAll() loading and iteration makes me think it's a good idea.
 
-        String id = null;
-        for (CanonicalIdResolver resolver : resolvers) {
-            id = resolver.resolveCanonicalId(idOrFullName, context);
-            if (id != null) {
-                LOGGER.log(Level.FINE, "{0} mapped {1} to {2}", new Object[] {resolver, idOrFullName, id});
-                break;
-            }
-        }
+        String id = CanonicalIdResolver.resolve(idOrFullName, context);
         // DefaultUserCanonicalIdResolver will always return a non-null id if all other CanonicalIdResolver failed
         if (id == null) {
             throw new IllegalStateException("The user id should be always non-null thanks to DefaultUserCanonicalIdResolver");
@@ -1081,6 +1074,16 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
 
     }
 
+    /**
+     * Resolves User IDs by ID, full names or other strings.
+     *
+     * This extension point may be useful to map SCM user names to Jenkins {@link User} IDs.
+     * Currently the extension point is used in {@link User#get(String, boolean, Map)}.
+     *
+     * @since 1.479
+     * @see jenkins.model.DefaultUserCanonicalIdResolver
+     * @see FullNameIdResolver
+     */
     public static abstract class CanonicalIdResolver extends AbstractDescribableImpl<CanonicalIdResolver> implements ExtensionPoint, Comparable<CanonicalIdResolver> {
 
         /**
@@ -1090,6 +1093,7 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
          */
         public static final String REALM = "realm";
 
+        @Override
         public int compareTo(CanonicalIdResolver o) {
             // reverse priority order
             int i = getPriority();
@@ -1101,12 +1105,56 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
          * extract user ID from idOrFullName with help from contextual infos.
          * can return <code>null</code> if no user ID matched the input
          */
-        public abstract @CheckForNull String resolveCanonicalId(String idOrFullName, Map<String, ?> context);
+        public abstract @CheckForNull String resolveCanonicalId(String idOrFullName, @Nonnull Map<String, ?> context);
 
+        /**
+         * Gets priority of the resolver.
+         * Higher priority means that it will be checked earlier.
+         *
+         * Overriding methods must not use {@link Integer#MIN_VALUE}, because it will cause collisions
+         * with {@link jenkins.model.DefaultUserCanonicalIdResolver}.
+         *
+         * @return Priority of the resolver.
+         */
         public int getPriority() {
             return 1;
         }
 
+        //TODO: It is too late to use Extension Point ordinals, right?
+        //Such sorting and collection rebuild is not good for User#get(...) method performance.
+        /**
+         * Gets all extension points, sorted by priority.
+         * @return Sorted list of extension point implementations.
+         * @since TODO
+         */
+        public static List<CanonicalIdResolver> all() {
+            List<CanonicalIdResolver> resolvers = new ArrayList<>(ExtensionList.lookup(CanonicalIdResolver.class));
+            Collections.sort(resolvers);
+            return resolvers;
+        }
+
+        /**
+         * Resolves users using all available {@link CanonicalIdResolver}s.
+         * @param idOrFullName ID or full name of the user
+         * @param context Context
+         * @return Resolved User ID or {@code null} if the user ID cannot be resolved.
+         * @since TODO
+         */
+        @CheckForNull
+        public static String resolve(@Nonnull String idOrFullName, @Nonnull Map<String, ?> context) {
+            for (CanonicalIdResolver resolver : CanonicalIdResolver.all()) {
+                //TODO: add try/catch for Runtime exceptions? It should not happen now && it may cause performance degradation
+                String id = resolver.resolveCanonicalId(idOrFullName, context);
+                if (id != null) {
+                    LOGGER.log(Level.FINE, "{0} mapped {1} to {2}", new Object[] {resolver, idOrFullName, id});
+                    return id;
+                }
+            }
+
+            // De-facto it is not going to happen OOTB, because the current DefaultUserCanonicalIdResolver
+            // always returns a value. But we still need to check nulls if somebody disables the extension point
+            return null;
+        }
     }
 
 

core/src/test/java/hudson/util/AtomicFileWriterTest.java

@@ -1,5 +1,6 @@
 package hudson.util;
 
+import org.apache.commons.io.FileUtils;
 import org.hamcrest.core.StringContains;
 import org.junit.Before;
 import org.junit.Rule;
@@ -21,6 +22,7 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class AtomicFileWriterTest {
+    private static final String PREVIOUS = "previous value \n blah";
     @Rule
     public TemporaryFolder tmp = new TemporaryFolder();
     File af;
@@ -30,6 +32,7 @@ public class AtomicFileWriterTest {
     @Before
     public void setUp() throws IOException {
         af = tmp.newFile();
+        FileUtils.writeStringToFile(af, PREVIOUS);
         afw = new AtomicFileWriter(af.toPath(), Charset.defaultCharset());
     }
 
@@ -56,25 +59,29 @@ public class AtomicFileWriterTest {
     public void writeToAtomicFile() throws Exception {
         // Given
         afw.write(expectedContent, 0, expectedContent.length());
+        afw.write(expectedContent);
+        afw.write(' ');
 
         // When
         afw.flush();
 
         // Then
         assertEquals("File writer did not properly flush to temporary file",
-                expectedContent.length(), Files.size(afw.getTemporaryPath()));
+                expectedContent.length()*2+1, Files.size(afw.getTemporaryPath()));
     }
 
     @Test
     public void commitToFile() throws Exception {
         // Given
         afw.write(expectedContent, 0, expectedContent.length());
+        afw.write(new char[]{'h', 'e', 'y'}, 0, 3);
 
         // When
         afw.commit();
 
         // Then
-        assertEquals(expectedContent.length(), Files.size(af.toPath()));
+        assertEquals(expectedContent.length()+3, Files.size(af.toPath()));
+        assertEquals(expectedContent+"hey", FileUtils.readFileToString(af));
     }
 
     @Test
@@ -87,8 +94,20 @@ public class AtomicFileWriterTest {
 
         // Then
         assertTrue(Files.notExists(afw.getTemporaryPath()));
+        assertEquals(PREVIOUS, FileUtils.readFileToString(af));
     }
 
+    @Test
+    public void indexOutOfBoundsLeavesOriginalUntouched() throws Exception {
+        // Given
+        try {
+            afw.write(expectedContent, 0, expectedContent.length() + 10);
+            fail("exception expected");
+        } catch (IndexOutOfBoundsException e) {
+        }
+
+        assertEquals(PREVIOUS, FileUtils.readFileToString(af));
+    }
     @Test
     public void badPath() throws Exception {
         final File newFile = tmp.newFile();

pom.xml

@@ -33,7 +33,7 @@ THE SOFTWARE.
 
   <groupId>org.jenkins-ci.main</groupId>
   <artifactId>pom</artifactId>
-  <version>2.93-SNAPSHOT</version>
+  <version>2.94-SNAPSHOT</version>
   <packaging>pom</packaging>
 
   <name>Jenkins main module</name>

test/pom.xml

@@ -28,7 +28,7 @@ THE SOFTWARE.
   <parent>
     <groupId>org.jenkins-ci.main</groupId>
     <artifactId>pom</artifactId>
-    <version>2.93-SNAPSHOT</version>
+    <version>2.94-SNAPSHOT</version>
   </parent>
 
   <artifactId>test</artifactId>

war/pom.xml

@@ -28,7 +28,7 @@ THE SOFTWARE.
   <parent>
     <groupId>org.jenkins-ci.main</groupId>
     <artifactId>pom</artifactId>
-    <version>2.93-SNAPSHOT</version>
+    <version>2.94-SNAPSHOT</version>
   </parent>
 
   <artifactId>jenkins-war</artifactId>

war/src/main/js/pluginSetupWizardGui.js

@@ -889,16 +889,17 @@ var createPluginSetupWizard = function(appendTarget) {
 			// ignore JSON parsing issues, this may be HTML
 		}
 		// we get 200 OK
-		var $page = $(data);
+		var responseText = data.responseText;
+		var $page = $(responseText);
 		var $errors = $page.find('.error');
 		if($errors.length > 0) {
 			var $main = $page.find('#main-panel').detach();
 			if($main.length > 0) {
-				data = data.replace(/body([^>]*)[>](.|[\r\n])+[<][/]body/,'body$1>'+$main.html()+'</body');
+				responseText = responseText.replace(/body([^>]*)[>](.|[\r\n])+[<][/]body/,'body$1>'+$main.html()+'</body');
 			}
 			var doc = $('iframe[src]').contents()[0];
 			doc.open();
-			doc.write(data);
+			doc.write(responseText);
 			doc.close();
 		}
 		else {