From 25e7631b9014a5d0729be7926313c498df1dc606 Mon Sep 17 00:00:00 2001 From: Steve Rowe Date: Thu, 21 Jun 2018 23:21:13 -0400 Subject: [PATCH] SOLR-12482: Config API returns status 0 for failed operations --- solr/CHANGES.txt | 2 + .../solr/handler/SolrConfigHandler.java | 11 ++---- .../solr/core/TestSolrConfigHandler.java | 38 +++++++++++++++++++ .../client/solrj/request/TestV2Request.java | 4 +- 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 960baf238d8..215c1285e34 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -104,6 +104,8 @@ Bug Fixes * SOLR-12413: If Zookeeper was pre-loaded with data before first-use, then the aliases information would be ignored. (David Smiley, Gaël Jourdan, Gus Heck) + +* SOLR-12482: Config API returns status 0 for failed operations. (Steve Rowe) Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java b/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java index ca9b69d6db8..53d543fa1d5 100644 --- a/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java @@ -425,8 +425,7 @@ public class SolrConfigHandler extends RequestHandlerBase implements SolrCoreAwa List errs = CommandOperation.captureErrors(ops); if (!errs.isEmpty()) { - resp.add(CommandOperation.ERR_MSGS, errs); - return; + throw new ApiBag.ExceptionWithErrObject(SolrException.ErrorCode.BAD_REQUEST,"error processing params", errs); } SolrResourceLoader loader = req.getCore().getResourceLoader(); @@ -489,9 +488,7 @@ public class SolrConfigHandler extends RequestHandlerBase implements SolrCoreAwa } List errs = CommandOperation.captureErrors(ops); if (!errs.isEmpty()) { - log.info("Failed to run commands. errors are {}", StrUtils.join(errs, ',')); - resp.add(CommandOperation.ERR_MSGS, errs); - return; + throw new ApiBag.ExceptionWithErrObject(SolrException.ErrorCode.BAD_REQUEST,"error processing commands", errs); } SolrResourceLoader loader = req.getCore().getResourceLoader(); @@ -633,7 +630,7 @@ public class SolrConfigHandler extends RequestHandlerBase implements SolrCoreAwa try { val = Integer.parseInt(val.toString()); } catch (Exception exp) { - op.addError(formatString(typeErr, typ.getSimpleName())); + op.addError(formatString(typeErr, name, typ.getSimpleName())); continue; } @@ -641,7 +638,7 @@ public class SolrConfigHandler extends RequestHandlerBase implements SolrCoreAwa try { val = Float.parseFloat(val.toString()); } catch (Exception exp) { - op.addError(formatString(typeErr, typ.getSimpleName())); + op.addError(formatString(typeErr, name, typ.getSimpleName())); continue; } diff --git a/solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java b/solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java index d2c9b80346c..88286e0397c 100644 --- a/solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java +++ b/solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java @@ -186,6 +186,19 @@ public class TestSolrConfigHandler extends RestTestBase { assertNull(response, map.get("errors")); // Will this ever be returned? } + public static void runConfigCommandExpectFailure(RestTestHarness harness, String uri, String payload, String expectedErrorMessage) throws Exception { + String json = SolrTestCaseJ4.json(payload); + log.info("going to send config command. path {} , payload: {}", uri, payload); + String response = harness.post(uri, json); + Map map = (Map)ObjectBuilder.getVal(new JSONParser(new StringReader(response))); + assertNotNull(response, map.get("errorMessages")); + assertNotNull(response, map.get("error")); + assertTrue("Expected status != 0: " + response, 0L != (Long)((Map)map.get("responseHeader")).get("status")); + List errorDetails = (List)((Map)map.get("error")).get("details"); + List errorMessages = (List)((Map)errorDetails.get(0)).get("errorMessages"); + assertTrue("Expected '" + expectedErrorMessage + "': " + response, + errorMessages.get(0).toString().contains(expectedErrorMessage)); + } public static void reqhandlertests(RestTestHarness writeHarness, String testServerBaseUrl, CloudSolrClient cloudSolrClient) throws Exception { String payload = "{\n" + @@ -500,6 +513,31 @@ public class TestSolrConfigHandler extends RestTestBase { assertEquals("Actual output "+ Utils.toJSONString(map), "org.apache.solr.search.LFUCache",getObjectByPath(map, true, ImmutableList.of( "caches", "lfuCacheDecayFalse"))); } + + public void testFailures() throws Exception { + String payload = "{ not-a-real-command: { param1: value1, param2: value2 } }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "Unknown operation 'not-a-real-command'"); + + payload = "{ set-property: { update.autoCreateFields: false } }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "'update.autoCreateFields' is not an editable property"); + + payload = "{ set-property: { updateHandler.autoCommit.maxDocs: false } }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "Property updateHandler.autoCommit.maxDocs must be of Integer type"); + + payload = "{ unset-property: not-an-editable-property }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "'[not-an-editable-property]' is not an editable property"); + + for (String component : new String[] { + "requesthandler", "searchcomponent", "initparams", "queryresponsewriter", "queryparser", + "valuesourceparser", "transformer", "updateprocessor", "queryconverter", "listener", "runtimelib"}) { + for (String operation : new String[] { "add", "update" }) { + payload = "{ " + operation + "-" + component + ": { param1: value1 } }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "'name' is a required field"); + } + payload = "{ delete-" + component + ": not-a-real-component-name }"; + runConfigCommandExpectFailure(restTestHarness, "/config", payload, "NO such "); + } + } public static class CacheTest extends DumpRequestHandler { @Override diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/request/TestV2Request.java b/solr/solrj/src/test/org/apache/solr/client/solrj/request/TestV2Request.java index 0280a9b668f..12b3271c887 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/request/TestV2Request.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/request/TestV2Request.java @@ -85,9 +85,11 @@ public class TestV2Request extends SolrCloudTestCase { assertSuccess(client, new V2Request.Builder("/c/_introspect").build()); + String requestHandlerName = "/x" + random().nextInt(); assertSuccess(client, new V2Request.Builder("/c/test/config") .withMethod(SolrRequest.METHOD.POST) - .withPayload("{'create-requesthandler' : { 'name' : '/x', 'class': 'org.apache.solr.handler.DumpRequestHandler' , 'startup' : 'lazy'}}") + .withPayload("{'create-requesthandler' : { 'name' : '" + requestHandlerName + + "', 'class': 'org.apache.solr.handler.DumpRequestHandler' , 'startup' : 'lazy'}}") .build()); assertSuccess(client, new V2Request.Builder("/c/test").withMethod(SolrRequest.METHOD.DELETE).build());