SOLR-17864: Provide backwards support for "<str name="hideStackTrace">${solr.hideStackTrace:false}</str>"

changelog/unreleased/SOLR-17864-legacy-sysprop-fallback-in-config-substitution.yml

@@ -0,0 +1,11 @@
+title: >
+  A solr.xml or solrconfig.xml property-substitution token (${...}) that still references a
+  legacy/deprecated system property name now falls back to the current property's value if that's
+  been set, instead of silently ignoring it. Previously, upgrading and setting only the new
+  property name had no effect unless the config file's own ${...} token was also updated by hand.
+type: fixed
+authors:
+  - name: Eric Pugh
+links:
+  - name: SOLR-17864
+    url: https://issues.apache.org/jira/browse/SOLR-17864

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

@@ -59,10 +59,10 @@
 import org.apache.lucene.util.ResourceLoaderAware;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.SolrClassLoader;
+import org.apache.solr.common.util.DeprecationLog;
 import org.apache.solr.common.util.EnvUtils;
 import org.apache.solr.handler.component.SearchComponent;
 import org.apache.solr.handler.component.ShardHandlerFactory;
-import org.apache.solr.logging.DeprecationLog;
 import org.apache.solr.pkg.PackageListeningClassLoader;
 import org.apache.solr.pkg.SolrPackageLoader;
 import org.apache.solr.request.SolrRequestHandler;

solr/core/src/java/org/apache/solr/schema/ScalarQuantizedDenseVectorField.java

@@ -25,7 +25,7 @@
 import org.apache.lucene.index.VectorEncoding;
 import org.apache.lucene.index.VectorSimilarityFunction;
 import org.apache.solr.common.SolrException;
-import org.apache.solr.logging.DeprecationLog;
+import org.apache.solr.common.util.DeprecationLog;
 
 public class ScalarQuantizedDenseVectorField extends DenseVectorField {
   public static final String BITS_PARAM = "bits"; //

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

@@ -20,6 +20,7 @@
 import java.util.stream.Collectors;
 import org.apache.solr.client.api.model.ErrorInfo;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.EnvUtils;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.slf4j.Logger;
@@ -28,9 +29,10 @@
 public class ResponseUtils {
   private ResponseUtils() {}
 
-  // System property to use if the Solr core does not exist or solr.hideStackTrace is not
-  // configured. (i.e.: a lot of unit test).
-  private static final boolean SYSTEM_HIDE_STACK_TRACES = Boolean.getBoolean("solr.hideStackTrace");
+  // Default used when solr.responses.stacktrace.enabled is not explicitly set (e.g. in many unit
+  // tests).
+  private static final boolean STACKTRACE_ENABLED =
+      EnvUtils.getPropertyAsBool("solr.responses.stacktrace.enabled", true);
 
   /**
    * Adds the given Throwable's message to the given NamedList.
@@ -61,7 +63,7 @@ public static int getErrorInfo(Throwable ex, NamedList<Object> info, Logger log)
    * <p>Status codes less than 100 are adjusted to be 500.
    *
    * <p>Stack trace will not be output if hideTrace=true OR system property
-   * solr.hideStackTrace=true.
+   * solr.responses.stacktrace.enabled=false.
    *
    * @see #getTypedErrorInfo(Throwable, Logger)
    */
@@ -160,7 +162,7 @@ public static ErrorInfo getTypedErrorInfo(Throwable ex, Logger log) {
    * <p>Status codes less than 100 are adjusted to be 500.
    *
    * <p>Stack trace will not be output if hideTrace=true OR system property
-   * solr.hideStackTrace=true.
+   * solr.responses.stacktrace.enabled=false.
    *
    * @see #getErrorInfo(Throwable, NamedList, Logger)
    */
@@ -231,6 +233,6 @@ public static ErrorInfo getTypedErrorInfo(Throwable ex, Logger log, boolean hide
   }
 
   private static boolean hideStackTrace(final boolean hideTrace) {
-    return hideTrace || SYSTEM_HIDE_STACK_TRACES;
+    return hideTrace || !STACKTRACE_ENABLED;
   }
 }

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java

@@ -40,7 +40,7 @@ public class HideStackTraceTest extends SolrTestCaseJ4 {
   @BeforeClass
   public static void setupSolrHome() throws Exception {
 
-    System.setProperty("solr.hideStackTrace", "true");
+    System.setProperty("solr.responses.stacktrace.enabled", "false");
 
     Path configSet = createTempDir("configSet");
     copyMinConf(configSet);

solr/packaging/test/test_start_solr.bats

@@ -88,6 +88,13 @@ teardown() {
   assert_file_contains "${SOLR_LOGS_DIR}/solr.log" 'Deprecated system property disable.config.edit has been replaced by solr.api.config.edit.enabled'
 }
 
+@test "solr.xml referencing a deprecated system property logs a deprecation warning" {
+  # solr.xml still contains the legacy ${solr.hideStackTrace:false} token; setting only the
+  # replacement property here should still resolve correctly and log a warning. See SOLR-17864.
+  solr start -Dsolr.responses.stacktrace.enabled=false
+  assert_file_contains "${SOLR_LOGS_DIR}/solr.log" 'A config file still references the deprecated system property solr.hideStackTrace; it was replaced by solr.responses.stacktrace.enabled'
+}
+
 @test "start with custom jetty options" {
   export ENABLE_REMOTE_JMX_OPTS=true
   export RMI_PORT=65535 # need to make sure we don't exceed port range so hard code it

solr/solr-ref-guide/modules/configuration-guide/pages/solr-properties.adoc

@@ -90,7 +90,7 @@ NOTE: Properties marked with "!" indicate inverted meaning between pre Solr 10 a
 
 |solr.responses.hidden.sys.props|solr.hiddenSysProps||Defines system properties that are hidden in responses.
 
-|solr.responses.stacktrace.enabled|!solr.hideStackTrace|false|Controls whether stack traces are included in responses. When set to `true`, stack traces are included in responses.
+|solr.responses.stacktrace.enabled|!solr.hideStackTrace|true|Controls whether stack traces are included in responses. When set to `false`, stack traces are omitted from responses.
 
 |solr.security.auth.basicauth.credentials|basicauth||Defines basic authentication credentials.
 

solr/core/src/java/org/apache/solr/logging/DeprecationLog.java → solr/solrj/src/java/org/apache/solr/common/util/DeprecationLog.java

@@ -15,11 +15,12 @@
  * limitations under the License.
  */
 
-package org.apache.solr.logging;
+package org.apache.solr.common.util;
 
 import java.lang.invoke.MethodHandles;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -41,11 +42,25 @@ public class DeprecationLog {
    * @return true if logged
    */
   public static boolean log(String featureId, String message) {
-    if (alreadyLogged.putIfAbsent(featureId, message) != null) {
+    return log(featureId, () -> message);
+  }
+
+  /**
+   * Like {@link #log(String, String)}, but only builds the message if this is the first time {@code
+   * featureId} is logged. Use this when building the message isn't free and the call site runs
+   * often (e.g. on every property lookup).
+   *
+   * @return true if logged
+   */
+  public static boolean log(String featureId, Supplier<String> message) {
+    if (alreadyLogged.containsKey(featureId)) {
+      return false;
+    }
+    if (alreadyLogged.putIfAbsent(featureId, featureId) != null) {
       return false;
     }
     Logger log = LoggerFactory.getLogger(LOG_PREFIX + featureId);
-    log.warn(message);
+    log.warn(message.get());
     return true;
   }
 }

solr/solrj/src/java/org/apache/solr/common/util/EnvUtils.java

@@ -47,8 +47,14 @@ public class EnvUtils {
   /** Maps ENV keys to sys prop keys for special/custom mappings */
   private static final Map<String, String> CUSTOM_MAPPINGS = new HashMap<>();
 
+  /**
+   * Maps a legacy/deprecated sys prop key (dot-separated form) to the current property it was
+   * replaced by, and whether the replacement is boolean-inverted relative to the legacy property.
+   */
+  private record DeprecatedMapping(String currentName, boolean inverted) {}
+
   /** Maps deprecated sys prop keys to current sys prop keys with special/custom mappings */
-  private static final Map<String, String> DEPRECATED_MAPPINGS = new HashMap<>();
+  private static final Map<String, DeprecatedMapping> DEPRECATED_MAPPINGS = new HashMap<>();
 
   private static final Map<String, String> camelCaseToDotsMap = new ConcurrentHashMap<>();
 
@@ -74,8 +80,14 @@ public class EnvUtils {
         for (String key : props.stringPropertyNames()) {
           CUSTOM_MAPPINGS.put(key, props.getProperty(key));
         }
-        for (String key : deprecatedProps.stringPropertyNames()) {
-          DEPRECATED_MAPPINGS.put(camelCaseToDotSeparated(deprecatedProps.getProperty(key)), key);
+        for (String currentName : deprecatedProps.stringPropertyNames()) {
+          String legacyName = deprecatedProps.getProperty(currentName);
+          boolean inverted = legacyName.startsWith("!");
+          if (inverted) {
+            legacyName = legacyName.substring(1);
+          }
+          DEPRECATED_MAPPINGS.put(
+              camelCaseToDotSeparated(legacyName), new DeprecatedMapping(currentName, inverted));
         }
         init(false, System.getenv(), System.getProperties());
       }
@@ -108,24 +120,68 @@ public static String getProperty(String key) {
    * @param defaultValue fallback value if property is not found
    */
   public static String getProperty(String key, String defaultValue) {
-    String value = getPropertyWithCamelCaseFallback(key);
+    String value = resolvePropertyValue(key);
     return value != null ? value : defaultValue;
   }
 
   /**
-   * Get a property from given key or an alias key converted from CamelCase to dot separated.
+   * Resolves {@code key} by trying, in order: the key as given; the key converted from CamelCase to
+   * dot-separated; and -- if {@code key} is itself a legacy/deprecated property name -- the current
+   * property it was replaced by. See {@link #resolveViaDeprecatedMapping} for why that last
+   * fallback is needed.
    *
-   * @return property value or value of dot-separated alias key or null if not found
+   * @return the resolved value, or null if none of the above are found
    */
-  private static String getPropertyWithCamelCaseFallback(String key) {
+  private static String resolvePropertyValue(String key) {
     String value = System.getProperty(key);
     if (value != null) {
       return value;
-    } else {
-      // Figure out if string is CamelCase and convert to dot separated
-      String altKey = camelCaseToDotSeparated(key);
-      return System.getProperty(altKey);
     }
+    // Figure out if string is CamelCase and convert to dot separated
+    String altKey = camelCaseToDotSeparated(key);
+    value = System.getProperty(altKey);
+    if (value != null) {
+      return value;
+    }
+    return resolveViaDeprecatedMapping(key, altKey);
+  }
+
+  /**
+   * If {@code dotKey} is a known legacy/deprecated property name, returns the value of the current
+   * property it was replaced by (inverted, if the mapping is boolean-inverted), or null if that
+   * current property hasn't been set either.
+   *
+   * <p>This matters for config files (e.g. {@code solr.xml}) that still contain a {@code
+   * ${legacyName:default}} substitution token from before a property was renamed: without this,
+   * setting only the new property name would silently have no effect on that token, since nothing
+   * else ever rewrites the token's text. See SOLR-17864.
+   *
+   * @param key the property key as originally looked up (used only for the deprecation message)
+   * @param dotKey {@code key} converted to dot-separated form; this is what {@code
+   *     DEPRECATED_MAPPINGS} is keyed by
+   */
+  private static String resolveViaDeprecatedMapping(String key, String dotKey) {
+    DeprecatedMapping mapping = DEPRECATED_MAPPINGS.get(dotKey);
+    if (mapping == null) {
+      return null;
+    }
+    String newValue = System.getProperty(mapping.currentName());
+    if (newValue == null) {
+      return null;
+    }
+    DeprecationLog.log(
+        dotKey,
+        () ->
+            "A config file still references the deprecated system property "
+                + key
+                + "; it was replaced by "
+                + mapping.currentName()
+                + ". Support for the old property will be removed in a future version of Solr.");
+    return mapping.inverted() ? invertBooleanString(newValue) : newValue;
+  }
+
+  private static String invertBooleanString(String value) {
+    return String.valueOf(!Boolean.parseBoolean(value));
   }
 
   private static String camelCaseToDotSeparated(String key) {
@@ -218,26 +274,24 @@ static synchronized void init(
 
     for (String deprecatedKey : sysProperties.stringPropertyNames()) {
       var dotKey = camelCaseToDotSeparated(deprecatedKey);
-      if (DEPRECATED_MAPPINGS.containsKey(dotKey)
-          || DEPRECATED_MAPPINGS.containsKey("!" + dotKey)) {
-        applyDeprecatedPropertyMapping(deprecatedKey, dotKey, sysProperties);
+      DeprecatedMapping mapping = DEPRECATED_MAPPINGS.get(dotKey);
+      if (mapping != null) {
+        applyDeprecatedPropertyMapping(deprecatedKey, mapping, sysProperties);
       }
     }
   }
 
   private static void applyDeprecatedPropertyMapping(
-      String deprecatedKey, String lookupKey, Properties sysProperties) {
-    var newPropName =
-        DEPRECATED_MAPPINGS.getOrDefault(lookupKey, DEPRECATED_MAPPINGS.get("!" + lookupKey));
+      String deprecatedKey, DeprecatedMapping mapping, Properties sysProperties) {
     var newValue =
-        DEPRECATED_MAPPINGS.containsKey(lookupKey)
-            ? sysProperties.getProperty(deprecatedKey)
-            : String.valueOf(!Boolean.parseBoolean(sysProperties.getProperty(deprecatedKey)));
+        mapping.inverted()
+            ? invertBooleanString(sysProperties.getProperty(deprecatedKey))
+            : sysProperties.getProperty(deprecatedKey);
     log.warn(
         "Deprecated system property {} has been replaced by {}. Support for the old property will be removed in a future version of Solr.",
         deprecatedKey,
-        newPropName);
-    setProperty(newPropName, newValue);
+        mapping.currentName());
+    setProperty(mapping.currentName(), newValue);
   }
 
   protected static String envNameToSyspropName(String envName) {

solr/solrj/src/test/org/apache/solr/common/util/EnvUtilsTest.java

@@ -133,6 +133,34 @@ public void deprecatedCamelCaseInvertedPropertyIsTranslatedAndValueIsFlipped() {
     assertFalse(EnvUtils.getPropertyAsBool("solr.index.replication.fingerprint.enabled"));
   }
 
+  @Test
+  public void legacyPropertyNameLookupFallsBackToCurrentPropertyWhenOnlyThatIsSet() {
+    // Simulates an un-migrated ${bootstrap_confdir:...} token in a user's own solr.xml: nobody
+    // ever sets the legacy name as a real system property, only the current one -- e.g. because
+    // the user followed current docs after upgrading, without touching their old config file.
+    EnvUtils.setProperty("solr.configset.bootstrap.confdir", "/opt/solr/configsets/mine");
+    try {
+      assertEquals(
+          "/opt/solr/configsets/mine",
+          EnvUtils.getProperty("bootstrap_confdir", "should-not-see-this-default"));
+    } finally {
+      System.clearProperty("solr.configset.bootstrap.confdir");
+    }
+  }
+
+  @Test
+  public void legacyInvertedPropertyNameLookupFallsBackToCurrentPropertyFlipped() {
+    // Same gap, but for a boolean-inverted mapping: an un-migrated ${solr.hideStackTrace:false}
+    // token should honor solr.responses.stacktrace.enabled once that's set, with the value
+    // correctly flipped back to the legacy property's sense.
+    EnvUtils.setProperty("solr.responses.stacktrace.enabled", "true");
+    try {
+      assertFalse(EnvUtils.getPropertyAsBool("solr.hideStackTrace", true));
+    } finally {
+      System.clearProperty("solr.responses.stacktrace.enabled");
+    }
+  }
+
   @Test
   public void testFlippingDisabledToEnabledPropertyName() {