feat: removed missing_recommended_column and related annotation + c…

…hanged `missing_timepoint_value` notice description (#1963)
MobilityData · Feb 4, 2025 · d0b5263 · d0b5263
1 parent 2ed4dd7
commit d0b5263
Show file tree

Hide file tree

Showing 18 changed files with 2 additions and 230 deletions.
diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MissingRecommendedColumnNotice.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MissingRecommendedColumnNotice.java
diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/table/CsvFileLoader.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/table/CsvFileLoader.java
@@ -161,10 +161,6 @@ private NoticeContainer validateHeaders(
                 .filter(GtfsColumnDescriptor::headerRequired)
                 .map(GtfsColumnDescriptor::columnName)
                 .collect(Collectors.toSet()),
-            columnDescriptors.stream()
-                .filter(GtfsColumnDescriptor::headerRecommended)
-                .map(GtfsColumnDescriptor::columnName)
-                .collect(Collectors.toSet()),
             headerNotices);
     return headerNotices;
   }

diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsColumnDescriptor.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsColumnDescriptor.java
@@ -11,8 +11,6 @@ public abstract class GtfsColumnDescriptor {
 
   public abstract boolean headerRequired();
 
-  public abstract boolean headerRecommended();
-
   public abstract FieldLevelEnum fieldLevel();
 
   public abstract Optional<RowParser.NumberBounds> numberBounds();
@@ -35,8 +33,6 @@ public abstract static class Builder {
 
     public abstract Builder setHeaderRequired(boolean value);
 
-    public abstract Builder setHeaderRecommended(boolean value);
-
     public abstract Builder setFieldLevel(FieldLevelEnum value);
 
     public abstract Builder setNumberBounds(Optional<RowParser.NumberBounds> value);

diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultTableHeaderValidator.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultTableHeaderValidator.java
@@ -23,7 +23,6 @@
 import java.util.TreeSet;
 import org.mobilitydata.gtfsvalidator.notice.DuplicatedColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.EmptyColumnNameNotice;
-import org.mobilitydata.gtfsvalidator.notice.MissingRecommendedColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.MissingRequiredColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.notice.UnknownColumnNotice;
@@ -37,7 +36,6 @@ public void validate(
       CsvHeader actualHeader,
       Set<String> supportedColumns,
       Set<String> requiredColumns,
-      Set<String> recommendedColumns,
       NoticeContainer noticeContainer) {
     if (actualHeader.getColumnCount() == 0) {
       // This is an empty file.
@@ -48,9 +46,6 @@ public void validate(
     // We remove the columns that are properly present and well formed from that set, and at the
     // end only the missing required columns are left in the set.
     TreeSet<String> missingColumns = new TreeSet<>(requiredColumns);
-    // We also want to find the recommended columns that are absent. We use the same scheme for
-    // these.
-    TreeSet<String> missingRecommendedColumns = new TreeSet<>(recommendedColumns);
     for (int i = 0; i < actualHeader.getColumnCount(); ++i) {
       String column = actualHeader.getColumnName(i);
       // Column indices are zero-based. We add 1 to make them 1-based.
@@ -69,20 +64,11 @@ public void validate(
 
       // If the column is present, it should not be in the missing required columns set
       missingColumns.remove(column);
-
-      // If the column is present, it should not be in the missing recommended columns set
-      missingRecommendedColumns.remove(column);
     }
     if (!missingColumns.isEmpty()) {
       for (String column : missingColumns) {
         noticeContainer.addValidationNotice(new MissingRequiredColumnNotice(filename, column));
       }
     }
-
-    if (!missingRecommendedColumns.isEmpty()) {
-      for (String column : missingRecommendedColumns) {
-        noticeContainer.addValidationNotice(new MissingRecommendedColumnNotice(filename, column));
-      }
-    }
   }
 }
diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/TableHeaderValidator.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/TableHeaderValidator.java
@@ -28,6 +28,5 @@ void validate(
       CsvHeader actualHeader,
       Set<String> supportedHeaders,
       Set<String> requiredHeaders,
-      Set<String> recommendedHeaders,
       NoticeContainer noticeContainer);
 }
diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java
@@ -56,11 +56,6 @@ public boolean headerRequired() {
           return false;
         }
 
-        @Override
-        public boolean headerRecommended() {
-          return false;
-        }
-
         @Override
         public FieldLevelEnum fieldLevel() {
           return FieldLevelEnum.REQUIRED;
@@ -146,7 +141,6 @@ public void asString_recommended_missing() {
         GtfsColumnDescriptor.builder()
             .setColumnName("column name")
             .setHeaderRequired(false)
-            .setHeaderRecommended(false)
             .setFieldLevel(FieldLevelEnum.RECOMMENDED)
             .setIsMixedCase(false)
             .setIsCached(false)

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/table/CsvTableLoaderTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/table/CsvTableLoaderTest.java
@@ -88,7 +88,6 @@ public void validate(
               CsvHeader actualHeader,
               Set<String> supportedHeaders,
               Set<String> requiredHeaders,
-              Set<String> recommendedHeaders,
               NoticeContainer noticeContainer) {
             noticeContainer.addValidationNotice(headerValidationNotice);
           }
@@ -148,15 +147,13 @@ public void missingRequiredField() {
                 GtfsColumnDescriptor.builder()
                     .setColumnName(GtfsTestEntity.ID_FIELD_NAME)
                     .setHeaderRequired(true)
-                    .setHeaderRecommended(false)
                     .setFieldLevel(FieldLevelEnum.REQUIRED)
                     .setIsMixedCase(false)
                     .setIsCached(false)
                     .build(),
                 GtfsColumnDescriptor.builder()
                     .setColumnName(GtfsTestEntity.CODE_FIELD_NAME)
                     .setHeaderRequired(false)
-                    .setHeaderRecommended(false)
                     .setFieldLevel(FieldLevelEnum.REQUIRED)
                     .setIsMixedCase(false)
                     .setIsCached(false)

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestTableDescriptor.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestTableDescriptor.java
@@ -40,7 +40,6 @@ public ImmutableList<GtfsColumnDescriptor> getColumns() {
         GtfsColumnDescriptor.builder()
             .setColumnName(GtfsTestEntity.ID_FIELD_NAME)
             .setHeaderRequired(true)
-            .setHeaderRecommended(false)
             .setFieldLevel(FieldLevelEnum.REQUIRED)
             .setIsMixedCase(false)
             .setIsCached(false)
@@ -49,7 +48,6 @@ public ImmutableList<GtfsColumnDescriptor> getColumns() {
         GtfsColumnDescriptor.builder()
             .setColumnName(GtfsTestEntity.CODE_FIELD_NAME)
             .setHeaderRequired(false)
-            .setHeaderRecommended(false)
             .setFieldLevel(FieldLevelEnum.OPTIONAL)
             .setIsMixedCase(false)
             .setIsCached(false)

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestTableDescriptor2.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/testgtfs/GtfsTestTableDescriptor2.java
@@ -41,7 +41,6 @@ public ImmutableList<GtfsColumnDescriptor> getColumns() {
         GtfsColumnDescriptor.builder()
             .setColumnName(GtfsTestEntity.ID_FIELD_NAME)
             .setHeaderRequired(true)
-            .setHeaderRecommended(false)
             .setFieldLevel(FieldLevelEnum.REQUIRED)
             .setIsMixedCase(false)
             .setIsCached(false)
@@ -50,7 +49,6 @@ public ImmutableList<GtfsColumnDescriptor> getColumns() {
         GtfsColumnDescriptor.builder()
             .setColumnName(GtfsTestEntity.CODE_FIELD_NAME)
             .setHeaderRequired(false)
-            .setHeaderRecommended(false)
             .setFieldLevel(FieldLevelEnum.OPTIONAL)
             .setIsMixedCase(false)
             .setIsCached(false)

diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/validator/TableHeaderValidatorTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/validator/TableHeaderValidatorTest.java
@@ -19,13 +19,11 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableSet;
-import java.util.Set;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mobilitydata.gtfsvalidator.notice.DuplicatedColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.EmptyColumnNameNotice;
-import org.mobilitydata.gtfsvalidator.notice.MissingRecommendedColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.MissingRequiredColumnNotice;
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.notice.UnknownColumnNotice;
@@ -42,7 +40,6 @@ public void expectedColumns() {
             new CsvHeader(new String[] {"stop_id", "stop_name"}),
             ImmutableSet.of("stop_id", "stop_name", "stop_lat", "stop_lon"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices()).isEmpty();
@@ -58,7 +55,6 @@ public void unknownColumnShouldGenerateNotice() {
             new CsvHeader(new String[] {"stop_id", "stop_name", "stop_extra"}),
             ImmutableSet.of("stop_id", "stop_name"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices())
@@ -75,29 +71,12 @@ public void missingRequiredColumnShouldGenerateNotice() {
             new CsvHeader(new String[] {"stop_name"}),
             ImmutableSet.of("stop_id", "stop_name"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices())
         .containsExactly(new MissingRequiredColumnNotice("stops.txt", "stop_id"));
   }
 
-  @Test
-  public void missingRecommendedColumnShouldGenerateNotice() {
-    NoticeContainer container = new NoticeContainer();
-    new DefaultTableHeaderValidator()
-        .validate(
-            "stops.txt",
-            new CsvHeader(new String[] {"stop_name"}),
-            Set.of("stop_id", "stop_name"),
-            Set.of(),
-            Set.of("stop_id"),
-            container);
-
-    assertThat(container.getValidationNotices())
-        .containsExactly(new MissingRecommendedColumnNotice("stops.txt", "stop_id"));
-  }
-
   @Test
   public void duplicatedColumnShouldGenerateNotice() {
     NoticeContainer container = new NoticeContainer();
@@ -107,7 +86,6 @@ public void duplicatedColumnShouldGenerateNotice() {
             new CsvHeader(new String[] {"stop_id", "stop_name", "stop_id"}),
             ImmutableSet.of("stop_id", "stop_name"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices())
@@ -124,7 +102,6 @@ public void emptyFileShouldNotGenerateNotice() {
             CsvHeader.EMPTY,
             ImmutableSet.of("stop_id", "stop_name"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices()).isEmpty();
@@ -140,7 +117,6 @@ public void emptyColumnNameShouldGenerateNotice() {
             new CsvHeader(new String[] {"stop_id", null, "stop_name", ""}),
             ImmutableSet.of("stop_id", "stop_name"),
             ImmutableSet.of("stop_id"),
-            Set.of(),
             container);
 
     assertThat(container.getValidationNotices())

diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java
@@ -116,8 +116,8 @@ static class StopTimeTimepointWithoutTimesNotice extends ValidationNotice {
   /**
    * `stop_times.timepoint` value is missing for a record.
    *
-   * <p>Even though the column `timepoint` is optional in `stop_times.txt` according to the
-   * specification, `stop_times.timepoint` should not be empty when provided.
+   * <p>When at least one of `stop_times.arrival_time` or `stop_times.departure_time` are provided,
+   * `stop_times.timepoint` should be defined
    */
   @GtfsValidationNotice(severity = WARNING, files = @FileRefs(GtfsStopTimeSchema.class))
   static class MissingTimepointValueNotice extends ValidationNotice {

diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java
@@ -60,25 +60,6 @@ private static List<ValidationNotice> generateNotices(
     return noticeContainer.getValidationNotices();
   }
 
-  // Using this header will trigger a MissingRecommendedColumnNotice since the timepoint column is
-  // missing.
-  private static CsvHeader createLegacyHeader() {
-    return new CsvHeader(
-        new String[] {
-          TRIP_ID_FIELD_NAME,
-          ARRIVAL_TIME_FIELD_NAME,
-          DEPARTURE_TIME_FIELD_NAME,
-          STOP_ID_FIELD_NAME,
-          STOP_SEQUENCE_FIELD_NAME,
-          STOP_HEADSIGN_FIELD_NAME,
-          PICKUP_TYPE_FIELD_NAME,
-          DROP_OFF_TYPE_FIELD_NAME,
-          CONTINUOUS_PICKUP_FIELD_NAME,
-          CONTINUOUS_DROP_OFF_FIELD_NAME,
-          SHAPE_DIST_TRAVELED_FIELD_NAME
-        });
-  }
-
   private static CsvHeader createHeaderWithTimepointColumn() {
     return new CsvHeader(
         new String[] {

diff --git a/model/src/main/java/org/mobilitydata/gtfsvalidator/annotation/RecommendedColumn.java b/model/src/main/java/org/mobilitydata/gtfsvalidator/annotation/RecommendedColumn.java
diff --git a/processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/Analyser.java b/processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/Analyser.java
@@ -71,7 +71,6 @@ public GtfsFileDescriptor analyzeGtfsFileType(TypeElement type) {
       fieldBuilder.setRecommended(method.getAnnotation(Recommended.class) != null);
       fieldBuilder.setColumnRequired(method.getAnnotation(RequiredColumn.class) != null);
       fieldBuilder.setValueRequired(method.getAnnotation(Required.class) != null);
-      fieldBuilder.setColumnRecommended(method.getAnnotation(RecommendedColumn.class) != null);
       fieldBuilder.setMixedCase(method.getAnnotation(MixedCase.class) != null);
       PrimaryKey primaryKey = method.getAnnotation(PrimaryKey.class);
       if (primaryKey != null) {

diff --git a/processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/GtfsFieldDescriptor.java b/processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/GtfsFieldDescriptor.java
@@ -59,8 +59,6 @@ public static GtfsFieldDescriptor.Builder builder() {
 
   public abstract boolean columnRequired();
 
-  public abstract boolean columnRecommended();
-
   public boolean isHeaderRequired() {
     return columnRequired() || valueRequired();
   }
@@ -83,8 +81,6 @@ public abstract static class Builder {
 
     public abstract Builder setColumnRequired(boolean value);
 
-    public abstract Builder setColumnRecommended(boolean value);
-
     public abstract Builder setMixedCase(boolean value);
 
     public abstract Builder setPrimaryKey(PrimaryKey annotation);