You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "831973741yy (via GitHub)" <gi...@apache.org> on 2025/01/12 15:11:49 UTC
[PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
831973741yy opened a new pull request, #3028:
URL: https://github.com/apache/solr/pull/3028
https://issues.apache.org/jira/browse/SOLR-17221
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# 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:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "831973741yy (via GitHub)" <gi...@apache.org>.
831973741yy commented on code in PR #3028:
URL: https://github.com/apache/solr/pull/3028#discussion_r1912637510
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -784,7 +784,7 @@ private Request fillContentStream(
}
} else {
// application/x-www-form-urlencoded
- Fields fields = new Fields();
+ Fields fields = new Fields(true);
Iterator<String> iter = wparams.getParameterNamesIterator();
while (iter.hasNext()) {
Review 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
1. 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.
2. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3028:
URL: https://github.com/apache/solr/pull/3028#discussion_r1912565128
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java:
##########
Review 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?).
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -784,7 +784,7 @@ private Request fillContentStream(
}
} else {
// application/x-www-form-urlencoded
- Fields fields = new Fields();
+ Fields fields = new Fields(true);
Iterator<String> iter = wparams.getParameterNamesIterator();
while (iter.hasNext()) {
Review 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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3028:
URL: https://github.com/apache/solr/pull/3028#discussion_r1912650870
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -784,7 +784,7 @@ private Request fillContentStream(
}
} else {
// application/x-www-form-urlencoded
- Fields fields = new Fields();
+ Fields fields = new Fields(true);
Iterator<String> iter = wparams.getParameterNamesIterator();
while (iter.hasNext()) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "831973741yy (via GitHub)" <gi...@apache.org>.
831973741yy commented on code in PR #3028:
URL: https://github.com/apache/solr/pull/3028#discussion_r1912584876
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java:
##########
Review Comment:
correct. The other two prepare the requests that go through the flow. The case sensitive params are added there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "831973741yy (via GitHub)" <gi...@apache.org>.
831973741yy commented on PR #3028:
URL: https://github.com/apache/solr/pull/3028#issuecomment-2586024118
@dsmiley
> Can you propose a short description that shall go in CHANGES.txt ? Basically, how might a user experience this bug?
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "831973741yy (via GitHub)" <gi...@apache.org>.
831973741yy commented on code in PR #3028:
URL: https://github.com/apache/solr/pull/3028#discussion_r1913722823
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -784,7 +784,7 @@ private Request fillContentStream(
}
} else {
// application/x-www-form-urlencoded
- Fields fields = new Fields();
+ Fields fields = new Fields(true);
Iterator<String> iter = wparams.getParameterNamesIterator();
while (iter.hasNext()) {
Review Comment:
ok. updated the PR. I think it's still safer to check if wparams.toQueryString() has leading "?" before chop it off
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17221: Http2SolrClient merges case sensitive solr params [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #3028:
URL: https://github.com/apache/solr/pull/3028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org