SOLR-17153 Resolve IndexOutOfBoundsException #2393
Conversation
I had the same thought. But I now see the line of code that caused it is no longer being used. The call resolveDocCollection() is now being invoked within getCoreByCollection() and its signature has also changed. What would be the test case of the new test? We would need one that doesnt create a collection or noneof the usual ones. Eric tried to start solr and launch the admin page..... |
|
That the method signature changes doesn't affect our ability to test; maybe you were thinking a unit test. I much prefer an integration test (using SolrCloudTestCase). All we need to do is figure out what URL into Solr, in what situation (a collection existing or not? time-routed-alias? To a node with a core for the collection or not?) and do the same in a test. It should fail prior to this PR. |
|
I think a test of a Solr with no collections created should demonstrate the bug. Please let me know if I can help. I'd love to see this fixed so I can then merge some other code into |
Hi Eric, I started working on a Java test which didnt really work as expected to address this use case (launch the solr admin console ). I am now looking to add a test case in a ".bats" script under solr/packaging. Let me know if this sounds appropriate. |
…javadocs for a couple of methods
aparnasuresh85
force-pushed
the
solr-17153-fix
branch
from
5bbd498
to
d6ef098
Compare
| run curl -s -o /dev/null -w "%{http_code}" http://localhost:${SOLR_PORT}/solr/ | ||
|
|
||
| # Check if the HTTP response code is 200 (OK) | ||
| assert_output --partial "200" |
There was a problem hiding this comment.
The test failed with the bug and passed without it.
There was a problem hiding this comment.
Is the --partial part accurate -- won't 200 be the entirety of the output?
There was a problem hiding this comment.
I modified the test to instead match exact output.
There was a problem hiding this comment.
lets just call it test_adminconsole_urls.bats so that we leave room and encourage additional URLs to fetch that the admin console fetches. Even a TODO comment would encourage this.
| run curl -s -o /dev/null -w "%{http_code}" http://localhost:${SOLR_PORT}/solr/ | ||
|
|
||
| # Check if the HTTP response code is 200 (OK) | ||
| assert_output --partial "200" |
There was a problem hiding this comment.
Is the --partial part accurate -- won't 200 be the entirety of the output?
|
Looks good; I'd like to merge. If you disagree @epugh , please pipe in today. Minor nitpick: there's a comment that doesn't flow (newline) nicely but whatever. |
|
I just manually gave this PR a run through, and it looks great. Please merge! |
Resolve IndexOutOfBoundsException, a regression from the first change in this issue. * Call resolveDocCollection inside getCoreByCollection * New BATS test to check URLs that the Solr Admin UI (or a user trying to navigate to such) might use (cherry picked from commit 09e8d0c)
https://issues.apache.org/jira/browse/SOLR-17153
Description
Resolve an IndexOutOfBoundsException in HttpSolrCall.resolveDocCollection(), at line:
String collectionName = collectionsList.get(0);
Solution
Check if collectionsList is empty, in that case return early.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.