SOLR-3137: When solr.xml is persisted, you lose all system property substitution that was used.

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1245217 13f79535-47bb-0310-9956-ffa450edef68

solr/core/src/java/org/apache/solr/core/Config.java

@@ -72,6 +72,11 @@ public class Config {
     this( loader, name, null, null );
   }
 
+  
+  public Config(SolrResourceLoader loader, String name, InputSource is, String prefix) throws ParserConfigurationException, IOException, SAXException 
+  {
+    this(loader, name, is, prefix, true);
+  }
   /**
    * Builds a config:
    * <p>
@@ -91,7 +96,7 @@ public class Config {
    * @throws java.io.IOException
    * @throws org.xml.sax.SAXException
    */
-  public Config(SolrResourceLoader loader, String name, InputSource is, String prefix) throws ParserConfigurationException, IOException, SAXException 
+  public Config(SolrResourceLoader loader, String name, InputSource is, String prefix, boolean subProps) throws ParserConfigurationException, IOException, SAXException 
   {
     if( loader == null ) {
       loader = new SolrResourceLoader( null );
@@ -126,8 +131,9 @@ public class Config {
         // some XML parsers are broken and don't close the byte stream (but they should according to spec)
         IOUtils.closeQuietly(is.getByteStream());
       }
-
+      if (subProps) {
-      DOMUtil.substituteProperties(doc, loader.getCoreProperties());
+        DOMUtil.substituteProperties(doc, loader.getCoreProperties());
+      }
     } catch (ParserConfigurationException e)  {
       SolrException.log(log, "Exception during parsing file: " + name, e);
       throw e;
@@ -140,6 +146,13 @@ public class Config {
     }
   }
   
+  public Config(SolrResourceLoader loader, String name, Document doc) {
+    this.prefix = null;
+    this.doc = doc;
+    this.name = name;
+    this.loader = loader;
+  }
+
   /**
    * @since solr 1.3
    */
@@ -171,6 +184,10 @@ public class Config {
     return (prefix==null || path.startsWith("/")) ? path : prefix+path;
   }
   
+  public void substituteProperties() {
+    DOMUtil.substituteProperties(doc, loader.getCoreProperties());
+  }
+
 
   public Object evaluate(String path, QName type) {
     XPath xpath = xpathFactory.newXPath();

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -17,28 +17,43 @@
 
 package org.apache.solr.core;
 
-import java.io.*;
+import java.io.ByteArrayInputStream;
-import java.util.*;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.text.SimpleDateFormat;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Properties;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeoutException;
-import java.text.SimpleDateFormat;
-
-import org.apache.solr.handler.component.HttpShardHandlerFactory;
-import org.apache.solr.handler.component.ShardHandlerFactory;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import javax.xml.parsers.ParserConfigurationException;
-import javax.xml.xpath.XPathConstants;
+import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.dom.DOMResult;
+import javax.xml.transform.dom.DOMSource;
 import javax.xml.xpath.XPath;
+import javax.xml.xpath.XPathConstants;
 import javax.xml.xpath.XPathExpressionException;
 
+import org.apache.commons.io.IOUtils;
 import org.apache.solr.cloud.CloudDescriptor;
 import org.apache.solr.cloud.CurrentCoreDescriptorProvider;
 import org.apache.solr.cloud.SolrZkServer;
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.cloud.ZkSolrResourceLoader;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.cloud.ZooKeeperException;
 import org.apache.solr.common.params.CoreAdminParams;
@@ -48,13 +63,17 @@ import org.apache.solr.common.util.SystemIdResolver;
 import org.apache.solr.core.SolrXMLSerializer.SolrCoreXMLDef;
 import org.apache.solr.core.SolrXMLSerializer.SolrXMLDef;
 import org.apache.solr.handler.admin.CoreAdminHandler;
+import org.apache.solr.handler.component.HttpShardHandlerFactory;
+import org.apache.solr.handler.component.ShardHandlerFactory;
 import org.apache.solr.schema.IndexSchema;
 import org.apache.zookeeper.KeeperException;
-import org.apache.commons.io.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.w3c.dom.Document;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
-import org.xml.sax.SAXException;
 import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
 
 
 /**
@@ -63,7 +82,11 @@ import org.xml.sax.InputSource;
  */
 public class CoreContainer 
 {
+  private static final String DEFAULT_HOST_CONTEXT = "solr";
+  private static final String DEFAULT_HOST_PORT = "8983";
+  private static final int DEFAULT_ZK_CLIENT_TIMEOUT = 10000;
   private static final String DEFAULT_DEFAULT_CORE_NAME = "collection1";
+  private static final boolean DEFAULT_SHARE_SCHEMA = false;
   
   protected static Logger log = LoggerFactory.getLogger(CoreContainer.class);
   
@@ -92,6 +115,8 @@ public class CoreContainer
   private ShardHandlerFactory shardHandlerFactory;
 
   private String zkHost;
+  private Map<SolrCore,String> coreToOrigName = new ConcurrentHashMap<SolrCore,String>();
+
 
   {
     log.info("New CoreContainer " + System.identityHashCode(this));
@@ -301,7 +326,18 @@ public class CoreContainer
       throws ParserConfigurationException, IOException, SAXException {
     this.loader = new SolrResourceLoader(dir);
     solrHome = loader.getInstanceDir();
-    Config cfg = new Config(loader, null, cfgis, null);
+    
+    Config cfg = new Config(loader, null, cfgis, null, false);
+    
+    // keep orig config for persist to consult
+    try {
+      this.cfg = new Config(loader, null, copyDoc(cfg.getDocument()));
+    } catch (TransformerException e) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, "", e);
+    }
+    
+    cfg.substituteProperties();
+    
     String dcoreName = cfg.get("solr/cores/@defaultCoreName", null);
     if(dcoreName != null) {
       defaultCoreName = dcoreName;
@@ -310,12 +346,12 @@ public class CoreContainer
     libDir = cfg.get("solr/@sharedLib", null);
     zkHost = cfg.get("solr/@zkHost" , null);
     adminPath = cfg.get("solr/cores/@adminPath", null);
-    shareSchema = cfg.getBool("solr/cores/@shareSchema", false);
+    shareSchema = cfg.getBool("solr/cores/@shareSchema", DEFAULT_SHARE_SCHEMA);
-    zkClientTimeout = cfg.getInt("solr/cores/@zkClientTimeout", 10000);
+    zkClientTimeout = cfg.getInt("solr/cores/@zkClientTimeout", DEFAULT_ZK_CLIENT_TIMEOUT);
 
-    hostPort = cfg.get("solr/cores/@hostPort", "8983");
+    hostPort = cfg.get("solr/cores/@hostPort", DEFAULT_HOST_PORT);
 
-    hostContext = cfg.get("solr/cores/@hostContext", "solr");
+    hostContext = cfg.get("solr/cores/@hostContext", DEFAULT_HOST_CONTEXT);
     host = cfg.get("solr/cores/@host", null);
 
     if(shareSchema){
@@ -342,7 +378,7 @@ public class CoreContainer
     }
 
     try {
-      containerProperties = readProperties(cfg, ((NodeList) cfg.evaluate("solr", XPathConstants.NODESET)).item(0));
+      containerProperties = readProperties(cfg, ((NodeList) cfg.evaluate(DEFAULT_HOST_CONTEXT, XPathConstants.NODESET)).item(0));
     } catch (Throwable e) {
       SolrException.log(log,null,e);
     }
@@ -352,16 +388,19 @@ public class CoreContainer
     for (int i=0; i<nodes.getLength(); i++) {
       Node node = nodes.item(i);
       try {
-        String name = DOMUtil.getAttr(node, "name", null);
+        String rawName = DOMUtil.getAttr(node, "name", null);
-        if (null == name) {
+        if (null == rawName) {
           throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
                                   "Each core in solr.xml must have a 'name'");
         }
-        if (name.equals(defaultCoreName)){
+        String name;
+        if (rawName.equals(defaultCoreName)){
           // for the default core we use a blank name,
           // later on attempts to access it by it's full name will 
           // be mapped to this.
           name="";
+        } else {
+          name = rawName;
         }
         CoreDescriptor p = new CoreDescriptor(this, name, DOMUtil.getAttr(node, "instanceDir", null));
 
@@ -402,6 +441,9 @@ public class CoreContainer
 
         SolrCore core = create(p);
         register(name, core, false);
+        
+        // track original names
+        coreToOrigName.put(core, rawName);
       }
       catch (Throwable ex) {
         SolrException.log(log,null,ex);
@@ -409,6 +451,15 @@ public class CoreContainer
     }
   }
 
+  private Document copyDoc(Document document) throws TransformerException {
+    TransformerFactory tfactory = TransformerFactory.newInstance();
+    Transformer tx   = tfactory.newTransformer();
+    DOMSource source = new DOMSource(document);
+    DOMResult result = new DOMResult();
+    tx.transform(source,result);
+    return (Document)result.getNode();
+  }
+
   private Properties readProperties(Config cfg, Node node) throws XPathExpressionException {
     XPath xpath = cfg.getXPath();
     NodeList props = (NodeList) xpath.evaluate("property", node, XPathConstants.NODESET);
@@ -422,6 +473,8 @@ public class CoreContainer
   
   private volatile boolean isShutDown = false;
 
+  private Config cfg;
+  
   public boolean isShutDown() {
     return isShutDown;
   }
@@ -753,6 +806,9 @@ public class CoreContainer
     }
     
     SolrCore newCore = core.reload(solrLoader);
+    // keep core to orig name link
+    String origName = coreToOrigName.remove(core);
+    coreToOrigName.put(newCore, origName);
     register(name, newCore, false);
   }
 
@@ -794,11 +850,28 @@ public class CoreContainer
   /** Removes and returns registered core w/o decrementing it's reference count */
   public SolrCore remove( String name ) {
     name = checkDefault(name);    
+
     synchronized(cores) {
-      return cores.remove( name );
+      SolrCore core = cores.remove( name );
+      coreToOrigName.remove(core);
+      return core;
     }
+
   }
 
+  public void rename(String name, String toName) {
+    SolrCore core = getCore(name);
+    if (core != null) {
+      register(toName, core, false);
+      name = checkDefault(name);    
+
+      synchronized(cores) {
+        cores.remove(name);
+      }
+
+      core.close();
+    }
+  }
   
   /** Gets a core by name and increase its refcount.
    * @see SolrCore#open() 
@@ -888,6 +961,8 @@ public class CoreContainer
   /** Persists the cores config file in a user provided file. */
   public void persistFile(File file) {
     log.info("Persisting cores config to " + (file == null ? configFile : file));
+
+    
     // <solr attrib="value">
     Map<String,String> rootSolrAttribs = new HashMap<String,String>();
     if (libDir != null) rootSolrAttribs.put("sharedLib", libDir);
@@ -895,15 +970,21 @@ public class CoreContainer
     
     // <solr attrib="value"> <cores attrib="value">
     Map<String,String> coresAttribs = new HashMap<String,String>();
-    coresAttribs.put("adminPath", adminPath);
+    addCoresAttrib(coresAttribs, "adminPath", this.adminPath, null);
-    if (adminHandler != null) coresAttribs.put("adminHandler", adminHandler);
+    addCoresAttrib(coresAttribs, "adminHandler", this.adminHandler, null);
-    if (shareSchema) coresAttribs.put("shareSchema", "true");
+    addCoresAttrib(coresAttribs, "shareSchema",
+        Boolean.toString(this.shareSchema),
+        Boolean.toString(DEFAULT_SHARE_SCHEMA));
+    addCoresAttrib(coresAttribs, "host", this.host, null);
+
     if (!defaultCoreName.equals("")) coresAttribs.put("defaultCoreName",
         defaultCoreName);
-    if (host != null) coresAttribs.put("host", host);
+    
-    if (hostPort != null) coresAttribs.put("hostPort", hostPort);
+    addCoresAttrib(coresAttribs, "hostPort", this.hostPort, DEFAULT_HOST_PORT);
-    if (zkClientTimeout != null) coresAttribs.put("zkClientTimeout", Integer.toString(zkClientTimeout));
+    addCoresAttrib(coresAttribs, "zkClientTimeout",
-    if (hostContext != null) coresAttribs.put("hostContext", hostContext);
+        intToString(this.zkClientTimeout),
+        Integer.toString(DEFAULT_ZK_CLIENT_TIMEOUT));
+    addCoresAttrib(coresAttribs, "hostContext", this.hostContext, DEFAULT_HOST_CONTEXT);
     
     List<SolrCoreXMLDef> solrCoreXMLDefs = new ArrayList<SolrCoreXMLDef>();
     
@@ -912,36 +993,91 @@ public class CoreContainer
         Map<String,String> coreAttribs = new HashMap<String,String>();
         CoreDescriptor dcore = solrCore.getCoreDescriptor();
 
-        coreAttribs.put("name", dcore.name.equals("") ? defaultCoreName
+        String coreName = dcore.name.equals("") ? defaultCoreName
-            : dcore.name);
+            : dcore.name;
-        coreAttribs.put("instanceDir", dcore.getInstanceDir());
+        
-        // write config (if not default)
+        Node coreNode = null;
-        String opt = dcore.getConfigName();
+        
-        if (opt != null && !opt.equals(dcore.getDefaultConfigName())) {
+        if (cfg != null) {
-          coreAttribs.put("config", opt);
+          NodeList nodes = (NodeList) cfg.evaluate("solr/cores/core",
+              XPathConstants.NODESET);
+          
+          String origCoreName = coreToOrigName.get(solrCore);
+
+          if (origCoreName == null) {
+            origCoreName = coreName;
+          }
+          
+          // look for an existing node
+          
+          // first look for an exact match
+          for (int i = 0; i < nodes.getLength(); i++) {
+            Node node = nodes.item(i);
+            
+            String name = DOMUtil.getAttr(node, "name", null);
+            if (origCoreName.equals(name)) {
+              coreNode = node;
+              if (coreName.equals(origCoreName)) {
+                coreName = name;
+              }
+              break;
+            }
+          }
+          
+          if (coreNode == null) {
+            // see if we match with substitution
+            for (int i = 0; i < nodes.getLength(); i++) {
+              Node node = nodes.item(i);
+              String name = DOMUtil.getAttr(node, "name", null);
+              if (origCoreName.equals(DOMUtil.substituteProperty(name,
+                  loader.getCoreProperties()))) {
+                coreNode = node;
+                if (coreName.equals(origCoreName)) {
+                  coreName = name;
+                }
+                break;
+              }
+            }
+          }
         }
-        // write schema (if not default)
+
-        opt = dcore.getSchemaName();
+        coreAttribs.put("name", coreName);
-        if (opt != null && !opt.equals(dcore.getDefaultSchemaName())) {
+        
-          coreAttribs.put("schema", opt);
+        String instanceDir = dcore.getInstanceDir();
-        }
+        addCoreProperty(coreAttribs, coreNode, "instanceDir", instanceDir, null);
-        opt = dcore.getPropertiesName();
+        
-        if (opt != null) {
+        // write config 
-          coreAttribs.put("properties", opt);
+        String configName = dcore.getConfigName();
-        }
+        addCoreProperty(coreAttribs, coreNode, "conf", configName, dcore.getDefaultConfigName());
-        opt = dcore.dataDir;
+        
-        if (opt != null) coreAttribs.put("dataDir", opt);
+        // write schema
+        String schema = dcore.getSchemaName();
+        addCoreProperty(coreAttribs, coreNode, "schema", schema, dcore.getDefaultSchemaName());
+        
+        String dataDir = dcore.dataDir;
+        addCoreProperty(coreAttribs, coreNode, "dataDir", dataDir, null);
         
         CloudDescriptor cd = dcore.getCloudDescriptor();
+        String shard = null;
         if (cd != null) {
-          opt = cd.getShardId();
+          shard = cd.getShardId();
-          if (opt != null) coreAttribs.put("shard", opt);
+        }
-          // only write out the collection name if it's not the default (the
+        addCoreProperty(coreAttribs, coreNode, "shard", shard, null);
-          // core
+        
-          // name)
+        String collection = null;
-          opt = cd.getCollectionName();
+        // only write out the collection name if it's not the default (the
-          if (opt != null && !opt.equals(dcore.name)) coreAttribs.put(
+        // core
-              "collection", opt);
+        // name)
+        if (cd != null) {
+          collection = cd.getCollectionName();
+        }
+        
+        addCoreProperty(coreAttribs, coreNode, "collection", collection, dcore.name);
+        
+        // we don't try and preserve sys prop defs in these
+        String opt = dcore.getPropertiesName();
+        if (opt != null) {
+          coreAttribs.put("properties", opt);
         }
         
         SolrCoreXMLDef solrCoreXMLDef = new SolrCoreXMLDef();
@@ -960,6 +1096,52 @@ public class CoreContainer
     }
   }
 
+  private String intToString(Integer integer) {
+    if (integer == null) return null;
+    return Integer.toString(integer);
+  }
+
+  private void addCoresAttrib(Map<String,String> coresAttribs, String attribName, String attribValue, String defaultValue) {
+    if (cfg == null) {
+      coresAttribs.put(attribName, attribValue);
+      return;
+    }
+    
+    if (attribValue != null) {
+      String rawValue = cfg.get("solr/cores/@" + attribName, null);
+      if (rawValue == null && defaultValue != null && attribValue.equals(defaultValue)) return;
+      if (attribValue.equals(DOMUtil.substituteProperty(rawValue, loader.getCoreProperties()))) {
+        coresAttribs.put(attribName, rawValue);
+      } else {
+        coresAttribs.put(attribName, attribValue);
+      }
+    }
+  }
+
+  private void addCoreProperty(Map<String,String> coreAttribs, Node node, String name,
+      String value, String defaultValue) {
+    if (node == null) {
+      coreAttribs.put(name, value);
+      return;
+    }
+    
+    String attribValue = null;
+    if (node != null) {
+      String rawAttribValue = DOMUtil.getAttr(node, name, null);
+      if (value == null) {
+        coreAttribs.put(name, rawAttribValue);
+        return;
+      }
+      if (rawAttribValue == null && defaultValue != null && value.equals(defaultValue)) return;
+      if (rawAttribValue != null && value.equals(DOMUtil.substituteProperty(rawAttribValue, loader.getCoreProperties()))){
+        attribValue = rawAttribValue;
+      }
+    }
+    if (attribValue != null) {
+      coreAttribs.put(name, attribValue);
+    }
+  }
+
 
   public String getSolrHome() {
     return solrHome;

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/TestSolrProperties.java

@@ -210,12 +210,13 @@ public class TestSolrProperties extends LuceneTestCase {
       fis.close();
       fis = new FileInputStream(new File(solrXml.getParent(), "solr-persist.xml"));
       String solrPersistXml = IOUtils.toString(fis);
+      //System.out.println("xml:" + solrPersistXml);
       assertTrue("\"/solr/cores[@defaultCoreName='core0']\" doesn't match in:\n" + solrPersistXml,
                  exists("/solr/cores[@defaultCoreName='core0']", document));
       assertTrue("\"/solr/cores[@host='127.0.0.1']\" doesn't match in:\n" + solrPersistXml,
                  exists("/solr/cores[@host='127.0.0.1']", document));
-      assertTrue("\"/solr/cores[@hostPort='8983']\" doesn't match in:\n" + solrPersistXml,
+      assertTrue("\"/solr/cores[@hostPort='${hostPort:8983}']\" doesn't match in:\n" + solrPersistXml,
-                 exists("/solr/cores[@hostPort='8983']", document));
+                 exists("/solr/cores[@hostPort='${hostPort:8983}']", document));
       assertTrue("\"/solr/cores[@zkClientTimeout='8000']\" doesn't match in:\n" + solrPersistXml,
                  exists("/solr/cores[@zkClientTimeout='8000']", document));
       assertTrue("\"/solr/cores[@hostContext='solr']\" doesn't match in:\n" + solrPersistXml,
@@ -228,10 +229,18 @@ public class TestSolrProperties extends LuceneTestCase {
     CoreAdminRequest.renameCore(name, "renamed_core", coreadmin);
     mcr = CoreAdminRequest.persist("solr-persist.xml", getRenamedSolrAdmin());
     
+//    fis = new FileInputStream(new File(solrXml.getParent(), "solr-persist.xml"));
+//    String solrPersistXml = IOUtils.toString(fis);
+//    System.out.println("xml:" + solrPersistXml);
+//    fis.close();
+    
     fis = new FileInputStream(new File(solrXml.getParent(), "solr-persist.xml"));
     try {
       Document document = builder.parse(fis);
       assertTrue(exists("/solr/cores/core[@name='renamed_core']", document));
+      assertTrue(exists("/solr/cores/core[@instanceDir='${theInstanceDir:./}']", document));
+      assertTrue(exists("/solr/cores/core[@collection='${collection:acollection}']", document));
+      
     } finally {
       fis.close();
     }