SOLR-17221: Http2SolrClient merges case sensitive solr params #3028
Conversation
There was a problem hiding this comment.
I believe the only test changes needed are the ones you did here in this file. The other two source files extend this one and thus will run the same test code (right?).
There was a problem hiding this comment.
correct. The other two prepare the requests that go through the flow. The case sensitive params are added there
| // application/x-www-form-urlencoded | ||
| Fields fields = new Fields(); | ||
| Fields fields = new Fields(true); | ||
| Iterator<String> iter = wparams.getParameterNamesIterator(); | ||
| while (iter.hasNext()) { |
There was a problem hiding this comment.
This whole else block could be replaced with something very close to this, notwisthanding the annoying leading question mark to chop off of toQueryString:
req.body(new StringRequestContent("application/x-www-form-urlencoded", wparams.toQueryString(), FALLBACK_CHARSET));
I suppose if there are no params then we don't even need to send a body.
WDYT @jdyer1 ?
There was a problem hiding this comment.
yep, the logic in this else block + convert function in FormRequestContent is largely the same as wparams.toQueryString() except:
- leading question mark from wparams.toQueryString()
- wparams.toQueryString() uses hard coded utf-8 charset as oppose to the FALLBACK_CHARSET passed to FormRequestContent
I guess we can replace it (I tried it, and all unit tests passed), but we need to
- put in some logic (if wparams.toQueryString().startwith("?") then wparams.toQueryString().substring(1)) to handle the leading question mark from wparams.toQueryString(). Although not likely, if the toQueryString function changes the question mark logic in the future, we might have problem here.
- assume FALLBACK_CHARSET remains utf-8 or pass a charset to SolrParams.toQueryString function (not sure if adding a new toQueryString function in SolrParams is justified for this special use case)
The current implementation with Fields has less dependencies stated above
There was a problem hiding this comment.
The current code here is longer and goes through a Fields intermediary mapping. This is why I find SolrParams existing method toQueryString a tempting substitute. I'm doubtful we'll change toQueryString's leading question mark because it's not worth the disturbance on users for a triviality.
I think FALLBACK_CHARSET must be UTF8 always be design. It's not clear what purpose this constant serves vs referencing UTF8 directly.
There was a problem hiding this comment.
ok. updated the PR. I think it's still safer to check if wparams.toQueryString() has leading "?" before chop it off
Case sensitive solr params does not work reliably in multi-shard setting. For example, faceting per field params such as f.CASE_SENSITIVE_FIELD.facet.limit=50. |
…ueryString(). This will avoid to use Fields as an intermediary mapping
There was a problem hiding this comment.
+1 LGTM thanks! I'll add a CHANGES.txt entry
SOLR-17221: If multiple query param keys are sent that only vary by case, they were wrongly merged when doing distributed search (sharded collections). This could likely occur for fielded parameters such as f.CASE_SENSITIVE_FIELD.facet.limit=50.
(Yue Yu, David Smiley)
…3028) If multiple query param keys are sent that only vary by case, they were wrongly merged when doing distributed search (sharded collections). This could likely occur for fielded parameters such as f.CASE_SENSITIVE_FIELD.facet.limit=50 Also: compose the request content directly using SolrParams.toQueryString(). This will avoid to use Jetty Fields as an intermediary mapping --------- Co-authored-by: David Smiley <[email protected]> (cherry picked from commit 82083ea)
https://issues.apache.org/jira/browse/SOLR-17221
Description
The Request object composed by Http2SolrClient via POST/PUT method merges case sensitive solr params. This is because it uses the default constructor of the Fields class (org/eclipse/jetty/util/Fields.java) which sets caseSensitive=false
Solution
Set caseSensitive=true when instantiate Fields object
Tests
Added unit tests to cover case sensitive solr param cases, even for other methods such as GET, etc
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.