diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapper.java b/application/src/main/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapper.java index 50845aecef7..5f572c89da0 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapper.java +++ b/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 toLegacyPassThroughLocations( return passThroughPoints .stream() .map(TripViaLocationMapper::mapLegacyPassThroughViaLocation) + .filter(Objects::nonNull) .collect(toList()); } @@ -65,6 +68,13 @@ private static PassThroughViaLocation mapPassThroughViaLocation(Map mapStopLocationIds(Map map) { var c = (Collection) 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(); } @@ -72,12 +82,17 @@ private static List mapStopLocationIds(Map map) { * @deprecated Legacy passThrough, use via instead */ @Deprecated + @Nullable private static ViaLocation mapLegacyPassThroughViaLocation(Map inputMap) { final String name = (String) inputMap.get("name"); - final List stopLocationIds = - ((List) inputMap.get("placeIds")).stream() - .map(TransitIdMapper::mapIDToDomain) - .toList(); + List placeIds = (List) inputMap.get("placeIds"); + if (placeIds == null || placeIds.isEmpty()) { + return null; + } + final List stopLocationIds = placeIds + .stream() + .map(TransitIdMapper::mapIDToDomain) + .toList(); return new PassThroughViaLocation(name, stopLocationIds); } } diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/ViaLocationInputType.java b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/ViaLocationInputType.java index a808d934eed..ef13f8db18e 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/ViaLocationInputType.java +++ b/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))); } } diff --git a/application/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java b/application/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java index 1116031cec6..aa75a634004 100644 --- a/application/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java +++ b/application/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java @@ -16,7 +16,8 @@ public PassThroughViaLocation(@Nullable String label, Collection 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) ); } } diff --git a/application/src/main/java/org/opentripplanner/routing/api/request/via/VisitViaLocation.java b/application/src/main/java/org/opentripplanner/routing/api/request/via/VisitViaLocation.java index 2b59a749a1d..096fbfabc0b 100644 --- a/application/src/main/java/org/opentripplanner/routing/api/request/via/VisitViaLocation.java +++ b/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) ); } } diff --git a/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql b/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql index 9aa743fd42d..2f3aef2d9a2 100644 --- a/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql +++ b/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." diff --git a/application/src/test/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapperTest.java b/application/src/test/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapperTest.java index 49304ee1e05..9c33a149b49 100644 --- a/application/src/test/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapperTest.java +++ b/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 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 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 input = Map.of( - FIELD_VISIT, - Map.of(FIELD_STOP_LOCATION_IDS, List.of("F:1")) - ); + Map 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 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 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 input = Map.of(FIELD_PASS_THROUGH, passThroughInput(LABEL, LIST_IDS_INPUT)); + Map 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 input = Map.of( + Map 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 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 tetMapToPassThroughWithAnEmptyListOfIds() { + Map 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 input = Map.ofEntries( @@ -121,6 +172,48 @@ void testOneOf() { ); } + @Test + void testToLegacyPassThroughLocations() { + Map 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 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 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 visitInput(String label, Duration minWaitTime, List ids) { var map = new HashMap(); if (label != null) { @@ -138,4 +231,14 @@ private Map visitInput(String label, Duration minWaitTime, List< private Map passThroughInput(String label, List 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 mapOf(String key, Object value) { + var map = new HashMap(); + map.put(key, value); + return map; + } }