From d41a69c49b7e54ce209d58624a70948db47da9c5 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 14 Nov 2017 11:55:50 +0100 Subject: [PATCH 1/7] Document and refactor the CanonicalIdResolver extension point. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The extension point has been introduced in https://github.com/jenkinsci/jenkins/commit/af42534defea4503347dfc0ec442fadf51cf3336 It does not follow common extension point practices (e.g. “all()” methods) and has no Javadoc. So I decided to polish it a bit. The core also adds some TODOs for review --- core/src/main/java/hudson/model/User.java | 73 +++++++++++++++++++---- 1 file changed, 60 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index e8895d4baf..8490ae0714 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -401,23 +401,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 resolvers = new ArrayList(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"); @@ -1077,6 +1070,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 implements ExtensionPoint, Comparable { /** @@ -1086,6 +1089,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(); @@ -1097,12 +1101,55 @@ public class User extends AbstractModelObject implements AccessControlled, Descr * extract user ID from idOrFullName with help from contextual infos. * can return null if no user ID matched the input */ - public abstract @CheckForNull String resolveCanonicalId(String idOrFullName, Map context); + public abstract @CheckForNull String resolveCanonicalId(String idOrFullName, @Nonnull Map 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 all() { + List 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. + */ + @CheckForNull + public static String resolve(@Nonnull String idOrFullName, @Nonnull Map 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; + } } From 80b9ee6f4b2e36573108cf0df7e1c8f8a3b52533 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 14 Nov 2017 12:03:46 +0100 Subject: [PATCH 2/7] Add missing @since to CanonicalIdResolver#resolve() --- core/src/main/java/hudson/model/User.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index 8490ae0714..79cf9f876b 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -1134,6 +1134,7 @@ public class User extends AbstractModelObject implements AccessControlled, Descr * @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 context) { From 9b4e831c139a32cd53dd42419f7780d8cd7616ac Mon Sep 17 00:00:00 2001 From: Francisco Javier Fernandez Gonzalez Date: Wed, 29 Nov 2017 13:58:02 +0100 Subject: [PATCH 3/7] [JENKINS-48080] Shows errors --- war/src/main/js/pluginSetupWizardGui.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/war/src/main/js/pluginSetupWizardGui.js b/war/src/main/js/pluginSetupWizardGui.js index 923f6a3f27..2aac81ed33 100644 --- a/war/src/main/js/pluginSetupWizardGui.js +++ b/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()+']*)[>](.|[\r\n])+[<][/]body/,'body$1>'+$main.html()+' Date: Wed, 29 Nov 2017 15:04:52 +0100 Subject: [PATCH 4/7] More coverage around AtomicFileWriter behaviour --- .../hudson/util/AtomicFileWriterTest.java | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/hudson/util/AtomicFileWriterTest.java b/core/src/test/java/hudson/util/AtomicFileWriterTest.java index 716805bd31..51bd205cb4 100644 --- a/core/src/test/java/hudson/util/AtomicFileWriterTest.java +++ b/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(); From c79683bdd805fb6145fe25b48ba3849a90c243f6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 29 Nov 2017 14:48:24 -0500 Subject: [PATCH 5/7] Rethrow any kind of RuntimeException, such as errors from RobustReflectionConverter. --- core/src/main/java/hudson/XmlFile.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/XmlFile.java b/core/src/main/java/hudson/XmlFile.java index d5077b34db..16644262b9 100644 --- a/core/src/main/java/hudson/XmlFile.java +++ b/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(); From 7c9170a13ab7021ed5b9027a9a75a958b914bf77 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sun, 3 Dec 2017 14:01:54 -0800 Subject: [PATCH 6/7] [maven-release-plugin] prepare release jenkins-2.93 --- cli/pom.xml | 2 +- core/pom.xml | 2 +- pom.xml | 4 ++-- test/pom.xml | 2 +- war/pom.xml | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/pom.xml b/cli/pom.xml index be7db1f9a1..5826886099 100644 --- a/cli/pom.xml +++ b/cli/pom.xml @@ -5,7 +5,7 @@ org.jenkins-ci.main pom - 2.93-SNAPSHOT + 2.93 cli diff --git a/core/pom.xml b/core/pom.xml index 9ce61b96c8..1ca00efed2 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -29,7 +29,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93-SNAPSHOT + 2.93 jenkins-core diff --git a/pom.xml b/pom.xml index 4a9ed89841..137e383dbe 100644 --- a/pom.xml +++ b/pom.xml @@ -33,7 +33,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93-SNAPSHOT + 2.93 pom Jenkins main module @@ -58,7 +58,7 @@ THE SOFTWARE. scm:git:git://github.com/jenkinsci/jenkins.git scm:git:ssh://git@github.com/jenkinsci/jenkins.git https://github.com/jenkinsci/jenkins - HEAD + jenkins-2.93 diff --git a/test/pom.xml b/test/pom.xml index 282426e053..8064268fec 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -28,7 +28,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93-SNAPSHOT + 2.93 test diff --git a/war/pom.xml b/war/pom.xml index 94f80d2274..bbb93dd13c 100644 --- a/war/pom.xml +++ b/war/pom.xml @@ -28,7 +28,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93-SNAPSHOT + 2.93 jenkins-war From a04b2dd63c1605d305c0d0ecbb9918649d6ad0d5 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sun, 3 Dec 2017 14:01:54 -0800 Subject: [PATCH 7/7] [maven-release-plugin] prepare for next development iteration --- cli/pom.xml | 2 +- core/pom.xml | 2 +- pom.xml | 4 ++-- test/pom.xml | 2 +- war/pom.xml | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/pom.xml b/cli/pom.xml index 5826886099..b6771c7daf 100644 --- a/cli/pom.xml +++ b/cli/pom.xml @@ -5,7 +5,7 @@ org.jenkins-ci.main pom - 2.93 + 2.94-SNAPSHOT cli diff --git a/core/pom.xml b/core/pom.xml index 1ca00efed2..a4b053eee5 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -29,7 +29,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93 + 2.94-SNAPSHOT jenkins-core diff --git a/pom.xml b/pom.xml index 137e383dbe..c798586c2b 100644 --- a/pom.xml +++ b/pom.xml @@ -33,7 +33,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93 + 2.94-SNAPSHOT pom Jenkins main module @@ -58,7 +58,7 @@ THE SOFTWARE. scm:git:git://github.com/jenkinsci/jenkins.git scm:git:ssh://git@github.com/jenkinsci/jenkins.git https://github.com/jenkinsci/jenkins - jenkins-2.93 + HEAD diff --git a/test/pom.xml b/test/pom.xml index 8064268fec..2f87f51d5f 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -28,7 +28,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93 + 2.94-SNAPSHOT test diff --git a/war/pom.xml b/war/pom.xml index bbb93dd13c..3d7ea1e54e 100644 --- a/war/pom.xml +++ b/war/pom.xml @@ -28,7 +28,7 @@ THE SOFTWARE. org.jenkins-ci.main pom - 2.93 + 2.94-SNAPSHOT jenkins-war