merge output & output-dir flags (#157)

Author: ishmum123

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/ConnectorArguments.java

@@ -237,19 +237,12 @@ public class ConnectorArguments extends DefaultArguments {
   // specified)").withRequiredArg().describedAs("my_dbname").withValuesSeparatedBy(',');
   private final OptionSpec<File> optionOutput =
       parser
-          .accepts("output", "Output file (cannot be used together with --output-dir)")
+          .accepts(
+              "output",
+              "Output file or directory name. If the file name, along with the `.zip` extension, is not provided dumper will attempt to create the zip file with the default file name in the directory.")
           .withRequiredArg()
           .ofType(File.class)
           .describedAs("cw-dump.zip");
-  private final OptionSpec<String> optionOutputDir =
-      parser
-          .accepts(
-              "output-dir",
-              "Output directory where the zip file with the default filename should be created (cannot be used together with --output option). If you want to specify the filename, use --output option instead.")
-          .withRequiredArg()
-          .ofType(String.class)
-          .defaultsTo("")
-          .describedAs("/home/user/my-dump-dir");
   private final OptionSpec<Void> optionOutputContinue =
       parser.accepts("continue", "Continues writing a previous output file.");
   // TODO: Make this be an ISO instant.
@@ -683,14 +676,8 @@ public List<String> getConfiguration() {
     return getOptions().valuesOf(optionConfiguration);
   }
 
-  @CheckForNull
-  public File getOutputFile() {
-    return getOptions().valueOf(optionOutput);
-  }
-
-  @CheckForNull
-  public String getOutputDirectory() {
-    return getOptions().valueOf(optionOutputDir);
+  public Optional<File> getOutputFile() {
+    return optionAsOptional(optionOutput).filter(file -> !file.getName().isEmpty());
   }
 
   public boolean isOutputContinue() {
@@ -854,7 +841,6 @@ public String toString() {
         .add("user", getUser())
         .add("configuration", getConfiguration())
         .add("output", getOutputFile())
-        .add("output-dir", getOutputDirectory())
         .add("query-log-earliest-timestamp", getQueryLogEarliestTimestamp())
         .add("query-log-days", getQueryLogDays())
         .add("query-log-start", getQueryLogStart())

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/MetadataDumper.java

@@ -44,7 +44,6 @@
 import com.google.edwmigration.dumper.application.dumper.task.TaskState;
 import com.google.edwmigration.dumper.application.dumper.task.VersionTask;
 import java.io.File;
-import java.io.IOException;
 import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileSystem;
@@ -239,23 +238,8 @@ protected void run(@Nonnull Connector connector, @Nonnull ConnectorArguments arg
     // The default output file is based on the connector.
     // We had a customer request to base it on the database, but that isn't well-defined,
     // as there may be 0 or N databases in a single file.
-    File outputFile = arguments.getOutputFile();
-    String outputDirectory = arguments.getOutputDirectory();
-
-    boolean isDefaultPath = "".equals(outputDirectory);
-    if (!isDefaultPath && outputFile != null) {
-      System.out.println(
-          "**********************************************************\n"
-              + "* ERROR: Using both --output and --output-dir flags is not allowed.\n"
-              + "* Please use --help for more information.\n"
-              + "**********************************************************");
-      return;
-    }
+    File outputFile = getOutputFile(connector, arguments);
 
-    if (outputFile == null) {
-      outputFile =
-          new File(outputDirectory, connector.getDefaultFileName(arguments.isAssessment()));
-    }
     if (arguments.isDryRun()) {
       String title = "Dry run: Printing task list for " + connector.getName();
       System.out.println(title);
@@ -271,48 +255,24 @@ protected void run(@Nonnull Connector connector, @Nonnull ConnectorArguments arg
       LOG.info("Using " + connector);
       try (Closer closer = Closer.create()) {
 
-        final OutputHandleFactory sinkFactory;
-        if (StringUtils.endsWithIgnoreCase(outputFile.getName(), ".zip")) {
-          if (outputFile.exists()) {
-            // If it exists, it must be a file.
-            if (!outputFile.isFile())
-              throw new IllegalStateException("Cannot overwrite existing non-file with file.");
-            if (!arguments.isOutputContinue())
-              outputFile.delete(); // It's a simple file, and we were asked to overwrite it.
-          } else {
-            Files.createParentDirs(outputFile);
-          }
-
-          URI outputUri = URI.create("jar:" + outputFile.toURI());
-          // LOG.debug("Is a zip file: " + outputUri);
-          Map<String, Object> fileSystemProperties =
-              ImmutableMap.<String, Object>builder()
-                  .put("create", "true")
-                  .put("useTempFile", Boolean.TRUE)
-                  .build();
-          FileSystem fileSystem =
-              closer.register(FileSystems.newFileSystem(outputUri, fileSystemProperties));
-          sinkFactory =
-              new FileSystemOutputHandleFactory(fileSystem, "/"); // It's required to be "/"
+        if (outputFile.exists()) {
+          if (!arguments.isOutputContinue())
+            outputFile.delete(); // It's a simple file, and we were asked to overwrite it.
         } else {
-          if (outputFile.exists()) {
-            // If it exists, it must be an empty directory, OR we must have specified --continue. We
-            // never delete.
-            if (!outputFile.isDirectory())
-              throw new IllegalStateException(
-                  "Cannot overwrite existing non-directory with directory.");
-            if (!arguments.isOutputContinue() && !isNullOrEmpty(outputFile.list()))
-              throw new IllegalStateException(
-                  "Refusing to touch non-empty directory without --continue.");
-          } else {
-            // LOG.debug("Not a zip file: " + outputFile);
-            outputFile.mkdirs();
-            if (!outputFile.isDirectory())
-              throw new IOException("Unable to create directory " + outputFile);
-          }
-
-          sinkFactory = new FileSystemOutputHandleFactory(outputFile.toPath());
+          Files.createParentDirs(outputFile);
         }
+
+        URI outputUri = URI.create("jar:" + outputFile.toURI());
+        // LOG.debug("Is a zip file: " + outputUri);
+        Map<String, Object> fileSystemProperties =
+            ImmutableMap.<String, Object>builder()
+                .put("create", "true")
+                .put("useTempFile", Boolean.TRUE)
+                .build();
+        FileSystem fileSystem =
+            closer.register(FileSystems.newFileSystem(outputUri, fileSystemProperties));
+        OutputHandleFactory sinkFactory =
+            new FileSystemOutputHandleFactory(fileSystem, "/"); // It's required to be "/"
         LOG.debug("Target filesystem is " + sinkFactory);
 
         Handle handle = closer.register(connector.open(arguments));
@@ -348,6 +308,30 @@ public <T> T runChildTask(Task<T> task) throws MetadataDumperUsageException {
     }
   }
 
+  private File getOutputFile(Connector connector, ConnectorArguments arguments) {
+    // The default output file is based on the connector.
+    // We had a customer request to base it on the database, but that isn't well-defined,
+    // as there may be 0 or N databases in a single file.
+    String defaultFileName = connector.getDefaultFileName(arguments.isAssessment());
+    return arguments
+        .getOutputFile()
+        .map(
+            file -> {
+              String fileName = file.getPath();
+              if (StringUtils.endsWithIgnoreCase(fileName, ".zip")) return file;
+              if (file.isFile()) {
+                throw new IllegalStateException(
+                    String.format(
+                        "A file already exists at %1$s. "
+                            + "If you want to create a directory, please provide the path to the directory. "
+                            + "If you want to create %1$s.zip, please add the `.zip` extension manually.",
+                        fileName));
+              }
+              return new File(file, defaultFileName);
+            })
+        .orElseGet(() -> new File(defaultFileName));
+  }
+
   private void printDumperSummary(Connector connector, File outputFile) {
     if (connector instanceof MetadataConnector) {
       log("Metadata has been saved to " + outputFile);