Copilot commented on code in PR #4067:
URL: https://github.com/apache/solr/pull/4067#discussion_r2710283043
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3614,32 +3614,112 @@ public void cleanupOldIndexDirectories(boolean reload)
{
}
}
+ /**
+ * Parses implicit plugin definitions from a JSON map and converts them to
PluginInfo objects.
+ *
+ * @param implicitPluginsInfo the parsed JSON map containing plugin
definitions
+ * @return an unmodifiable list of PluginInfo objects for request handlers
+ * @throws NullPointerException if requestHandlers section is missing
+ */
+ static List<PluginInfo> parseImplicitPlugins(Map<String, ?>
implicitPluginsInfo) {
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> requestHandlers =
+ (Map<String, Map<String, Object>>)
implicitPluginsInfo.get(SolrRequestHandler.TYPE);
+
+ if (requestHandlers == null) {
+ throw new NullPointerException("No requestHandler section found in
implicit plugins");
+ }
+
+ List<PluginInfo> implicits = new ArrayList<>(requestHandlers.size());
+ for (Map.Entry<String, Map<String, Object>> entry :
requestHandlers.entrySet()) {
+ Map<String, Object> info = entry.getValue();
+ info.put(CommonParams.NAME, entry.getKey());
+ implicits.add(new PluginInfo(SolrRequestHandler.TYPE, info));
+ }
+ return Collections.unmodifiableList(implicits);
+ }
+
private static final class ImplicitHolder {
private ImplicitHolder() {}
- private static final List<PluginInfo> INSTANCE;
+ private static volatile List<PluginInfo> INSTANCE = null;
- static {
+ static List<PluginInfo> getInstance(SolrCore core) {
+ if (INSTANCE == null) {
+ synchronized (ImplicitHolder.class) {
+ if (INSTANCE == null) {
+ INSTANCE = loadImplicitPlugins(core);
+ }
+ }
+ }
+ return INSTANCE;
+ }
+
+ private static List<PluginInfo> loadImplicitPlugins(SolrCore core) {
+ // Check for custom implicit plugins file from solr.xml (global
configuration)
+ String customPluginsFile =
core.getCoreContainer().getConfig().getImplicitPluginsFile();
+ if (customPluginsFile != null && !customPluginsFile.isEmpty()) {
+ try {
+ // Resolve path similar to solr.xml - support both absolute and
relative paths
+ Path customPluginsPath = Path.of(customPluginsFile);
+ if (!customPluginsPath.isAbsolute()) {
+ // Resolve relative paths against SOLR_HOME
+ Path solrHome = core.getCoreContainer().getSolrHome();
+ customPluginsPath = solrHome.resolve(customPluginsFile);
+ }
+
+ if (!Files.exists(customPluginsPath)) {
+ log.warn(
+ "Custom implicit plugins file does not exist: {} (from
solr.xml). Falling back to default.",
+ customPluginsPath);
+ } else {
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Loading custom implicit plugins from {} (configured in
solr.xml)",
+ customPluginsPath);
+ }
+
+ // Load the custom plugins file directly from the filesystem
+ try (InputStream is = Files.newInputStream(customPluginsPath)) {
+ @SuppressWarnings("unchecked")
+ Map<String, ?> implicitPluginsInfo = (Map<String, ?>)
Utils.fromJSON(is);
+ List<PluginInfo> customHandlers =
parseImplicitPlugins(implicitPluginsInfo);
+
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Loaded {} custom implicit handlers from {}",
+ customHandlers.size(),
+ customPluginsPath);
+ }
+ return customHandlers;
+ }
+ }
+ } catch (NullPointerException e) {
+ log.warn(
+ "No requestHandler section found in custom implicit plugins
file: {} (from solr.xml)",
+ customPluginsFile);
+ } catch (Exception e) {
+ log.warn(
+ "Failed to load custom implicit plugins file: {} (from
solr.xml). Falling back to default.",
+ customPluginsFile,
Review Comment:
The error message uses the string representation of the customPluginsFile
variable instead of the actual path that was attempted to be loaded
(customPluginsPath). This makes debugging harder since the user sees the input
value, not the resolved path that was actually checked. Consider using
customPluginsPath.toString() in the log messages.
```suggestion
customPluginsPath.toString());
} catch (Exception e) {
log.warn(
"Failed to load custom implicit plugins file: {} (from
solr.xml). Falling back to default.",
customPluginsPath.toString(),
```
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3614,32 +3614,112 @@ public void cleanupOldIndexDirectories(boolean reload)
{
}
}
+ /**
+ * Parses implicit plugin definitions from a JSON map and converts them to
PluginInfo objects.
+ *
+ * @param implicitPluginsInfo the parsed JSON map containing plugin
definitions
+ * @return an unmodifiable list of PluginInfo objects for request handlers
+ * @throws NullPointerException if requestHandlers section is missing
+ */
+ static List<PluginInfo> parseImplicitPlugins(Map<String, ?>
implicitPluginsInfo) {
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> requestHandlers =
+ (Map<String, Map<String, Object>>)
implicitPluginsInfo.get(SolrRequestHandler.TYPE);
+
+ if (requestHandlers == null) {
+ throw new NullPointerException("No requestHandler section found in
implicit plugins");
+ }
+
+ List<PluginInfo> implicits = new ArrayList<>(requestHandlers.size());
+ for (Map.Entry<String, Map<String, Object>> entry :
requestHandlers.entrySet()) {
+ Map<String, Object> info = entry.getValue();
+ info.put(CommonParams.NAME, entry.getKey());
+ implicits.add(new PluginInfo(SolrRequestHandler.TYPE, info));
+ }
+ return Collections.unmodifiableList(implicits);
+ }
+
private static final class ImplicitHolder {
private ImplicitHolder() {}
- private static final List<PluginInfo> INSTANCE;
+ private static volatile List<PluginInfo> INSTANCE = null;
- static {
+ static List<PluginInfo> getInstance(SolrCore core) {
+ if (INSTANCE == null) {
+ synchronized (ImplicitHolder.class) {
+ if (INSTANCE == null) {
+ INSTANCE = loadImplicitPlugins(core);
+ }
+ }
+ }
+ return INSTANCE;
+ }
+
+ private static List<PluginInfo> loadImplicitPlugins(SolrCore core) {
+ // Check for custom implicit plugins file from solr.xml (global
configuration)
+ String customPluginsFile =
core.getCoreContainer().getConfig().getImplicitPluginsFile();
+ if (customPluginsFile != null && !customPluginsFile.isEmpty()) {
+ try {
+ // Resolve path similar to solr.xml - support both absolute and
relative paths
+ Path customPluginsPath = Path.of(customPluginsFile);
+ if (!customPluginsPath.isAbsolute()) {
+ // Resolve relative paths against SOLR_HOME
+ Path solrHome = core.getCoreContainer().getSolrHome();
+ customPluginsPath = solrHome.resolve(customPluginsFile);
+ }
+
Review Comment:
The custom implicit plugins file path is not validated using the
CoreContainer.assertPathAllowed() security check. While solr.xml is a
privileged configuration file, it would be more consistent with Solr's security
model to validate that the resolved customPluginsPath is within allowed paths,
especially since this code reads from the filesystem. Consider adding a call to
core.getCoreContainer().assertPathAllowed(customPluginsPath) after resolving
the path but before checking if it exists.
```suggestion
// Ensure the resolved path is within allowed locations
core.getCoreContainer().assertPathAllowed(customPluginsPath);
```
##########
solr/packaging/test/test_start_solr.bats:
##########
@@ -107,3 +110,36 @@ teardown() {
# Verify the techproducts configset was uploaded
config_exists "techproducts"
}
+
+@test "start with custom implicit plugins" {
+ # Create custom implicit plugins file inline
+ local custom_plugins_file="${SOLR_HOME}/custom-implicit-plugins.json"
+
+ cat > "${custom_plugins_file}" <<EOF
+ {
+ "requestHandler": {
+ "/custom-select": {
+ "useParams":"_UPDATE",
+ "class": "solr.SearchHandler"
+ }
+ }
+ }
+EOF
+
+ # Start Solr with custom implicit plugins configuration
+ solr start -Dsolr.implicitPluginsFile=custom-implicit-plugins.json
+ solr assert --started http://localhost:${SOLR_PORT} --timeout 5000
+
+
+ # Create a collection to test the custom handlers
+ solr create -c custom_plugins_test
+
+ # Verify the custom handler is available and default handlers like /update
are not
+ run curl -s
"http://localhost:${SOLR_PORT}/solr/custom_plugins_test/custom-select";
+ assert_output --partial '"status":0'
+
+ run curl -s "http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update";
+ assert_output --partial 'Searching for Solr'
Review Comment:
The test verifies that the /update endpoint is not available by checking for
"Searching for Solr" in the response. This appears to be checking for a 404
error page. However, this is fragile as the error page content could change.
Consider checking the HTTP status code directly (e.g., using curl's -w flag) or
checking for a more specific error indicator.
```suggestion
run curl -s -o /dev/null -w "%{http_code}"
"http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update";
assert_output "404"
```
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3614,32 +3614,112 @@ public void cleanupOldIndexDirectories(boolean reload)
{
}
}
+ /**
+ * Parses implicit plugin definitions from a JSON map and converts them to
PluginInfo objects.
+ *
+ * @param implicitPluginsInfo the parsed JSON map containing plugin
definitions
+ * @return an unmodifiable list of PluginInfo objects for request handlers
+ * @throws NullPointerException if requestHandlers section is missing
+ */
+ static List<PluginInfo> parseImplicitPlugins(Map<String, ?>
implicitPluginsInfo) {
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> requestHandlers =
+ (Map<String, Map<String, Object>>)
implicitPluginsInfo.get(SolrRequestHandler.TYPE);
+
+ if (requestHandlers == null) {
+ throw new NullPointerException("No requestHandler section found in
implicit plugins");
Review Comment:
Using NullPointerException for control flow is not a best practice. The
parseImplicitPlugins method throws NullPointerException when the requestHandler
section is missing, which is then caught and logged. Consider using a more
descriptive custom exception or IllegalArgumentException, which better conveys
that this is a validation error rather than an unexpected null reference.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc:
##########
@@ -339,6 +340,16 @@ or via the `SOLR_HIDDEN_SYS_PROPS` environment variable.
By default, Solr will hide all basicAuth, AWS, ZK or SSL secret sysProps. It
will also hide any sysProp that contains
"password" or "secret" in it.
+`implicitPluginsFile`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: none
+|===
++
+The path relative to `$SOLR_HOME` for a custom implicit plugins file that lets
you control exactly which endpoints are registerd for all cores.
Review Comment:
The documentation states the path is "relative to $SOLR_HOME", but looking
at the implementation in SolrCore.java (lines 3664-3668), the code also
supports absolute paths. The documentation should clarify that both absolute
and relative paths are supported.
```suggestion
The path to a custom implicit plugins file that lets you control exactly
which endpoints are registered for all cores. If the path is relative, it is
resolved against `$SOLR_HOME`; absolute paths are also supported.
```
##########
solr/core/src/test/org/apache/solr/core/SolrCoreTest.java:
##########
@@ -146,6 +151,62 @@ public void testImplicitPlugins() {
assertEquals("wrong number of implicit handlers", ihCount,
implicitHandlers.size());
}
+ @Test
+ public void testCustomImplicitPlugins() throws Exception {
+ // Test that the custom implicit plugins file can be loaded and parsed
into PluginInfo objects
+ String customPluginsPath = TEST_HOME() + "/custom-implicit-plugins.json";
+ Path pluginsFile = Paths.get(customPluginsPath);
+
+ // Verify file exists
+ assertTrue(
+ "Custom implicit plugins file should exist: " + customPluginsPath,
+ Files.exists(pluginsFile));
+
+ // Load and parse the custom plugins file
+ try (InputStream is = Files.newInputStream(pluginsFile)) {
+ @SuppressWarnings("unchecked")
+ Map<String, ?> implicitPluginsInfo = (Map<String, ?>) Utils.fromJSON(is);
+
+ assertNotNull("Should parse custom plugins JSON", implicitPluginsInfo);
+
+ // Call parseImplicitPlugins to convert JSON to PluginInfo objects
+ List<PluginInfo> customHandlers =
SolrCore.parseImplicitPlugins(implicitPluginsInfo);
+
+ assertNotNull("Should return list of PluginInfo objects",
customHandlers);
+ assertEquals("Should have 3 custom handlers", 3, customHandlers.size());
+
+ // Build a map for easy verification
+ Map<String, String> pathToClassMap = new
HashMap<>(customHandlers.size());
+ for (PluginInfo handler : customHandlers) {
+ assertEquals(
+ "All handlers should be of type requestHandler",
SolrRequestHandler.TYPE, handler.type);
+ pathToClassMap.put(handler.name, handler.className);
+ }
+
+ // Verify custom handlers are present with correct classes
+ assertEquals(
+ "Custom update handler should be UpdateRequestHandler",
+ "solr.UpdateRequestHandler",
+ pathToClassMap.get("/custom-update"));
+ assertEquals(
+ "Custom select handler should be SearchHandler",
+ "solr.SearchHandler",
+ pathToClassMap.get("/custom-select"));
+ assertEquals(
+ "Custom ping handler should be PingRequestHandler",
+ "solr.PingRequestHandler",
+ pathToClassMap.get("/admin/ping"));
+
+ // Verify default handlers are NOT present (since we're using custom
file)
+ assertNull(
+ "Default /debug/dump should not be present with custom plugins",
+ pathToClassMap.get("/debug/dump"));
+ assertNull(
+ "Default /update should not be present with custom plugins",
+ pathToClassMap.get("/update"));
+ }
+ }
Review Comment:
The test coverage for the custom implicit plugins functionality is
incomplete. While there are tests for the happy path (parsing a valid custom
plugins file), there are no tests for error scenarios such as: (1) a
non-existent file path, (2) malformed JSON, (3) missing requestHandler section,
or (4) IOException during file reading. These error paths are implemented in
SolrCore.loadImplicitPlugins but not tested. Consider adding test cases for
these error scenarios to ensure proper fallback behavior and error logging.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3614,32 +3614,112 @@ public void cleanupOldIndexDirectories(boolean reload)
{
}
}
+ /**
+ * Parses implicit plugin definitions from a JSON map and converts them to
PluginInfo objects.
+ *
+ * @param implicitPluginsInfo the parsed JSON map containing plugin
definitions
+ * @return an unmodifiable list of PluginInfo objects for request handlers
+ * @throws NullPointerException if requestHandlers section is missing
+ */
+ static List<PluginInfo> parseImplicitPlugins(Map<String, ?>
implicitPluginsInfo) {
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> requestHandlers =
+ (Map<String, Map<String, Object>>)
implicitPluginsInfo.get(SolrRequestHandler.TYPE);
+
+ if (requestHandlers == null) {
+ throw new NullPointerException("No requestHandler section found in
implicit plugins");
+ }
+
+ List<PluginInfo> implicits = new ArrayList<>(requestHandlers.size());
+ for (Map.Entry<String, Map<String, Object>> entry :
requestHandlers.entrySet()) {
+ Map<String, Object> info = entry.getValue();
+ info.put(CommonParams.NAME, entry.getKey());
+ implicits.add(new PluginInfo(SolrRequestHandler.TYPE, info));
+ }
+ return Collections.unmodifiableList(implicits);
+ }
+
private static final class ImplicitHolder {
private ImplicitHolder() {}
- private static final List<PluginInfo> INSTANCE;
+ private static volatile List<PluginInfo> INSTANCE = null;
- static {
+ static List<PluginInfo> getInstance(SolrCore core) {
+ if (INSTANCE == null) {
+ synchronized (ImplicitHolder.class) {
+ if (INSTANCE == null) {
+ INSTANCE = loadImplicitPlugins(core);
+ }
+ }
+ }
+ return INSTANCE;
+ }
Review Comment:
The ImplicitHolder uses a static singleton pattern that is shared across all
CoreContainer instances in the JVM. If multiple CoreContainer instances exist
(e.g., in tests), they could have different implicitPluginsFile configurations,
but only the first CoreContainer's configuration will be used for all of them.
Consider making the implicit plugins configuration per-CoreContainer rather
than using a static singleton, or document this limitation clearly.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3614,32 +3614,112 @@ public void cleanupOldIndexDirectories(boolean reload)
{
}
}
+ /**
+ * Parses implicit plugin definitions from a JSON map and converts them to
PluginInfo objects.
+ *
+ * @param implicitPluginsInfo the parsed JSON map containing plugin
definitions
+ * @return an unmodifiable list of PluginInfo objects for request handlers
+ * @throws NullPointerException if requestHandlers section is missing
+ */
+ static List<PluginInfo> parseImplicitPlugins(Map<String, ?>
implicitPluginsInfo) {
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> requestHandlers =
+ (Map<String, Map<String, Object>>)
implicitPluginsInfo.get(SolrRequestHandler.TYPE);
+
+ if (requestHandlers == null) {
+ throw new NullPointerException("No requestHandler section found in
implicit plugins");
+ }
+
+ List<PluginInfo> implicits = new ArrayList<>(requestHandlers.size());
+ for (Map.Entry<String, Map<String, Object>> entry :
requestHandlers.entrySet()) {
+ Map<String, Object> info = entry.getValue();
+ info.put(CommonParams.NAME, entry.getKey());
+ implicits.add(new PluginInfo(SolrRequestHandler.TYPE, info));
+ }
+ return Collections.unmodifiableList(implicits);
+ }
+
private static final class ImplicitHolder {
private ImplicitHolder() {}
- private static final List<PluginInfo> INSTANCE;
+ private static volatile List<PluginInfo> INSTANCE = null;
- static {
+ static List<PluginInfo> getInstance(SolrCore core) {
+ if (INSTANCE == null) {
+ synchronized (ImplicitHolder.class) {
+ if (INSTANCE == null) {
+ INSTANCE = loadImplicitPlugins(core);
+ }
+ }
+ }
+ return INSTANCE;
+ }
+
+ private static List<PluginInfo> loadImplicitPlugins(SolrCore core) {
+ // Check for custom implicit plugins file from solr.xml (global
configuration)
+ String customPluginsFile =
core.getCoreContainer().getConfig().getImplicitPluginsFile();
+ if (customPluginsFile != null && !customPluginsFile.isEmpty()) {
+ try {
+ // Resolve path similar to solr.xml - support both absolute and
relative paths
+ Path customPluginsPath = Path.of(customPluginsFile);
+ if (!customPluginsPath.isAbsolute()) {
+ // Resolve relative paths against SOLR_HOME
+ Path solrHome = core.getCoreContainer().getSolrHome();
+ customPluginsPath = solrHome.resolve(customPluginsFile);
+ }
+
+ if (!Files.exists(customPluginsPath)) {
+ log.warn(
+ "Custom implicit plugins file does not exist: {} (from
solr.xml). Falling back to default.",
+ customPluginsPath);
+ } else {
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Loading custom implicit plugins from {} (configured in
solr.xml)",
+ customPluginsPath);
+ }
+
+ // Load the custom plugins file directly from the filesystem
+ try (InputStream is = Files.newInputStream(customPluginsPath)) {
+ @SuppressWarnings("unchecked")
+ Map<String, ?> implicitPluginsInfo = (Map<String, ?>)
Utils.fromJSON(is);
+ List<PluginInfo> customHandlers =
parseImplicitPlugins(implicitPluginsInfo);
+
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Loaded {} custom implicit handlers from {}",
+ customHandlers.size(),
+ customPluginsPath);
+ }
+ return customHandlers;
+ }
+ }
+ } catch (NullPointerException e) {
+ log.warn(
+ "No requestHandler section found in custom implicit plugins
file: {} (from solr.xml)",
+ customPluginsFile);
Review Comment:
The error message uses the string representation of the customPluginsFile
variable instead of the actual path that was attempted to be loaded
(customPluginsPath). This makes debugging harder since the user sees the input
value, not the resolved path that was actually checked. Consider using
customPluginsPath.toString() in the log messages.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc:
##########
@@ -339,6 +340,16 @@ or via the `SOLR_HIDDEN_SYS_PROPS` environment variable.
By default, Solr will hide all basicAuth, AWS, ZK or SSL secret sysProps. It
will also hide any sysProp that contains
"password" or "secret" in it.
+`implicitPluginsFile`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: none
+|===
++
+The path relative to `$SOLR_HOME` for a custom implicit plugins file that lets
you control exactly which endpoints are registerd for all cores.
Review Comment:
There is a spelling error: "registerd" should be "registered".
```suggestion
The path relative to `$SOLR_HOME` for a custom implicit plugins file that
lets you control exactly which endpoints are registered for all cores.
```
##########
solr/packaging/test/test_start_solr.bats:
##########
@@ -60,6 +60,9 @@ teardown() {
# for start/stop/restart we parse the args separate from picking the command
# which means you don't get an error message for passing a start arg, like
--jvm-opts to a stop commmand.
Review Comment:
There is a spelling error: "commmand" should be "command".
```suggestion
# which means you don't get an error message for passing a start arg, like
--jvm-opts to a stop command.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
