fix: handle GeoJSON locations when computing distance between stops

main/src/main/java/org/mobilitydata/gtfsvalidator/util/StopUtil.java

@@ -32,6 +32,8 @@ public class StopUtil {
    *
    * <p>Returns (0, 0) if no coordinates are found in parent chain.
    */
+  // TODO: Replace with `getOptionalStopOrParentLatLng` everywhere. Falling back to
+  //   `S2LatLng.CENTER` can (and does) cause bugs.
   public static S2LatLng getStopOrParentLatLng(GtfsStopTableContainer stopTable, String stopId) {
     // Do not do an infinite loop since there may be a data bug and an infinite cycle of parents.
     for (int i = 0; i < 3; ++i) {
@@ -52,6 +54,12 @@ public static S2LatLng getStopOrParentLatLng(GtfsStopTableContainer stopTable, S
     return S2LatLng.CENTER;
   }
 
+  public static Optional<S2LatLng> getOptionalStopOrParentLatLng(
+      GtfsStopTableContainer stopTable, String stopId) {
+    return Optional.of(getStopOrParentLatLng(stopTable, stopId))
+        .filter(s2LatLng -> s2LatLng != S2LatLng.CENTER);
+  }
+
   /**
    * Finds station that the given location belongs to.
    *

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

@@ -26,8 +26,7 @@
 import com.google.common.hash.HashFunction;
 import com.google.common.hash.Hasher;
 import com.google.common.hash.Hashing;
-import java.util.List;
-import java.util.Optional;
+import java.util.*;
 import javax.inject.Inject;
 import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice;
 import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice.FileRefs;
@@ -50,6 +49,7 @@
 import org.mobilitydata.gtfsvalidator.table.GtfsTripTableContainer;
 import org.mobilitydata.gtfsvalidator.type.GtfsTime;
 import org.mobilitydata.gtfsvalidator.util.S2Earth;
+import org.mobilitydata.gtfsvalidator.util.StopUtil;
 
 /**
  * Validates that transit vehicles do not travel too fast between consecutive and between far stops.
@@ -100,8 +100,9 @@ public void validate(NoticeContainer noticeContainer) {
         continue;
       }
       final double maxSpeedKph = getMaxVehicleSpeedKph(route.get().routeType());
-      final double[] distancesKm = findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes());
-      validateConsecutiveStops(trips, distancesKm, maxSpeedKph, noticeContainer);
+      final double[] distancesKm =
+          findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes(), stopTable);
+      validateConsecutiveStops(trips, maxSpeedKph, noticeContainer);
       validateFarStops(trips, distancesKm, maxSpeedKph, noticeContainer);
     }
   }
@@ -154,17 +155,39 @@ public long tripFprint() {
    * <p>{@code distancesKm[i]} equals to distance between stops corresponding to stop times i, i +
    * 1.
    */
-  private double[] findDistancesKmBetweenStops(List<GtfsStopTime> stopTimes) {
+  @VisibleForTesting
+  static double[] findDistancesKmBetweenStops(
+      List<GtfsStopTime> stopTimes, GtfsStopTableContainer stopTable) {
     double[] distancesKm = new double[stopTimes.size() - 1];
     S2LatLng currLatLng = getStopOrParentLatLng(stopTable, stopTimes.get(0).stopId());
     for (int i = 0; i < distancesKm.length; ++i) {
-      S2LatLng nextLatLng = getStopOrParentLatLng(stopTable, stopTimes.get(i + 1).stopId());
-      distancesKm[i] = S2Earth.getDistanceKm(currLatLng, nextLatLng);
-      currLatLng = nextLatLng;
+      Optional<S2LatLng> maybeNextLatLng =
+          StopUtil.getOptionalStopOrParentLatLng(stopTable, stopTimes.get(i + 1).stopId());
+      if (maybeNextLatLng.isPresent()) {
+        S2LatLng nextLatLng = maybeNextLatLng.get();
+        distancesKm[i] = S2Earth.getDistanceKm(currLatLng, nextLatLng);
+        currLatLng = nextLatLng;
+      } else {
+        distancesKm[i] = 0;
+      }
     }
     return distancesKm;
   }
 
+  private Optional<Double> getDistanceKm(GtfsStopTime start, GtfsStopTime end) {
+    Optional<S2LatLng> maybeFirstLatLng =
+        StopUtil.getOptionalStopOrParentLatLng(stopTable, start.stopId());
+    Optional<S2LatLng> maybeSecondLatLng =
+        StopUtil.getOptionalStopOrParentLatLng(stopTable, end.stopId());
+
+    if (maybeFirstLatLng.isEmpty() || maybeSecondLatLng.isEmpty()) {
+      return Optional.empty();
+    }
+
+    double distanceKm = S2Earth.getDistanceKm(maybeFirstLatLng.get(), maybeSecondLatLng.get());
+    return Optional.of(distanceKm);
+  }
+
   /**
    * Validates travel speed between far stops for all trips that belong to the same route and visit
    * the same stops at the same times.
@@ -234,40 +257,44 @@ private void validateFarStops(
    * <p>If there is a fast travel detected, then a separate notice is issued for each trip.
    */
   private void validateConsecutiveStops(
-      List<TripAndStopTimes> trips,
-      double[] distancesKm,
-      double maxSpeedKph,
-      NoticeContainer noticeContainer) {
+      List<TripAndStopTimes> trips, double maxSpeedKph, NoticeContainer noticeContainer) {
     final List<GtfsStopTime> stopTimes = trips.get(0).getStopTimes();
-    for (int i = 0; i < distancesKm.length; ++i) {
-      final GtfsStopTime stopTime1 = stopTimes.get(i);
-      final GtfsStopTime stopTime2 = stopTimes.get(i + 1);
-      if (!(stopTime1.hasDepartureTime() && stopTime2.hasArrivalTime())) {
+    GtfsStopTime start = stopTimes.get(0);
+    for (int i = 0; i < stopTimes.size() - 1; ++i) {
+      GtfsStopTime end = stopTimes.get(i + 1);
+
+      Optional<Double> maybeDistanceKm = getDistanceKm(start, end);
+      // We couldn't calculate the distance, for instance because one of the stops is
+      // actually a GeoJSON location and doesn't have a specific latitude and longitude.
+      // We try comparing with the next stop instead.
+      if (maybeDistanceKm.isEmpty()) {
         continue;
       }
-      final double distanceKm = distancesKm[i];
-      final double speedKph = getSpeedKphBetweenStops(distanceKm, stopTime1, stopTime2);
-      if (speedKph <= maxSpeedKph) {
+      double distanceKm = maybeDistanceKm.get();
+
+      // We can't calculate the speed if we're missing a departure time or
+      // arrival time.
+      if (!start.hasDepartureTime() || !end.hasArrivalTime()) {
         continue;
       }
-      final Optional<GtfsStop> stop1 = stopTable.byStopId(stopTime1.stopId());
-      final Optional<GtfsStop> stop2 = stopTable.byStopId(stopTime2.stopId());
-      if (stop1.isEmpty() || stop2.isEmpty()) {
-        // Broken reference is reported in another rule.
-        return;
-      }
-      // Issue one notice per each trip.
-      for (TripAndStopTimes trip : trips) {
-        noticeContainer.addValidationNotice(
-            new FastTravelBetweenConsecutiveStopsNotice(
-                trip.getTrip(),
-                trip.getStopTimes().get(i),
-                stop1.get(),
-                trip.getStopTimes().get(i + 1),
-                stop2.get(),
-                speedKph,
-                distanceKm));
+      double speedKph = getSpeedKphBetweenStops(distanceKm, start, end);
+
+      if (speedKph > maxSpeedKph) {
+        final Optional<GtfsStop> stop1 = stopTable.byStopId(start.stopId());
+        final Optional<GtfsStop> stop2 = stopTable.byStopId(end.stopId());
+        // This should always evaluate to true since we check whether both stops exist
+        // in `getDistanceAndSpeed`; this is just here as a precaution.
+        if (stop1.isPresent() && stop2.isPresent()) {
+          // Issue one notice for each trip.
+          for (TripAndStopTimes trip : trips) {
+            noticeContainer.addValidationNotice(
+                new FastTravelBetweenConsecutiveStopsNotice(
+                    trip.getTrip(), start, stop1.get(), end, stop2.get(), speedKph, distanceKm));
+          }
+        }
       }
+
+      start = end;
     }
   }
 

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java

@@ -17,13 +17,16 @@
 package org.mobilitydata.gtfsvalidator.validator;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.mobilitydata.gtfsvalidator.validator.StopTimeTravelSpeedValidator.findDistancesKmBetweenStops;
 import static org.mobilitydata.gtfsvalidator.validator.StopTimeTravelSpeedValidator.getSpeedKphBetweenStops;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.geometry.S2LatLng;
+import com.ibm.icu.impl.Pair;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -298,6 +301,8 @@ public void farStopsMissingTimeInBetween_yieldsNotice() {
                 GtfsTime.fromString("08:02:30")));
     assertThat(generateNotices(ImmutableList.of(route), ImmutableList.of(trip), stopTimes, stops))
         .containsExactly(
+            createFastTravelBetweenConsecutiveStopsNotice(
+                trip, stopTimes.get(0), stops.get(0), stopTimes.get(2), stops.get(2)),
             createFastTravelBetweenFarStopsNotice(
                 trip,
                 stopTimes.get(0),
@@ -326,4 +331,43 @@ public void getSpeedKphBetweenStops_teleport() {
         createStopTimesSameDepartureArrival(ImmutableList.of(GtfsTime.fromString("08:00:00")));
     assertThat(getSpeedKphBetweenStops(2, stopTimes.get(0), stopTimes.get(0))).isEqualTo(120);
   }
+
+  @Test
+  public void findDistancesKmBetweenStops_geoJsonLocation() {
+    List<S2LatLng> points =
+        ImmutableList.of(
+            S2LatLng.fromDegrees(42.879, -73.19313), // s0
+            S2LatLng.fromDegrees(42.89522, -73.20155)); // 21
+    List<GtfsStop> stops = createStops(points);
+
+    List<Optional<Pair<String, GtfsTime>>> stopIdsAndTimes =
+        ImmutableList.of(
+            Optional.of(Pair.of("s0", GtfsTime.fromString("14:48:00"))),
+            // There's a GeoJSON location between the two stops.
+            Optional.empty(),
+            Optional.of(Pair.of("s1", GtfsTime.fromString("14:56:00"))));
+
+    List<GtfsStopTime> stopTimes = new ArrayList<>(stopIdsAndTimes.size());
+    for (int i = 0; i < stopIdsAndTimes.size(); ++i) {
+      GtfsStopTime.Builder stopTimeBuilder =
+          new GtfsStopTime.Builder().setTripId(TEST_TRIP_ID).setStopSequence(i);
+
+      stopIdsAndTimes
+          .get(i)
+          .ifPresent(
+              t ->
+                  stopTimeBuilder
+                      .setArrivalTime(t.second)
+                      .setDepartureTime(t.second)
+                      .setStopId(t.first));
+      stopTimes.add(stopTimeBuilder.build());
+    }
+
+    double[] distancesKm =
+        findDistancesKmBetweenStops(stopTimes, GtfsStopTableContainer.forEntities(stops, null));
+
+    assertThat(distancesKm).hasLength(2);
+    assertThat(distancesKm[0]).isEqualTo(0.0);
+    assertThat(distancesKm[1]).isEqualTo(S2Earth.getDistanceKm(points.get(0), points.get(1)));
+  }
 }