This is an automated email from the ASF dual-hosted git repository. jinmeiliao pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push: new b16a709 GEODE-3819: correctly set the result status code when building the re… (#935) b16a709 is described below commit b16a709bc72589446959740f88de8d25793c9e17 Author: jinmeiliao <jil...@pivotal.io> AuthorDate: Wed Oct 18 08:59:57 2017 -0700 GEODE-3819: correctly set the result status code when building the re… (#935) * GEODE-3819: correctly set the result status code when building the result from json string --- .../internal/cli/commands/CreateRegionCommand.java | 8 +------- .../management/internal/cli/result/ErrorResultData.java | 6 ++++-- .../management/internal/cli/result/ResultBuilder.java | 7 +++++++ .../management/internal/cli/result/ResultBuilderTest.java | 9 +++++++++ .../internal/security/GfshCommandsSecurityTest.java | 14 +++++--------- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java index e5aced8..708c3f1 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java @@ -29,7 +29,6 @@ import org.springframework.shell.core.annotation.CliCommand; import org.springframework.shell.core.annotation.CliOption; import org.apache.geode.cache.DataPolicy; -import org.apache.geode.cache.PartitionResolver; import org.apache.geode.cache.Region; import org.apache.geode.cache.RegionAttributes; import org.apache.geode.cache.RegionShortcut; @@ -159,7 +158,6 @@ public class CreateRegionCommand implements GfshCommand { ) { Result result; AtomicReference<XmlEntity> xmlEntity = new AtomicReference<>(); - String lastError = null; try { InternalCache cache = getCache(); @@ -297,7 +295,7 @@ public class CreateRegionCommand implements GfshCommand { if (success) { xmlEntity.set(regionCreateResult.getXmlEntity()); } else { - lastError = regionCreateResult.getMessage(); + tabularResultData.setStatus(Result.Status.ERROR); } } result = ResultBuilder.buildResult(tabularResultData); @@ -308,10 +306,6 @@ public class CreateRegionCommand implements GfshCommand { result = ResultBuilder.createUserErrorResult(e.getMessage()); } - if (lastError != null) { - result = ResultBuilder.createUserErrorResult(lastError); - } - if (xmlEntity.get() != null) { persistClusterConfiguration(result, () -> getSharedConfiguration().addXmlEntity(xmlEntity.get(), groups)); diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ErrorResultData.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ErrorResultData.java index 9edcdbf..9734bf2 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ErrorResultData.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ErrorResultData.java @@ -98,8 +98,10 @@ public class ErrorResultData extends InfoResultData { @Override public void setStatus(final Status status) { - throw new UnsupportedOperationException( - "The status of an ErrorResultData result must always be ERROR"); + if (status != Status.ERROR) { + throw new UnsupportedOperationException( + "The status of an ErrorResultData result must always be ERROR"); + } } @Override diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java index 460ade2..22db17c 100755 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java @@ -230,6 +230,13 @@ public class ResultBuilder { resultData = errorResultData; } + Integer statusCode = (Integer) jsonObject.get("status"); + if (Result.Status.OK.getCode() == statusCode) { + resultData.setStatus(Result.Status.OK); + } else { + resultData.setStatus(Result.Status.ERROR); + } + result = buildResult(resultData); String fileToDownloadPath = jsonObject.getString("fileToDownload"); diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java index 0ce89c1..3b4e371 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java @@ -128,4 +128,13 @@ public class ResultBuilderTest { result.addSection(); } + + @Test + public void errorCodeCorrectlyUpdated() throws Exception { + String json = + "{\"contentType\":\"table\",\"data\":{\"content\":{\"Member\":[\"server\"],\"Status\":[\"ERROR: Bad.\"]},\"footer\":\"\",\"header\":\"\",\"type-class\":\"org.apache.geode.management.internal.cli.CommandResponse.Data\"},\"debugInfo\":\"\",\"failedToPersist\":false,\"fileToDownload\":null,\"page\":\"1/1\",\"sender\":\"server\",\"status\":-1,\"tokenAccessor\":\"__NULL__\",\"type-class\":\"org.apache.geode.management.internal.cli.CommandResponse\",\"version\":\"1.3.0-SNAPSHOT\",\"whe [...] + + CommandResult result = ResultBuilder.fromJson(json); + assertThat(result.getStatus().getCode()).isEqualTo(-1); + } } diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java index abbf7c0..648784f 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java @@ -32,11 +32,11 @@ import org.apache.geode.management.internal.cli.result.CommandResult; import org.apache.geode.management.internal.cli.result.ErrorResultData; import org.apache.geode.management.internal.cli.result.ResultBuilder; import org.apache.geode.security.SimpleTestSecurityManager; +import org.apache.geode.test.junit.categories.IntegrationTest; +import org.apache.geode.test.junit.categories.SecurityTest; import org.apache.geode.test.junit.rules.ConnectionConfiguration; import org.apache.geode.test.junit.rules.GfshShellConnectionRule; import org.apache.geode.test.junit.rules.ServerStarterRule; -import org.apache.geode.test.junit.categories.IntegrationTest; -import org.apache.geode.test.junit.categories.SecurityTest; @Category({IntegrationTest.class, SecurityTest.class}) public class GfshCommandsSecurityTest { @@ -135,12 +135,9 @@ public class GfshCommandsSecurityTest { CommandResult result = gfshConnection.executeCommand(permitted.getCommand()); assertThat(result).isNotNull(); - if (result.getResultData() instanceof ErrorResultData) { - assertThat(ResultBuilder.ERRORCODE_UNAUTHORIZED).describedAs(permitted.getCommand()) - .isNotEqualTo(((ErrorResultData) result.getResultData()).getErrorCode()); - } else { - assertThat(Result.Status.OK).describedAs(permitted.toString()) - .isEqualTo(result.getStatus()); + // for permitted commands, if any error happens, it's not an Unauthorized error + if (result.getStatus() == Result.Status.ERROR) { + assertThat(result.getContent().toString()).doesNotContain("not authorized"); } } @@ -148,7 +145,6 @@ public class GfshCommandsSecurityTest { List<TestCommand> others = TestCommand.getOnlineCommands(); others.removeAll(allPermitted); for (TestCommand other : others) { - System.out.println("Processing unauthorized command: " + other.getCommand()); CommandResult result = gfshConnection.executeCommand(other.getCommand()); int errorCode = ((ErrorResultData) result.getResultData()).getErrorCode(); -- To stop receiving notification emails like this one, please contact ['"commits@geode.apache.org" <commits@geode.apache.org>'].