feat: 1537 get list of validators that couldnt run because of a parsi…

…ng problem (#1565) * update comments for skippedValidator * added in validators with parsing errors logic and fixed tests * continue fix broken tests * applied the same logic to singleEntityValidators * fix RunTimeException ClassCastError * use Mockito's ArgumentCaptor to capture the argument for fixing "Strict stubbing argument mismatch" error * minor change * formatted code * renamed validatorsWithParsingErrors to singleFileValidatorsWithParsingErrors * renamed skippedFileValidators to multiFileValidatorsWithParsingErrors formatted code * combine three types of validators with parsing errors together and output validators that couldn't run due to parsing problems * renamed skippedValidators to multiFileValidatorsWithParsingErrors * formatted code * Added a test for single file skipped validators. Reformatted the output --------- Co-authored-by: jcpitre <[email protected]> Co-authored-by: jcpitre <[email protected]>
MobilityData · Sep 15, 2023 · 57eed2f · 57eed2f
1 parent 8834684
commit 57eed2f
Show file tree

Hide file tree

Showing 14 changed files with 428 additions and 63 deletions.
diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/table/AnyTableLoader.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/table/AnyTableLoader.java
@@ -7,6 +7,7 @@
 import com.univocity.parsers.csv.CsvParserSettings;
 import java.io.InputStream;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -21,13 +22,27 @@
 import org.mobilitydata.gtfsvalidator.parsing.CsvRow;
 import org.mobilitydata.gtfsvalidator.parsing.FieldCache;
 import org.mobilitydata.gtfsvalidator.parsing.RowParser;
+import org.mobilitydata.gtfsvalidator.validator.FileValidator;
 import org.mobilitydata.gtfsvalidator.validator.SingleEntityValidator;
 import org.mobilitydata.gtfsvalidator.validator.ValidatorProvider;
 import org.mobilitydata.gtfsvalidator.validator.ValidatorUtil;
 
 public final class AnyTableLoader {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+  private static final List<Class<? extends FileValidator>> singleFileValidatorsWithParsingErrors =
+      new ArrayList<>();
+
+  private static final List<Class<? extends SingleEntityValidator>>
+      singleEntityValidatorsWithParsingErrors = new ArrayList<>();
+
+  public List<Class<? extends FileValidator>> getValidatorsWithParsingErrors() {
+    return Collections.unmodifiableList(singleFileValidatorsWithParsingErrors);
+  }
+
+  public List<Class<? extends SingleEntityValidator>> getSingleEntityValidatorsWithParsingErrors() {
+    return Collections.unmodifiableList(singleEntityValidatorsWithParsingErrors);
+  }
 
   public static GtfsTableContainer load(
       GtfsTableDescriptor tableDescriptor,
@@ -84,7 +99,8 @@ public static GtfsTableContainer load(
     final List<GtfsEntity> entities = new ArrayList<>();
     boolean hasUnparsableRows = false;
     final List<SingleEntityValidator<GtfsEntity>> singleEntityValidators =
-        validatorProvider.createSingleEntityValidators(tableDescriptor.getEntityClass());
+        validatorProvider.createSingleEntityValidators(
+            tableDescriptor.getEntityClass(), singleEntityValidatorsWithParsingErrors::add);
     try {
       for (CsvRow row : csvFile) {
         if (row.getRowNumber() % 200000 == 0) {
@@ -130,7 +146,9 @@ public static GtfsTableContainer load(
     GtfsTableContainer table =
         tableDescriptor.createContainerForHeaderAndEntities(header, entities, noticeContainer);
     ValidatorUtil.invokeSingleFileValidators(
-        validatorProvider.createSingleFileValidators(table), noticeContainer);
+        validatorProvider.createSingleFileValidators(
+            table, singleFileValidatorsWithParsingErrors::add),
+        noticeContainer);
     return table;
   }
 
@@ -194,7 +212,9 @@ public static GtfsTableContainer loadMissingFile(
       noticeContainer.addValidationNotice(new MissingRequiredFileNotice(gtfsFilename));
     }
     ValidatorUtil.invokeSingleFileValidators(
-        validatorProvider.createSingleFileValidators(table), noticeContainer);
+        validatorProvider.createSingleFileValidators(
+            table, singleFileValidatorsWithParsingErrors::add),
+        noticeContainer);
     return table;
   }
 }
diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoader.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoader.java
@@ -50,9 +50,10 @@ public class GtfsFeedLoader {
 
   /**
    * The set of validators that were skipped during validation because their file dependencies had
-   * parse errors.
+   * parse errors plus validators that are optional.
    */
-  private final List<Class<? extends FileValidator>> skippedValidators = new ArrayList<>();
+  private final List<Class<? extends FileValidator>> multiFileValidatorsWithParsingErrors =
+      new ArrayList<>();
 
   public GtfsFeedLoader(
       ImmutableList<Class<? extends GtfsTableDescriptor<?>>> tableDescriptorClasses) {
@@ -79,8 +80,8 @@ public void setNumThreads(int numThreads) {
     this.numThreads = numThreads;
   }
 
-  public List<Class<? extends FileValidator>> getSkippedValidators() {
-    return Collections.unmodifiableList(skippedValidators);
+  public List<Class<? extends FileValidator>> getMultiFileValidatorsWithParsingErrors() {
+    return Collections.unmodifiableList(multiFileValidatorsWithParsingErrors);
   }
 
   @SuppressWarnings("unchecked")
@@ -147,7 +148,8 @@ public GtfsFeedContainer loadAndValidate(
       // 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)) {
+          validatorProvider.createMultiFileValidators(
+              feed, multiFileValidatorsWithParsingErrors::add)) {
         validatorCallables.add(
             () -> {
               NoticeContainer validatorNotices = new NoticeContainer();

diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultValidatorProvider.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultValidatorProvider.java
@@ -76,14 +76,23 @@ public TableHeaderValidator getTableHeaderValidator() {
   @Override
   @SuppressWarnings("unchecked")
   public <T extends GtfsEntity> List<SingleEntityValidator<T>> createSingleEntityValidators(
-      Class<T> clazz) {
+      Class<T> clazz,
+      Consumer<Class<? extends SingleEntityValidator<T>>> singleEntityValidatorsWithParsingErrors) {
     List<SingleEntityValidator<T>> validators = new ArrayList<>();
     for (Class<? extends SingleEntityValidator<?>> validatorClass :
         singleEntityValidators.get(clazz)) {
       try {
-        validators.add(
-            ValidatorLoader.createValidatorWithContext(
-                ((Class<? extends SingleEntityValidator<T>>) validatorClass), validationContext));
+        ValidatorWithDependencyStatus<? extends SingleEntityValidator<?>>
+            validatorWithDependencyStatus =
+                ValidatorLoader.createValidatorWithContext(
+                    ((Class<? extends SingleEntityValidator<?>>) validatorClass),
+                    validationContext);
+        if (validatorWithDependencyStatus.dependenciesHaveErrors()) {
+          singleEntityValidatorsWithParsingErrors.accept(
+              (Class<? extends SingleEntityValidator<T>>) validatorClass);
+        } else {
+          validators.add((SingleEntityValidator<T>) validatorWithDependencyStatus.validator());
+        }
       } catch (ReflectiveOperationException e) {
         logger.atSevere().withCause(e).log(
             "Cannot instantiate validator %s", validatorClass.getCanonicalName());
@@ -95,13 +104,19 @@ public <T extends GtfsEntity> List<SingleEntityValidator<T>> createSingleEntityV
   @Override
   @SuppressWarnings("unchecked")
   public <T extends GtfsEntity> List<FileValidator> createSingleFileValidators(
-      GtfsTableContainer<T> table) {
+      GtfsTableContainer<T> table,
+      Consumer<Class<? extends FileValidator>> validatorsWithParsingErrors) {
     List<FileValidator> validators = new ArrayList<>();
     for (Class<? extends FileValidator> validatorClass :
         singleFileValidators.get((Class<? extends GtfsTableContainer<?>>) table.getClass())) {
       try {
-        validators.add(
-            ValidatorLoader.createSingleFileValidator(validatorClass, table, validationContext));
+        ValidatorWithDependencyStatus<? extends FileValidator> validatorWithStatus =
+            ValidatorLoader.createSingleFileValidator(validatorClass, table, validationContext);
+        if (validatorWithStatus.dependenciesHaveErrors()) {
+          validatorsWithParsingErrors.accept(validatorClass);
+        } else {
+          validators.add(validatorWithStatus.validator());
+        }
       } catch (ReflectiveOperationException e) {
         logger.atSevere().withCause(e).log(
             "Cannot instantiate validator %s", validatorClass.getCanonicalName());

diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java
@@ -169,7 +169,11 @@ private static <T> Constructor<T> chooseConstructor(Class<T> validatorClass)
             validatorClass.getCanonicalName()));
   }
 
-  /** Instantiates a validator of given class and injects its dependencies. */
+  /**
+   * Instantiates a validator of given class and injects its dependencies. A dependency is typically
+   * a table that is used by the validator. If any of the dependencies have errors, we put this
+   * information in the returned {@link ValidatorWithDependencyStatus}.
+   */
   private static <T> ValidatorWithDependencyStatus<T> createValidator(
       Class<T> clazz, DependencyResolver dependencyResolver) throws ReflectiveOperationException {
     Constructor<T> chosenConstructor;
@@ -199,20 +203,22 @@ private static <T> ValidatorWithDependencyStatus<T> createValidator(
    * @param <T> type of the validator to instantiate
    * @return a new validator
    */
-  public static <T> T createValidatorWithContext(
-      Class<T> clazz, ValidationContext validationContext) throws ReflectiveOperationException {
-    return createValidator(clazz, new DependencyResolver(validationContext, null, null))
-        .validator();
+  public static <T extends SingleEntityValidator>
+      ValidatorWithDependencyStatus<T> createValidatorWithContext(
+          Class<T> clazz, ValidationContext validationContext) throws ReflectiveOperationException {
+    return (ValidatorWithDependencyStatus<T>)
+        createValidator(clazz, new DependencyResolver(validationContext, null, null));
   }
 
   /** Instantiates a {@code FileValidator} for a single table. */
-  public static FileValidator createSingleFileValidator(
-      Class<? extends FileValidator> clazz,
-      GtfsTableContainer<?> table,
-      ValidationContext validationContext)
-      throws ReflectiveOperationException {
-    return createValidator(clazz, new DependencyResolver(validationContext, table, null))
-        .validator();
+  public static <T extends FileValidator>
+      ValidatorWithDependencyStatus<T> createSingleFileValidator(
+          Class<? extends FileValidator> clazz,
+          GtfsTableContainer<?> table,
+          ValidationContext validationContext)
+          throws ReflectiveOperationException {
+    return (ValidatorWithDependencyStatus<T>)
+        createValidator(clazz, new DependencyResolver(validationContext, table, null));
   }
 
   /** Instantiates a {@code FileValidator} for multiple tables in a given feed. */
@@ -244,6 +250,13 @@ public DependencyResolver(
       this.feedContainer = feedContainer;
     }
 
+    /**
+     * This is typically called to obtain the container related to each parameter in a validator's
+     * constructor or a basic type related to country code or date.
+     *
+     * @param parameterClass The parameter of the constructor we want to examine.
+     * @return
+     */
     public Object resolveDependency(Class<?> parameterClass) {
       if (feedContainer != null && parameterClass.isAssignableFrom(GtfsFeedContainer.class)) {
         if (!feedContainer.isParsedSuccessfully()) {

diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorProvider.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorProvider.java
@@ -46,7 +46,8 @@ public interface ValidatorProvider {
    * @return a list of validators
    */
   <T extends GtfsEntity> List<SingleEntityValidator<T>> createSingleEntityValidators(
-      Class<T> clazz);
+      Class<T> clazz,
+      Consumer<Class<? extends SingleEntityValidator<T>>> singleEntityValidatorsWithParsingErrors);
 
   /**
    * Creates a list of validators for the given table.
@@ -57,7 +58,8 @@ <T extends GtfsEntity> List<SingleEntityValidator<T>> createSingleEntityValidato
    * @param <T> type of the GTFS entity
    */
   <T extends GtfsEntity> List<FileValidator> createSingleFileValidators(
-      GtfsTableContainer<T> table);
+      GtfsTableContainer<T> table,
+      Consumer<Class<? extends FileValidator>> validatorsWithParsingErrors);
 
   /**
    * Creates a list of cross-table validators. Any validator that has a dependency with parse errors

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/table/AnyTableLoaderTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/table/AnyTableLoaderTest.java
@@ -9,18 +9,22 @@
 import java.io.InputStream;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Consumer;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.mobilitydata.gtfsvalidator.annotation.FieldLevelEnum;
 import org.mobilitydata.gtfsvalidator.notice.*;
 import org.mobilitydata.gtfsvalidator.parsing.CsvHeader;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestEntity;
-import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestFileValidator;
+import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestSingleFileValidator;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestTableDescriptor;
+import org.mobilitydata.gtfsvalidator.validator.FileValidator;
 import org.mobilitydata.gtfsvalidator.validator.GtfsFieldValidator;
 import org.mobilitydata.gtfsvalidator.validator.TableHeaderValidator;
 import org.mobilitydata.gtfsvalidator.validator.ValidatorProvider;
+import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatchers;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
@@ -124,8 +128,11 @@ public void parsableTableRows() {
     var testTableDescriptor = new GtfsTestTableDescriptor();
     when(validatorProvider.getTableHeaderValidator()).thenReturn(mock(TableHeaderValidator.class));
     when(validatorProvider.getFieldValidator()).thenReturn(mock(GtfsFieldValidator.class));
-    GtfsTestFileValidator validator = mock(GtfsTestFileValidator.class);
-    when(validatorProvider.createSingleFileValidators(any())).thenReturn(List.of(validator));
+    GtfsTestSingleFileValidator validator = mock(GtfsTestSingleFileValidator.class);
+    ArgumentCaptor<Consumer<Class<? extends FileValidator>>> captor =
+        ArgumentCaptor.forClass(Consumer.class);
+    when(validatorProvider.createSingleFileValidators(ArgumentMatchers.any(), captor.capture()))
+        .thenReturn((List.of(validator)));
     InputStream inputStream = toInputStream("id,stop_lat,_no_name_\n" + "s1, 23.00, no_value\n");
 
     var loadedContainer =

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoaderTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoaderTest.java
@@ -13,7 +13,7 @@
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestEntity;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestEntityValidator;
-import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestFileValidator;
+import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestSingleFileValidator;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestTableContainer;
 import org.mobilitydata.gtfsvalidator.testgtfs.GtfsTestTableDescriptor;
 import org.mobilitydata.gtfsvalidator.testgtfs.WholeFeedValidator;
@@ -26,7 +26,9 @@
 public class GtfsFeedLoaderTest {
   private static final ImmutableList<Class<?>> VALIDATOR_CLASSES =
       ImmutableList.of(
-          GtfsTestEntityValidator.class, GtfsTestFileValidator.class, WholeFeedValidator.class);
+          GtfsTestEntityValidator.class,
+          GtfsTestSingleFileValidator.class,
+          WholeFeedValidator.class);
 
   private MockGtfs mockGtfs;
   private NoticeContainer noticeContainer = new NoticeContainer();
@@ -74,6 +76,7 @@ public void testInvalidDataInTable() throws Exception {
     GtfsTestTableContainer container = feedContainer.getTable(GtfsTestTableContainer.class);
     assertThat(container.getEntities()).isEmpty();
 
-    assertThat(loader.getSkippedValidators()).containsExactly(WholeFeedValidator.class);
+    assertThat(loader.getMultiFileValidatorsWithParsingErrors())
+        .containsExactly(WholeFeedValidator.class);
   }
 }
diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestMultiFileValidator.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestMultiFileValidator.java
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2021 Google LLC
+ *
+ * Licensed 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.mobilitydata.gtfsvalidator.testgtfs;
+
+import javax.inject.Inject;
+import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
+import org.mobilitydata.gtfsvalidator.validator.FileValidator;
+
+public class GtfsTestMultiFileValidator extends FileValidator {
+
+  private final GtfsTestTableContainer table;
+  private final GtfsTestTableContainer2 unparsableTable;
+
+  @Inject
+  public GtfsTestMultiFileValidator(GtfsTestTableContainer table, GtfsTestTableContainer2 table2) {
+    this.table = table;
+    this.unparsableTable = table2;
+  }
+
+  @Override
+  public void validate(NoticeContainer noticeContainer) {}
+}
diff --git a/...dator/testgtfs/GtfsTestFileValidator.java → ...testgtfs/GtfsTestSingleFileValidator.java b/...dator/testgtfs/GtfsTestFileValidator.java → ...testgtfs/GtfsTestSingleFileValidator.java
@@ -22,14 +22,14 @@
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.validator.FileValidator;
 
-public class GtfsTestFileValidator extends FileValidator {
+public class GtfsTestSingleFileValidator extends FileValidator {
 
   private final GtfsTestTableContainer table;
   private final CountryCode countryCode;
   private final CurrentDateTime currentDateTime;
 
   @Inject
-  public GtfsTestFileValidator(
+  public GtfsTestSingleFileValidator(
       GtfsTestTableContainer table, CountryCode countryCode, CurrentDateTime currentDateTime) {
     this.table = table;
     this.countryCode = countryCode;