Merge pull request #6162 from entur/otp2_fix_npe_passthrough

Fix regression error in passThroughPoints in the Transmodel API

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;
+ }
 }