Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-17263: HttpJdkSolrClient doesn't encode curly braces etc #2433

Merged
merged 5 commits into from
May 15, 2024

Conversation

andywebb1975
Copy link
Contributor

@andywebb1975 andywebb1975 commented Apr 30, 2024

https://issues.apache.org/jira/browse/SOLR-17263

Description

HttpJdkSolrClient doesn't encode curly braces etc

NB see also #2454

Solution

Use SolrParams' toQueryString() method rather than implicitly using toString() which doesn't encode all chars.

Tests

I've updated tests to include a curly brace character which should be encoded to %7B, and a length check on the request body content length for PUT/POST requests. Each subsequent commit resolves some of the test failures.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Contributor

@jdyer1 jdyer1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you plan to merge this PR and then I can close out #2435? Your PR looks like a comprehensive fix!

@andywebb1975 andywebb1975 force-pushed the solr-17263 branch 6 times, most recently from 754635a to 6c6e183 Compare May 10, 2024 15:39
@andywebb1975 andywebb1975 marked this pull request as ready for review May 10, 2024 15:59
@andywebb1975 andywebb1975 requested a review from jdyer1 May 10, 2024 15:59
@andywebb1975
Copy link
Contributor Author

@jdyer1 does this look good to go to you?

Note I've another version which refactors part of the code, I think making it clearer - but let's keep that separate. I've a question in there which applies to the current code too - see #2454. Any thoughts on that?

Andy

Copy link
Contributor

@jdyer1 jdyer1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug fix looks fine to me, all it needs is a CHANGES.txt entry.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Get this in for 9.6.1 ?

@dsmiley dsmiley merged commit 4c439d0 into apache:main May 15, 2024
4 checks passed
dsmiley pushed a commit that referenced this pull request May 15, 2024
dsmiley pushed a commit that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants