fix: False positives for StopTimeTimepointWithoutTimesNotice (#1044)

RULES.md

@@ -134,6 +134,8 @@ Additional details regarding the notices' context is provided in [`NOTICES.md`](
 | [`InconsistentAgencyLangNotice`](#InconsistentAgencyLangNotice)                   	| Inconsistent language among agencies.                                                                                                                       	|
 | [`LeadingOrTrailingWhitespacesNotice`](#LeadingOrTrailingWhitespacesNotice)         | The value in CSV file has leading or trailing whitespaces.                                                                                                  	|
 | [`MissingFeedInfoDateNotice`](#MissingFeedInfoDateNotice)                         	| `feed_end_date` should be provided if `feed_start_date` is provided. `feed_start_date` should be provided if `feed_end_date` is provided.                   	|
+| [`MissingTimepointColumnNotice`](#MissingTimepointColumnNotice)                         	        | `timepoint` column is missing for a dataset.                                                                                                        	        |
+| [`MissingTimepointValueNotice`](#MissingTimepointValueNotice)                         	        | `stop_times.timepoint` value is missing for a record.                                                                                                        	|
 | [`MoreThanOneEntityNotice`](#MoreThanOneEntityNotice)                             	| More than one row in CSV.                                                                                                                                   	|
 | [`NonAsciiOrNonPrintableCharNotice`](#NonAsciiOrNonPrintableCharNotice)           	| Non ascii or non printable char in  `id`.                                                                                                                   	|
 | [`PathwayDanglingGenericNodeNotice`](#PathwayDanglingGenericNodeNotice)           	| A generic node has only one incident location in a pathway graph.                                                                                             |
@@ -770,6 +772,24 @@ Even though `feed_info.start_date` and `feed_info.end_date` are optional, if one
 
 ##### References:
 * [feed_info.txt Best practices](http://gtfs.org/best-practices/#feed_infotxt)
+
+<a name="MissingTimepointValueNotice"/>
+
+#### MissingTimepointValueNotice
+
+Even though the column `timepoint` is optional in `stop_times.txt` according to the specification, `stop_times.timepoint` should not be empty when provided. 
+
+##### References:
+* [stop_times.txt specification](https://github.com/google/transit/blob/master/gtfs/spec/en/reference.md#stop_timestxt)
+
+<a name="MissingTimepointColumnNotice"/>
+
+#### MissingTimepointColumnNotice
+
+The `timepoint` column should be provided.
+
+##### References:
+* [stop_times.txt bets practices](https://github.com/MobilityData/GTFS_Schedule_Best-Practices/blob/master/en/stop_times.md)
 
 <a name="MoreThanOneEntityNotice"/>
 

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

@@ -59,6 +59,10 @@ public int entityCount() {
 
   public abstract String gtfsFilename();
 
+  public boolean hasColumn(String columnName) {
+    return header.hasColumn(columnName);
+  }
+
   /**
    * Returns names of the columns that are keys in that table.
    *

docs/NOTICES.md

@@ -724,6 +724,8 @@
 | `inconsistent_agency_lang`                 	| [`InconsistentAgencyLangNotice`](#InconsistentAgencyLangNotice)                   	|
 | `leading_or_trailing_whitespaces`           | [`LeadingOrTrailingWhitespacesNotice`](#LeadingOrTrailingWhitespacesNotice)         |
 | `missing_feed_info_date`                   	| [`MissingFeedInfoDateNotice`](#MissingFeedInfoDateNotice)                         	|
+| `missing_timepoint_column`                   	| [`MissingTimepointColumnNotice`](#MissingTimepointColumnNotice)                       |
+| `missing_timepoint_value`                   	| [`MissingTimepointValueNotice`](#MissingTimepointValueNotice)                         |
 | `more_than_one_entity`                     	| [`MoreThanOneEntityNotice`](#MoreThanOneEntityNotice)                             	|
 | `non_ascii_or_non_printable_char`          	| [`NonAsciiOrNonPrintableCharNotice`](#NonAsciiOrNonPrintableCharNotice)           	|
 | `pathway_dangling_generic_node`               | [`PathwayDanglingGenericNodeNotice`](#PathwayDanglingGenericNodeNotice)	            |
@@ -854,6 +856,28 @@
 ##### Affected files
 * [`feed_info.txt`](http://gtfs.org/reference/static#feed_infotxt)
 
+#### [MissingTimepointColumnNotice](/RULES.md#MissingTimepointColumnNotice)
+##### Fields description
+
+| Field name     	| Description                                     	| Type   	|
+|----------------	|-------------------------------------------------	|--------	|
+| `filename`    	| The name of the affected file.                  	| String   	|
+
+##### Affected files
+* [`stop_times.txt`](https://github.com/google/transit/blob/master/gtfs/spec/en/reference.md#stop_timestxt)
+
+#### [MissingTimepointValueNotice](/RULES.md#MissingTimepointValueNotice)
+##### Fields description
+
+| Field name     	| Description                                     	| Type   	|
+|----------------	|-------------------------------------------------	|--------	|
+| `csvRowNumber` 	| The row number of the faulty record.            	| Long   	|
+| `tripId`       	| The faulty record's `stop_times.trip_id`.         | String 	|
+| `stopSequence` 	| The faulty record's `stop_times.stop_sequence`. 	| String 	|
+
+##### Affected files
+* [`stop_times.txt`](https://github.com/google/transit/blob/master/gtfs/spec/en/reference.md#stop_timestxt)
+
 #### [MoreThanOneEntityNotice](/RULES.md#MoreThanOneEntityNotice)
 ##### Fields description
 

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

@@ -16,11 +16,13 @@
 
 package org.mobilitydata.gtfsvalidator.validator;
 
+import javax.inject.Inject;
 import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator;
 import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
 import org.mobilitydata.gtfsvalidator.notice.SeverityLevel;
 import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
 import org.mobilitydata.gtfsvalidator.table.GtfsStopTime;
+import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableContainer;
 import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableLoader;
 import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTimepoint;
 
@@ -32,56 +34,102 @@
  * <ul>
  *   <li>{@link StopTimeTimepointWithoutTimesNotice} - a timepoint does not specifies arrival_time
  *       or departure_time
+ *   <li>{@link MissingTimepointValueNotice} - value for {@code stop_times.timepoint} is missing
+ *   <li>{@link MissingTimepointColumnNotice} - field {@code stop_times.timepoint} is missing
  * </ul>
  */
 @GtfsValidator
-public class TimepointTimeValidator extends SingleEntityValidator<GtfsStopTime> {
+public class TimepointTimeValidator extends FileValidator {
+  private final GtfsStopTimeTableContainer stopTimes;
+
+  @Inject
+  TimepointTimeValidator(GtfsStopTimeTableContainer stopTimes) {
+    this.stopTimes = stopTimes;
+  }
 
   @Override
-  public void validate(GtfsStopTime stopTime, NoticeContainer noticeContainer) {
-    if (!isTimepoint(stopTime)) {
+  public void validate(NoticeContainer noticeContainer) {
+    if (!stopTimes.hasColumn(GtfsStopTimeTableLoader.TIMEPOINT_FIELD_NAME)) {
+      // legacy datasets do not use timepoint column in stop_times.txt as a result:
+      // - this should be flagged;
+      // - but also no notice regarding the absence of arrival_time or departure_time should be
+      // generated
+      noticeContainer.addValidationNotice(new MissingTimepointColumnNotice());
       return;
     }
-    if (!stopTime.hasArrivalTime()) {
-      noticeContainer.addValidationNotice(
-          new StopTimeTimepointWithoutTimesNotice(
-              stopTime.csvRowNumber(),
-              stopTime.tripId(),
-              stopTime.stopSequence(),
-              String.format(GtfsStopTimeTableLoader.ARRIVAL_TIME_FIELD_NAME)));
-    }
-    if (!stopTime.hasDepartureTime()) {
-      noticeContainer.addValidationNotice(
-          new StopTimeTimepointWithoutTimesNotice(
-              stopTime.csvRowNumber(),
-              stopTime.tripId(),
-              stopTime.stopSequence(),
-              GtfsStopTimeTableLoader.DEPARTURE_TIME_FIELD_NAME));
+    for (GtfsStopTime stopTime : stopTimes.getEntities()) {
+      if (!stopTime.hasTimepoint()) {
+        noticeContainer.addValidationNotice(new MissingTimepointValueNotice(stopTime));
+      }
+      if (isTimepoint(stopTime)) {
+        if (!stopTime.hasArrivalTime()) {
+          noticeContainer.addValidationNotice(
+              new StopTimeTimepointWithoutTimesNotice(
+                  stopTime, GtfsStopTimeTableLoader.ARRIVAL_TIME_FIELD_NAME));
+        }
+        if (!stopTime.hasDepartureTime()) {
+          noticeContainer.addValidationNotice(
+              new StopTimeTimepointWithoutTimesNotice(
+                  stopTime, GtfsStopTimeTableLoader.DEPARTURE_TIME_FIELD_NAME));
+        }
+      }
     }
   }
 
   private boolean isTimepoint(GtfsStopTime stopTime) {
-    return stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);
+    return stopTime.hasTimepoint() && stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);
   }
 
   /**
    * Timepoint without time
    *
-   * <p>Severity: {@code SeverityLevel.WARNING}
+   * <p>Severity: {@code SeverityLevel.WARNING} - to be upgraded as {@code SeverityLevel.ERROR} see
+   * https://github.com/MobilityData/gtfs-validator/issues/1017
    */
   static class StopTimeTimepointWithoutTimesNotice extends ValidationNotice {
     private final long csvRowNumber;
     private final String tripId;
     private final long stopSequence;
     private final String specifiedField;
 
-    StopTimeTimepointWithoutTimesNotice(
-        long csvRowNumber, String tripId, long stopSequence, String specifiedField) {
+    StopTimeTimepointWithoutTimesNotice(GtfsStopTime stopTime, String specifiedField) {
       super(SeverityLevel.WARNING);
-      this.csvRowNumber = csvRowNumber;
-      this.tripId = tripId;
-      this.stopSequence = stopSequence;
+      this.csvRowNumber = stopTime.csvRowNumber();
+      this.tripId = stopTime.tripId();
+      this.stopSequence = stopTime.stopSequence();
       this.specifiedField = specifiedField;
     }
   }
+
+  /**
+   * {@code stop_times.timepoint} value is missing
+   *
+   * <p>Severity: {@code SeverityLevel.WARNING}
+   */
+  static class MissingTimepointValueNotice extends ValidationNotice {
+    private final long csvRowNumber;
+    private final String tripId;
+    private final long stopSequence;
+
+    MissingTimepointValueNotice(GtfsStopTime stopTime) {
+      super(SeverityLevel.WARNING);
+      this.csvRowNumber = stopTime.csvRowNumber();
+      this.tripId = stopTime.tripId();
+      this.stopSequence = stopTime.stopSequence();
+    }
+  }
+
+  /**
+   * Column {@code stop_times.timepoint} is missing.
+   *
+   * <p>Severity: {@code SeverityLevel.WARNING}
+   */
+  static class MissingTimepointColumnNotice extends ValidationNotice {
+    private final String filename;
+
+    MissingTimepointColumnNotice() {
+      super(SeverityLevel.WARNING);
+      this.filename = GtfsStopTimeTableLoader.FILENAME;
+    }
+  }
 }