GEODE-5971: refactor ExportLogsCommand and ExecuteScriptCommand to us…

geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java

@@ -17,7 +17,6 @@
 import java.nio.file.Path;
 
 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.shell.GfshExecutionStrategy;
 
@@ -34,7 +33,7 @@ public interface CliAroundInterceptor {
    * called by the OperationInvoker before the command is executed
    */
   default Object preExecution(GfshParseResult parseResult) {
-    return ResultBuilder.createInfoResult("");
+    return ResultModel.createInfo("");
   }
 
   /**

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AlterRuntimeConfigCommand.java

@@ -36,15 +36,13 @@
 import org.apache.geode.internal.logging.log4j.LogLevel;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
 import org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor;
 import org.apache.geode.management.internal.cli.CliUtil;
 import org.apache.geode.management.internal.cli.GfshParseResult;
 import org.apache.geode.management.internal.cli.functions.AlterRuntimeConfigFunction;
 import org.apache.geode.management.internal.cli.functions.CliFunctionResult;
 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.ResultBuilder;
 import org.apache.geode.management.internal.cli.result.model.InfoResultModel;
 import org.apache.geode.management.internal.cli.result.model.ResultModel;
 import org.apache.geode.management.internal.configuration.domain.XmlEntity;
@@ -243,13 +241,13 @@ public ResultModel alterRuntimeConfig(
 
   public static class AlterRuntimeInterceptor extends AbstractCliAroundInterceptor {
     @Override
-    public Result preExecution(GfshParseResult parseResult) {
+    public ResultModel preExecution(GfshParseResult parseResult) {
       // validate log level
       String logLevel = parseResult.getParamValueAsString("log-level");
       if (StringUtils.isNotBlank(logLevel) && (LogLevel.getLevel(logLevel) == null)) {
-        return ResultBuilder.createUserErrorResult("Invalid log level: " + logLevel);
+        return ResultModel.createError("Invalid log level: " + logLevel);
       }
-      return ResultBuilder.createInfoResult("");
+      return ResultModel.createInfo("");
     }
   }
 }

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ChangeLogLevelCommand.java

@@ -34,7 +34,6 @@
 import org.apache.geode.internal.logging.log4j.LogLevel;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
 import org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor;
 import org.apache.geode.management.internal.cli.GfshParseResult;
 import org.apache.geode.management.internal.cli.functions.ChangeLogLevelFunction;
@@ -148,14 +147,14 @@ public ResultModel changeLogLevel(
 
   public static class ChangeLogLevelCommandInterceptor extends AbstractCliAroundInterceptor {
     @Override
-    public Result preExecution(GfshParseResult parseResult) {
+    public ResultModel preExecution(GfshParseResult parseResult) {
       // validate log level
       String logLevel = parseResult.getParamValueAsString("loglevel");
       if (StringUtils.isBlank(logLevel) || LogLevel.getLevel(logLevel) == null) {
-        return ResultBuilder.createUserErrorResult("Invalid log level: " + logLevel);
+        return ResultModel.createError("Invalid log level: " + logLevel);
       }
 
-      return ResultBuilder.createInfoResult("");
+      return ResultModel.createInfo("");
     }
   }
 }

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExecuteScriptCommand.java

@@ -22,31 +22,23 @@
 
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
 import org.apache.geode.management.internal.cli.i18n.CliStrings;
-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.shell.Gfsh;
 
-public class ExecuteScriptCommand extends InternalGfshCommand {
+public class ExecuteScriptCommand extends OfflineGfshCommand {
   @CliCommand(value = {CliStrings.RUN}, help = CliStrings.RUN__HELP)
   @CliMetaData(shellOnly = true, relatedTopic = {CliStrings.TOPIC_GFSH})
-  public Result executeScript(
+  public ResultModel executeScript(
       @CliOption(key = CliStrings.RUN__FILE, optionContext = ConverterHint.FILE, mandatory = true,
           help = CliStrings.RUN__FILE__HELP) File file,
       @CliOption(key = {CliStrings.RUN__QUIET}, specifiedDefaultValue = "true",
           unspecifiedDefaultValue = "false", help = CliStrings.RUN__QUIET__HELP) boolean quiet,
       @CliOption(key = {CliStrings.RUN__CONTINUEONERROR}, specifiedDefaultValue = "true",
           unspecifiedDefaultValue = "false",
           help = CliStrings.RUN__CONTINUEONERROR__HELP) boolean continueOnError) {
-    Result result;
-
     Gfsh gfsh = Gfsh.getCurrentInstance();
-    try {
-      result = gfsh.executeScript(file, quiet, continueOnError);
-    } catch (IllegalArgumentException e) {
-      result = ResultBuilder.createShellClientErrorResult(e.getMessage());
-    } // let CommandProcessingException go to the caller
-    return result;
+    return gfsh.executeScript(file, quiet, continueOnError);
   }
 
 }

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java

@@ -38,18 +38,17 @@
 import org.apache.geode.management.ManagementException;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.cli.GfshCommand;
 import org.apache.geode.management.internal.cli.functions.ExportLogsFunction;
 import org.apache.geode.management.internal.cli.functions.SizeExportLogsFunction;
 import org.apache.geode.management.internal.cli.i18n.CliStrings;
-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.ResultModel;
 import org.apache.geode.management.internal.cli.util.ExportLogsCacheWriter;
 import org.apache.geode.management.internal.configuration.utils.ZipUtils;
 import org.apache.geode.management.internal.security.ResourceOperation;
 import org.apache.geode.security.ResourcePermission;
 
-public class ExportLogsCommand extends InternalGfshCommand {
+public class ExportLogsCommand extends GfshCommand {
 
   private static final Logger logger = LogService.getLogger();
 
@@ -66,7 +65,7 @@ public class ExportLogsCommand extends InternalGfshCommand {
       relatedTopic = {CliStrings.TOPIC_GEODE_SERVER, CliStrings.TOPIC_GEODE_DEBUG_UTIL})
   @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
       operation = ResourcePermission.Operation.READ)
-  public Result exportLogs(
+  public ResultModel exportLogs(
       @CliOption(key = CliStrings.EXPORT_LOGS__DIR,
           help = CliStrings.EXPORT_LOGS__DIR__HELP) String dirName,
       @CliOption(key = {CliStrings.GROUP, CliStrings.GROUPS},
@@ -100,7 +99,6 @@ public Result exportLogs(
       throws Exception {
 
     long totalEstimatedExportSize = 0;
-    Result result;
     InternalCache cache = (InternalCache) getCache();
     try {
       Set<DistributedMember> targetMembers = getMembersIncludingLocators(groups, memberIds);
@@ -121,14 +119,14 @@ public Result exportLogs(
               totalEstimatedExportSize += estimatedSize;
             } else if (results.get(0) instanceof ManagementException) {
               ManagementException exception = (ManagementException) results.get(0);
-              return ResultBuilder.createUserErrorResult(exception.getMessage());
+              return ResultModel.createError(exception.getMessage());
             }
           }
         }
 
         // first check if totalEstimate file size exceeds available disk space on locator
         if (totalEstimatedExportSize > getLocalDiskAvailable()) {
-          return ResultBuilder.createUserErrorResult(
+          return ResultModel.createError(
               "Estimated logs size will exceed the available disk space on the locator.");
         }
         // then check if total estimated file size exceeds user specified value
@@ -139,7 +137,7 @@ public Result exportLogs(
               .append(CliStrings.EXPORT_LOGS__FILESIZELIMIT).append(" = ")
               .append(userSpecifiedLimit).append(
                   ". To disable exported logs file size check use option \"--file-size-limit=0\".");
-          return ResultBuilder.createUserErrorResult(sb.toString());
+          return ResultModel.createError(sb.toString());
         }
       }
 
@@ -167,7 +165,7 @@ public Result exportLogs(
       }
 
       if (zipFilesFromMembers.isEmpty()) {
-        return ResultBuilder.createUserErrorResult("No files to be exported.");
+        return ResultModel.createError("No files to be exported.");
       }
 
       Path tempDir = Files.createTempDirectory("exportedLogs");
@@ -196,14 +194,12 @@ public Result exportLogs(
       ZipUtils.zipDirectory(exportedLogsDir, exportedLogsZipFile);
       FileUtils.deleteDirectory(tempDir.toFile());
 
-      result = new LegacyCommandResult(exportedLogsZipFile);
+      ResultModel result = new ResultModel();
+      result.setFileToDownload(exportedLogsZipFile);
+      return result;
     } finally {
       ExportLogsFunction.destroyExportLogsRegion(cache);
     }
-    if (logger.isDebugEnabled()) {
-      logger.debug("Exporting logs returning = {}", result);
-    }
-    return result;
   }
 
   /**

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsInterceptor.java

@@ -30,8 +30,7 @@
 import org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor;
 import org.apache.geode.management.internal.cli.GfshParseResult;
 import org.apache.geode.management.internal.cli.functions.ExportLogsFunction;
-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;
 
 /**
  * after the export logs, will need to copy the tempFile to the desired location and delete the temp
@@ -41,18 +40,18 @@ public class ExportLogsInterceptor extends AbstractCliAroundInterceptor {
   private static final Logger logger = LogService.getLogger();
 
   @Override
-  public Result preExecution(GfshParseResult parseResult) {
+  public ResultModel preExecution(GfshParseResult parseResult) {
 
     // validates groupId and memberIds not both set
     if (parseResult.getParamValueAsString("group") != null
         && parseResult.getParamValueAsString("member") != null) {
-      return ResultBuilder.createUserErrorResult("Can't specify both group and member.");
+      return ResultModel.createError("Can't specify both group and member.");
     }
 
     // validate log level
     String logLevel = parseResult.getParamValueAsString("log-level");
     if (StringUtils.isBlank(logLevel) || LogLevel.getLevel(logLevel) == null) {
-      return ResultBuilder.createUserErrorResult("Invalid log level: " + logLevel);
+      return ResultModel.createError("Invalid log level: " + logLevel);
     }
 
     // validate start date and end date
@@ -63,25 +62,25 @@ public Result preExecution(GfshParseResult parseResult) {
       LocalDateTime startTime = ExportLogsFunction.parseTime(start);
       LocalDateTime endTime = ExportLogsFunction.parseTime(end);
       if (startTime.isAfter(endTime)) {
-        return ResultBuilder.createUserErrorResult("start-time has to be earlier than end-time.");
+        return ResultModel.createError("start-time has to be earlier than end-time.");
       }
     }
 
     // validate onlyLogs and onlyStats
     boolean onlyLogs = (boolean) parseResult.getParamValue("logs-only");
     boolean onlyStats = (boolean) parseResult.getParamValue("stats-only");
     if (onlyLogs && onlyStats) {
-      return ResultBuilder.createUserErrorResult("logs-only and stats-only can't both be true");
+      return ResultModel.createError("logs-only and stats-only can't both be true");
     }
 
-    return ResultBuilder.createInfoResult("");
+    return ResultModel.createInfo("");
   }
 
   @Override
-  public CommandResult postExecution(GfshParseResult parseResult, CommandResult commandResult,
+  public ResultModel postExecution(GfshParseResult parseResult, ResultModel commandResult,
       Path tempFile) {
-    // in the command over http case, the command result is in the downloaded temp file
     if (tempFile != null) {
+      // in the command over http case, the command result is in the downloaded temp file
       Path dirPath;
       String dirName = parseResult.getParamValueAsString("dir");
       if (StringUtils.isBlank(dirName)) {
@@ -94,16 +93,21 @@ public CommandResult postExecution(GfshParseResult parseResult, CommandResult co
       try {
         FileUtils.copyFile(tempFile.toFile(), exportedLogFile);
         FileUtils.deleteQuietly(tempFile.toFile());
-        commandResult = (CommandResult) ResultBuilder
-            .createInfoResult("Logs exported to: " + exportedLogFile.getAbsolutePath());
+        return ResultModel.createInfo("Logs exported to: " + exportedLogFile.getAbsolutePath());
       } catch (IOException e) {
         logger.error(e.getMessage(), e);
-        commandResult = ResultBuilder.createGemFireErrorResult(e.getMessage());
+        return ResultModel.createError(e.getMessage());
       }
-    } else if (commandResult.getStatus() == Result.Status.OK) {
-      commandResult = (CommandResult) ResultBuilder.createInfoResult(
-          "Logs exported to the connected member's file system: " + commandResult.nextLine());
     }
-    return commandResult;
+
+    // otherwise if result status is false, return it
+    if (commandResult.getStatus() != Result.Status.OK) {
+      return commandResult;
+    }
+
+    // if there is no downloaded file. File is saved on the locator/manager.
+    return ResultModel.createInfo(
+        "Logs exported to the connected member's file system: "
+            + commandResult.getFileToDownload().toString());
   }
 }

geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ModelCommandResult.java

@@ -62,12 +62,12 @@ public ModelCommandResult(ResultModel result) {
 
   @Override
   public Path getFileToDownload() {
-    return null;
+    return result.getFileToDownload();
   }
 
   @Override
   public boolean hasFileToDownload() {
-    return false;
+    return getFileToDownload() != null;
   }
 
   @Override
@@ -88,8 +88,8 @@ public void resetToFirstLine() {
     commandOutputIndex = 0;
   }
 
-  // ModelCommandResult should not handle saving files. File saving should be done by each
-  // command's postExecutor in the ResultModel
+  // ModelCommandResult should not handle saving files. File contents saved in FileResultModel's
+  // byte[] should be saved by each command's postExecutor in the ResultModel
   @Override
   public boolean hasIncomingFiles() {
     return false;

geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java

@@ -26,6 +26,10 @@
 import org.apache.geode.management.internal.cli.result.ResultData;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
+/**
+ * this is used to save the content of the file in the byte[] and send the results
+ * back to the gfsh client.
+ */
 public class FileResultModel {
   public static final int FILE_TYPE_BINARY = 0;
   public static final int FILE_TYPE_TEXT = 1;

geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java

@@ -18,6 +18,7 @@
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -75,8 +76,14 @@ public class ResultModel {
   private Map<String, AbstractResultModel> sections = new LinkedHashMap<>();
   private Result.Status status = Result.Status.OK;
   private Object configObject;
+  // this is used by commands (e.g. ExportConfigCommand) saving the file content in the memory and
+  // transfer the byte[] in json string back to the client
   private Map<String, FileResultModel> files = new LinkedHashMap<>();
 
+  // this is used by commands (e.g ExportlogsCommand) that can download file to the client when in
+  // http connection
+  private Path fileToDownload;
+
   @JsonIgnore
   public Object getConfigObject() {
     return configObject;
@@ -155,6 +162,14 @@ public void addFile(File file, int fileType) {
     files.put(file.getName(), new FileResultModel(file, fileType));
   }
 
+  public Path getFileToDownload() {
+    return fileToDownload;
+  }
+
+  public void setFileToDownload(Path fileToDownload) {
+    this.fileToDownload = fileToDownload;
+  }
+
   /**
    * Overloaded method to create an {@code InfoResultModel} section called "info".
    */

geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java

@@ -62,7 +62,7 @@
 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.LegacyCommandResult;
-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.shell.jline.ANSIHandler;
 import org.apache.geode.management.internal.cli.shell.jline.ANSIHandler.ANSIStyle;
 import org.apache.geode.management.internal.cli.shell.jline.GfshHistory;
@@ -895,8 +895,8 @@ public boolean logToFile(String message, Throwable t) {
     return loggedMessage;
   }
 
-  public Result executeScript(File scriptFile, boolean quiet, boolean continueOnError) {
-    Result result;
+  public ResultModel executeScript(File scriptFile, boolean quiet, boolean continueOnError) {
+    ResultModel result = new ResultModel();
     String initialIsQuiet = getEnvProperty(ENV_APP_QUIET_EXECUTION);
     try {
       this.isScriptRunning = true;
@@ -973,7 +973,7 @@ public Result executeScript(File scriptFile, boolean quiet, boolean continueOnEr
       scriptInfo.logScriptExecutionInfo(gfshFileLogger, result);
       if (quiet) {
         // Create empty result when in quiet mode
-        result = ResultBuilder.createInfoResult("");
+        result = ResultModel.createInfo("");
       }
     } catch (IOException e) {
       throw new CommandProcessingException("Error while reading file " + scriptFile,