SOLR-18168: Remove path exclusion filter#4243
Conversation
Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
…evert-exclude-patterns # Conflicts: # solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java # solr/webapp/web/WEB-INF/web.xml
Revert excludePatterns changes; merge main into branch
# Conflicts: # solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java # solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java # solr/webapp/web/WEB-INF/web.xml
Removed abortErrorMessage -- unused
# Conflicts: # solr/core/src/java/org/apache/solr/core/ConfigSetService.java
…lter # Conflicts: # solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java # solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java # solr/core/src/java/org/apache/solr/servlet/SolrServlet.java # solr/core/src/test/org/apache/solr/servlet/LoadAdminUiServletTest.java # solr/server/solr/configsets/_default/conf/solrconfig.xml # solr/server/solr/configsets/sample_techproducts_configs/conf/solrconfig.xml # solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java # solr/test-framework/src/java/org/apache/solr/util/TestHarness.java # solr/webapp/web/WEB-INF/web.xml
github-actions
bot
added
test-framework
jetty-server
client:solrj
tool:build
admin-ui
tests
labels
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
several files disappear here :-)
I plan to merge this ~Wednesday next week if I hear no need to improve anything.
There was a problem hiding this comment.
The filter came and went inside a release. So instead I'm communicating the observed sum of the two.
There was a problem hiding this comment.
The Gradle up-to-date tracking wasn't working so I fixed it. CC @epugh as I suspect you run these tests more than any of us.
| <filter-mapping> | ||
| <filter-name>SolrFilter</filter-name> | ||
| <url-pattern>/*</url-pattern> | ||
| <servlet-name>default</servlet-name> |
There was a problem hiding this comment.
referencing by name instead of URL pattern.
|
|
||
| <servlet> | ||
| <servlet-name>SolrServlet</servlet-name> | ||
| <servlet-name>default</servlet-name> |
There was a problem hiding this comment.
by changing this to "default", it means we don't need to register it for /*
There was a problem hiding this comment.
I think I liked the simplicity of the pattern versus needing to know what "default" is, but we should do whatever is the standard approxh for these!
There was a problem hiding this comment.
more testing here to make up for the removal elsewhere. I was sure to check a random CSS file too as it's served via different routing path.
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
Shared some comments on bats test style!
| } | ||
|
|
||
| inputs.dir(distDir) | ||
| inputs.dir('test') // Track .bats test files and helper as inputs |
There was a problem hiding this comment.
Thanks for this, there are times when I get weirdness, and maybe this fixes that!
| assert_output "200" | ||
|
|
||
| # Check Content-Type header is text/html | ||
| local content_type=$(curl -s -I http://localhost:${SOLR_PORT}/solr/ | grep -i "Content-Type:" | tr -d '\r') |
There was a problem hiding this comment.
Can this be a run and an assert include? Just to be more bats like and less fancy bass?
| # Check if the HTTP response code is 200 (OK) | ||
| # Check that response body contains HTML | ||
| local response_body=$(curl -s http://localhost:${SOLR_PORT}/solr/) | ||
| [[ "$response_body" == *"<html"* ]] || [[ "$response_body" == *"<!DOCTYPE"* ]] |
There was a problem hiding this comment.
Likewise with the assert partial command?
| local csp_header=$(curl -s -I http://localhost:${SOLR_PORT}/solr/ | grep -i "Content-Security-Policy:" | tr -d '\r') | ||
|
|
||
| # Check that the CSP header contains each of our custom URLs | ||
| [[ "$csp_header" == *"http://example1.com/token"* ]] |
There was a problem hiding this comment.
These could use the standard bats? I like keeping to the bats conventions as much as possible to reduce the learning when you read these!
|
|
||
| <servlet> | ||
| <servlet-name>SolrServlet</servlet-name> | ||
| <servlet-name>default</servlet-name> |
There was a problem hiding this comment.
I think I liked the simplicity of the pattern versus needing to know what "default" is, but we should do whatever is the standard approxh for these!
Now that SolrServlet exists, we are unblocked from removing PathExclusionFilter. Instead, its task can be accomplished with web.xml configuration. Furthermore, admin UI serving should be exclusively tested with BATS (integration) – JettySolrRunner can be simpler.
SOLR-18168