From 53040984f62eedd425c98eb6f60bc54a0e83258b Mon Sep 17 00:00:00 2001 From: Munendra S N Date: Wed, 10 Jun 2020 19:11:52 +0530 Subject: [PATCH] SOLR-14345: return correct err msg when non-binary resp parser is used * This adds support to parse error properly in case of non-binary resp parser but the problem still exists for noopResponseParser --- solr/CHANGES.txt | 2 ++ .../solr/search/json/TestJsonRequest.java | 31 ++++++++++--------- .../client/solrj/impl/Http2SolrClient.java | 25 +++++++++++---- .../client/solrj/impl/HttpSolrClient.java | 22 ++++++++++--- .../java/org/apache/solr/SolrTestCaseHS.java | 17 ++++++---- 5 files changed, 65 insertions(+), 32 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index ef87d32ce7a..ee44d1b4d3f 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -233,6 +233,8 @@ Bug Fixes * SOLR-14550: Fix duplicates issue in Atomic updates with add-distinct (Thomas Corthals, Munendra S N) +* SOLR-14345: Return proper error message when non-BinaryResponseParser is used in solrJ (Munendra S N) + Other Changes --------------------- * SOLR-14197: SolrResourceLoader: marked many methods as deprecated, and in some cases rerouted exiting logic to avoid diff --git a/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java b/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java index 9451ddf9ec7..420730599b4 100644 --- a/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java +++ b/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java @@ -32,6 +32,8 @@ import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; +import static org.hamcrest.core.StringContains.containsString; + @LuceneTestCase.SuppressCodecs({"Lucene3x","Lucene40","Lucene41","Lucene42","Lucene45","Appending"}) public class TestJsonRequest extends SolrTestCaseHS { @@ -84,6 +86,8 @@ public class TestJsonRequest extends SolrTestCaseHS { public static void doJsonRequest(Client client, boolean isDistrib) throws Exception { addDocs(client); + ignoreException("Expected JSON"); + // test json param client.testJQ( params("json","{query:'cat_s:A'}") , "response/numFound==2" @@ -92,6 +96,7 @@ public class TestJsonRequest extends SolrTestCaseHS { // invalid value SolrException ex = expectThrows(SolrException.class, () -> client.testJQ(params("q", "*:*", "json", "5"))); assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertThat(ex.getMessage(), containsString("Expected JSON Object but got Long=5")); // this is to verify other json params are not affected client.testJQ( params("q", "cat_s:A", "json.limit", "1"), @@ -389,23 +394,19 @@ public class TestJsonRequest extends SolrTestCaseHS { , "response/numFound==3", isDistrib? "" : "response/docs==[{id:'4'},{id:'1'},{id:'5'}]" ); - try { - client.testJQ(params("json", "{query:{'lucene':'foo_s:ignore_exception'}}")); // TODO: this seems like a reasonable capability that we would want to support in the future. It should be OK to make this pass. - fail(); - } catch (Exception e) { - assertTrue(e.getMessage().contains("foo_s")); - } + // TODO: this seems like a reasonable capability that we would want to support in the future. It should be OK to make this pass. + Exception e = expectThrows(Exception.class, () -> { + client.testJQ(params("json", "{query:{'lucene':'foo_s:ignore_exception'}}")); + }); + assertThat(e.getMessage(), containsString("foo_s")); - try { - // test failure on unknown parameter - client.testJQ(params("json", "{query:'cat_s:A', foobar_ignore_exception:5}") - , "response/numFound==2" - ); - fail(); - } catch (Exception e) { - assertTrue(e.getMessage().contains("foobar")); - } + // test failure on unknown parameter + e = expectThrows(Exception.class, () -> { + client.testJQ(params("json", "{query:'cat_s:A', foobar_ignore_exception:5}"), "response/numFound==2"); + }); + assertThat(e.getMessage(), containsString("foobar")); + resetExceptionIgnores(); } private static void doParamRefDslTest(Client client) throws Exception { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java index 3c321468194..9e46a96d6ca 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java @@ -69,6 +69,7 @@ import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.ObjectReleaseTracker; import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.apache.solr.common.util.Utils; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.ProtocolHandlers; @@ -629,6 +630,7 @@ public class Http2SolrClient extends SolrClient { return processor == null || processor instanceof InputStreamResponseParser; } + @SuppressWarnings({"unchecked"}) private NamedList processErrorsAndResponse(Response response, final ResponseParser processor, InputStream is, @@ -701,13 +703,24 @@ public class Http2SolrClient extends SolrClient { NamedList metadata = null; String reason = null; try { - NamedList err = (NamedList) rsp.get("error"); - if (err != null) { - reason = (String) err.get("msg"); - if (reason == null) { - reason = (String) err.get("trace"); + if (error != null) { + reason = (String) Utils.getObjectByPath(error, false, Collections.singletonList("msg")); + if(reason == null) { + reason = (String) Utils.getObjectByPath(error, false, Collections.singletonList("trace")); + } + Object metadataObj = Utils.getObjectByPath(error, false, Collections.singletonList("metadata")); + if (metadataObj instanceof NamedList) { + metadata = (NamedList) metadataObj; + } else if (metadataObj instanceof List) { + // NamedList parsed as List convert to NamedList again + List list = (List) metadataObj; + metadata = new NamedList<>(list.size()/2); + for (int i = 0; i < list.size(); i+=2) { + metadata.add((String)list.get(i), (String) list.get(i+1)); + } + } else if (metadataObj instanceof Map) { + metadata = new NamedList((Map) metadataObj); } - metadata = (NamedList) err.get("metadata"); } } catch (Exception ex) {} if (reason == null) { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index 4ee169858ef..a7348318f36 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -533,6 +533,7 @@ public class HttpSolrClient extends BaseHttpSolrClient { private static final List errPath = Arrays.asList("metadata", "error-class");//Utils.getObjectByPath(err, false,"metadata/error-class") + @SuppressWarnings({"unchecked"}) protected NamedList executeMethod(HttpRequestBase method, Principal userPrincipal, final ResponseParser processor, final boolean isV2Api) throws SolrServerException { method.addHeader("User-Agent", AGENT); @@ -643,13 +644,24 @@ public class HttpSolrClient extends BaseHttpSolrClient { NamedList metadata = null; String reason = null; try { - NamedList err = (NamedList) rsp.get("error"); - if (err != null) { - reason = (String) err.get("msg"); + if (error != null) { + reason = (String) Utils.getObjectByPath(error, false, Collections.singletonList("msg")); if(reason == null) { - reason = (String) err.get("trace"); + reason = (String) Utils.getObjectByPath(error, false, Collections.singletonList("trace")); + } + Object metadataObj = Utils.getObjectByPath(error, false, Collections.singletonList("metadata")); + if (metadataObj instanceof NamedList) { + metadata = (NamedList) metadataObj; + } else if (metadataObj instanceof List) { + // NamedList parsed as List convert to NamedList again + List list = (List) metadataObj; + metadata = new NamedList<>(list.size()/2); + for (int i = 0; i < list.size(); i+=2) { + metadata.add((String)list.get(i), (String) list.get(i+1)); + } + } else if (metadataObj instanceof Map) { + metadata = new NamedList((Map) metadataObj); } - metadata = (NamedList)err.get("metadata"); } } catch (Exception ex) {} if (reason == null) { diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseHS.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseHS.java index 7d3e7cab2a6..cc572a962b8 100644 --- a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseHS.java +++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseHS.java @@ -45,12 +45,14 @@ import org.apache.solr.client.solrj.embedded.JettySolrRunner; import org.apache.solr.client.solrj.impl.NoOpResponseParser; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.UpdateRequest; +import org.apache.solr.client.solrj.response.DelegationTokenResponse; import org.apache.solr.client.solrj.response.UpdateResponse; import org.apache.solr.common.SolrException; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreDescriptor; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.schema.IndexSchema; @@ -225,12 +227,15 @@ public class SolrTestCaseHS extends SolrTestCaseJ4 { query.setPath(path); } - query.setResponseParser(new NoOpResponseParser(wt)); - NamedList rsp = client.request(query); - - String raw = (String)rsp.get("response"); - - return raw; + if ("json".equals(wt)) { + query.setResponseParser(new DelegationTokenResponse.JsonMapResponseParser()); + NamedList rsp = client.request(query); + return Utils.toJSONString(rsp); + } else { + query.setResponseParser(new NoOpResponseParser(wt)); + NamedList rsp = client.request(query); + return (String)rsp.get("response"); + } } public static String getQueryResponse(String wt, SolrParams params) throws Exception {