Fixes for Connector#validate (google#944)

- Add a method validateForConnector called by AbstractSnowflakeConnector#validate.
- Add Nonnull to the param of validate.
- Make validateDateRange non-default.
- Simplify some checks in validateDateRange to make it less nested.

Author: misolt

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/Connector.java

@@ -16,7 +16,9 @@
  */
 package com.google.edwmigration.dumper.application.dumper.connector;
 
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.edwmigration.dumper.application.dumper.ConnectorArguments;
 import com.google.edwmigration.dumper.application.dumper.handle.Handle;
 import com.google.edwmigration.dumper.application.dumper.task.Task;
@@ -50,25 +52,21 @@ default String getDescription() {
    * @param arguments cli params
    * @throws RuntimeException if incorrect set of arguments passed to the particular connector
    */
-  default void validate(ConnectorArguments arguments) {}
+  default void validate(@Nonnull ConnectorArguments arguments) {}
 
-  default void validateDateRange(ConnectorArguments arguments) {
-    ZonedDateTime startDate = arguments.getStartDate();
-    ZonedDateTime endDate = arguments.getEndDate();
+  static void validateDateRange(@Nonnull ConnectorArguments arguments) {
+    ZonedDateTime start = arguments.getStartDate();
+    ZonedDateTime end = arguments.getEndDate();
 
-    if (startDate != null) {
-      Preconditions.checkNotNull(
-          endDate, "End date must be specified with start date, but was null.");
-      Preconditions.checkState(
-          startDate.isBefore(endDate),
-          "Start date [%s] must be before end date [%s].",
-          startDate,
-          endDate);
-    } else {
-      Preconditions.checkState(
-          endDate == null,
-          "End date can be specified only with start date, but start date was null.");
+    if (start == null && end == null) {
+      return;
     }
+    checkNotNull(start, "End date can be specified only with start date, but start date was null.");
+    // The assignment makes 'end' recognized as @Nonnull.
+    end = checkNotNull(end, "End date must be specified with start date, but was null.");
+
+    String message = String.format("Start date [%s] must be before end date [%s].", start, end);
+    checkState(end.isAfter(start), message);
   }
 
   void addTasksTo(@Nonnull List<? super Task<?>> out, @Nonnull ConnectorArguments arguments)

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/airflow/AirflowConnector.java

@@ -17,6 +17,7 @@
 package com.google.edwmigration.dumper.application.dumper.connector.airflow;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.edwmigration.dumper.application.dumper.connector.Connector.validateDateRange;
 import static com.google.edwmigration.dumper.application.dumper.connector.airflow.AirflowDatabaseDriverClasses.jdbcPrefixForClassName;
 
 import com.google.auto.service.AutoService;
@@ -209,7 +210,7 @@ private static void addQueryTask(
   }
 
   @Override
-  public void validate(ConnectorArguments arguments) {
+  public final void validate(@Nonnull ConnectorArguments arguments) {
     Preconditions.checkState(arguments.isAssessment(), "--assessment flag is required");
     Preconditions.checkState(
         arguments.getDriverPaths() != null && !arguments.getDriverPaths().isEmpty(),

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/cloudera/manager/ClouderaManagerConnector.java

@@ -16,6 +16,7 @@
  */
 package com.google.edwmigration.dumper.application.dumper.connector.cloudera.manager;
 
+import static com.google.edwmigration.dumper.application.dumper.connector.Connector.validateDateRange;
 import static com.google.edwmigration.dumper.application.dumper.connector.cloudera.manager.AbstractClouderaTimeSeriesTask.TimeSeriesAggregation.DAILY;
 import static com.google.edwmigration.dumper.application.dumper.task.TaskCategory.OPTIONAL;
 
@@ -129,7 +130,7 @@ public ClouderaManagerHandle open(@Nonnull ConnectorArguments arguments) throws
   }
 
   @Override
-  public void validate(ConnectorArguments arguments) {
+  public final void validate(@Nonnull ConnectorArguments arguments) {
     String clouderaUri = arguments.getUri();
     Preconditions.checkNotNull(clouderaUri, "--url for Cloudera Manager API is required");
 

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/hadoop/oozie/OozieConnector.java

@@ -16,6 +16,8 @@
  */
 package com.google.edwmigration.dumper.application.dumper.connector.hadoop.oozie;
 
+import static com.google.edwmigration.dumper.application.dumper.connector.Connector.validateDateRange;
+
 import com.google.auto.service.AutoService;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -79,7 +81,7 @@ public String getDefaultFileName(boolean isAssessment, Clock clock) {
   }
 
   @Override
-  public void validate(ConnectorArguments arguments) {
+  public final void validate(@Nonnull ConnectorArguments arguments) {
     Preconditions.checkState(arguments.isAssessment(), "--assessment flag is required");
 
     validateDateRange(arguments);

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/snowflake/AbstractSnowflakeConnector.java

@@ -101,9 +101,7 @@ public Handle open(@Nonnull ConnectorArguments arguments)
   }
 
   @Override
-  public void validate(ConnectorArguments arguments) {
-    super.validate(arguments);
-
+  public final void validate(@Nonnull ConnectorArguments arguments) {
     ArrayList<String> messages = new ArrayList<>();
     MetadataDumperUsageException exception = null;
 
@@ -133,8 +131,18 @@ public void validate(ConnectorArguments arguments) {
       exception = new MetadataDumperUsageException(unsupportedFilter, messages);
     }
     removeDuplicateMessageAndThrow(exception);
+    validateForConnector(arguments);
   }
 
+  /**
+   * Called by {@link #validate} to perform connector-specific checks.
+   *
+   * <p>Subclasses should override this with logic to run after the common validation for Snowflake.
+   *
+   * @param arguments User-provided arguments of the Dumper run.
+   */
+  protected abstract void validateForConnector(@Nonnull ConnectorArguments arguments);
+
   private static void removeDuplicateMessageAndThrow(
       @Nullable MetadataDumperUsageException exception) {
     if (exception != null) {

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/snowflake/SnowflakeLiteConnector.java

@@ -20,15 +20,12 @@
 import com.google.edwmigration.dumper.application.dumper.ConnectorArguments;
 import com.google.edwmigration.dumper.application.dumper.MetadataDumperUsageException;
 import com.google.edwmigration.dumper.application.dumper.connector.Connector;
-import com.google.edwmigration.dumper.application.dumper.handle.Handle;
 import com.google.edwmigration.dumper.application.dumper.task.DumpMetadataTask;
 import com.google.edwmigration.dumper.application.dumper.task.FormatTask;
 import com.google.edwmigration.dumper.application.dumper.task.Task;
 import com.google.edwmigration.dumper.application.dumper.utils.ArchiveNameUtil;
-import java.sql.SQLException;
 import java.time.Clock;
 import java.util.List;
-import java.util.Objects;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.annotation.ParametersAreNonnullByDefault;
@@ -46,13 +43,6 @@ public SnowflakeLiteConnector() {
     super(NAME);
   }
 
-  @Override
-  @Nonnull
-  public Handle open(ConnectorArguments arguments)
-      throws MetadataDumperUsageException, SQLException {
-    return super.open(arguments);
-  }
-
   @Override
   @Nonnull
   public String getDefaultFileName(boolean isAssessment, @Nullable Clock clock) {
@@ -73,10 +63,7 @@ public final void addTasksTo(List<? super Task<?>> out, ConnectorArguments argum
   }
 
   @Override
-  public final void validate(@Nullable ConnectorArguments arguments) {
-    super.validate(arguments);
-
-    Objects.requireNonNull(arguments);
+  protected void validateForConnector(ConnectorArguments arguments) {
     if (!arguments.isAssessment()) {
       throw noAssessmentException();
     }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/snowflake/SnowflakeLogsConnector.java

@@ -161,9 +161,7 @@ private TaskDescription(
   }
 
   @Override
-  public final void validate(ConnectorArguments arguments) {
-    super.validate(arguments);
-
+  protected final void validateForConnector(@Nonnull ConnectorArguments arguments) {
     if (arguments.isAssessment() && arguments.hasQueryLogEarliestTimestamp()) {
       throw unsupportedOption(ConnectorArguments.OPT_QUERY_LOG_EARLIEST_TIMESTAMP);
     }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/snowflake/SnowflakeMetadataConnector.java

@@ -129,6 +129,9 @@ public Iterable<ConnectorProperty> getPropertyConstants() {
     return builder.build();
   }
 
+  @Override
+  protected final void validateForConnector(@Nonnull ConnectorArguments arguments) {}
+
   private void addSqlTasksWithInfoSchemaFallback(
       @Nonnull List<? super Task<?>> out,
       @Nonnull Class<? extends Enum<?>> header,

dumper/app/src/test/java/com/google/edwmigration/dumper/application/dumper/connector/ConnectorTest.java

@@ -16,101 +16,62 @@
  */
 package com.google.edwmigration.dumper.application.dumper.connector;
 
+import static com.google.edwmigration.dumper.application.dumper.connector.Connector.validateDateRange;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThrows;
 
 import com.google.edwmigration.dumper.application.dumper.ConnectorArguments;
-import com.google.edwmigration.dumper.application.dumper.handle.Handle;
-import com.google.edwmigration.dumper.application.dumper.task.Task;
-import java.time.Clock;
-import java.util.List;
-import javax.annotation.Nonnull;
 import org.junit.Test;
 
 public class ConnectorTest {
-  private final Connector connector = new EmptyConnector();
 
   @Test
   public void validateDateRange_startDateAndEndDate_success() throws Exception {
-    String argsStr = "--connector test --start-date=2001-02-20 --end-date=2001-02-25";
+    ConnectorArguments args =
+        new ConnectorArguments(
+            "--connector", "test", "--start-date=2001-02-20", "--end-date=2001-02-25");
 
     // Act
-    connector.validateDateRange(toArgs(argsStr));
+    validateDateRange(args);
   }
 
   @Test
-  public void validateDateRange_startDateAfterEndDate_throws() {
-    String argsStr = "--connector test --start-date=2001-02-20 --end-date=2001-02-20";
+  public void validateDateRange_startDateAfterEndDate_throws() throws Exception {
+    ConnectorArguments args =
+        new ConnectorArguments(
+            "--connector", "test", "--start-date=2001-02-20", "--end-date=2001-02-20");
 
     Exception exception =
-        assertThrows(
-            IllegalStateException.class, () -> connector.validateDateRange(toArgs(argsStr)));
+        assertThrows(RuntimeException.class, () -> Connector.validateDateRange(args));
     assertEquals(
         "Start date [2001-02-20T00:00Z] must be before end date [2001-02-20T00:00Z].",
         exception.getMessage());
   }
 
   @Test
-  public void validateDateRange_endDateAlone_throws() {
-    String argsStr = "--connector test --end-date=2001-02-20";
+  public void validateDateRange_endDateAlone_throws() throws Exception {
+    ConnectorArguments args =
+        new ConnectorArguments("--connector", "test", "--end-date=2001-02-20");
 
-    Exception exception =
-        assertThrows(
-            IllegalStateException.class, () -> connector.validateDateRange(toArgs(argsStr)));
+    Exception exception = assertThrows(RuntimeException.class, () -> validateDateRange(args));
     assertEquals(
         "End date can be specified only with start date, but start date was null.",
         exception.getMessage());
   }
 
   @Test
-  public void validateDateRange_startDateAlone_throws() {
-    String argsStr = "--connector test --start-date=2001-02-20";
+  public void validateDateRange_startDateAlone_throws() throws Exception {
+    ConnectorArguments args =
+        new ConnectorArguments("--connector", "test", "--start-date=2001-02-20");
 
-    Exception exception =
-        assertThrows(RuntimeException.class, () -> connector.validateDateRange(toArgs(argsStr)));
+    Exception exception = assertThrows(RuntimeException.class, () -> validateDateRange(args));
     assertEquals(
         "End date must be specified with start date, but was null.", exception.getMessage());
   }
 
   @Test
   public void validateDateRange_requiredArgs_success() throws Exception {
     // Act
-    connector.validateDateRange(toArgs("--connector test"));
-  }
-
-  private static class EmptyConnector implements Connector {
-
-    @Nonnull
-    @Override
-    public String getName() {
-      return null;
-    }
-
-    @Nonnull
-    @Override
-    public String getDefaultFileName(boolean isAssessment, Clock clock) {
-      return null;
-    }
-
-    @Override
-    public void addTasksTo(
-        @Nonnull List<? super Task<?>> out, @Nonnull ConnectorArguments arguments)
-        throws Exception {}
-
-    @Nonnull
-    @Override
-    public Handle open(@Nonnull ConnectorArguments arguments) throws Exception {
-      return null;
-    }
-
-    @Nonnull
-    @Override
-    public Iterable<ConnectorProperty> getPropertyConstants() {
-      return null;
-    }
-  }
-
-  private static ConnectorArguments toArgs(String args) throws Exception {
-    return new ConnectorArguments(args.split(" "));
+    validateDateRange(new ConnectorArguments("--connector", "test"));
   }
 }