feat: Conditionally run more multi-file validators if their dependenc…

…ies don't have parse errors (#1496)

core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoader.java

@@ -20,6 +20,7 @@
 import com.google.common.flogger.FluentLogger;
 import java.io.InputStream;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -47,6 +48,12 @@ public class GtfsFeedLoader {
   private final HashMap<String, GtfsTableDescriptor<?>> tableDescriptors = new HashMap<>();
   private int numThreads = 1;
 
+  /**
+   * The set of validators that were skipped during validation because their file dependencies had
+   * parse errors.
+   */
+  private final List<Class<? extends FileValidator>> skippedValidators = new ArrayList<>();
+
   public GtfsFeedLoader(
       ImmutableList<Class<? extends GtfsTableDescriptor<?>>> tableDescriptorClasses) {
     for (Class<? extends GtfsTableDescriptor<?>> clazz : tableDescriptorClasses) {
@@ -72,6 +79,10 @@ public void setNumThreads(int numThreads) {
     this.numThreads = numThreads;
   }
 
+  public List<Class<? extends FileValidator>> getSkippedValidators() {
+    return Collections.unmodifiableList(skippedValidators);
+  }
+
   @SuppressWarnings("unchecked")
   public GtfsFeedContainer loadAndValidate(
       GtfsInput gtfsInput, ValidatorProvider validatorProvider, NoticeContainer noticeContainer)
@@ -132,17 +143,11 @@ public GtfsFeedContainer loadAndValidate(
         }
       }
       GtfsFeedContainer feed = new GtfsFeedContainer(tableContainers);
-      if (!feed.isParsedSuccessfully()) {
-        // No need to call file validators if any file failed to parse. File validations in that
-        // case may lead to confusing error messages.
-        //
-        // Consider we failed to parse a row trip.txt but there is another row in stop_times.txt
-        // that references a trip. Then foreign key validator may notify about a missing trip_id
-        // which would be wrong.
-        return feed;
-      }
       List<Callable<NoticeContainer>> validatorCallables = new ArrayList<>();
-      for (FileValidator validator : validatorProvider.createMultiFileValidators(feed)) {
+      // Validators with parser-error dependencies will not be returned here, but instead added to
+      // the skippedValidators list.
+      for (FileValidator validator :
+          validatorProvider.createMultiFileValidators(feed, skippedValidators::add)) {
         validatorCallables.add(
             () -> {
               NoticeContainer validatorNotices = new NoticeContainer();

core/src/main/java/org/mobilitydata/gtfsvalidator/testing/MockGtfs.java

@@ -0,0 +1,64 @@
+package org.mobilitydata.gtfsvalidator.testing;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+public class MockGtfs {
+  private final File file;
+
+  private final Map<String, byte[]> _fileContentsByName = new HashMap<>();
+
+  private MockGtfs(File file) {
+    this.file = file;
+  }
+
+  public static MockGtfs create() throws IOException {
+    File file = File.createTempFile("MockGtfs-", ".zip");
+    file.deleteOnExit();
+    return new MockGtfs(file);
+  }
+
+  public File getFile() {
+    return file;
+  }
+
+  public Path getPath() {
+    return file.toPath();
+  }
+
+  public void putFileFromString(String fileName, String content) {
+    _fileContentsByName.put(fileName, content.getBytes(StandardCharsets.UTF_8));
+    updateZipContents();
+  }
+
+  public void putFileFromLines(String fileName, String... lines) {
+    putFileFromString(fileName, String.join("\n", lines));
+  }
+
+  private void updateZipContents() {
+    try {
+      if (file.exists()) {
+        file.delete();
+      }
+      ZipOutputStream out = new ZipOutputStream(new FileOutputStream(file));
+      for (Map.Entry<String, byte[]> entry : _fileContentsByName.entrySet()) {
+        String fileName = entry.getKey();
+        byte[] content = entry.getValue();
+        ZipEntry zipEntry = new ZipEntry(fileName);
+        out.putNextEntry(zipEntry);
+        out.write(content);
+        out.closeEntry();
+      }
+      out.close();
+    } catch (IOException ex) {
+      throw new IllegalStateException(ex);
+    }
+  }
+}

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultValidatorProvider.java

@@ -20,9 +20,11 @@
 import com.google.common.flogger.FluentLogger;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Consumer;
 import org.mobilitydata.gtfsvalidator.table.GtfsEntity;
 import org.mobilitydata.gtfsvalidator.table.GtfsFeedContainer;
 import org.mobilitydata.gtfsvalidator.table.GtfsTableContainer;
+import org.mobilitydata.gtfsvalidator.validator.ValidatorLoader.ValidatorWithDependencyStatus;
 
 /** Default implementation of {@link ValidatorProvider}. */
 public class DefaultValidatorProvider implements ValidatorProvider {
@@ -109,13 +111,19 @@ public <T extends GtfsEntity> List<FileValidator> createSingleFileValidators(
   }
 
   @Override
-  public List<FileValidator> createMultiFileValidators(GtfsFeedContainer feed) {
+  public List<FileValidator> createMultiFileValidators(
+      GtfsFeedContainer feed, Consumer<Class<? extends FileValidator>> skippedValidators) {
     ArrayList<FileValidator> validators = new ArrayList<>();
     validators.ensureCapacity(multiFileValidators.size());
     for (Class<? extends FileValidator> validatorClass : multiFileValidators) {
       try {
-        validators.add(
-            ValidatorLoader.createMultiFileValidator(validatorClass, feed, validationContext));
+        ValidatorWithDependencyStatus<? extends FileValidator> validatorWithStatus =
+            ValidatorLoader.createMultiFileValidator(validatorClass, feed, validationContext);
+        if (validatorWithStatus.dependenciesHaveErrors()) {
+          skippedValidators.accept(validatorClass);
+        } else {
+          validators.add(validatorWithStatus.validator());
+        }
       } catch (ReflectiveOperationException e) {
         logger.atSevere().withCause(e).log(
             "Cannot instantiate validator %s", validatorClass.getCanonicalName());

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

@@ -26,7 +26,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.function.Function;
+import javax.annotation.Nullable;
 import javax.inject.Inject;
 import org.mobilitydata.gtfsvalidator.input.CountryCode;
 import org.mobilitydata.gtfsvalidator.input.CurrentDateTime;
@@ -169,16 +169,9 @@ private static <T> Constructor<T> chooseConstructor(Class<T> validatorClass)
             validatorClass.getCanonicalName()));
   }
 
-  /**
-   * Instantiates a validator of given class and injects its dependencies.
-   *
-   * @param clazz validator class to instantiate
-   * @param provider dependency provider
-   * @param <T> type of the validator to instantiate
-   * @return a new validator
-   */
-  private static <T> T createValidator(Class<T> clazz, Function<Class<?>, Object> provider)
-      throws ReflectiveOperationException {
+  /** Instantiates a validator of given class and injects its dependencies. */
+  private static <T> ValidatorWithDependencyStatus<T> createValidator(
+      Class<T> clazz, DependencyResolver dependencyResolver) throws ReflectiveOperationException {
     Constructor<T> chosenConstructor;
     try {
       chosenConstructor = chooseConstructor(clazz);
@@ -189,10 +182,13 @@ private static <T> T createValidator(Class<T> clazz, Function<Class<?>, Object>
     // Inject constructor parameters.
     Object[] parameters = new Object[chosenConstructor.getParameterCount()];
     for (int i = 0; i < parameters.length; ++i) {
-      parameters[i] = provider.apply(chosenConstructor.getParameters()[i].getType());
+      parameters[i] =
+          dependencyResolver.resolveDependency(chosenConstructor.getParameters()[i].getType());
     }
     chosenConstructor.setAccessible(true);
-    return chosenConstructor.newInstance(parameters);
+    T validator = chosenConstructor.newInstance(parameters);
+    return new ValidatorWithDependencyStatus<T>(
+        validator, dependencyResolver.dependenciesHaveErrors);
   }
 
   /**
@@ -205,7 +201,8 @@ private static <T> T createValidator(Class<T> clazz, Function<Class<?>, Object>
    */
   public static <T> T createValidatorWithContext(
       Class<T> clazz, ValidationContext validationContext) throws ReflectiveOperationException {
-    return createValidator(clazz, validationContext::get);
+    return createValidator(clazz, new DependencyResolver(validationContext, null, null))
+        .validator();
   }
 
   /** Instantiates a {@code FileValidator} for a single table. */
@@ -214,29 +211,81 @@ public static FileValidator createSingleFileValidator(
       GtfsTableContainer<?> table,
       ValidationContext validationContext)
       throws ReflectiveOperationException {
-    return createValidator(
-        clazz,
-        parameterClass ->
-            parameterClass.isAssignableFrom(table.getClass())
-                ? table
-                : validationContext.get(parameterClass));
+    return createValidator(clazz, new DependencyResolver(validationContext, table, null))
+        .validator();
   }
 
   /** Instantiates a {@code FileValidator} for multiple tables in a given feed. */
   @SuppressWarnings("unchecked")
-  public static FileValidator createMultiFileValidator(
-      Class<? extends FileValidator> clazz,
-      GtfsFeedContainer feed,
-      ValidationContext validationContext)
+  public static <T extends FileValidator> ValidatorWithDependencyStatus<T> createMultiFileValidator(
+      Class<T> clazz, GtfsFeedContainer feed, ValidationContext validationContext)
       throws ReflectiveOperationException {
-    return createValidator(
-        clazz,
-        parameterClass ->
-            GtfsFeedContainer.class.isAssignableFrom(parameterClass)
-                ? feed
-                : GtfsTableContainer.class.isAssignableFrom(parameterClass)
-                    ? feed.getTable((Class<? extends GtfsTableContainer<?>>) parameterClass)
-                    : validationContext.get(parameterClass));
+    return createValidator(clazz, new DependencyResolver(validationContext, null, feed));
+  }
+
+  /**
+   * Helper class for resolving injected dependencies of validators, while also tracking if those
+   * dependencies have blocking errors.
+   */
+  private static class DependencyResolver {
+    private final ValidationContext context;
+    @Nullable private final GtfsTableContainer<?> tableContainer;
+    @Nullable private final GtfsFeedContainer feedContainer;
+
+    /** This will be set to true if a resolved dependency was not parsed successfully. */
+    private boolean dependenciesHaveErrors = false;
+
+    public DependencyResolver(
+        ValidationContext context,
+        @Nullable GtfsTableContainer<?> tableContainer,
+        @Nullable GtfsFeedContainer feedContainer) {
+      this.context = context;
+      this.tableContainer = tableContainer;
+      this.feedContainer = feedContainer;
+    }
+
+    public Object resolveDependency(Class<?> parameterClass) {
+      if (feedContainer != null && parameterClass.isAssignableFrom(GtfsFeedContainer.class)) {
+        if (!feedContainer.isParsedSuccessfully()) {
+          dependenciesHaveErrors = true;
+        }
+        return feedContainer;
+      }
+      if (tableContainer != null && parameterClass.isAssignableFrom(tableContainer.getClass())) {
+        if (!tableContainer.isParsedSuccessfully()) {
+          dependenciesHaveErrors = true;
+        }
+        return tableContainer;
+      }
+      if (feedContainer != null && GtfsTableContainer.class.isAssignableFrom(parameterClass)) {
+        GtfsTableContainer<?> container =
+            feedContainer.getTable((Class<? extends GtfsTableContainer<?>>) parameterClass);
+        if (container != null && !container.isParsedSuccessfully()) {
+          dependenciesHaveErrors = true;
+        }
+        return container;
+      }
+      return context.get(parameterClass);
+    }
+  }
+
+  public static final class ValidatorWithDependencyStatus<T> {
+    private final T validator;
+    // Will be true if one of the injected dependencies of the validator has parse errors.
+    private final boolean dependenciesHaveErrors;
+
+    public ValidatorWithDependencyStatus(T validator, boolean dependenciesHaveErrors) {
+      this.validator = validator;
+      this.dependenciesHaveErrors = dependenciesHaveErrors;
+    }
+
+    public T validator() {
+      return validator;
+    }
+
+    public boolean dependenciesHaveErrors() {
+      return dependenciesHaveErrors;
+    }
   }
 
   /** Describes all loaded validators. */

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorProvider.java

@@ -17,6 +17,7 @@
 package org.mobilitydata.gtfsvalidator.validator;
 
 import java.util.List;
+import java.util.function.Consumer;
 import org.mobilitydata.gtfsvalidator.table.GtfsEntity;
 import org.mobilitydata.gtfsvalidator.table.GtfsFeedContainer;
 import org.mobilitydata.gtfsvalidator.table.GtfsTableContainer;
@@ -59,11 +60,15 @@ <T extends GtfsEntity> List<FileValidator> createSingleFileValidators(
       GtfsTableContainer<T> table);
 
   /**
-   * Creates a list of cross-table validators.
+   * Creates a list of cross-table validators. Any validator that has a dependency with parse errors
+   * will not be returned by this method, but instead noted with a call to the `skippedValidators`
+   * callback.
    *
    * <p>Use {@link ValidatorUtil#safeValidate} to invoke each validator.
    *
    * @param feed GTFS feed to validate
+   * @param skippedValidators
    */
-  List<FileValidator> createMultiFileValidators(GtfsFeedContainer feed);
+  List<FileValidator> createMultiFileValidators(
+      GtfsFeedContainer feed, Consumer<Class<? extends FileValidator>> skippedValidators);
 }

core/src/test/java/org/mobilitydata/gtfsvalidator/TestUtils.java

@@ -5,9 +5,13 @@
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
+import java.time.ZonedDateTime;
 import java.util.List;
+import org.mobilitydata.gtfsvalidator.input.CountryCode;
+import org.mobilitydata.gtfsvalidator.input.CurrentDateTime;
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
+import org.mobilitydata.gtfsvalidator.validator.ValidationContext;
 
 public class TestUtils {
 
@@ -24,4 +28,11 @@ public static List<Class<? extends ValidationNotice>> validationNoticeTypes(
   public static InputStream toInputStream(String s) {
     return new ByteArrayInputStream(s.getBytes(StandardCharsets.UTF_8));
   }
+
+  public static ValidationContext contextForTest() {
+    return ValidationContext.builder()
+        .setCountryCode(CountryCode.forStringOrUnknown("ca"))
+        .setCurrentDateTime(new CurrentDateTime(ZonedDateTime.now()))
+        .build();
+  }
 }