fix: Avoid sending request body with GET (issue #318)

[DhavalGojiya]

Description

Fixes: Issue#318

This PR fixes an issue where parameters were being sent in the body of a GET request.
GET requests should not contain a request body.

The fix updates the request to pass parameters through the URL query string using
params=, which is what Solr expects.

References

• CoreAdmin Status API
  https://solr.apache.org/guide/solr/9_9/configuration-guide/coreadmin-api.html#coreadmin-status

• CoreAdmin Create API
  https://solr.apache.org/guide/solr/9_9/configuration-guide/coreadmin-api.html#coreadmin-create

• Collections DeleteStatus API
  https://solr.apache.org/guide/solr/9_9/configuration-guide/collections-api.html#deletestatus

[cclauss]

Could we avoid future regressions by adding a test that fails with the current code but passes with the proposed code?

[DhavalGojiya]

Could we avoid future regressions by adding a test that fails with the current code but passes with the proposed code?

The test cases already reference this function.
Do we need additional test cases for this?
This is not actually a breaking change.
We are just changing the way data is transferred.

[cclauss]

Adding tests for changes should be the default behavior; so, please avoid pushing back on requests to add tests.

If the existing tests covered this case, they would have failed. They currently do not fail, so adding a test helps avoid regressions.

[DhavalGojiya]

If the existing tests covered this case, they would have failed.

This completely depends on where we are sending the HTTP network requests.

The Apache Solr web backend still has backward compatibility to respond to HTTP GET requests with a request body. Howwever, it recommends using query parameters instead of sending data in the request body. This behavior may be removed in the future (I don't know), and Solr may completely stop reading the body of GET requests.

[DhavalGojiya]

They currently do not fail, so adding a test helps avoid regressions.

Currently, it is not possible to test this because Solr still responds by reading data from the HTTP GET request body.

[DhavalGojiya]

Any thoughts? @acdha

[acdha]

Looking at this, I'm thinking the answer might be to dub the next major release 4.x. I don't think there's a case where this would break on a version of Solr we still support but that would be a clear signal. We already have a bit of a breaking change it appears because in 11c881e the tests recognize that in the absence of wt=xml the responses are now JSON by default, which is definitely what I'd prefer to work with but could potentially cause a problem for someone parsing the output. We could add wt=xml to the query string now but I was thinking it might be easier simply to note that in the change log for 4.0.0 since there are probably not all that many people using this to manage their Solr cores compared to how many use it to run queries.

I was already wondering about that because this commit makes me question whether SolrCoreAdmin._get_url() should still exist because the only thing it does beyond the requests.get call is passing response.content through force_unicode, which is basically what response.text would do and in all of the implemented methods it seems like that would be better as response.json(), perhaps with some additional error handling checks. Changing the response from text to a structured object would certainly simplify the extra code you had to add to the test suite but that's a breaking change.

I also noticed that the admin code is calling requests.get() without something like the get_session() method which Solr has. That makes me wonder whether we should support something similar or perhaps add a accessor like Solr.get_admin() which would use the same session instance so people doing that for things like custom CAs wouldn't need to change anything.

[DhavalGojiya]

I'm thinking the answer might be to dub the next major release 4.x

I'm also thinking the same, since there are some breaking changes (specially solr api response xml vs json).
We can bump the major version.

response.text would do and in all of the implemented methods it seems like that would be better as response.json(), perhaps with some additional error handling checks. Changing the response from text to a structured object would certainly simplify the extra code you had to add to the test suite but that's a breaking change.

If we've already decided to bump the major release, we can definitely consider this. It would really make sense and help avoid some repeated code.

I also noticed that the admin code is calling requests.get() without something like the get_session() method which Solr has. That makes me wonder whether we should support something similar or perhaps add a accessor like Solr.get_admin() which would use the same session instance so people doing that for things like custom CAs wouldn't need to change anything.

+1 for this. We can create requests session inside SolrCoreAdmin constructor method.

[DhavalGojiya]

@acdha | @cclauss

Updated. (Breaking changes)

• Added a session to SolrCoreAdmin with improved logging
• Removed redundant self.session.stream = False (default requests behaviour).
• Updated Solr admin test cases

[acdha]

Should we call raise_for_status() here? Otherwise the caller doesn't have a simple way to tell whether the request was a success or error response unless we refactor the return signature to be status_code, decoded_json.

[DhavalGojiya]

@acdha The previous implementation also didn't have this feature.
Users can simply inspect the Solr JSON response’s status key if they want to know the HTTP status of the request.

[acdha]

This looks good, although I did have one question about error handling.

[cclauss]

Suggested change

	)
	)
	resp.raise_for_status()

[cclauss]

Please fix our tests to properly handle this change.

[DhavalGojiya]

@cclauss will do.

[DhavalGojiya]

(PR Updated) Implement handling for bad HTTP responses from Solr and update tests