[JENKINS-47718] - Deprecate User#getUser(String) (#3114)

* [JENKINS-47718] - Deprecate User#getUser(String) `User#getUser(String)` method causes lots of confusion for plugin developers. It implicitly creates a user if it cannot be found, and in many cases this is actually a not-expected behavior. This change deprecates the method and to creates a new `User#getOrCreate()` which is more explicit * [JENKINS-47718] - Address comments from @jglick
jenkinsci · Nov 19, 2017 · 8cb3351 · 8cb3351
1 parent d688c15
commit 8cb3351
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 8 deletions.
diff --git a/core/src/main/java/hudson/model/Cause.java b/core/src/main/java/hudson/model/Cause.java
@@ -25,6 +25,7 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import hudson.console.ModelHyperlinkNote;
@@ -366,9 +367,15 @@ public UserCause() {
             this.authenticationName = Jenkins.getAuthentication().getName();
         }
 
+        /**
+         * Gets user display name when possible.
+         * @return User display name.
+         *         If the User does not exist, returns its ID.
+         */
         @Exported(visibility=3)
         public String getUserName() {
-        	return User.get(authenticationName).getDisplayName();
+        	final User user = User.getById(authenticationName, false);
+        	return user != null ? user.getDisplayName() : authenticationName;
         }
 
         @Override
@@ -409,12 +416,15 @@ public String getUserId() {
 
         @Exported(visibility = 3)
         public String getUserName() {
-            return userId == null ? "anonymous" : User.get(userId).getDisplayName();
+            final User user = userId == null ? null : User.getById(userId, false);
+            return user == null ? "anonymous" : user.getDisplayName();
         }
 
         @Restricted(DoNotUse.class) // for Jelly
+        @CheckForNull
         public String getUserUrl() {
-            return userId == null ? null : User.get(userId).getUrl();
+            final User user = userId == null ? null : User.getById(userId, false);
+            return user != null ? user.getUrl() : null;
         }
 
         @Override

diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
@@ -1710,11 +1710,14 @@ protected final void execute(@Nonnull RunExecution job) {
 
                     Authentication auth = Jenkins.getAuthentication();
                     if (!auth.equals(ACL.SYSTEM)) {
-                        String name = auth.getName();
+                        String id = auth.getName();
                         if (!auth.equals(Jenkins.ANONYMOUS)) {
-                            name = ModelHyperlinkNote.encodeTo(User.get(name));
+                            final User usr = User.getById(id, false);
+                            if (usr != null) { // Encode user hyperlink for existing users
+                                id = ModelHyperlinkNote.encodeTo(usr);
+                            }
                         }
-                        listener.getLogger().println(Messages.Run_running_as_(name));
+                        listener.getLogger().println(Messages.Run_running_as_(id));
                     }
 
                     RunListener.fireStarted(this,listener);

diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java
@@ -387,6 +387,10 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
     /**
      * Gets the {@link User} object by its id or full name.
      *
+     * In order to resolve the user ID, the method invokes {@link CanonicalIdResolver} extension points.
+     * Note that it may cause significant performance degradation.
+     * If you are sure the passed value is a User ID, it is recommended to use {@link #getById(String, boolean)}.
+     *
      * @param create
      *      If true, this method will never return null for valid input
      *      (by creating a new {@link User} object if none exists.)
@@ -539,12 +543,45 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
 
     /**
      * Gets the {@link User} object by its id or full name.
+     *
+     * Creates a user on-demand.
+     *
+     * <p>
      * Use {@link #getById} when you know you have an ID.
+     * In this method Jenkins will try to resolve the {@link User} by full name with help of various
+     * {@link hudson.tasks.UserNameResolver}.
+     * This is slow (see JENKINS-23281).
+     *
+     * @deprecated This method is deprecated, because it causes unexpected {@link User} creation
+     *             by API usage code and causes performance degradation of used to retrieve users by ID.
+     *             Use {@link #getById} when you know you have an ID.
+     *             Otherwise use {@link #getOrCreateByIdOrFullName(String)} or {@link #get(String, boolean, Map)}.
      */
+    @Deprecated
     public static @Nonnull User get(String idOrFullName) {
-        return get(idOrFullName,true);
+        return getOrCreateByIdOrFullName(idOrFullName);
+    }
+
+    /**
+     * Get the user by ID or Full Name.
+     *
+     * If the user does not exist, creates a new one on-demand.
+     *
+     * <p>
+     * Use {@link #getById} when you know you have an ID.
+     * In this method Jenkins will try to resolve the {@link User} by full name with help of various
+     * {@link hudson.tasks.UserNameResolver}.
+     * This is slow (see JENKINS-23281).
+     *
+     * @param idOrFullName User ID or full name
+     * @return User instance. It will be created on-demand.
+     * @since TODO
+     */
+    public static @Nonnull User getOrCreateByIdOrFullName(@Nonnull String idOrFullName) {
+        return get(idOrFullName,true, Collections.emptyMap());
     }
 
+
     /**
      * Gets the {@link User} object representing the currently logged-in user, or null
      * if the current user is anonymous.

diff --git a/core/src/main/java/jenkins/scm/RunWithSCM.java b/core/src/main/java/jenkins/scm/RunWithSCM.java
@@ -84,6 +84,9 @@ public interface RunWithSCM<JobT extends Job<JobT, RunT>,
      * This list at least always include people who made changes in this build, but
      * if the previous build was a failure it also includes the culprit list from there.
      *
+     * <p>
+     * Missing {@link User}s will be created on-demand.
+     *
      * @return
      *      can be empty but never null.
      */
@@ -99,7 +102,8 @@ public interface RunWithSCM<JobT extends Job<JobT, RunT>,
             public Iterator<User> iterator() {
                 return new AdaptedIterator<String,User>(culpritIds.iterator()) {
                     protected User adapt(String id) {
-                        return User.get(id);
+                        // TODO: Probably it should not auto-create users
+                        return User.getById(id, true);
                     }
                 };
             }