geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jinmeil...@apache.org
Subject [geode] branch develop updated: GEODE-5727: rework how ResultModel deal with file contents. (#2460)
Date Thu, 13 Sep 2018 14:30:53 GMT
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 4fbe655  GEODE-5727: rework how ResultModel deal with file contents. (#2460)
4fbe655 is described below

commit 4fbe65515c82bbdea715c488ab82baa5fda05bd9
Author: jinmeiliao <jiliao@pivotal.io>
AuthorDate: Thu Sep 13 07:30:43 2018 -0700

    GEODE-5727: rework how ResultModel deal with file contents. (#2460)
    
    * GEODE-5727: rework how ResultModel deal with file contents.
    
    * file saving should be handled by the command's postExecutor to save to appropriate places
    * after saving file content to a directory, the ResultModel turned the FileResultModel
into InfoResultModel.
    * gfsh should not be the place to save files in ModelCommandResult.
---
 ...ExportClusterConfigurationCommandDUnitTest.java | 68 +++++++++++++++-
 .../ClusterConfigImportDUnitTest.java              | 60 +-------------
 .../result/model/ResultModelIntegrationTest.java   | 92 ++++++++++++++++++++++
 .../ExportClusterConfigurationCommand.java         |  5 +-
 .../internal/cli/commands/ExportConfigCommand.java | 17 ++--
 .../internal/cli/result/ModelCommandResult.java    | 11 +--
 .../internal/cli/result/model/FileResultModel.java | 86 +++++---------------
 .../internal/cli/result/model/ResultModel.java     | 34 +++++++-
 .../geode/management/internal/cli/shell/Gfsh.java  |  6 +-
 .../test/junit/assertions/ResultModelAssert.java   | 50 ++++++++++++
 .../cli/commands/ExportConfigCommandDUnitTest.java |  4 +-
 .../internal/cli/commands/CommandOverHttpTest.java |  5 +-
 12 files changed, 283 insertions(+), 155 deletions(-)

diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommandDUnitTest.java
b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommandDUnitTest.java
index 9c14c43..75ffc25 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommandDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommandDUnitTest.java
@@ -21,7 +21,13 @@ import static org.assertj.core.api.Assertions.assertThat;
 import java.io.File;
 import java.io.IOException;
 import java.nio.charset.Charset;
+import java.nio.file.Path;
+import java.util.HashSet;
 import java.util.Properties;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
 
 import org.apache.commons.io.FileUtils;
 import org.junit.BeforeClass;
@@ -29,6 +35,8 @@ import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
+import org.apache.geode.management.internal.configuration.ClusterConfig;
+import org.apache.geode.management.internal.configuration.ConfigGroup;
 import org.apache.geode.test.dunit.rules.ClusterStartupRule;
 import org.apache.geode.test.dunit.rules.MemberVM;
 import org.apache.geode.test.junit.rules.GfshCommandRule;
@@ -45,7 +53,7 @@ public class ExportClusterConfigurationCommandDUnitTest {
 
 
   private static File xmlFile;
-  private static MemberVM locator;
+  private static MemberVM locator, server;
 
   @BeforeClass
   public static void beforeClass() throws Exception {
@@ -53,7 +61,7 @@ public class ExportClusterConfigurationCommandDUnitTest {
     locator = cluster.startLocatorVM(0);
     Properties properties = new Properties();
     properties.setProperty(GROUPS_NAME, "groupB");
-    cluster.startServerVM(1, properties, locator.getPort());
+    server = cluster.startServerVM(1, properties, locator.getPort());
     gfsh.connectAndVerify(locator);
     gfsh.executeAndAssertThat("create region --name=regionA --type=REPLICATE").statusIsSuccess();
     gfsh.executeAndAssertThat("create region --name=regionB --type=REPLICATE --group=groupB")
@@ -86,4 +94,60 @@ public class ExportClusterConfigurationCommandDUnitTest {
     assertThat(content).startsWith("<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>")
         .contains("<region name=\"regionA\">");
   }
+
+  @Test
+  public void testExportWithAbsolutePath() throws Exception {
+    Path exportedZipPath =
+        tempFolder.getRoot().toPath().resolve("exportedCC.zip").toAbsolutePath();
+
+    testExportClusterConfig(exportedZipPath.toString());
+  }
+
+  @Test
+  public void testExportWithRelativePath() throws Exception {
+    testExportClusterConfig("mytemp/exportedCC.zip");
+    FileUtils.deleteQuietly(new File("mytemp"));
+  }
+
+  @Test
+  public void testExportWithOnlyFileName() throws Exception {
+    testExportClusterConfig("exportedCC.zip");
+    FileUtils.deleteQuietly(new File("exportedCC.zip"));
+  }
+
+  public void testExportClusterConfig(String zipFilePath) throws Exception {
+    ConfigGroup cluster = new ConfigGroup("cluster").regions("regionA");
+    ConfigGroup groupB = new ConfigGroup("groupB").regions("regionB");
+    ClusterConfig expectedClusterConfig = new ClusterConfig(cluster, groupB);
+    expectedClusterConfig.verify(locator);
+    expectedClusterConfig.verify(server);
+
+    String expectedFilePath = new File(zipFilePath).getAbsolutePath();
+    gfsh.executeAndAssertThat("export cluster-configuration --zip-file-name=" + zipFilePath)
+        .statusIsSuccess()
+        .containsOutput("File saved to " + expectedFilePath);
+
+    File exportedZip = new File(zipFilePath);
+    assertThat(exportedZip).exists();
+
+    Set<String> actualZipEnries =
+        new ZipFile(exportedZip).stream().map(ZipEntry::getName).collect(Collectors.toSet());
+
+    ConfigGroup exportedClusterGroup = cluster.configFiles("cluster.xml", "cluster.properties");
+    ConfigGroup exportedGroupB = groupB.configFiles("groupB.xml", "groupB.properties");
+    ClusterConfig expectedExportedClusterConfig =
+        new ClusterConfig(exportedClusterGroup, exportedGroupB);
+
+    Set<String> expectedZipEntries = new HashSet<>();
+    for (ConfigGroup group : expectedExportedClusterConfig.getGroups()) {
+      String groupDir = group.getName() + File.separator;
+
+      for (String jarOrXmlOrPropFile : group.getAllFiles()) {
+        expectedZipEntries.add(groupDir + jarOrXmlOrPropFile);
+      }
+    }
+
+    assertThat(actualZipEnries).isEqualTo(expectedZipEntries);
+  }
+
 }
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java
b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java
index 180d0d4..b8d495e 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java
@@ -20,14 +20,7 @@ import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.io.File;
-import java.nio.file.Path;
-import java.util.HashSet;
-import java.util.Set;
-import java.util.stream.Collectors;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipFile;
-
-import org.apache.commons.io.FileUtils;
+
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -171,55 +164,4 @@ public class ClusterConfigImportDUnitTest extends ClusterConfigTestBase
{
     REPLICATED_CONFIG_FROM_ZIP.verify(locator1);
     REPLICATED_CONFIG_FROM_ZIP.verify(locator2);
   }
-
-  @Test
-  public void testExportWithAbsolutePath() throws Exception {
-    Path exportedZipPath =
-        temporaryFolder.getRoot().toPath().resolve("exportedCC.zip").toAbsolutePath();
-
-    testExportClusterConfig(exportedZipPath.toString());
-  }
-
-  @Test
-  public void testExportWithRelativePath() throws Exception {
-    testExportClusterConfig("mytemp/exportedCC.zip");
-    FileUtils.deleteQuietly(new File("mytemp"));
-  }
-
-  public void testExportClusterConfig(String zipFilePath) throws Exception {
-    MemberVM server1 = lsRule.startServerVM(1, serverProps, locatorVM.getPort());
-
-    gfshConnector.executeAndAssertThat("create region --name=myRegion --type=REPLICATE")
-        .statusIsSuccess();
-
-    ConfigGroup cluster = new ConfigGroup("cluster").regions("myRegion");
-    ClusterConfig expectedClusterConfig = new ClusterConfig(cluster);
-    expectedClusterConfig.verify(server1);
-    expectedClusterConfig.verify(locatorVM);
-
-    gfshConnector
-        .executeAndAssertThat("export cluster-configuration --zip-file-name=" + zipFilePath)
-        .statusIsSuccess();
-
-    File exportedZip = new File(zipFilePath);
-    assertThat(exportedZip).exists();
-
-    Set<String> actualZipEnries =
-        new ZipFile(exportedZip).stream().map(ZipEntry::getName).collect(Collectors.toSet());
-
-    ConfigGroup exportedClusterGroup = cluster.configFiles("cluster.xml", "cluster.properties");
-    ClusterConfig expectedExportedClusterConfig = new ClusterConfig(exportedClusterGroup);
-
-    Set<String> expectedZipEntries = new HashSet<>();
-    for (ConfigGroup group : expectedExportedClusterConfig.getGroups()) {
-      String groupDir = group.getName() + File.separator;
-
-      for (String jarOrXmlOrPropFile : group.getAllFiles()) {
-        expectedZipEntries.add(groupDir + jarOrXmlOrPropFile);
-      }
-    }
-
-    assertThat(actualZipEnries).isEqualTo(expectedZipEntries);
-  }
-
 }
diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/result/model/ResultModelIntegrationTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/result/model/ResultModelIntegrationTest.java
new file mode 100644
index 0000000..e298c7b
--- /dev/null
+++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/result/model/ResultModelIntegrationTest.java
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information
regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version
2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain
a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under
the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express
+ * or implied. See the License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.result.model;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.management.internal.cli.result.ModelCommandResult;
+import org.apache.geode.test.junit.assertions.ResultModelAssert;
+
+public class ResultModelIntegrationTest {
+
+  private ResultModel result;
+
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  @Before
+  public void setUp() throws Exception {
+    result = new ResultModel();
+    result.addFile("test1.txt", "hello");
+    result.addFile("test2.txt", "hello again");
+  }
+
+  @Test
+  public void emptyFileSizeDoesNothing() throws IOException {
+    ResultModel emptyFileResult = new ResultModel();
+    result.saveFileTo(temporaryFolder.newFolder());
+    assertThat(emptyFileResult.getInfoSections()).hasSize(0);
+  }
+
+  @Test
+  public void savesToNoWhereDoesNothing() throws IOException {
+    result.saveFileTo(null);
+    assertThat(result.getInfoSections()).hasSize(0);
+  }
+
+  @Test
+  public void notADirectory() throws IOException {
+    result.saveFileTo(temporaryFolder.newFile());
+    assertThis(result).hasInfoResultModel("fileSave").hasOutput().contains("is not a directory");
+  }
+
+  @Test
+  public void dirNotExistBefore() throws IOException {
+    File dir = temporaryFolder.newFolder("test");
+    dir.delete();
+
+    result.saveFileTo(dir);
+    assertThat(dir).exists();
+    File file1 = new File(dir, "test1.txt");
+    File file2 = new File(dir, "test2.txt");
+    assertThat(dir.listFiles()).contains(file1, file2);
+
+    assertThis(result).hasInfoResultModel("fileSave").hasLines().hasSize(2)
+        .containsExactlyInAnyOrder(
+            "File saved to " + file1.getAbsolutePath(),
+            "File saved to " + file2.getAbsolutePath());
+  }
+
+  @Test
+  public void modelCommandResultShouldNotDealWithFiles() throws IOException {
+    result.saveFileTo(temporaryFolder.newFolder("test"));
+    ModelCommandResult commandResult = new ModelCommandResult(result);
+    assertThat(commandResult.hasIncomingFiles()).isFalse();
+    assertThat(commandResult.getNumTimesSaved()).isEqualTo(0);
+  }
+
+  public static ResultModelAssert assertThis(ResultModel model) {
+    return new ResultModelAssert(model);
+  }
+}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommand.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommand.java
index aa55092..90d2106 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommand.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportClusterConfigurationCommand.java
@@ -195,12 +195,11 @@ public class ExportClusterConfigurationCommand extends InternalGfshCommand
{
         }
       } else if (zipFile != null) {
         // delete the existing file since at this point, user is OK to replace the old zip.
-        File file = new File(zipFile);
+        File file = new File(zipFile).getAbsoluteFile();
         if (file.exists()) {
           FileUtils.deleteQuietly(file);
         }
-        FileResultModel fileResultModel = result.getFiles().values().iterator().next();
-        fileResultModel.writeFile(file.getParentFile().getAbsolutePath());
+        result.saveFileTo(file.getParentFile());
       }
       return result;
     }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
index 6e80a6b..bf19148 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java
@@ -34,7 +34,6 @@ import org.apache.geode.management.internal.cli.GfshParseResult;
 import org.apache.geode.management.internal.cli.functions.CliFunctionResult;
 import org.apache.geode.management.internal.cli.functions.ExportConfigFunction;
 import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.apache.geode.management.internal.cli.result.model.FileResultModel;
 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.security.ResourceOperation;
@@ -87,8 +86,8 @@ public class ExportConfigCommand extends InternalGfshCommand {
         String cacheFileName = result.getMemberIdOrName() + "-cache.xml";
         String propsFileName = result.getMemberIdOrName() + "-gf.properties";
         String[] fileContent = (String[]) result.getSerializables();
-        crm.addFile(cacheFileName, fileContent[0], "Downloading Cache XML file: " + cacheFileName);
-        crm.addFile(propsFileName, fileContent[1], "Downloading properties file: " + propsFileName);
+        crm.addFile(cacheFileName, fileContent[0]);
+        crm.addFile(propsFileName, fileContent[1]);
       }
     }
 
@@ -99,17 +98,17 @@ public class ExportConfigCommand extends InternalGfshCommand {
    * Interceptor used by gfsh to intercept execution of export config command at "shell".
    */
   public static class Interceptor extends AbstractCliAroundInterceptor {
-    private String saveDirString;
+    private File saveDirFile;
 
     @Override
     public ResultModel preExecution(GfshParseResult parseResult) {
       String dir = parseResult.getParamValueAsString("dir");
       if (StringUtils.isBlank(dir)) {
-        saveDirString = new File(".").getAbsolutePath();
+        saveDirFile = new File(".").getAbsoluteFile();
         return new ResultModel();
       }
 
-      File saveDirFile = new File(dir.trim());
+      saveDirFile = new File(dir.trim()).getAbsoluteFile();
 
       if (!saveDirFile.exists() && !saveDirFile.mkdirs()) {
         return ResultModel
@@ -130,17 +129,13 @@ public class ExportConfigCommand extends InternalGfshCommand {
         return ResultModel.createError(
             CliStrings.format(CliStrings.EXPORT_CONFIG__MSG__NOT_WRITEABLE, saveDirFile.getName()));
       }
-
-      saveDirString = saveDirFile.getAbsolutePath();
       return new ResultModel();
     }
 
     @Override
     public ResultModel postExecution(GfshParseResult parseResult, ResultModel commandResult,
         Path tempFile) throws Exception {
-      for (FileResultModel file : commandResult.getFiles().values()) {
-        file.writeFile(saveDirString);
-      }
+      commandResult.saveFileTo(saveDirFile);
       return commandResult;
     }
   }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ModelCommandResult.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ModelCommandResult.java
index 106401e..65e3d56 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ModelCommandResult.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ModelCommandResult.java
@@ -31,7 +31,6 @@ import org.apache.geode.management.internal.cli.GfshParser;
 import org.apache.geode.management.internal.cli.json.GfJsonObject;
 import org.apache.geode.management.internal.cli.result.model.AbstractResultModel;
 import org.apache.geode.management.internal.cli.result.model.DataResultModel;
-import org.apache.geode.management.internal.cli.result.model.FileResultModel;
 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.cli.result.model.TabularResultModel;
@@ -83,9 +82,11 @@ public class ModelCommandResult implements CommandResult {
     commandOutputIndex = 0;
   }
 
+  // ModelCommandResult should not handle saving files. File saving should be done by each
+  // command's postExecutor in the ResultModel
   @Override
   public boolean hasIncomingFiles() {
-    return result.getFiles().size() > 0;
+    return false;
   }
 
   @Override
@@ -94,11 +95,7 @@ public class ModelCommandResult implements CommandResult {
   }
 
   @Override
-  public void saveIncomingFiles(String directory) throws IOException {
-    for (FileResultModel file : result.getFiles().values()) {
-      file.writeFile(directory);
-    }
-  }
+  public void saveIncomingFiles(String directory) throws IOException {}
 
   @Override
   public boolean hasNextLine() {
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
index ca3474f..6726fcf 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
@@ -20,11 +20,9 @@ import java.io.File;
 import java.io.FileOutputStream;
 import java.io.FileWriter;
 import java.io.IOException;
-import java.text.MessageFormat;
 
 import org.apache.commons.io.FileUtils;
 
-import org.apache.geode.management.internal.cli.i18n.CliStrings;
 import org.apache.geode.management.internal.cli.result.ResultData;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
@@ -34,18 +32,16 @@ public class FileResultModel {
 
   private String filename;
   private int type;
-  private String message;
   private byte[] data;
   private int length;
 
   public FileResultModel() {}
 
-  public FileResultModel(String fileName, String content, String message) {
+  public FileResultModel(String fileName, String content) {
     this.filename = fileName;
     this.data = content.getBytes();
     this.length = data.length;
     this.type = FILE_TYPE_TEXT;
-    this.message = message;
   }
 
   public FileResultModel(File file, int fileType) {
@@ -79,14 +75,6 @@ public class FileResultModel {
     this.type = type;
   }
 
-  public String getMessage() {
-    return message;
-  }
-
-  public void setMessage(String message) {
-    this.message = message;
-  }
-
   public byte[] getData() {
     return data;
   }
@@ -103,79 +91,45 @@ public class FileResultModel {
     this.length = length;
   }
 
-  public void writeFile(String directory) throws IOException {
-    String options = "(y/N)";
-
-    File fileToDumpData = new File(filename);
-    if (!fileToDumpData.isAbsolute()) {
-      if (directory == null || directory.isEmpty()) {
-        directory = System.getProperty("user.dir", ".");
-      }
-      fileToDumpData = new File(directory, filename);
-    }
+  /**
+   * at this point, the dir should already exist and is confirmed as a directory
+   * filename in this instance should be file name only. no path in the file name
+   *
+   * @param directory the directory where to write the content of byte[] to with the filename
+   * @return the message you would like to return to the user.
+   */
+  public String saveFile(File directory) throws IOException {
+    String options = "(Y/N)";
+    File file = new File(directory, filename).getAbsoluteFile();
 
-    File parentDirectory = fileToDumpData.getParentFile();
-    if (parentDirectory != null) {
-      parentDirectory.mkdirs();
-    }
     Gfsh gfsh = Gfsh.getCurrentInstance();
-    if (fileToDumpData.exists()) {
-      String fileExistsMessage =
-          CliStrings.format(CliStrings.ABSTRACTRESULTDATA__MSG__FILE_WITH_NAME_0_EXISTS_IN_1,
-              filename, fileToDumpData.getParent(), options);
+    if (file.exists()) {
+      String fileExistsMessage = String.format("File with name \"$s\" already exists in \"$s\".",
+          filename, directory.getAbsolutePath());
       if (gfsh != null && !gfsh.isQuietMode()) {
-        fileExistsMessage = fileExistsMessage + " Overwrite? " + options + " : ";
+        fileExistsMessage += " Overwrite? " + options + " : ";
         String interaction = gfsh.interact(fileExistsMessage);
         if (!"y".equalsIgnoreCase(interaction.trim())) {
           // do not save file & continue
-          return;
+          return "User aborted. Did not overwrite " + file.getAbsolutePath();
         }
       } else {
-        throw new IOException(fileExistsMessage);
+        return fileExistsMessage;
       }
-    } else if (!parentDirectory.exists()) {
-      handleCondition(CliStrings.format(
-          CliStrings.ABSTRACTRESULTDATA__MSG__PARENT_DIRECTORY_OF_0_DOES_NOT_EXIST,
-          fileToDumpData.getAbsolutePath()));
-      return;
-    } else if (!parentDirectory.canWrite()) {
-      handleCondition(CliStrings.format(
-          CliStrings.ABSTRACTRESULTDATA__MSG__PARENT_DIRECTORY_OF_0_IS_NOT_WRITABLE,
-          fileToDumpData.getAbsolutePath()));
-      return;
-    } else if (!parentDirectory.isDirectory()) {
-      handleCondition(
-          CliStrings.format(CliStrings.ABSTRACTRESULTDATA__MSG__PARENT_OF_0_IS_NOT_DIRECTORY,
-              fileToDumpData.getAbsolutePath()));
-      return;
     }
     if (type == ResultData.FILE_TYPE_TEXT) {
-      FileWriter fw = new FileWriter(fileToDumpData);
+      FileWriter fw = new FileWriter(file);
       BufferedWriter bw = new BufferedWriter(fw);
       bw.write(new String(data));
       bw.flush();
       fw.flush();
       fw.close();
     } else if (type == ResultData.FILE_TYPE_BINARY) {
-      FileOutputStream fos = new FileOutputStream(fileToDumpData);
+      FileOutputStream fos = new FileOutputStream(file);
       fos.write(data);
       fos.flush();
       fos.close();
     }
-    if (message != null && !message.isEmpty()) {
-      if (gfsh != null) {
-        Gfsh.println(MessageFormat.format(message, fileToDumpData.getAbsolutePath()));
-      }
-    }
-  }
-
-  private void handleCondition(String message) throws IOException {
-    Gfsh gfsh = Gfsh.getCurrentInstance();
-    // null check required in GfshVM too to avoid test issues
-    if (gfsh != null && !gfsh.isQuietMode()) {
-      gfsh.logWarning(message, null);
-    } else {
-      throw new IOException(message);
-    }
+    return "File saved to " + file.getAbsolutePath();
   }
 }
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 9a20b62..d416c32 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
@@ -146,8 +146,8 @@ public class ResultModel {
     this.files = files;
   }
 
-  public void addFile(String fileName, String content, String message) {
-    FileResultModel fileModel = new FileResultModel(fileName, content, message);
+  public void addFile(String fileName, String content) {
+    FileResultModel fileModel = new FileResultModel(fileName, content);
     files.put(fileName, fileModel);
   }
 
@@ -384,4 +384,34 @@ public class ResultModel {
     return result;
   }
 
+  /**
+   * this saves the file data in this result model to the specified directory, and add appropriate
+   * information to the result model to indicate the result of the file save.
+   *
+   */
+  public void saveFileTo(File dir) throws IOException {
+    if (getFiles().size() == 0 || dir == null) {
+      return;
+    }
+    InfoResultModel info = addInfo("fileSave");
+    if (!dir.exists() && !dir.mkdirs()) {
+      info.addLine(dir.getAbsolutePath() + " can not be created.");
+      setStatus(Result.Status.ERROR);
+      return;
+    }
+    if (!dir.isDirectory()) {
+      info.addLine(dir.getAbsolutePath() + " is not a directory.");
+      setStatus(Result.Status.ERROR);
+      return;
+    }
+    if (!dir.canWrite()) {
+      info.addLine("Can not write to " + dir.getAbsolutePath());
+      setStatus(Result.Status.ERROR);
+      return;
+    }
+
+    for (FileResultModel fileResult : files.values()) {
+      info.addLine(fileResult.saveFile(dir));
+    }
+  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
index cb3a369..ca9aeb9 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
@@ -58,6 +58,7 @@ import org.apache.geode.management.internal.cli.GfshParser;
 import org.apache.geode.management.internal.cli.LogWrapper;
 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.shell.jline.ANSIHandler;
 import org.apache.geode.management.internal.cli.shell.jline.ANSIHandler.ANSIStyle;
@@ -708,7 +709,10 @@ public class Gfsh extends JLineShell {
 
         resultTypeTL.set(null);
 
-        if (result instanceof CommandResult) {
+        // this only saves the incoming file to the user.dir. We should not rely on this
+        // to save the exported file at this point. All file saving should be done in the
+        // specific command's postExecutor
+        if (result instanceof LegacyCommandResult) {
           CommandResult cmdResult = (CommandResult) result;
           if (cmdResult.hasIncomingFiles()) {
             boolean isAlreadySaved = cmdResult.getNumTimesSaved() > 0;
diff --git a/geode-junit/src/main/java/org/apache/geode/test/junit/assertions/ResultModelAssert.java
b/geode-junit/src/main/java/org/apache/geode/test/junit/assertions/ResultModelAssert.java
new file mode 100644
index 0000000..99b8b8d
--- /dev/null
+++ b/geode-junit/src/main/java/org/apache/geode/test/junit/assertions/ResultModelAssert.java
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information
regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version
2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain
a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under
the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express
+ * or implied. See the License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+
+package org.apache.geode.test.junit.assertions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.assertj.core.api.AbstractAssert;
+
+import org.apache.geode.management.internal.cli.result.model.DataResultModel;
+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.cli.result.model.TabularResultModel;
+
+public class ResultModelAssert extends AbstractAssert<ResultModelAssert, ResultModel>
{
+
+  public ResultModelAssert(ResultModel infoResultModel) {
+    super(infoResultModel, ResultModelAssert.class);
+  }
+
+  public InfoResultModelAssert hasInfoResultModel(String name) {
+    InfoResultModel infoSection = actual.getInfoSection(name);
+    assertThat(infoSection).isNotNull();
+    return new InfoResultModelAssert(infoSection);
+  }
+
+  public DataResultModelAssert hasDateResultModel(String name) {
+    DataResultModel dataSection = actual.getDataSection(name);
+    assertThat(dataSection).isNotNull();
+    return new DataResultModelAssert(dataSection);
+  }
+
+  public TabularResultModelAssert hasTabularResultModel(String name) {
+    TabularResultModel dataSection = actual.getTableSection(name);
+    assertThat(dataSection).isNotNull();
+    return new TabularResultModelAssert(dataSection);
+  }
+}
diff --git a/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommandDUnitTest.java
b/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommandDUnitTest.java
index ba5df56..c5090c3 100644
--- a/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommandDUnitTest.java
+++ b/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommandDUnitTest.java
@@ -85,7 +85,7 @@ public class ExportConfigCommandDUnitTest {
     // export just one member's config
     tempDir = temporaryFolder.newFolder("member0");
     gfsh.executeAndAssertThat("export config --member=server-0 --dir=" + tempDir.getAbsolutePath())
-        .statusIsSuccess();
+        .statusIsSuccess().containsOutput(tempDir.getAbsolutePath());
 
     expectedFiles = Arrays.asList("server-0-cache.xml", "server-0-gf.properties");
 
@@ -97,7 +97,7 @@ public class ExportConfigCommandDUnitTest {
     // export group2 config into a folder
     tempDir = temporaryFolder.newFolder("group2");
     gfsh.executeAndAssertThat("export config --group=Group2 --dir=" + tempDir.getAbsolutePath())
-        .statusIsSuccess();
+        .statusIsSuccess().containsOutput(tempDir.getAbsolutePath());
 
     expectedFiles = Arrays.asList("server-1-cache.xml", "server-2-cache.xml",
         "server-1-gf.properties", "server-2-gf.properties");
diff --git a/geode-web/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpTest.java
b/geode-web/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpTest.java
index 6d57377..0285b1e 100644
--- a/geode-web/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpTest.java
+++ b/geode-web/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpTest.java
@@ -18,6 +18,7 @@ package org.apache.geode.management.internal.cli.commands;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.io.File;
+import java.nio.file.Paths;
 
 import org.junit.Before;
 import org.junit.ClassRule;
@@ -86,7 +87,7 @@ public class CommandOverHttpTest {
   public void exportConfig() throws Exception {
     String dir = temporaryFolder.getRoot().getAbsolutePath();
     gfshRule.executeAndAssertThat("export config --dir=" + dir).statusIsSuccess()
-        .containsOutput("Downloading Cache XML file: server-cache.xml")
-        .containsOutput("Downloading properties file: server-gf.properties");
+        .containsOutput("File saved to " + Paths.get(dir, "server-cache.xml").toString())
+        .containsOutput("File saved to " + Paths.get(dir, "server-gf.properties").toString());
   }
 }


Mime
View raw message