From d3154bc8d3e950e40219d3a422d9dd927dbe620c Mon Sep 17 00:00:00 2001 From: jinmeiliao Date: Wed, 24 Apr 2019 10:54:16 -0700 Subject: [PATCH] GEODE-5971: have command pipeline send ResultModel json across the wire (#3495) Co-authored-by: Owen Nichols --- .../extension/mock/MockExtensionCommands.java | 41 +++---- .../internal/beans/MemberMBeanBridge.java | 10 +- .../internal/cli/CliAroundInterceptor.java | 13 +-- .../internal/cli/remote/CommandExecutor.java | 22 +++- .../cli/remote/OnlineCommandProcessor.java | 15 +-- .../cli/result/model/ResultModel.java | 1 + .../cli/shell/GfshExecutionStrategy.java | 106 +++++------------- .../cli/shell/JmxOperationInvoker.java | 5 +- .../web/http/support/HttpRequester.java | 6 + .../web/shell/HttpOperationInvoker.java | 2 +- .../remote/OnlineCommandProcessorTest.java | 13 +-- .../cli/result/model/ResultModelTest.java | 11 ++ .../cli/shell/GfshExecutionStrategyTest.java | 29 ++--- .../LuceneCommandsSecurityDUnitTest.java | 6 +- ...lCommandsControllerProcessCommandTest.java | 50 ++++----- .../controllers/ShellCommandsController.java | 10 +- 16 files changed, 141 insertions(+), 199 deletions(-) diff --git a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java index cb3b8ef68093..70a5c2ae4c13 100644 --- a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java +++ b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java @@ -17,8 +17,8 @@ import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; +import org.springframework.shell.core.CommandMarker; import org.springframework.shell.core.annotation.CliCommand; import org.springframework.shell.core.annotation.CliOption; @@ -30,13 +30,12 @@ import org.apache.geode.distributed.internal.InternalLocator; import org.apache.geode.internal.cache.GemFireCacheImpl; import org.apache.geode.internal.cache.InternalCache; -import org.apache.geode.management.cli.GfshCommand; import org.apache.geode.management.cli.Result; -import org.apache.geode.management.cli.Result.Status; import org.apache.geode.management.internal.cli.CliUtil; import org.apache.geode.management.internal.cli.functions.CliFunctionResult; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.ModelCommandResult; import org.apache.geode.management.internal.cli.result.TabularResultData; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.management.internal.configuration.domain.XmlEntity; import org.apache.geode.management.internal.security.ResourceOperation; import org.apache.geode.security.ResourcePermission.Operation; @@ -47,7 +46,7 @@ * * @since GemFire 8.1 */ -public class MockExtensionCommands extends GfshCommand { +public class MockExtensionCommands implements CommandMarker { public static final String OPTION_VALUE = "value"; @@ -182,38 +181,26 @@ protected Result executeFunctionOnAllMembersTabulateResultPersist(final Function final ResultCollector> resultCollector = (ResultCollector>) CliUtil .executeFunction(function, args, members); - final List functionResults = - (List) resultCollector.getResult(); - - AtomicReference xmlEntity = new AtomicReference<>(); - final TabularResultData tabularResultData = ResultBuilder.createTabularResultData(); - final String errorPrefix = "ERROR: "; - for (CliFunctionResult functionResult : functionResults) { - boolean success = functionResult.isSuccessful(); - tabularResultData.accumulate("Member", functionResult.getMemberIdOrName()); - if (success) { - tabularResultData.accumulate("Status", functionResult.getMessage()); - xmlEntity.set(functionResult.getXmlEntity()); - } else { - tabularResultData.accumulate("Status", errorPrefix + functionResult.getMessage()); - tabularResultData.setStatus(Status.ERROR); - } - } + final List functionResults = resultCollector.getResult(); + + ResultModel resultModel = ResultModel.createMemberStatusResult(functionResults); - final Result result = ResultBuilder.buildResult(tabularResultData); + XmlEntity xmlEntity = functionResults.stream().filter(CliFunctionResult::isSuccessful) + .map(CliFunctionResult::getXmlEntity) + .findFirst().orElse(null); InternalConfigurationPersistenceService ccService = InternalLocator.getLocator().getConfigurationPersistenceService(); System.out.println("MockExtensionCommands: persisting xmlEntity=" + xmlEntity); - if (null != xmlEntity.get()) { + if (xmlEntity != null) { if (addXmlElement) { - ccService.addXmlEntity(xmlEntity.get(), null); + ccService.addXmlEntity(xmlEntity, null); } else { - ccService.deleteXmlEntity(xmlEntity.get(), null); + ccService.deleteXmlEntity(xmlEntity, null); } } - return result; + return new ModelCommandResult(resultModel); } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java index 902e8116f37b..311611f91ed6 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java @@ -108,9 +108,7 @@ import org.apache.geode.management.internal.beans.stats.StatsLatency; import org.apache.geode.management.internal.beans.stats.StatsRate; import org.apache.geode.management.internal.beans.stats.VMStatsMonitor; -import org.apache.geode.management.internal.cli.CommandResponseBuilder; import org.apache.geode.management.internal.cli.remote.OnlineCommandProcessor; -import org.apache.geode.management.internal.cli.result.CommandResult; import org.apache.geode.management.internal.cli.result.model.ResultModel; /** @@ -1504,13 +1502,9 @@ public String processCommand(String commandString, Map env, + commandServiceInitError); } - Object result = commandProcessor.executeCommand(commandString, env, stagedFilePaths); + ResultModel result = commandProcessor.executeCommand(commandString, env, stagedFilePaths); - if (result instanceof CommandResult) { - return CommandResponseBuilder.createCommandResponseJson(getMember(), (CommandResult) result); - } else { - return CommandResponseBuilder.createCommandResponseJson(getMember(), (ResultModel) result); - } + return result.toJson(); } public long getTotalDiskUsage() { diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java index 61e2a84125fa..7710f293b8da 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java @@ -16,7 +16,6 @@ import java.nio.file.Path; -import org.apache.geode.management.internal.cli.result.CommandResult; import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.management.internal.cli.shell.GfshExecutionStrategy; @@ -32,20 +31,10 @@ public interface CliAroundInterceptor { /** * called by the OperationInvoker before the command is executed */ - default Object preExecution(GfshParseResult parseResult) { + default ResultModel preExecution(GfshParseResult parseResult) { return ResultModel.createInfo(""); } - /** - * called by the OperationInvoker after the command is executed - * - * @param tempFile if the command's isFileDownloadOverHttp is true, the is the File downloaded - * after the http response is processed. - */ - default CommandResult postExecution(GfshParseResult parseResult, CommandResult commandResult, - Path tempFile) throws Exception { - return commandResult; - } default ResultModel postExecution(GfshParseResult parseResult, ResultModel resultModel, Path tempFile) throws Exception { diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java index 5ed084a66b3f..d65fb50f40f4 100755 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java @@ -25,6 +25,7 @@ import org.springframework.util.ReflectionUtils; import org.apache.geode.SystemFailure; +import org.apache.geode.annotations.VisibleForTesting; import org.apache.geode.distributed.internal.InternalConfigurationPersistenceService; import org.apache.geode.internal.logging.LogService; import org.apache.geode.internal.util.ArgumentRedactor; @@ -53,12 +54,20 @@ public class CommandExecutor { private Logger logger = LogService.getLogger(); - // used by the product + /** + * + * @return always return ResultModel for online command, for offline command, return either + * ResultModel or ExitShellRequest + */ public Object execute(GfshParseResult parseResult) { return execute(null, parseResult); } - // used by the GfshParserRule to pass in a mock command + /** + * @return always return ResultModel for online command, for offline command, return either + * ResultModel or ExitShellRequest + */ + @VisibleForTesting public Object execute(Object command, GfshParseResult parseResult) { String userInput = parseResult.getUserInput(); if (userInput != null) { @@ -68,6 +77,15 @@ public Object execute(Object command, GfshParseResult parseResult) { try { Object result = invokeCommand(command, parseResult); + if (result instanceof Result) { + Result customResult = (Result) result; + result = new ResultModel(); + InfoResultModel info = ((ResultModel) result).addInfo(); + while (customResult.hasNextLine()) { + info.addLine(customResult.nextLine()); + } + } + if (result == null) { return ResultModel.createError("Command returned null: " + parseResult); } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java index 630966562aee..7e9a6222cdee 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java @@ -32,7 +32,7 @@ import org.apache.geode.management.internal.cli.CommandManager; import org.apache.geode.management.internal.cli.GfshParseResult; import org.apache.geode.management.internal.cli.GfshParser; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.management.internal.cli.util.CommentSkipHelper; import org.apache.geode.management.internal.security.ResourceOperation; import org.apache.geode.security.ResourcePermission; @@ -82,11 +82,11 @@ public ParseResult parseCommand(String commentLessLine) throw new IllegalStateException("Command String should not be null."); } - public Object executeCommand(String command) { + public ResultModel executeCommand(String command) { return executeCommand(command, Collections.emptyMap(), null); } - public Object executeCommand(String command, Map env, + public ResultModel executeCommand(String command, Map env, List stagedFilePaths) { CommentSkipHelper commentSkipper = new CommentSkipHelper(); String commentLessLine = commentSkipper.skipComments(command); @@ -101,7 +101,7 @@ public Object executeCommand(String command, Map env, ParseResult parseResult = parseCommand(commentLessLine); if (parseResult == null) { - return ResultBuilder.createParsingErrorResult(command); + return ResultModel.createError("Could not parse command string. " + command); } Method method = parseResult.getMethod(); @@ -116,10 +116,11 @@ public Object executeCommand(String command, Map env, // this command processor does not execute commands that need fileData passed from client CliMetaData metaData = method.getAnnotation(CliMetaData.class); if (metaData != null && metaData.isFileUploaded() && stagedFilePaths == null) { - return ResultBuilder - .createUserErrorResult(command + " can not be executed only from server side"); + return ResultModel + .createError(command + " can not be executed only from server side"); } - return commandExecutor.execute((GfshParseResult) parseResult); + // we can do a direct cast because this only process online commands + return (ResultModel) commandExecutor.execute((GfshParseResult) parseResult); } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java index 5a7b77c20a34..e6f20a6b3565 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java @@ -293,6 +293,7 @@ public DataResultModel getDataSection(String name) { return (DataResultModel) getSection(name); } + @JsonIgnore public List getSectionNames() { List sectionNames = new ArrayList<>(); sections.forEach((k, v) -> sectionNames.add(k)); diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java index 36d7cb9927c4..f662b38d5a76 100755 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java @@ -20,7 +20,6 @@ import java.util.List; import java.util.Map; -import org.apache.commons.lang3.StringUtils; import org.springframework.shell.core.ExecutionStrategy; import org.springframework.shell.core.Shell; import org.springframework.shell.event.ParseResult; @@ -28,19 +27,13 @@ import org.apache.geode.internal.ClassPathLoader; import org.apache.geode.management.cli.CliMetaData; -import org.apache.geode.management.cli.Result; import org.apache.geode.management.cli.Result.Status; import org.apache.geode.management.internal.cli.CliAroundInterceptor; import org.apache.geode.management.internal.cli.CommandRequest; -import org.apache.geode.management.internal.cli.CommandResponse; -import org.apache.geode.management.internal.cli.CommandResponseBuilder; import org.apache.geode.management.internal.cli.GfshParseResult; import org.apache.geode.management.internal.cli.LogWrapper; -import org.apache.geode.management.internal.cli.i18n.CliStrings; import org.apache.geode.management.internal.cli.remote.CommandExecutor; -import org.apache.geode.management.internal.cli.result.CommandResult; import org.apache.geode.management.internal.cli.result.ModelCommandResult; -import org.apache.geode.management.internal.cli.result.ResultBuilder; import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.security.NotAuthorizedException; @@ -67,11 +60,12 @@ public class GfshExecutionStrategy implements ExecutionStrategy { * * @param parseResult that should be executed (never presented as null) * @return an object which will be rendered by the {@link Shell} implementation (may return null) + * this returns a ModelCommandResult for all the online commands. For offline-commands, + * this can return either a ModelCommandResult or ExitShellRequest * @throws RuntimeException which is handled by the {@link Shell} implementation */ @Override public Object execute(ParseResult parseResult) { - Result result; Method method = parseResult.getMethod(); // check if it's a shell only command @@ -94,8 +88,12 @@ public Object execute(ParseResult parseResult) { throw new IllegalStateException("Configuration error!"); } - result = executeOnRemote((GfshParseResult) parseResult); - return result; + ResultModel resultModel = executeOnRemote((GfshParseResult) parseResult); + + if (resultModel == null) { + return null; + } + return new ModelCommandResult(resultModel); } /** @@ -143,7 +141,7 @@ public void terminate() { * @return result of execution/processing of the command * @throws IllegalStateException if gfsh doesn't have an active connection. */ - private Result executeOnRemote(GfshParseResult parseResult) { + private ResultModel executeOnRemote(GfshParseResult parseResult) { Path tempFile = null; if (!shell.isConnectedAndReady()) { @@ -159,7 +157,6 @@ private Result executeOnRemote(GfshParseResult parseResult) { CliAroundInterceptor interceptor = null; String interceptorClass = getInterceptor(parseResult.getMethod()); - boolean useResultModel = false; // 1. Pre Remote Execution if (!CliMetaData.ANNOTATION_NULL_VALUE.equals(interceptorClass)) { @@ -171,26 +168,14 @@ private Result executeOnRemote(GfshParseResult parseResult) { } if (interceptor == null) { - return ResultBuilder.createBadConfigurationErrorResult("Interceptor Configuration Error"); - } - - Object preExecResult = interceptor.preExecution(parseResult); - if (preExecResult instanceof ResultModel) { - useResultModel = true; - if (((ResultModel) preExecResult).getStatus() != Status.OK) { - return new ModelCommandResult((ResultModel) preExecResult); - } - } else { // Must be Result - if (Status.ERROR.equals(((Result) preExecResult).getStatus())) { - return (Result) preExecResult; - } + return ResultModel.createError("Interceptor Configuration Error"); } - // when the preExecution yields a FileResult, we will get the fileData out of it - if (preExecResult instanceof ResultModel) { - ResultModel fileResult = (ResultModel) preExecResult; - fileData = fileResult.getFileList(); + ResultModel preExecResult = interceptor.preExecution(parseResult); + if (preExecResult.getStatus() != Status.OK) { + return preExecResult; } + fileData = preExecResult.getFileList(); } // 2. Remote Execution @@ -201,51 +186,25 @@ private Result executeOnRemote(GfshParseResult parseResult) { .processCommand(new CommandRequest(parseResult, env, fileData)); if (response == null) { - return ResultBuilder - .createBadResponseErrorResult("Response was null for: " + parseResult.getUserInput()); + return ResultModel.createError("Response was null for: " + parseResult.getUserInput()); } } catch (NotAuthorizedException e) { - return ResultBuilder - .createGemFireUnAuthorizedErrorResult("Unauthorized. Reason : " + e.getMessage()); + return ResultModel.createError("Unauthorized. Reason : " + e.getMessage()); } catch (Exception e) { shell.logSevere(e.getMessage(), e); - e.printStackTrace(); - return ResultBuilder.createBadResponseErrorResult( + return ResultModel.createError( "Error occurred while executing \"" + parseResult.getUserInput() + "\" on manager."); } finally { env.clear(); } // the response could be a string which is a json representation of the - // CommandResult/ResultModel object + // ResultModel object // it can also be a Path to a temp file downloaded from the rest http request - Object commandResult = null; + ResultModel commandResult = null; if (response instanceof String) { - try { - // if it's ResultModel - commandResult = ResultModel.fromJson((String) response); - useResultModel = true; - } catch (Exception ex) { - // if it's a CommandResult - CommandResponse commandResponse = - CommandResponseBuilder.prepareCommandResponseFromJson((String) response); + commandResult = ResultModel.fromJson((String) response); - if (commandResponse.isFailedToPersist()) { - shell.printAsSevere(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES); - logWrapper.severe(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES); - } - - String debugInfo = commandResponse.getDebugInfo(); - if (StringUtils.isNotBlank(debugInfo)) { - debugInfo = debugInfo.replaceAll("\n\n\n", "\n"); - debugInfo = debugInfo.replaceAll("\n\n", "\n"); - debugInfo = - debugInfo.replaceAll("\n", "\n[From Manager : " + commandResponse.getSender() + "]"); - debugInfo = "[From Manager : " + commandResponse.getSender() + "]" + debugInfo; - this.logWrapper.info(debugInfo); - } - commandResult = ResultBuilder.fromJson((String) response); - } } else if (response instanceof Path) { tempFile = (Path) response; } @@ -253,31 +212,20 @@ private Result executeOnRemote(GfshParseResult parseResult) { // 3. Post Remote Execution if (interceptor != null) { try { - if (useResultModel) { - commandResult = - interceptor.postExecution(parseResult, (ResultModel) commandResult, tempFile); - } else { - commandResult = - interceptor.postExecution(parseResult, (CommandResult) commandResult, tempFile); - } + commandResult = + interceptor.postExecution(parseResult, commandResult, tempFile); + } catch (Exception e) { logWrapper.severe("error running post interceptor", e); - commandResult = ResultBuilder.createGemFireErrorResult(e.getMessage()); + commandResult = ResultModel.createError(e.getMessage()); } } if (commandResult == null) { - commandResult = ResultBuilder - .createGemFireErrorResult("Unable to build commandResult using the remote response."); - } - - CommandResult gfshResult; - if (commandResult instanceof ResultModel) { - gfshResult = new ModelCommandResult((ResultModel) commandResult); - } else { - gfshResult = (CommandResult) commandResult; + commandResult = + ResultModel.createError("Unable to build ResultModel using the remote response."); } - return gfshResult; + return commandResult; } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java index 2eba0b518132..38c20b021cc9 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java @@ -263,7 +263,10 @@ public Set queryNames(final ObjectName objectName, final QueryExp qu } @Override - public Object processCommand(final CommandRequest commandRequest) { + /** + * this should only returns a json representation of ResultModel + */ + public String processCommand(final CommandRequest commandRequest) { List stagedFilePaths = null; try { diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/web/http/support/HttpRequester.java b/geode-core/src/main/java/org/apache/geode/management/internal/web/http/support/HttpRequester.java index d7ac1752a76b..3b93ad1805d9 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/web/http/support/HttpRequester.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/web/http/support/HttpRequester.java @@ -145,6 +145,9 @@ T exchange(URI url, HttpMethod method, MediaType mediaType, Object content, return response.getBody(); } + /** + * @return either a json representation of ResultModel or a Path + */ public Object executeWithResponseExtractor(URI url) { return restTemplate.execute(url, HttpMethod.POST, this::addHeaderValues, this::extractResponse); } @@ -153,6 +156,9 @@ void addHeaderValues(ClientHttpRequest request) { addHeaderValues(request.getHeaders()); } + /** + * @return either a json representation of ResultModel or a Path + */ Object extractResponse(ClientHttpResponse response) throws IOException { MediaType mediaType = response.getHeaders().getContentType(); if (mediaType.equals(MediaType.APPLICATION_JSON)) { diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/web/shell/HttpOperationInvoker.java b/geode-core/src/main/java/org/apache/geode/management/internal/web/shell/HttpOperationInvoker.java index d5485c4b13ff..a925e06373ee 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/web/shell/HttpOperationInvoker.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/web/shell/HttpOperationInvoker.java @@ -421,7 +421,7 @@ public String getRemoteVersion() { * (execution). * * @param command the command requested/entered by the user to be processed. - * @return the result of the command execution. + * @return the result of the command execution, either a json string of ResultModel or a Path */ @Override public Object processCommand(final CommandRequest command) { diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java index 4c3d27dbecc1..b2f3193bf14e 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java @@ -29,8 +29,7 @@ import org.junit.rules.TemporaryFolder; import org.apache.geode.internal.security.SecurityService; -import org.apache.geode.management.cli.Result; -import org.apache.geode.management.internal.cli.result.CommandResult; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.security.NotAuthorizedException; public class OnlineCommandProcessorTest { @@ -39,7 +38,7 @@ public class OnlineCommandProcessorTest { SecurityService securityService; CommandExecutor executor; OnlineCommandProcessor onlineCommandProcessor; - Result result; + ResultModel result; @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); @@ -49,7 +48,7 @@ public void before() { properties = new Properties(); securityService = mock(SecurityService.class); executor = mock(CommandExecutor.class); - result = mock(Result.class); + result = mock(ResultModel.class); when(executor.execute(any())).thenReturn(result); onlineCommandProcessor = @@ -75,7 +74,7 @@ public void executeStripsComments() throws Exception { @Test public void executeReturnsExecutorResult() throws Exception { - Object commandResult = onlineCommandProcessor.executeCommand("start locator"); + ResultModel commandResult = onlineCommandProcessor.executeCommand("start locator"); assertThat(commandResult).isSameAs(result); } @@ -88,8 +87,8 @@ public void handlesNotAuthorizedException() throws Exception { @Test public void handlesParsingError() throws Exception { - Object commandResult = onlineCommandProcessor.executeCommand("foo --bar"); - assertThat(commandResult).isInstanceOf(CommandResult.class); + ResultModel commandResult = onlineCommandProcessor.executeCommand("foo --bar"); + assertThat(commandResult).isInstanceOf(ResultModel.class); assertThat(commandResult.toString()).contains("Could not parse command string. foo --bar"); } } diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/model/ResultModelTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/model/ResultModelTest.java index b5e29c418a8d..51c97df78bbd 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/model/ResultModelTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/result/model/ResultModelTest.java @@ -181,4 +181,15 @@ public void serializeFileToDownload() throws Exception { ResultModel resultModel = mapper.readValue(json, ResultModel.class); assertThat(resultModel.getFileToDownload()).isEqualTo(file.toPath()); } + + @Test + public void serializeInfoResult() throws Exception { + InfoResultModel info = result.addInfo(); + info.addLine("line2"); + ObjectMapper mapper = GeodeJsonMapper.getMapper(); + String json = mapper.writeValueAsString(result); + System.out.println(json); + ResultModel resultModel = mapper.readValue(json, ResultModel.class); + assertThat(resultModel.getSectionNames()).containsExactly("info"); + } } diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyTest.java index 40dfdd2d7f37..dc2e8b7b650f 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyTest.java @@ -29,11 +29,8 @@ import org.apache.geode.management.cli.Result; import org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor; import org.apache.geode.management.internal.cli.CommandRequest; -import org.apache.geode.management.internal.cli.CommandResponseBuilder; import org.apache.geode.management.internal.cli.GfshParseResult; import org.apache.geode.management.internal.cli.LogWrapper; -import org.apache.geode.management.internal.cli.result.CommandResult; -import org.apache.geode.management.internal.cli.result.ResultBuilder; import org.apache.geode.management.internal.cli.result.model.ResultModel; /** @@ -97,9 +94,8 @@ public void testOnLineCommandWhenGfshisOnLine() throws Exception { when(gfsh.isConnectedAndReady()).thenReturn(true); OperationInvoker invoker = mock(OperationInvoker.class); - Result offLineResult = new Commands().onlineCommand(); - String jsonResult = CommandResponseBuilder.createCommandResponseJson("memberName", - (CommandResult) offLineResult); + ResultModel offLineResult = new Commands().onlineCommand(); + String jsonResult = offLineResult.toJson(); when(invoker.processCommand(any(CommandRequest.class))).thenReturn(jsonResult); when(gfsh.getOperationInvoker()).thenReturn(invoker); Result result = (Result) gfshExecutionStrategy.execute(parsedCommand); @@ -114,9 +110,8 @@ public void resolveInterceptorClassName() throws Exception { when(gfsh.isConnectedAndReady()).thenReturn(true); OperationInvoker invoker = mock(OperationInvoker.class); - Result interceptedResult = new Commands().interceptedCommand(); - String jsonResult = CommandResponseBuilder.createCommandResponseJson("memberName", - (CommandResult) interceptedResult); + ResultModel interceptedResult = new Commands().interceptedCommand(); + String jsonResult = interceptedResult.toJson(); when(invoker.processCommand(any(CommandRequest.class))).thenReturn(jsonResult); when(gfsh.getOperationInvoker()).thenReturn(invoker); Result result = (Result) gfshExecutionStrategy.execute(parsedCommand); @@ -129,8 +124,8 @@ public void resolveInterceptorClassName() throws Exception { */ public static class Commands implements CommandMarker { @CliMetaData(shellOnly = true) - public Result offlineCommand() { - return ResultBuilder.createInfoResult(COMMAND1_SUCCESS); + public ResultModel offlineCommand() { + return ResultModel.createInfo(COMMAND1_SUCCESS); } @CliMetaData(shellOnly = true) @@ -139,14 +134,14 @@ public ResultModel offlineCommand2() { } @CliMetaData(shellOnly = false) - public Result onlineCommand() { - return ResultBuilder.createInfoResult(COMMAND2_SUCCESS); + public ResultModel onlineCommand() { + return ResultModel.createInfo(COMMAND2_SUCCESS); } @CliMetaData(shellOnly = false, interceptor = "org.apache.geode.management.internal.cli.shell.GfshExecutionStrategyTest$TestInterceptor") - public Result interceptedCommand() { - return ResultBuilder.createInfoResult(COMMAND4_SUCCESS); + public ResultModel interceptedCommand() { + return ResultModel.createInfo(COMMAND4_SUCCESS); } } @@ -156,10 +151,10 @@ public Result interceptedCommand() { public static class TestInterceptor extends AbstractCliAroundInterceptor { @Override - public Result preExecution(GfshParseResult parseResult) { + public ResultModel preExecution(GfshParseResult parseResult) { parseResult.setUserInput(AFTER_INTERCEPTION_MESSAGE); - return ResultBuilder.createInfoResult("Interceptor Result"); + return ResultModel.createInfo("Interceptor Result"); } } } diff --git a/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java b/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java index aa75f041e1e4..76087b21f016 100644 --- a/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java +++ b/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java @@ -37,8 +37,6 @@ import org.apache.geode.management.cli.Result; import org.apache.geode.management.internal.cli.i18n.CliStrings; 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.management.internal.cli.util.CommandStringBuilder; import org.apache.geode.test.dunit.rules.ClusterStartupRule; import org.apache.geode.test.dunit.rules.MemberVM; @@ -208,9 +206,7 @@ protected void createIndexAndRegion() throws Exception { private void verifyResult(UserNameAndExpectedResponse user, CommandResult result) { if (user.getExpectAuthorizationError()) { - assertTrue(result.getResultData() instanceof ErrorResultData); - assertEquals(ResultBuilder.ERRORCODE_UNAUTHORIZED, - ((ErrorResultData) result.getResultData()).getErrorCode()); + assertEquals(Result.Status.ERROR, result.getStatus()); } else { assertEquals(Result.Status.OK, result.getStatus()); } diff --git a/geode-web/src/integrationTest/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerProcessCommandTest.java b/geode-web/src/integrationTest/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerProcessCommandTest.java index 1247e4c47547..8cb0dc98166d 100644 --- a/geode-web/src/integrationTest/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerProcessCommandTest.java +++ b/geode-web/src/integrationTest/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerProcessCommandTest.java @@ -15,10 +15,13 @@ package org.apache.geode.management.internal.web.controllers; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; import java.io.File; import java.io.IOException; -import java.util.Map; import org.apache.commons.io.FileUtils; import org.apache.commons.io.IOUtils; @@ -30,59 +33,48 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; -import org.springframework.web.multipart.MultipartFile; -import org.apache.geode.management.internal.cli.CommandResponseBuilder; -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.InfoResultData; -import org.apache.geode.management.internal.cli.result.LegacyCommandResult; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.model.FileResultModel; +import org.apache.geode.management.internal.cli.result.model.ResultModel; public class ShellCommandsControllerProcessCommandTest { @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); private ShellCommandsController controller; - private CommandResult fakeResult; + private ResultModel fakeResult; @Before - public void setup() { - - controller = new ShellCommandsController() { - @Override - protected String processCommand(String command, final Map environment, - MultipartFile[] fileData) { - return CommandResponseBuilder.createCommandResponseJson("someMember", fakeResult); - } - }; + public void setup() throws IOException { + controller = spy(ShellCommandsController.class); } @Test public void infoOkResult() throws IOException { - fakeResult = new LegacyCommandResult(new InfoResultData("Some info message")); - + fakeResult = ResultModel.createInfo("Some info message"); + doReturn(fakeResult.toJson()).when(controller).processCommand(anyString(), any(), any()); ResponseEntity responseJsonStream = controller.command("xyz", null); assertThatContentTypeEquals(responseJsonStream, MediaType.APPLICATION_JSON); String responseJson = toString(responseJsonStream); - CommandResult result = ResultBuilder.fromJson(responseJson); + ResultModel result = ResultModel.fromJson(responseJson); - assertThat(result.nextLine()).isEqualTo(fakeResult.nextLine()); + assertThat(result.getInfoSection("info").getContent().get(0)) + .isEqualTo(fakeResult.getInfoSection("info").getContent().get(0)); } @Test public void errorResult() throws IOException { - ErrorResultData errorResultData = new ErrorResultData("Some error message"); - fakeResult = new LegacyCommandResult(errorResultData); - + fakeResult = ResultModel.createError("Some error message"); + doReturn(fakeResult.toJson()).when(controller).processCommand(anyString(), any(), any()); ResponseEntity responseJsonStream = controller.command("xyz", null); assertThatContentTypeEquals(responseJsonStream, MediaType.APPLICATION_JSON); String responseJson = toString(responseJsonStream); - CommandResult result = ResultBuilder.fromJson(responseJson); + ResultModel result = ResultModel.fromJson(responseJson); - assertThat(result.nextLine()).isEqualTo(fakeResult.nextLine()); + assertThat(result.getInfoSection("info").getContent().get(0)) + .isEqualTo(fakeResult.getInfoSection("info").getContent().get(0)); } @Test @@ -90,8 +82,10 @@ public void resultWithFile() throws IOException { File tempFile = temporaryFolder.newFile(); FileUtils.writeStringToFile(tempFile, "some file contents", "UTF-8"); - fakeResult = new LegacyCommandResult(tempFile.toPath()); + fakeResult = new ResultModel(); + fakeResult.addFile(tempFile, FileResultModel.FILE_TYPE_FILE); + doReturn(fakeResult.toJson()).when(controller).processCommand(anyString(), any(), any()); ResponseEntity responseFileStream = controller.command("xyz", null); assertThatContentTypeEquals(responseFileStream, MediaType.APPLICATION_OCTET_STREAM); diff --git a/geode-web/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java b/geode-web/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java index 16e53e59b0ae..55dd04e30109 100644 --- a/geode-web/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java +++ b/geode-web/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java @@ -52,8 +52,7 @@ import org.apache.geode.internal.util.IOUtils; import org.apache.geode.management.cli.Result; import org.apache.geode.management.internal.cli.i18n.CliStrings; -import org.apache.geode.management.internal.cli.result.CommandResult; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.management.internal.cli.util.CommandStringBuilder; import org.apache.geode.management.internal.web.domain.QueryParameterSource; @@ -189,8 +188,9 @@ public String releaseVersion() { private ResponseEntity getResponse(String result) { - CommandResult commandResult = ResultBuilder.fromJson(result); - if (commandResult.getStatus().equals(Result.Status.OK) && commandResult.hasFileToDownload()) { + ResultModel commandResult = ResultModel.fromJson(result); + if (commandResult.getStatus().equals(Result.Status.OK) + && commandResult.getFileToDownload() != null) { return getFileDownloadResponse(commandResult); } else { return getJsonResponse(result); @@ -208,7 +208,7 @@ private ResponseEntity getJsonResponse(String result) { } } - private ResponseEntity getFileDownloadResponse(CommandResult commandResult) { + private ResponseEntity getFileDownloadResponse(ResultModel commandResult) { HttpHeaders respHeaders = new HttpHeaders(); Path filePath = commandResult.getFileToDownload(); try {