-
Notifications
You must be signed in to change notification settings - Fork 805
SOLR-18079: Allow user to provide custom Implicit Plugins Mapping File #4067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
implicitPluginsFileconfiguration 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.
| customPluginsFile); | ||
| } catch (Exception e) { | ||
| log.warn( | ||
| "Failed to load custom implicit plugins file: {} (from solr.xml). Falling back to default.", | ||
| customPluginsFile, |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| 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(), |
| if (requestHandlers == null) { | ||
| throw new NullPointerException("No requestHandler section found in implicit plugins"); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| @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")); | ||
| } | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| |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. |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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".
| 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. |
solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc
Outdated
Show resolved
Hide resolved
| 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; | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| } catch (NullPointerException e) { | ||
| log.warn( | ||
| "No requestHandler section found in custom implicit plugins file: {} (from solr.xml)", | ||
| customPluginsFile); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); | ||
| } | ||
|
|
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| // Ensure the resolved path is within allowed locations | |
| core.getCoreContainer().assertPathAllowed(customPluginsPath); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
| run curl -s "http://localhost:${SOLR_PORT}/solr/custom_plugins_test/update" | ||
| assert_output --partial 'Searching for Solr' |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
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.
| 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for suggestion.
janhoy
left a comment
There was a problem hiding this 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ing-solr-xml.adoc Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
https://issues.apache.org/jira/browse/SOLR-18079
Description
Allow user to provide their own
ImplicitPlugins.jsonfile viasolr.xmlsetting.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.