Skip to content

Commit

Permalink
Fix: Regression error in passThroughPoints in the Transmodel API
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@t2gran
t2gran committed Oct 16, 2024
1 parent 8e36ae2 commit 1b8da58
Show file tree
Hide file tree
Showing 6 changed files with 144 additions and 23 deletions.
23 changes: 19 additions & 4 deletions ...tion/src/main/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,6 +32,7 @@ static List<ViaLocation> toLegacyPassThroughLocations(
return passThroughPoints
.stream()
.map(TripViaLocationMapper::mapLegacyPassThroughViaLocation)
.filter(Objects::nonNull)
.collect(toList());
}

Expand Down Expand Up @@ -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);
}
}
9 changes: 5 additions & 4 deletions ...on/src/main/java/org/opentripplanner/apis/transmodel/model/plan/ViaLocationInputType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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();

Expand All @@ -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)));
}
}
3 changes: 2 additions & 1 deletion ...ion/src/main/java/org/opentripplanner/routing/api/request/via/PassThroughViaLocation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
);
}
}
Expand Down
3 changes: 2 additions & 1 deletion application/src/main/java/org/opentripplanner/routing/api/request/via/VisitViaLocation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!]!
}

"""
Expand Down Expand Up @@ -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."
Expand Down
125 changes: 114 additions & 11 deletions .../src/test/java/org/opentripplanner/apis/transmodel/mapping/TripViaLocationMapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();
Expand All @@ -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();

Expand All @@ -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();
Expand All @@ -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(
Expand All @@ -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) {
Expand All @@ -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;
}
}

0 comments on commit 1b8da58

Please sign in to comment.