Uh oh!
There was an error while loading. Please reload this page.
SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP - #4215
SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP#4215gsingh-code wants to merge 5 commits into
Conversation
c6b55d8 to
9073006Comparegsingh-code
commented
Mar 15, 2026
Quick update: I rebased/restacked this branch on top of the latest No functional change intended versus the prior version; this is just to keep the diff focused and easier to review. |
gsingh-code
commented
Mar 15, 2026
Quick follow-up: this one is rebased on current main and scoped to the SOLR-18123 change only. If any part should be split further, I can update it. |
epugh
left a comment
There was a problem hiding this comment.
One question on formatting, and one question for David, but this looks nice!
| EnvUtils.setProperty( | ||
| ALLOW_PATHS_SYSPROP, ExternalPaths.SERVER_HOME.toAbsolutePath().toString()); | ||
| solrTestRule.startSolr(createTempDir()); | ||
| public static void beforeTest() throws Exception { solrTestRule.startSolr(createTempDir()); |
There was a problem hiding this comment.
this formatting line looks werid, really surpirse tidy didn't wrap it!
There was a problem hiding this comment.
Thanks for flagging this. I rechecked formatting while addressing the broader follow-up; precommit passes now on the updated branch (including spotless/tidy via precommit).
| ExternalPaths.DEFAULT_CONFIGSET); | ||
| } | ||
| final String allowPaths = EnvUtils.getProperty(CoreContainer.ALLOW_PATHS_SYSPROP); |
There was a problem hiding this comment.
I think this makes sense, but would love @dsmiley to weigh in!
There was a problem hiding this comment.
Thanks for the nudge here. I followed up on David's concern by removing the ALLOW_PATHS_SYSPROP helper from SolrTestCase entirely and setting it only in the tests that need external configsets. I also removed the framework-level test that only validated the old default behavior. Revalidated with full ./gradlew --no-daemon precommit in Docker (JDK 21), passing on commit 0161fb7.
dsmiley
left a comment
There was a problem hiding this comment.
TBH I would rather we not touch SolrTestCase. My instinct is to simply not check such things in tests by default... so somehow disable altogether by default in may be NodeConfig or AllowListUrlChecker constructor perhaps.
Albeit... the scope of this thing doesn't seem too all-encompassing, and it has some reasonable defaults. I'd like to understand better why so many tests need to configure it.
gsingh-code
commented
Mar 16, 2026
Updated |
gsingh-code
commented
Mar 16, 2026
Thanks @epugh for the formatting callout— now has its own statement so tidy is happy. The automatic solr.security.allow.paths helper is back inside SolrTestCase.beforeSolrTestCase(), so the test no longer needs to reach for EnvUtils.setProperty. |
gsingh-code
commented
Mar 16, 2026
Thanks @epugh for the formatting callout— |
epugh
commented
Mar 16, 2026
Checking precheck, it looks like one more formatting error: https://github.com/apache/solr/actions/runs/23120891467/job/67158091809?pr=4215 |
epugh
left a comment
There was a problem hiding this comment.
I've fixed up the merge conflicts and a tidy... ran precommit... going to rerun tests and then merge.
epugh
commented
Mar 18, 2026
@dsmiley can you look at the |
dsmiley
commented
Mar 18, 2026
Can you please address my question first? It's not yet clear what the right path is. |
epugh
commented
Mar 18, 2026
Okay, I looked some more at the use of the
|
dsmiley
commented
Mar 18, 2026
My suspicion is that a very small number of tests require any allow-paths manipulation. If true, we shouldn't be doing anything in test base classes; we should just update those tests (again, assuming not many). I suspect security.policy isn't really related. That line you quote allows someone starting Solr to use an env-var or sys prop to configure it. Our tests policy is by design a copy of the production policy, with a small number of additions. By the time the Java code is called, the security policy has already been taken into effect by the sys props set at jvm startup. Thus a test can't possibly influence it. |
gsingh-code
commented
Apr 26, 2026
Addressed the remaining review concern and pushed commit 0161fb7. What changed:
Validation:
This keeps the behavior local to the small set of tests that need it, rather than in the base test class. |
dsmiley
commented
Apr 27, 2026
The current state of this PR makes minor incremental progress but doesn't really address what the JIRA description says for SOLR-18123. Whoever tackles this should attempt to fundamentally understand what this system property is for, what reads it (hint: solr.xml allowPaths default, and NodeConfig, CoreContainer as well) to see what is being protected, and wether that even makes sense to do in tests. The ref guide probably documents allowPaths. I wrote the JIRA description -- I do not think it makes sense to protect which dirs can be written in a test situation. It appears this setting has been abused to apply to other related things beyond its original scope from its original purpose. Please review that. I don't see why a core would ever need to be written outside of the coreRootDir, and so I'm not sure why allowPaths needs to exist if it only protects that. So I only have partial information here; exploration is needed. I think the ultimate outcome should be that very few tests if any explicitly customize allowPaths. Like, only tests actually testing that this mechanism works. |
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! |
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. |
Description
This PR addresses SOLR-18123 by reducing the need for tests to explicitly set
ALLOW_PATHS_SYSPROP(solr.security.allow.paths).Changes:
solr.security.allow.pathsinSolrTestCasewhen not already defined, using the test framework derived server home pathTestFrameworkAllowPathsTestto verify the default is applied and includes the expected test pathALLOW_PATHS_SYSPROPsetup from tests that only needed it to read standard test-owned pathsNo production security defaults are changed.
Testing
Passed targeted tests for all touched classes:
./gradlew test --continue --tests org.apache.solr.TestFrameworkAllowPathsTest --tests org.apache.solr.client.solrj.apache.BasicHttpSolrClientTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientBadInputTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientTest --tests org.apache.solr.client.solrj.apache.HttpSolrClientConPoolTest --tests org.apache.solr.handler.admin.ShowFileRequestHandlerTest --tests org.apache.solr.handler.admin.api.RenameCoreAPITest --tests org.apache.solr.handler.component.DistributedDebugComponentTest --tests org.apache.solr.response.TestPrometheusResponseWriter --tests org.apache.solr.client.solrj.jetty.ConcurrentUpdateJettySolrClientBadInputTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientCompatibilityTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientProxyTest --tests org.apache.solr.client.solrj.SolrExampleTests --tests org.apache.solr.client.solrj.TestBatchUpdate --tests org.apache.solr.client.solrj.TestSolrJErrorHandling --tests org.apache.solr.client.solrj.impl.ConcurrentUpdateSolrClientTestBase --tests org.apache.solr.client.solrj.impl.HttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.impl.HttpSolrClientTestBase --tests org.apache.solr.client.solrj.impl.LBHttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.response.InputStreamResponseParserTest --tests org.apache.solr.client.solrj.response.TestSuggesterResponseAlso ran:
./gradlew tidy./gradlew check