SOLR-12813 followup -- preserve user Principal in alternate codepath in EmbeddedSolrServer #2429
Conversation
EmbeddedSolrServer#request() has two separate codepaths where a SolrQueryRequest is created using the _parser.buildRequestFrom() utility method. The first codepath is active when the relevant SolrRequestHandler can be gotten from the CoreContainer. The second codepath is active when coreContainer.getRequestHandler(path) returns null and instead we have to get the SolrRequestHandler directly from the SolrCore. This second codepath is the one that's used in subquery execution. It was updated in the initial fix for SOLR-12813 so that the call to _parser.buildRequestFrom() would now include the user Principal. However, the first codepath was left alone because it was not found to be involved in subquery execution. In the present commit, the first codepath is being updated as well. This change is not needed for addressing the issue described in SOLR-12813, but it is being made in the interest of keeping the logic as consistent as possible across the two codepaths in EmbeddedSolrServer.request()
There was a problem hiding this comment.
+1
I don't get why I needed to click "Approve and Run" since Rudi isn't a first-time contributor: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#approving-workflow-runs-on-a-pull-request-from-a-public-fork It's not just Rudi; seems for any non-committer.
| @@ -164,7 +164,8 @@ public NamedList<Object> request(SolrRequest<?> request, String coreName) | |||
| if (handler != null) { | |||
| try { | |||
| SolrQueryRequest req = | |||
| _parser.buildRequestFrom(null, request.getParams(), getContentStreams(request)); | |||
| _parser.buildRequestFrom( | |||
There was a problem hiding this comment.
I hate that we have _parser, I mean, we don't use that as a global pattern to define private variables! Renaming the various _myObject would be for another ticket, I know ;-).
There was a problem hiding this comment.
okay, looked more, and yeah, i guess one method is used from http, but the other isn't cause it's for tests etc.... okay.
There was a problem hiding this comment.
i wish there was a better way than passing null around everywhere.
There was a problem hiding this comment.
That underscore is from 2008 by Ryan McKinley -- long time ago :-)
BTW I really appreciate that you are caring to these minor matters,
https://issues.apache.org/jira/browse/SOLR-12813
Description
This is a followup to a previous PR that fixed SOLR-12813.
This current change is intended as proactive code cleanup and is not needed for resolving the issue described in SOLR-12813, which was fully resolved by the previous PR.
EmbeddedSolrServer#request() has two separate places where the _parser.buildRequestFrom() utility method is called.
The first codepath is active when the
coreContainer.getRequestHandler(path)returns non-null, which is to say that we're able to retrieve the designated SolrRequestHandler by looking it up on the available CoreContainer.The second codepath is active when
coreContainer.getRequestHandler(path)return null and instead we need to retrieve the SolrCore from the CoreContainer and get the SolrRequestHandler from that SolrCore. This second codepath is the one that's used in subquery execution. It was updated in the initial fix for SOLR-12813 so that the call to _parser.buildRequestFrom() would now include the user Principal.However, the first codepath was left alone because it was not found to be involved in subquery execution. In the present PR, the first codepath is being updated as well, in the interest of keeping the logic as consistent as possible across the two codepaths in EmbeddedSolrServer.request()
Solution
N/A
Tests
I am looking for advice on how to create a test for this change. I believe this change is non-intrusive and safe: when we pass the Principal, it will either be null, in which case the code will act the same as before -- or it will be non-null, in which case we should be passing it. However, it appears that none of the existing unit tests "care" about this change, so it would be good to create a test that targets it. I found some tests which invoke the codepath in EmbededSolrServer.request that we're modifying here, for example TestEmbeddedSolrServerAdminHandler, but they are local SolrTestCaseJ4 instances and I believe we need a SolrCloudTestCase to be able to set up security.json in zookeeper to create a meaningful test... or maybe there's an easier way? If anyone has insight on when/why one codepath would be active instead of the other -- that's to say, why a request handler would be found on the CoreContainer versus only on the SolrCore, I'm interested to know the background.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.