Skip to content

Conversation

@epugh
Copy link
Contributor

@epugh epugh commented Jan 20, 2026

https://issues.apache.org/jira/browse/SOLR-18079

Description

Allow user to provide their own ImplicitPlugins.json file via solr.xml setting.

Solution

Add a property to solr.xml that can be set via system property.

Tests

Added test for parsing of ImplicitPlugins.json and the two code paths.

@epugh epugh requested review from Copilot and dsmiley January 20, 2026 22:13
@github-actions github-actions bot added documentation Improvements or additions to documentation configs tests labels Jan 20, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request implements the ability to provide a custom implicit plugins mapping file via solr.xml configuration, addressing JIRA issue SOLR-18079. The feature allows users to override the default ImplicitPlugins.json that ships with Solr, giving them control over which request handlers are registered for all cores.

Changes:

  • Added implicitPluginsFile configuration property to solr.xml that can be set via system property
  • Refactored the implicit plugins loading mechanism to support both default classpath resource and custom file paths
  • Added comprehensive unit tests and integration tests for the new functionality

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc Added documentation for the new implicitPluginsFile configuration property
solr/server/solr/solr.xml Added implicitPluginsFile property with empty default value
solr/packaging/test/test_start_solr.bats Added integration test for starting Solr with custom implicit plugins configuration
solr/core/src/test/org/apache/solr/core/TestSolrXml.java Added unit tests for parsing implicitPluginsFile from solr.xml
solr/core/src/test/org/apache/solr/core/SolrCoreTest.java Added test for parsing custom implicit plugins JSON into PluginInfo objects
solr/core/src/test-files/solr/custom-implicit-plugins.json Added test resource file with sample custom implicit plugins configuration
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java Added parsing of implicitPluginsFile property from solr.xml
solr/core/src/java/org/apache/solr/core/SolrCore.java Implemented custom implicit plugins loading with fallback to default, refactored ImplicitHolder to support dynamic loading
solr/core/src/java/org/apache/solr/core/RequestHandlers.java Minor comment formatting improvement
solr/core/src/java/org/apache/solr/core/NodeConfig.java Added implicitPluginsFile field and corresponding getter/setter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +3700 to +3704
customPluginsFile);
} catch (Exception e) {
log.warn(
"Failed to load custom implicit plugins file: {} (from solr.xml). Falling back to default.",
customPluginsFile,
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
customPluginsFile);
} catch (Exception e) {
log.warn(
"Failed to load custom implicit plugins file: {} (from solr.xml). Falling back to default.",
customPluginsFile,
customPluginsPath.toString());
} catch (Exception e) {
log.warn(
"Failed to load custom implicit plugins file: {} (from solr.xml). Falling back to default.",
customPluginsPath.toString(),

Copilot uses AI. Check for mistakes.
Comment on lines 3629 to 3630
if (requestHandlers == null) {
throw new NullPointerException("No requestHandler section found in implicit plugins");
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +154 to +208
@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"));
}
}
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
|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.
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a spelling error: "registerd" should be "registered".

Suggested change
The path relative to `$SOLR_HOME` for a custom implicit plugins file that lets you control exactly which endpoints are registerd for all cores.
The path relative to `$SOLR_HOME` for a custom implicit plugins file that lets you control exactly which endpoints are registered for all cores.

Copilot uses AI. Check for mistakes.
Comment on lines 3645 to 3656
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;
}
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +3697 to +3700
} catch (NullPointerException e) {
log.warn(
"No requestHandler section found in custom implicit plugins file: {} (from solr.xml)",
customPluginsFile);
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, but we don't actually ahve this available to us in the catch

Path solrHome = core.getCoreContainer().getSolrHome();
customPluginsPath = solrHome.resolve(customPluginsFile);
}

Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// Ensure the resolved path is within allowed locations
core.getCoreContainer().assertPathAllowed(customPluginsPath);

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@janhoy janhoy Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find by copilot! (The fact that assertPathAllowed is not called before using the string)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, heck, I went and fussed aroudn for a while and missed this fix, which was what i did eventually!

Comment on lines 141 to 142
run curl -s "http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update"
assert_output --partial 'Searching for Solr'
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
run curl -s "http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update"
assert_output --partial 'Searching for Solr'
run curl -s -o /dev/null -w "%{http_code}" "http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update"
assert_output "404"

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for suggestion.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review

Path solrHome = core.getCoreContainer().getSolrHome();
customPluginsPath = solrHome.resolve(customPluginsFile);
}

Copy link
Contributor

@janhoy janhoy Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find by copilot! (The fact that assertPathAllowed is not called before using the string)

if (!customPluginsPath.isAbsolute()) {
// Resolve relative paths against SOLR_HOME
Path solrHome = core.getCoreContainer().getSolrHome();
customPluginsPath = solrHome.resolve(customPluginsFile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't it suggested to disallow path.resolve due to how it will resolve ../../.. patterns perhaps unexpectedly? However, if calling assertPathAllowed before use, we should be safe anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't remember how to do the assert! We need it in our prompt.md ;-)

<str name="allowUrls">${solr.security.allow.urls:}</str>
<str name="hideStackTrace">${solr.hideStackTrace:false}</str>
<int name="indexSearcherExecutorThreads">${solr.searchThreads:0}</int>
<str name="implicitPluginsFile">${solr.implicitPluginsFile:}</str>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider a "modern" property name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really because all the other ones are the same. So kept the existing style. In another PR I'd love to get these all renamed!

epugh and others added 5 commits January 21, 2026 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configs documentation Improvements or additions to documentation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants