modified the Descriptor persistence scheme.

We used to use Map<String,Object> as a data store for Descriptor, but this requires a lot of boiler plate code to move values in/out from this Map, making the code uglier.
I modified this to just use XStream like the rest of the Hudson, so that the information can be persisted by simply storing them in fields.
Existing Descriptors are enhanced so that they can convert formats from pre-1.62 XML format.


git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@1126 71c3de6d-444a-0410-be80-ed276b4c234a

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

@@ -51,11 +51,15 @@ public final class XmlFile {
 
     /**
      * Loads the contents of this file into an existing object.
+     *
+     * @return
+     *      The unmarshalled object. Usually the same as <tt>o</tt>, but would be different
+     *      if the XML representation if completely new.
      */
-    public void unmarshal( Object o ) throws IOException {
+    public Object unmarshal( Object o ) throws IOException {
         Reader r = new BufferedReader(new InputStreamReader(new FileInputStream(file),"UTF-8"));
         try {
-            xs.unmarshal(new XppReader(r),o);
+            return xs.unmarshal(new XppReader(r),o);
         } catch (StreamException e) {
             throw new IOException2(e);
         } catch(ConversionException e) {

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

@@ -2,17 +2,17 @@ package hudson.model;
 
 import hudson.XmlFile;
 import hudson.scm.CVSSCM;
+import org.kohsuke.stapler.StaplerRequest;
 
 import javax.servlet.http.HttpServletRequest;
 import java.io.File;
 import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.List;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
-
-import org.kohsuke.stapler.StaplerRequest;
+import java.util.List;
+import java.util.Map;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Metadata about a configurable instance.
@@ -47,15 +47,23 @@ import org.kohsuke.stapler.StaplerRequest;
  * @see Describable
  */
 public abstract class Descriptor<T extends Describable<T>> {
-    private Map<String,Object> properties;
+    /**
+     * Up to Hudson 1.61 this was used as the primary persistence mechanism.
+     * Going forward Hudson simply persists all the non-transient fields
+     * of {@link Descriptor}, just like others, so this is pointless.
+     *
+     * @deprecated
+     */
+    private transient Map<String,Object> properties;
 
     /**
      * The class being described by this descriptor.
      */
-    public final Class<? extends T> clazz;
+    public transient final Class<? extends T> clazz;
 
     protected Descriptor(Class<? extends T> clazz) {
         this.clazz = clazz;
+        load();
     }
 
     /**
@@ -92,21 +100,6 @@ public abstract class Descriptor<T extends Describable<T>> {
         return clazz.isInstance(instance);
     }
 
-    /**
-     * Returns the data store that can be used to store configuration info.
-     *
-     * <p>
-     * The data store is local to each {@link Descriptor}.
-     *
-     * @return
-     *      never return null.
-     */
-    protected synchronized Map<String,Object> getProperties() {
-        if(properties==null)
-            properties = load();
-        return properties;
-    }
-
     /**
      * Invoked when the global configuration page is submitted.
      *
@@ -132,27 +125,39 @@ public abstract class Descriptor<T extends Describable<T>> {
      * Saves the configuration info to the disk.
      */
     protected synchronized void save() {
-        if(properties!=null)
-            try {
-                getConfigFile().write(properties);
-            } catch (IOException e) {
-                e.printStackTrace();
-            }
+        try {
+            getConfigFile().write(this);
+        } catch (IOException e) {
+            LOGGER.log(Level.WARNING, "Failed to save "+getConfigFile(),e);
+        }
     }
 
-    private Map<String,Object> load() {
+    private void load() {
         // load
         XmlFile file = getConfigFile();
         if(!file.exists())
-            return new HashMap<String,Object>();
+            return;
 
         try {
-            return (Map<String,Object>)file.read();
+            Object o = file.unmarshal(this);
+            if(o instanceof Map) {
+                // legacy format
+                convert((Map<String,Object>)o);
+                save();     // convert to the new format
+            }
         } catch (IOException e) {
-            return new HashMap<String,Object>();
+            LOGGER.log(Level.WARNING, "Failed to load "+file, e);
         }
     }
 
+    /**
+     * {@link Descriptor}s that has existed &lt;= 1.61 needs to
+     * be able to read in the old configuration in a property bag
+     * and reflect that into the new layout.
+     */
+    protected void convert(Map<String, Object> oldPropertyBag) {
+    }
+
     private XmlFile getConfigFile() {
         return new XmlFile(new File(Hudson.getInstance().getRootDir(),clazz.getName()+".xml"));
     }
@@ -203,4 +208,6 @@ public abstract class Descriptor<T extends Describable<T>> {
             return formField;
         }
     }
+
+    private static final Logger LOGGER = Logger.getLogger(Descriptor.class.getName());
 }

core/src/main/java/hudson/scm/CVSSCM.java

@@ -46,6 +46,7 @@ import java.util.Map;
 import java.util.StringTokenizer;
 import java.util.Set;
 import java.util.HashSet;
+import java.util.HashMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -490,10 +491,29 @@ public class CVSSCM extends AbstractCVSFamilySCM {
     static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<SCM> implements ModelObject {
+        /**
+         * Path to <tt>.cvspass</tt>. Null to default.
+         */
+        private String cvsPassFile;
+
+        /**
+         * Copy-on-write.
+         */
+        private volatile Map<String,RepositoryBrowser> browsers = new HashMap<String,RepositoryBrowser>();
+
+        class RepositoryBrowser {
+            String diffURL;
+            String browseURL;
+        }
+
         DescriptorImpl() {
             super(CVSSCM.class);
         }
 
+        protected void convert(Map<String, Object> oldPropertyBag) {
+            cvsPassFile = (String)oldPropertyBag.get("cvspass");
+        }
+
         public String getDisplayName() {
             return "CVS";
         }
@@ -510,14 +530,14 @@ public class CVSSCM extends AbstractCVSFamilySCM {
         }
 
         public String getCvspassFile() {
-            String value = (String)getProperties().get("cvspass");
+            String value = cvsPassFile;
             if(value==null)
                 value = "";
             return value;
         }
 
         public void setCvspassFile(String value) {
-            getProperties().put("cvspass",value);
+            cvsPassFile = value;
             save();
         }
 
@@ -525,45 +545,35 @@ public class CVSSCM extends AbstractCVSFamilySCM {
          * Gets the URL that shows the diff.
          */
         public String getDiffURL(String cvsRoot, String pathName, String oldRev, String newRev) {
-            String url = getProperties().get("repository-browser.diff." + cvsRoot).toString();
-            if(url==null)   return null;
-            return url.replaceAll("%%P",pathName).replace("%%r",oldRev).replace("%%R",newRev);
+            RepositoryBrowser b = browsers.get(cvsRoot);
+            if(b==null)   return null;
+            return b.diffURL.replaceAll("%%P",pathName).replace("%%r",oldRev).replace("%%R",newRev);
 
         }
 
         public boolean configure( HttpServletRequest req ) {
             setCvspassFile(req.getParameter("cvs_cvspass"));
 
-            Map<String,Object> properties = getProperties();
-
+            Map<String,RepositoryBrowser> browsers = new HashMap<String, RepositoryBrowser>();
             int i=0;
             while(true) {
                 String root = req.getParameter("cvs_repobrowser_cvsroot" + i);
                 if(root==null)  break;
 
-                setBrowser(req.getParameter("cvs_repobrowser"+i), properties, root, "repository-browser.");
-                setBrowser(req.getParameter("cvs_repobrowser_diff"+i), properties, root, "repository-browser.diff.");
+                RepositoryBrowser rb = new RepositoryBrowser();
+                rb.browseURL = req.getParameter("cvs_repobrowser"+i);
+                rb.diffURL = req.getParameter("cvs_repobrowser_diff"+i);
+                browsers.put(root,rb);
+
                 i++;
             }
+            this.browsers = browsers;
 
             save();
 
             return true;
         }
 
-        private void setBrowser(String key, Map<String, Object> properties, String root, String prefi) {
-            String value = Util.nullify(key);
-            if(value==null) {
-                properties.remove(prefi +root);
-            } else {
-                properties.put(prefi +root,value);
-            }
-        }
-
-        public Map<String,Object> getProperties() {
-            return super.getProperties();
-        }
-
     //
     // web methods
     //

core/src/main/java/hudson/scm/SubversionSCM.java

@@ -409,10 +409,19 @@ public class SubversionSCM extends AbstractCVSFamilySCM {
     static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<SCM> {
+        /**
+         * Path to <tt>svn.exe</tt>. Null to default.
+         */
+        private String svnExe;
+
         DescriptorImpl() {
             super(SubversionSCM.class);
         }
 
+        protected void convert(Map<String, Object> oldPropertyBag) {
+            svnExe = (String)oldPropertyBag.get("svn_exe");
+        }
+
         public String getDisplayName() {
             return "Subversion";
         }
@@ -427,19 +436,19 @@ public class SubversionSCM extends AbstractCVSFamilySCM {
         }
 
         public String getSvnExe() {
-            String value = (String)getProperties().get("svn_exe");
+            String value = svnExe;
             if(value==null)
                 value = "svn";
             return value;
         }
 
         public void setSvnExe(String value) {
-            getProperties().put("svn_exe",value);
+            svnExe = value;
             save();
         }
 
         public boolean configure( HttpServletRequest req ) {
-            setSvnExe(req.getParameter("svn_exe"));
+            svnExe = req.getParameter("svn_exe");
             return true;
         }
 

core/src/main/java/hudson/tasks/Ant.java

 package hudson.tasks;
 
+import hudson.CopyOnWrite;
 import hudson.Launcher;
 import hudson.Util;
-import hudson.util.FormFieldValidator;
-import hudson.model.Action;
 import hudson.model.Build;
 import hudson.model.BuildListener;
 import hudson.model.Descriptor;
 import hudson.model.Project;
+import hudson.util.FormFieldValidator;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
 
-import javax.servlet.http.HttpServletRequest;
 import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
 import java.io.File;
 import java.io.IOException;
 import java.util.Map;
 
-import org.kohsuke.stapler.StaplerRequest;
-import org.kohsuke.stapler.StaplerResponse;
-
 /**
  * @author Kohsuke Kawaguchi
  */
@@ -103,10 +102,18 @@ public class Ant extends Builder {
     public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<Builder> {
+        @CopyOnWrite
+        private volatile AntInstallation[] installations = new AntInstallation[0];
+
         private DescriptorImpl() {
             super(Ant.class);
         }
 
+        protected void convert(Map<String,Object> oldPropertyBag) {
+            if(oldPropertyBag.containsKey("installations"))
+                installations = (AntInstallation[]) oldPropertyBag.get("installations");
+        }
+
         public String getHelpFile() {
             return "/help/project-config/ant.html";
         }
@@ -116,12 +123,7 @@ public class Ant extends Builder {
         }
 
         public AntInstallation[] getInstallations() {
-            AntInstallation[] r = (AntInstallation[]) getProperties().get("installations");
-
-            if(r==null)
-                return new AntInstallation[0];
-
-            return r.clone();
+            return installations;
         }
 
         public boolean configure(HttpServletRequest req) {
@@ -142,7 +144,7 @@ public class Ant extends Builder {
                 insts[i] = new AntInstallation(names[i],homes[i]);
             }
 
-            getProperties().put("installations",insts);
+            this.installations = insts;
 
             save();
 

core/src/main/java/hudson/tasks/Mailer.java

@@ -12,13 +12,13 @@ import hudson.scm.ChangeLogSet.Entry;
 import org.apache.tools.ant.types.selectors.SelectorUtils;
 import org.kohsuke.stapler.StaplerRequest;
 
+import javax.mail.Address;
 import javax.mail.Authenticator;
 import javax.mail.Message;
 import javax.mail.MessagingException;
 import javax.mail.PasswordAuthentication;
 import javax.mail.Session;
 import javax.mail.Transport;
-import javax.mail.Address;
 import javax.mail.internet.InternetAddress;
 import javax.mail.internet.MimeMessage;
 import javax.servlet.http.HttpServletRequest;
@@ -27,13 +27,13 @@ import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.util.ArrayList;
+import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
 import java.util.StringTokenizer;
-import java.util.Date;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
@@ -77,11 +77,11 @@ public class Mailer extends Publisher {
             if(mail!=null) {
                 Address[] allRecipients = mail.getAllRecipients();
                 if(allRecipients!=null) {
-                    StringBuffer buf = new StringBuffer("Sending e-mails to ");
+                StringBuffer buf = new StringBuffer("Sending e-mails to ");
                     for (Address a : allRecipients)
-                        buf.append(' ').append(a);
-                    listener.getLogger().println(buf);
-                    Transport.send(mail);
+                    buf.append(' ').append(a);
+                listener.getLogger().println(buf);
+                Transport.send(mail);
                 } else {
                     listener.getLogger().println("An attempt to send an e-mail"
                             + " to empty list of recipients, ignored.");
@@ -261,11 +261,50 @@ public class Mailer extends Publisher {
     public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<Publisher> {
+        /**
+         * The default e-mail address suffix appended to the user name found from changelog,
+         * to send e-mails. Null if not configured.
+         */
+        private String defaultSuffix;
+
+        /**
+         * Hudson's own URL, to put into the e-mail.
+         */
+        private String hudsonUrl;
+
+        /**
+         * If non-null, use SMTP-AUTH with these information.
+         */
+        private String smtpAuthPassword,smtpAuthUsername;
+
+        /**
+         * The e-mail address that Hudson puts to "From:" field in outgoing e-mails.
+         * Null if unconfigured.
+         */
+        private String adminAddress;
+
+        /**
+         * The SMTP server to use for sending e-mail. Null for default to the environment,
+         * which is usually <tt>localhost</tt>.
+         */
+        private String smtpHost;
 
         public DescriptorImpl() {
             super(Mailer.class);
         }
 
+        /**
+         * For backward compatibility.
+         */
+        protected void convert(Map<String, Object> oldPropertyBag) {
+            defaultSuffix = (String)oldPropertyBag.get("mail.default.suffix");
+            hudsonUrl = (String)oldPropertyBag.get("mail.hudson.url");
+            smtpAuthUsername = (String)oldPropertyBag.get("mail.hudson.smtpauth.username");
+            smtpAuthPassword = (String)oldPropertyBag.get("mail.hudson.smtpauth.password");
+            adminAddress = (String)oldPropertyBag.get("mail.admin.address");
+            smtpHost = (String)oldPropertyBag.get("mail.smtp.host");
+        }
+
         public String getDisplayName() {
             return "E-mail Notification";
         }
@@ -275,17 +314,14 @@ public class Mailer extends Publisher {
         }
 
         public String getDefaultSuffix() {
-            return (String)getProperties().get("mail.default.suffix");
+            return defaultSuffix;
         }
 
         /** JavaMail session. */
         public Session createSession() {
             Properties props = new Properties(System.getProperties());
-            // can't use putAll
-            for (Map.Entry o : ((Map<?,?>)getProperties()).entrySet()) {
-                if(o.getValue()!=null)
-                    props.put(o.getKey(),o.getValue());
-            }
+            if(smtpHost!=null)
+                props.put("mail.smtp.host",smtpHost);
 
             return Session.getInstance(props,getAuthenticator());
         }
@@ -302,16 +338,20 @@ public class Mailer extends Publisher {
 
         public boolean configure(HttpServletRequest req) throws FormException {
             // this code is brain dead
-            getProperties().put("mail.smtp.host",nullify(req.getParameter("mailer_smtp_server")));
-            getProperties().put("mail.admin.address",req.getParameter("mailer_admin_address"));
-            getProperties().put("mail.default.suffix",nullify(req.getParameter("mailer_default_suffix")));
+            smtpHost = nullify(req.getParameter("mailer_smtp_server"));
+            adminAddress = req.getParameter("mailer_admin_address");
+            defaultSuffix = nullify(req.getParameter("mailer_default_suffix"));
             String url = nullify(req.getParameter("mailer_hudson_url"));
             if(url!=null && !url.endsWith("/"))
                 url += '/';
-            getProperties().put("mail.hudson.url",url);
+            hudsonUrl = url;
 
-            getProperties().put("mail.hudson.smtpauth.username",nullify(req.getParameter("mailer.SMTPAuth.userName")));
-            getProperties().put("mail.hudson.smtpauth.password",nullify(req.getParameter("mailer.SMTPAuth.password")));
+            if(req.getParameter("mailer.useSMTPAuth")!=null) {
+                smtpAuthUsername = nullify(req.getParameter("mailer.SMTPAuth.userName"));
+                smtpAuthPassword = nullify(req.getParameter("mailer.SMTPAuth.password"));
+            } else {
+                smtpAuthUsername = smtpAuthPassword = null;
+            }
 
             save();
             return super.configure(req);
@@ -323,25 +363,25 @@ public class Mailer extends Publisher {
         }
 
         public String getSmtpServer() {
-            return (String)getProperties().get("mail.smtp.host");
+            return smtpHost;
         }
 
         public String getAdminAddress() {
-            String v = (String)getProperties().get("mail.admin.address");
+            String v = adminAddress;
             if(v==null)     v = "address not configured yet <nobody>";
             return v;
         }
 
         public String getUrl() {
-            return (String)getProperties().get("mail.hudson.url");
+            return hudsonUrl;
         }
 
         public String getSmtpAuthUserName() {
-            return (String)getProperties().get("mail.hudson.smtpauth.username");
+            return smtpAuthUsername;
         }
 
         public String getSmtpAuthPassword() {
-            return (String)getProperties().get("mail.hudson.smtpauth.password");
+            return smtpAuthPassword;
         }
 
         /** Check whether a path (/-separated) will be archived. */
@@ -433,7 +473,6 @@ public class Mailer extends Publisher {
 
         StringBuilder sb = new StringBuilder(s.length() * 2);
         sb.append("\\Q");
-        slashEIndex = 0;
         int current = 0;
         while ((slashEIndex = s.indexOf("\\E", current)) != -1) {
             sb.append(s.substring(current, slashEIndex));

core/src/main/java/hudson/tasks/Maven.java

 package hudson.tasks;
 
+import hudson.CopyOnWrite;
 import hudson.Launcher;
 import hudson.Util;
-import hudson.util.FormFieldValidator;
-import hudson.model.Action;
 import hudson.model.Build;
 import hudson.model.BuildListener;
 import hudson.model.Descriptor;
 import hudson.model.Project;
+import hudson.util.FormFieldValidator;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
 
-import javax.servlet.http.HttpServletRequest;
 import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
 import java.io.File;
 import java.io.IOException;
 import java.util.Map;
 
-import org.kohsuke.stapler.StaplerRequest;
-import org.kohsuke.stapler.StaplerResponse;
-
 /**
  * Build by using Maven.
  *
@@ -104,10 +103,19 @@ public class Maven extends Builder {
     public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<Builder> {
+        @CopyOnWrite
+        private MavenInstallation[] installations = new MavenInstallation[0];
+
         private DescriptorImpl() {
             super(Maven.class);
         }
 
+
+        protected void convert(Map<String, Object> oldPropertyBag) {
+            if(oldPropertyBag.containsKey("installations"))
+                installations = (MavenInstallation[]) oldPropertyBag.get("installations");
+        }
+
         public String getHelpFile() {
             return "/help/project-config/maven.html";
         }
@@ -117,12 +125,7 @@ public class Maven extends Builder {
         }
 
         public MavenInstallation[] getInstallations() {
-            MavenInstallation[] r = (MavenInstallation[]) getProperties().get("installations");
-
-            if(r==null)
-                return new MavenInstallation[0];
-
-            return r.clone();
+            return installations;
         }
 
         public boolean configure(HttpServletRequest req) {
@@ -144,7 +147,7 @@ public class Maven extends Builder {
                 insts[i] = new MavenInstallation(names[i],homes[i]);
             }
 
-            getProperties().put("installations",insts);
+            this.installations = insts;
 
             save();
 

core/src/main/java/hudson/tasks/Shell.java

@@ -14,6 +14,7 @@ import javax.servlet.http.HttpServletRequest;
 import java.io.FileWriter;
 import java.io.IOException;
 import java.io.Writer;
+import java.util.Map;
 
 /**
  * Executes a series of commands by using a shell.
@@ -93,19 +94,28 @@ public class Shell extends Builder {
     public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
 
     public static final class DescriptorImpl extends Descriptor<Builder> {
+        /**
+         * Shell executable, or null to default.
+         */
+        private String shell;
+
         private DescriptorImpl() {
             super(Shell.class);
         }
 
+
+        protected void convert(Map<String, Object> oldPropertyBag) {
+            shell = (String)oldPropertyBag.get("shell");
+        }
+
         public String getShell() {
-            String shell = (String)getProperties().get("shell");
             if(shell==null)
-                shell = isWindows()?"sh":"/bin/sh";
+                return isWindows()?"sh":"/bin/sh";
             return shell;
         }
 
         public void setShell(String shell) {
-            getProperties().put("shell",shell);
+            this.shell = shell;
             save();
         }
 

core/src/main/java/hudson/triggers/SCMTrigger.java

@@ -130,10 +130,16 @@ public class SCMTrigger extends Trigger {
          */
         transient volatile ExecutorService executor;
 
+        /**
+         * Max number of threads for SCM polling.
+         * 0 for unbounded.
+         */
+        private int maximumThreads;
+
         DescriptorImpl() {
             super(SCMTrigger.class);
             // create an executor
-            update(getPollingThreadCount());
+            update();
         }
 
         public ExecutorService getExecutor() {
@@ -163,28 +169,24 @@ public class SCMTrigger extends Trigger {
          *      0 if unlimited.
          */
         public int getPollingThreadCount() {
-            String value = (String)getProperties().get("poll_scm_threads");
-            if(value==null)
-                return 0;
-            return Integer.parseInt(value);
+            return maximumThreads;
         }
 
         public void setPollingThreadCount(int n) {
-            getProperties().put("poll_scm_threads",String.valueOf(n));
+            // fool proof
+            if(n<0)     n=0;
+            if(n>100)   n=100;
+
+            maximumThreads = n;
             save();
-            update(n);
         }
 
         /**
          * Update the {@link ExecutorService} instance.
          */
-        /*package*/ synchronized void update(int n) {
-            // fool proof
-            if(n<0) n=0;
-            if(n>100)   n=0;
-
+        /*package*/ synchronized void update() {
             // swap to a new one, and shut down the old one gradually
-            ExecutorService newExec = n==0 ? Executors.newCachedThreadPool() : Executors.newFixedThreadPool(n);
+            ExecutorService newExec = maximumThreads==0 ? Executors.newCachedThreadPool() : Executors.newFixedThreadPool(maximumThreads);
             ExecutorService old = executor;
             executor = newExec;
             if(old!=null)