Fix: Regression error in passThroughPoints in the Transmodel API

The list of ids inside passThroughPoints is allowed to be empty or null. We cannot
change this - that would be a breaking change. So, when the via search enforced this,
the API was not backward compatible anymore. This commit reverts the behavior and
just ignores the passThroughPoints if the list of ids is null or empty. This bug was
 introduced in PR #6084.

Author: t2gran

application/src/main/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapper.java

@@ -6,6 +6,8 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
+import javax.annotation.Nullable;
 import org.opentripplanner.apis.transmodel.model.plan.TripQuery;
 import org.opentripplanner.apis.transmodel.model.plan.ViaLocationInputType;
 import org.opentripplanner.apis.transmodel.support.OneOfInputValidator;
@@ -30,6 +32,7 @@ static List<ViaLocation> toLegacyPassThroughLocations(
     return passThroughPoints
       .stream()
       .map(TripViaLocationMapper::mapLegacyPassThroughViaLocation)
+      .filter(Objects::nonNull)
       .collect(toList());
   }
 
@@ -65,19 +68,31 @@ private static PassThroughViaLocation mapPassThroughViaLocation(Map<String, Obje
 
   private static List<FeedScopedId> mapStopLocationIds(Map<String, Object> map) {
     var c = (Collection<String>) map.get(ViaLocationInputType.FIELD_STOP_LOCATION_IDS);
+
+    // When coordinates are added, we need to accept null here...
+    if (c == null) {
+      throw new IllegalArgumentException(
+        "'" + ViaLocationInputType.FIELD_STOP_LOCATION_IDS + "' is not set!"
+      );
+    }
     return c.stream().map(TransitIdMapper::mapIDToDomain).toList();
   }
 
   /**
    * @deprecated Legacy passThrough, use via instead
    */
   @Deprecated
+  @Nullable
   private static ViaLocation mapLegacyPassThroughViaLocation(Map<String, Object> inputMap) {
     final String name = (String) inputMap.get("name");
-    final List<FeedScopedId> stopLocationIds =
-      ((List<String>) inputMap.get("placeIds")).stream()
-        .map(TransitIdMapper::mapIDToDomain)
-        .toList();
+    List<String> placeIds = (List<String>) inputMap.get("placeIds");
+    if (placeIds == null || placeIds.isEmpty()) {
+      return null;
+    }
+    final List<FeedScopedId> stopLocationIds = placeIds
+      .stream()
+      .map(TransitIdMapper::mapIDToDomain)
+      .toList();
     return new PassThroughViaLocation(name, stopLocationIds);
   }
 }

application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/ViaLocationInputType.java

@@ -5,6 +5,7 @@
 
 import graphql.language.StringValue;
 import graphql.schema.GraphQLInputObjectType;
+import graphql.schema.GraphQLInputType;
 import graphql.schema.GraphQLList;
 import graphql.schema.GraphQLNonNull;
 import java.time.Duration;
@@ -84,7 +85,7 @@ be accepted. To visit a coordinate, the traveler must walk(bike or drive) to the
       b
         .name(FIELD_STOP_LOCATION_IDS)
         .description(DOC_STOP_LOCATION_IDS)
-        .type(gqlListOfNonNullStrings())
+        .type(requiredListOfNonNullStrings())
     )
     /*
       TODO: Add support for coordinates
@@ -101,7 +102,7 @@ be accepted. To visit a coordinate, the traveler must walk(bike or drive) to the
       b
         .name(FIELD_STOP_LOCATION_IDS)
         .description(DOC_STOP_LOCATION_IDS)
-        .type(gqlListOfNonNullStrings())
+        .type(requiredListOfNonNullStrings())
     )
     .build();
 
@@ -119,7 +120,7 @@ be accepted. To visit a coordinate, the traveler must walk(bike or drive) to the
     )
     .build();
 
-  private static GraphQLList gqlListOfNonNullStrings() {
-    return new GraphQLList(new GraphQLNonNull(GraphQLString));
+  private static GraphQLInputType requiredListOfNonNullStrings() {
+    return new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(GraphQLString)));
   }
 }

application/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java

@@ -16,7 +16,8 @@ public PassThroughViaLocation(@Nullable String label, Collection<FeedScopedId> s
     super(label, stopLocationIds);
     if (stopLocationIds.isEmpty()) {
       throw new IllegalArgumentException(
-        "A pass through via location must have at least one stop location. Label: " + label
+        "A pass through via location must have at least one stop location." +
+        (label == null ? "" : " Label: " + label)
       );
     }
   }

application/src/main/java/org/opentripplanner/routing/api/request/via/VisitViaLocation.java

@@ -41,7 +41,8 @@ public VisitViaLocation(
 
     if (stopLocationIds().isEmpty() && coordinates().isEmpty()) {
       throw new IllegalArgumentException(
-        "A via location must have at least one stop location or a coordinate. Label: " + label
+        "A via location must have at least one stop location or a coordinate." +
+        (label == null ? "" : " Label: " + label)
       );
     }
   }

application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql

@@ -2204,7 +2204,7 @@ input TripPassThroughViaLocationInput {
   stop place or a group of stop places. It is enough to visit ONE of the locations
   listed.
   """
-  stopLocationIds: [String!]
+  stopLocationIds: [String!]!
 }
 
 """
@@ -2239,7 +2239,7 @@ input TripVisitViaLocationInput {
   stop place or a group of stop places. It is enough to visit ONE of the locations
   listed.
   """
-  stopLocationIds: [String!]
+  stopLocationIds: [String!]!
 }
 
 "Input format for specifying a location through either a place reference (id), coordinates or both. If both place and coordinates are provided the place ref will be used if found, coordinates will only be used if place is not known. The location also contain information about the minimum and maximum time the user is willing to stay at the via location."

application/src/test/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapperTest.java

@@ -21,10 +21,15 @@
 
 class TripViaLocationMapperTest {
 
-  public static final String LABEL = "TestLabel";
-  public static final Duration MIN_WAIT_TIME = Duration.ofMinutes(5);
-  public static final List<String> LIST_IDS_INPUT = List.of("F:ID1", "F:ID2");
-  public static final String EXPECTED_IDS_AS_STRING = "[F:ID1, F:ID2]";
+  private static final String LABEL = "TestLabel";
+  private static final Duration MIN_WAIT_TIME = Duration.ofMinutes(5);
+  private static final List<String> LIST_IDS_INPUT = List.of("F:ID1", "F:ID2");
+  private static final String EXPECTED_IDS_AS_STRING = "[F:ID1, F:ID2]";
+  private static final String REASON_EMPTY_IDS_ALLOWED_PASS_THROUGH =
+    """
+    Unfortunately the 'placeIds' is not required. Making it required would be a breaking change,
+    so wee just ignore it."
+    """;
 
   @BeforeEach
   void setup() {
@@ -52,10 +57,7 @@ void testMapToVisitViaLocations() {
 
   @Test
   void testMapToVisitViaLocationsWithBareMinimum() {
-    Map<String, Object> input = Map.of(
-      FIELD_VISIT,
-      Map.of(FIELD_STOP_LOCATION_IDS, List.of("F:1"))
-    );
+    Map<String, Object> input = mapOf(FIELD_VISIT, mapOf(FIELD_STOP_LOCATION_IDS, List.of("F:1")));
     var result = TripViaLocationMapper.mapToViaLocations(List.of(input));
 
     var via = result.getFirst();
@@ -66,9 +68,32 @@ void testMapToVisitViaLocationsWithBareMinimum() {
     assertFalse(via.isPassThroughLocation());
   }
 
+  @Test
+  void testMapToVisitViaLocationsWithoutIds() {
+    Map<String, Object> input = mapOf(FIELD_VISIT, mapOf(FIELD_STOP_LOCATION_IDS, null));
+    var ex = assertThrows(
+      IllegalArgumentException.class,
+      () -> TripViaLocationMapper.mapToViaLocations(List.of(input))
+    );
+    assertEquals("'stopLocationIds' is not set!", ex.getMessage());
+  }
+
+  @Test
+  void testMapToVisitViaLocationsWithAnEmptyListOfIds() {
+    Map<String, Object> input = mapOf(FIELD_VISIT, mapOf(FIELD_STOP_LOCATION_IDS, List.of()));
+    var ex = assertThrows(
+      IllegalArgumentException.class,
+      () -> TripViaLocationMapper.mapToViaLocations(List.of(input))
+    );
+    assertEquals(
+      "A via location must have at least one stop location or a coordinate.",
+      ex.getMessage()
+    );
+  }
+
   @Test
   void tetMapToPassThrough() {
-    Map<String, Object> input = Map.of(FIELD_PASS_THROUGH, passThroughInput(LABEL, LIST_IDS_INPUT));
+    Map<String, Object> input = mapOf(FIELD_PASS_THROUGH, passThroughInput(LABEL, LIST_IDS_INPUT));
     var result = TripViaLocationMapper.mapToViaLocations(List.of(input));
     var via = result.getFirst();
 
@@ -83,9 +108,9 @@ void tetMapToPassThrough() {
 
   @Test
   void tetMapToPassThroughWithBareMinimum() {
-    Map<String, Object> input = Map.of(
+    Map<String, Object> input = mapOf(
       FIELD_PASS_THROUGH,
-      Map.of(FIELD_STOP_LOCATION_IDS, List.of("F:1"))
+      mapOf(FIELD_STOP_LOCATION_IDS, List.of("F:1"))
     );
     var result = TripViaLocationMapper.mapToViaLocations(List.of(input));
     var via = result.getFirst();
@@ -95,6 +120,32 @@ void tetMapToPassThroughWithBareMinimum() {
     assertTrue(via.isPassThroughLocation());
   }
 
+  @Test
+  void tetMapToPassThroughWithoutIds() {
+    Map<String, Object> input = mapOf(FIELD_PASS_THROUGH, mapOf(FIELD_STOP_LOCATION_IDS, null));
+    var ex = assertThrows(
+      IllegalArgumentException.class,
+      () -> TripViaLocationMapper.mapToViaLocations(List.of(input))
+    );
+    assertEquals("'stopLocationIds' is not set!", ex.getMessage());
+  }
+
+  @Test
+  void testMapToPassThroughWithAnEmptyListOfIds() {
+    Map<String, Object> input = mapOf(
+      FIELD_PASS_THROUGH,
+      mapOf(FIELD_STOP_LOCATION_IDS, List.of())
+    );
+    var ex = assertThrows(
+      IllegalArgumentException.class,
+      () -> TripViaLocationMapper.mapToViaLocations(List.of(input))
+    );
+    assertEquals(
+      "A pass through via location must have at least one stop location.",
+      ex.getMessage()
+    );
+  }
+
   @Test
   void testOneOf() {
     Map<String, Object> input = Map.ofEntries(
@@ -121,6 +172,48 @@ void testOneOf() {
     );
   }
 
+  @Test
+  void testToLegacyPassThroughLocations() {
+    Map<String, Object> input = Map.of("name", LABEL, "placeIds", LIST_IDS_INPUT);
+    var result = TripViaLocationMapper.toLegacyPassThroughLocations(List.of(input));
+    var via = result.getFirst();
+
+    assertEquals(LABEL, via.label());
+    assertEquals(EXPECTED_IDS_AS_STRING, via.stopLocationIds().toString());
+    assertTrue(via.isPassThroughLocation());
+    assertEquals(
+      "PassThroughViaLocation{label: TestLabel, stopLocationIds: [F:ID1, F:ID2]}",
+      via.toString()
+    );
+  }
+
+  @Test
+  void testToLegacyPassThroughLocationsWithBareMinimum() {
+    Map<String, Object> input = mapOf("placeIds", LIST_IDS_INPUT);
+    var result = TripViaLocationMapper.toLegacyPassThroughLocations(List.of(input));
+    var via = result.getFirst();
+
+    assertNull(via.label());
+    assertEquals(EXPECTED_IDS_AS_STRING, via.stopLocationIds().toString());
+    assertTrue(via.isPassThroughLocation());
+    assertEquals("PassThroughViaLocation{stopLocationIds: [F:ID1, F:ID2]}", via.toString());
+  }
+
+  @Test
+  void testToLegacyPassThroughLocationsWithoutIds() {
+    var result = TripViaLocationMapper.toLegacyPassThroughLocations(
+      List.of(mapOf("placeIds", null))
+    );
+    assertTrue(result.isEmpty(), REASON_EMPTY_IDS_ALLOWED_PASS_THROUGH);
+  }
+
+  @Test
+  void testToLegacyPassThroughLocationsWithEmptyList() {
+    Map<String, Object> input = Map.ofEntries(entry("name", LABEL), entry("placeIds", List.of()));
+    var result = TripViaLocationMapper.toLegacyPassThroughLocations(List.of(input));
+    assertTrue(result.isEmpty(), REASON_EMPTY_IDS_ALLOWED_PASS_THROUGH);
+  }
+
   private Map<String, Object> visitInput(String label, Duration minWaitTime, List<String> ids) {
     var map = new HashMap<String, Object>();
     if (label != null) {
@@ -138,4 +231,14 @@ private Map<String, Object> visitInput(String label, Duration minWaitTime, List<
   private Map<String, Object> passThroughInput(String label, List<String> ids) {
     return visitInput(label, null, ids);
   }
+
+  /**
+   * Create a new HashMap with the {@code key} and {@code value}, the value may be {@code null}.
+   * The {@link Map#of(Object, Object)} does not support {@code null} values.
+   */
+  private static Map<String, Object> mapOf(String key, Object value) {
+    var map = new HashMap<String, Object>();
+    map.put(key, value);
+    return map;
+  }
 }