Merge pull request #357 from jglick/SCMSource.id

Stop assigning random `SCMSource.id`s

Author: jglick

src/main/java/jenkins/scm/api/SCMSource.java

@@ -47,7 +47,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.TransientActionFactory;
@@ -136,7 +135,7 @@ public boolean equals(Object obj) {
      */
     @Deprecated
     protected SCMSource(@CheckForNull String id) {
-        this.id = id == null ? UUID.randomUUID().toString() : id;
+        this.id = id == null ? "" : id;
     }
 
     /**
@@ -148,18 +147,16 @@ protected SCMSource() {
     /**
      * Sets the ID that is used to ensure this {@link SCMSource} can be retrieved from its {@link SCMSourceOwner}
      * provided that this {@link SCMSource} does not already {@link #hasId()}.
-     * <p>
-     * Note this is a <strong>one-shot</strong> setter. If {@link #getId()} is called first, then its value will stick,
-     * otherwise the first call to {@link #setId(String)} will stick.
-     *
+     * This should be called from the {@link SCMSourceOwner} when saving the list of {@link SCMSource}s,
+     * though for compatibility reasons it may also be called directly.
      * @param id the ID, this is an opaque token expected to be unique within any one {@link SCMSourceOwner}.
      * @see #hasId()
      * @see #getId()
      * @since 2.2.0
      */
     @DataBoundSetter
     public final synchronized void setId(@CheckForNull String id) {
-        if (this.id == null) {
+        if (this.id == null || this.id.isEmpty()) {
             this.id = id;
         } else if (!this.id.equals(id)) {
             throw new IllegalStateException("The ID cannot be changed after it has been set.");
@@ -186,20 +183,19 @@ public final SCMSource withId(@CheckForNull String id) {
      * @since 2.2.0
      */
     public final synchronized boolean hasId() {
-        return this.id != null;
+        return this.id != null && !this.id.isEmpty();
     }
     /**
-     * The ID of this source. The ID is not related to anything at all. If this {@link SCMSource} does not have an ID
-     * then one will be generated.
+     * The ID of this source.
      *
-     * @return the ID of this source.
+     * @return the ID of this source; the empty string until assigned
      * @see #setId(String)
      * @see #hasId()
      */
     @NonNull
     public final synchronized String getId() {
         if (id == null) {
-            id = UUID.randomUUID().toString();
+            id = "";
         }
         return id;
     }

src/main/java/jenkins/scm/api/SCMSourceDescriptor.java

@@ -39,7 +39,6 @@
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.UUID;
 
 /**
  * A {@link Descriptor} for {@link SCMSource}s.
@@ -59,12 +58,12 @@ public abstract class SCMSourceDescriptor extends Descriptor<SCMSource> implemen
      * Return or generate the ID for a source instance.
      *
      * @param source the source or {@code null} if a new source.
-     * @return the ID of the supplied source or a newly generated ID to use for a new source instance.
+     * @return the ID of the supplied source or the empty string
      */
     @NonNull
     @SuppressWarnings("unused") // use by stapler as well as elsewhere
     public String getId(@CheckForNull SCMSource source) {
-        return source == null ? UUID.randomUUID().toString() : source.getId();
+        return source == null ? "" : source.getId();
     }
 
     /**

src/test/java/jenkins/scm/impl/SymbolAnnotationsTest.java

@@ -94,7 +94,6 @@ public class SymbolAnnotationsTest {
             ));
             assertThat(DescribableModel.uninstantiate2_(instance).toString(), is("@mockScm("
                     + "controllerId=" + c.getId() + ","
-                    + "id=" + instance.getId() + ","
                     + "repository=test,"
                     + "traits=["
                     + "@discoverBranches$MockSCMDiscoverBranches(), "
@@ -158,7 +157,6 @@ public class SymbolAnnotationsTest {
             assertThat(DescribableModel.uninstantiate2_(instance).toString(), is("@fromSource(name=foo,"
                     + "sources=[@mockScm$MockSCMSource("
                     + "controllerId=" + c.getId() + ","
-                    + "id=" + instance.getSources().get(0).getId() + ","
                     + "repository=test,"
                     + "traits=[]"
                     + ")"