SOLR-17549: v2 SolrRequest/SolrResponse should throw exception on error#4183
Conversation
Prior to this commit our generated v2 SolrRequest implementations only reported errors in the POJO itself. A failure response from Solr wouldn't trigger a RemoteSolrException as SolrJ users are used to with our v1 APIs. Error detection in our SolrClient implementations attempted to detect-and-throw in these v2 cases, but that detection was keying off of a 'NamedList' key that wasn't being populated by JacksonDatabindResponseParser (which is used for these v2 SolrRequest/Response pairs). This commit updates JacksonDatabindResponseParser to populate this "error" key that SolrClient's were looking for.
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
LGTM with two small comments... This is exciting!
| } | ||
|
|
||
| private void assertRSECodeAndMessage( | ||
| RemoteSolrException rse, int expectedCode, String... expectedMessagePices) { |
There was a problem hiding this comment.
ding ding, I found a typo... time to pick up the pieces. heh heh.
...j/src/test/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParserTest.java
Show resolved
Hide resolved
gerlowskija
changed the title
|
Alright, addressed your comments @epugh - lmk when you get a chance to take another look. I'll aim to merge this Friday or so to get you unblocked on some of the tests you were hoping to write using the generated SolrJ objects. |
| @Test | ||
| public void testErrorResponseSurfacesErrorAtTopLevel() throws IOException { | ||
| final var parser = new JacksonDataBindResponseParser<>(SolrJerseyResponse.class); | ||
| final var json = |
There was a problem hiding this comment.
so nitpucky, but triple double uqotes?
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
LGTM!
If you wanted to merge this, and then take one of the new V2 conversions and update it in light of htis, I'd lvoe to see that. Maybe the CLI v1 to v2 PR?
This PR does do a bit of that updating I think? Albeit in test code rather than, say, the CLI. See the RenameCoreAPITest changes in this PR if you're looking for examples? Or is it less about having syntax examples to reference and more about getting additional hands helping move those PRs forward? In any case - going to merge this now and we can iterate on those other PRs separately. |
|
Great! I think once this is merged, and I update the CLI migration PR, I just want you to re-review that we are using this new logic properly... And since I don't think I ever took into account in the CLI migration that the v2 could/was different, then the PR should be just fine once this is in! |
…or (#4183) This commit updates our v2 SolrJ code to throw RemoteSolrException when error-responses are encountered. It relies on a pre-existing attempt at error detection already present in our SolrClient implementations. Code in many of our clients already attempts to detect these errors by looking for a 'NamedList' key, "error", which it (incorrectly) assumed would be present in the v2 case but which isn't populated by the ResponseParser that our v2 SolrRequest/SolrResponse implementations use by default. This commit updates JacksonDatabindResponseParser to populate this "error" key that SolrClient's were looking for, which in turn enables the clients to correctly throw RemoteSolrException in these cases.
2 participants
https://issues.apache.org/jira/browse/SOLR-17549
Description
Prior to this commit our generated v2 SolrRequest implementations only
reported errors in the POJO itself. A failure response from Solr
wouldn't trigger a RemoteSolrException as SolrJ users are used to with
our v1 APIs.
This was tough for users to remember to do, and even when done properly
ends up looking verbose and ugly.
Solution
This commit updates our v2 SolrJ code to throw RemoteSolrException when
error-responses are encountered.
It relies on a pre-existing attempt at error detection already present in our
SolrClient implementations. Code in many of our clients already attempts to
detect these errors by looking for a 'NamedList' key, "error", which it
(incorrectly) assumed would be present in the v2 case but which isn't
populated by the ResponseParser that our v2 SolrRequest/SolrResponse
implementations use by default.
This commit updates JacksonDatabindResponseParser to populate this
"error" key that SolrClient's were looking for, which in turn enables the
clients to correctly throw RemoteSolrException in these cases.
Tests
Unit tests for JacksonDatabindResponseParser in JacksonDatabindResponseParserTest. An integration test validating the end-to-end behavior has also been added to V2ApiIntegrationTest.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.