SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers - #4790

Merged
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers
Aug 27, 2026
Merged

SOLR-18357: Remove SolrTestCaseJ4 URL-allowlist system-property helpers#4790
epugh merged 7 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18357-remove-url-allowlist-helpers

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

What

Removes the deprecated SolrTestCaseJ4.systemSetPropertyEnableUrlAllowList(boolean) and systemClearPropertySolrEnableUrlAllowList() methods. Both were deprecated since 9.0 (SOLR-17864) as backwards-compatibility shims only, with no replacement API — systemClearPropertySolrEnableUrlAllowList() was already an empty no-op.

Scope

Bigger than a single method: 15 call sites across 13 files (solr-core tests + 2 solr-test-framework base classes: BaseDistributedSearchTestCase, SolrTestCaseHS).

  • Migrated every systemSetPropertyEnableUrlAllowList(value) call to System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, String.valueOf(value)) directly — the exact body of the removed setter.
  • Deleted every systemClearPropertySolrEnableUrlAllowList() call outright (it did nothing).
  • In BaseDistributedSearchTestCase, that left clearSolrEnableUrlUrlAllowList() as an empty @AfterClass method with no other callers, so the whole method (and its now-unused @AfterClass import) was removed rather than left as a hollow shell.
  • Dropped 8 @SuppressWarnings("deprecation") annotations across 4 test files that existed solely to cover these calls.

Verification

  • Full-repo ./gradlew compileTestJava (exit 0) — confirms zero remaining references anywhere in the tree.
  • Targeted test runs green across every touched test class (TestTolerantSearch, TestHealthCheckHandlerLegacyMode, DistributedDebugComponentTest, TestSmileRequest, TestJsonRequest, TestJsonFacetRefinement, TestJsonFacetErrors, TestJsonRangeFacets, TestJsonFacets, TestReplicationHandler, TestUserManagedReplicationWithAuth).

Changelog

solr-test-framework is a published Maven artifact and these were protected static methods reachable by subclasses, so a type: removed changelog entry is included.

AI-assisted (Claude Sonnet 5)

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

cc @epugh (deprecated it, SOLR-17864)

@epughepugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.. one spurious thought, is there any chance that some of these tests do NOT need this setting at all? Sometimes settings like this get copy/n/pasted over multiple tests.

@epughepugh self-assigned this Aug 21, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

@epugh

Copy link
Copy Markdown
Contributor

One did: TestJsonFacetErrors had SolrInstances scaffolding dead since it was split out of TestJsonFacets in SOLR-14348 (2020) -- initServers() never called, all 5 tests are single-node. Removed it (dc21831e629). The other 12 checked out -- each fails with a 403 without the setting.

Awesome. did dc21831e629 get pushed? I didn't see it in this PR...

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Sorry, pushed.

…/systemClearPropertySolrEnableUrlAllowList
Both methods were deprecated since 9.0 (SOLR-17864, Eric Pugh,
2025-09-28) as backwards-compatibility shims only, with no
replacement API -- systemClearPropertySolrEnableUrlAllowList() was
already an empty no-op.
Migrated all 15 call sites across 13 files to call
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, ...)
directly (the exact body of the removed setter). Calls to the no-op
clear method were deleted outright rather than replaced with
anything. In BaseDistributedSearchTestCase, this left
clearSolrEnableUrlUrlAllowList() as an empty @afterclass method with
no other callers, so the whole method (and its now-unused
@afterclass import) was deleted rather than left as a hollow shell.
Also dropped 8 @SuppressWarnings("deprecation") annotations across 4
files that existed solely to cover these calls.
Verified with a full-repo ./gradlew compileTestJava (exit 0) plus
targeted test runs across every touched test class.
solr-test-framework is a published Maven artifact and these were
protected static methods reachable by subclasses, so this removal is
a real API break for downstream test code -- warrants a changelog
entry per the same standard applied to SOLR-18354/SOLR-18356 earlier
in this batch.
…etErrors
Eric asked whether some of the migrated URL-allowlist settings were
copy-pasted without actually being needed. Checked all 13 files by
verifying each one's actual code path into AllowListUrlChecker
(explicit shards= param, SolrInstances distributed queries, or
IndexFetcher replication) -- confirmed empirically by temporarily
removing the setting and re-running the test to see it fail with a
403, one representative case per category (TestTolerantSearch,
TestSmileRequest, TestHealthCheckHandlerLegacyMode).
TestJsonFacetErrors was the one exception: its SolrInstances
scaffolding (initServers(), the servers field, the null-guarded
stop() in tearDown) has been dead code since the file's creation in
SOLR-14348 (2020-03-23, split out of TestJsonFacets) -- initServers()
is declared but never called by any of the 5 test methods, all of
which are single-node error-validation checks. Removed the scaffolding
along with the now-pointless allow-list setting; the class's actual
purpose (facet API error handling) is untouched and needs no
distributed servers.
…temSetPropertyEnableUrlAllowList
Rebasing onto latest main pulled in solr/webapp (added by SOLR-8474 after this
branch's original base), which still had one call to the now-removed
systemSetPropertyEnableUrlAllowList(false). Migrated to
System.setProperty(AllowListUrlChecker.ENABLE_URL_ALLOW_LIST, "false") directly,
the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy
serhiy-bzhezytskyyforce-pushed the SOLR-18357-remove-url-allowlist-helpers branch from dc21831 to b4062afCompareAugust 22, 2026 11:36
@dsmiley

Copy link
Copy Markdown
Contributor

There's another PR that has stalled on the subject of ALLOW_PATHS in tests. I'm not sure if we should merge this right now, as it may delay that one? (not sure but just want to raise the risk/concern)

serhiy-bzhezytskyy added a commit to serhiy-bzhezytskyy/solr that referenced this pull request Aug 23, 2026
…eplicas() call
solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as
SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed
getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the
method's return type), the same pattern used at every other call site in this PR.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Checked -- #4215 (SOLR-18123) uses CoreContainer.ALLOW_PATHS_SYSPROP (filesystem path restrictions), a different mechanism from this PR's systemSetPropertyEnableUrlAllowList/systemClearPropertySolrEnableUrlAllowList (URL-allowlist). No file or symbol overlap. If anything, this PR does what you asked for on #4215 for a different property -- removes the base-class toggle-helper wrappers and inlines the one System.setProperty(...) call at its only remaining caller, instead of keeping ceremony in SolrTestCaseJ4.

@dsmileydsmiley added this to the 10.x milestone Aug 25, 2026
@dsmiley

Copy link
Copy Markdown
Contributor

Thanks for looking into that.

lots of conflicts to resolve and a changelog to remove but otherwise I approve

…-allowlist-helpers
Resolves conflicts from SOLR-18354's ErrorLogMuter migration (main), which
touched the same 4 files: kept both sides' imports (AllowListUrlChecker from
this branch, ErrorLogMuter from main), and in the two @afterclass teardown
methods that had both a now-dead resetExceptionIgnores() call (superseded by
main's scoped try-with-resources ErrorLogMuter usage) and a now-dead
systemClearPropertySolrEnableUrlAllowList() call (removed by this branch),
dropped both -- neither has any remaining purpose.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Both done -- rebased on main and resolved a real conflict from SOLR-18354's ErrorLogMuter migration (dead resetExceptionIgnores()/systemClearPropertySolrEnableUrlAllowList() calls in two @AfterClass teardowns, both superseded), and removed the changelog entry.

@epugh

Copy link
Copy Markdown
Contributor

running CI and then will merge... thnaks @dsmiley for reviewing.

@epugh

Copy link
Copy Markdown
Contributor

test passed locally

@epugh
epugh merged commit 1869a60 into apache:mainAug 27, 2026
5 of 7 checks passed
dsmiley pushed a commit that referenced this pull request Aug 29, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley