SOLR-17589 First use of PUT/POST request with HttpJdkSolrClient generates error log entry on solr server due to initial HEAD request

[Paul-Blanchaert]

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

Description

Adding header "Content-Type"="application/json" to HEAD request prevents that the server side raises the SolrException.

Solution

Added header "Content-Type"="application/json" to HEAD request

Tests

Existing unit tests show the issue in the test log (and ignore the server side SolrException).
Re-running the tests with this commit don't show the issue.
Adding a new integration test (client side changes raises server side exception) seems too cumbersome for this change where positive result is demonstrated in the test log and sufficient existing unit test validate that the functionality works fine. Furthermore, it seems to me that adding a (formerly missing) valid http header to a client side http request should not raise issues that are not covered by current test set. When that specific header fails, it should fail fast.

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, not available for branches on forks living under an organisation)
• I have developed this patch against the main branch.
• I have run ./gradlew check (with -Pvalidation.rat.failOnError=false).
• I have added tests for my changes.
• I have added documentation for the Reference Guide

[jdyer1]

This is good. Is there any possibility you can add an assert somewhere to HttpJdkSolrClientTest to test the change?

[Paul-Blanchaert]

Thanks for the encouragement.
Can you help me understand what you're asking with "an assert somewhere"?
In order to be able to test the solr server exception, I'll need to change the scope of the function to "collect" the http statuscode (400 is returned in the case of the solr server exception). Currently, the http response (and status) is ignored (headSucceeded = true when no exception is thrown by the jdk http client; this probably goes for any 4xx/5xx statusses; but I didn't verify that). Should I change the function to set headSucceeded to false when http statuscode <> 200?
That way, we're able to capture the exception prior to the change of this PR and we can build a unit test to validate the "enhanced" function.
Is that what you had in mind?

[jdyer1]

Here is an approach that won't need any change to the client itself.

1. See DebugServlet.java method doHead. Here it records the request body in a variable named requestBody, but unfortunately any subsequent POST request will overwrite it before the test can examine it. DebugServlet will need to keep the HEAD request body in a separate variable.
2. See HttpJdkSolrClientTest.java method assertNoHeadRequestWithSsl. Here it asserts that if the randomized test runner configured an 'https' connection, then the HEAD request should not be fired. We can additionally assert that in the other case, whenever the HEAD request is fired, that it contains a body, by looking at the new variable created in DebugServlet.

[Paul-Blanchaert]

The initial problem was solved by adding the http header "Content-Type" while keeping the noBody() (see below), so no need to store the body in a separate variable.

HttpRequest.newBuilder(uriNoQueryParams)
.method("HEAD", HttpRequest.BodyPublishers.noBody())
.header("Content-Type", ClientUtils.TEXT_JSON);

I reverted the change and test on the http status.
And I followed your suggestion by assuring that the header "Content-Type" is set via the DebugServlet. This is basically just testing whether the header function on the builder works fine, but It might prevent that future changes omit the content-type header that is apparently required to avoid the server exception on the HEAD request.
Hope this helps!

[epugh]

Interesting unit test for something hard to actually test!

[jdyer1]

@Paul-Blanchaert This is great, although I am surprised the host jetty behaves such.

Can you both sync this branch to main and add a CHANGES.txt entry under solr 9.9 (be sure to give yourself credit)? Otherwise, if you give access to push to your branch, I will take care of those details.

Once the CHANGES.txt entry is there, we can go ahead and merge & backport for 9.9.

[Paul-Blanchaert]

@jdyer1
I've added the changes.txt entry but I'm probably not able to close the PR. Can you take care of remaining tasks?
I should have created the fork under my user (not the organisation), but apparently I didn't. So I've granted you write access to the fork.
Let me know if I can help with anything else...