jdyer1 commented on code in PR #2433:
URL: https://github.com/apache/solr/pull/2433#discussion_r1591485125
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -299,10 +299,23 @@ private PreparedRequest preparePutOrPost(
InputStream is = streams.iterator().next().getStream();
bodyPublisher = HttpRequest.BodyPublishers.ofInputStream(() -> is);
} else if (queryParams != null && urlParamNames != null) {
+ // rP is a new name for the incoming qP
ModifiableSolrParams requestParams = queryParams;
+ // qP becomes a new object, with selected params moved from original set
- when would urlParamNames be specified?
queryParams = calculateQueryParams(urlParamNames, requestParams);
+ // qP gains further (?) params moved from original set if solrRequest
has params, but only if urlParamNames was set too
queryParams.add(calculateQueryParams(solrRequest.getQueryParams(),
requestParams));
- bodyPublisher =
HttpRequest.BodyPublishers.ofString(requestParams.toString());
+ // bP receives any remaining params from original set
+ // with this version the params are not fully encoded - we get raw
Unicode chars, curly braces etc
+ // and the body content length changes, presumably due to re-encoding
+ // String bodyQueryString = requestParams.toString();
+ // with this version the params are fully encoded - note the
toQueryString() method adds an unwanted leading question mark
+ // but there's no longer a length change as the query is fully encoded
before being given to HttpRequest
+ String bodyQueryString = requestParams.toQueryString().substring(1);
+ bodyPublisher = HttpRequest.BodyPublishers.ofString(bodyQueryString);
+ // this isn't intended to be merged - but it shows the content length
change noted above
+ if (bodyQueryString.length() != bodyPublisher.contentLength()) throw new
URISyntaxException("inconsistent content length", bodyQueryString + " - " +
bodyQueryString.length() + " -> " + bodyPublisher.contentLength());
Review Comment:
I agree that we should remove this and not throw an exception if the content
length is off.
Do you think instead it would be possible to add a content length check to
one of the unit tests (that would fail without your code modification)?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java:
##########
@@ -337,24 +347,27 @@ protected void testCollectionParameters(
protected void setReqParamsOf(UpdateRequest req, String... keys) {
if (keys != null) {
for (String k : keys) {
- req.setParam(k, k + "Value");
+ // note inclusion of non-ASCII character, and curly quotes which
should be URI encoded
+ req.setParam(k, k + "Value\u1234{}");
}
}
}
- protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?>
request) {
+ protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?>
request) throws Exception {
// check query String
Iterator<String> paramNames =
request.getParams().getParameterNamesIterator();
while (paramNames.hasNext()) {
String name = paramNames.next();
String[] values = request.getParams().getParams(name);
if (values != null) {
+ // this can be enabled to see the raw query string (it's not expected
to be an empty string!)
+ // assertEquals("", DebugServlet.queryString);
Review Comment:
I would prefer to get rid of the commented-out code here before merging. If
you think it is helpful you could add a debug-level log message. Better yet,
you could include the raw query string in the failure message in the subsequent
assert.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
