Skip to content

Fix handling of implicit access and egress mode parameters. #5821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 10, 2024
Merged

Fix handling of implicit access and egress mode parameters. #5821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
576326b
Fix handling of implicit access and egress mode parameters.
jtorin Apr 24, 2024
2dd2825
Add test-case for explicit null modes parameter.
jtorin May 3, 2024
c5ac169
Refactor to set values directly in the mapper and not rely on later l…
jtorin May 6, 2024
7b07cd5
Update src/main/java/org/opentripplanner/apis/transmodel/mapping/Requ…
jtorin May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/main/java/org/opentripplanner/apis/transmodel/mapping/RequestModesMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package org.opentripplanner.apis.transmodel.mapping;

import java.util.Map;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.function.Predicate;
import org.opentripplanner.routing.api.request.RequestModes;
import org.opentripplanner.routing.api.request.RequestModesBuilder;
import org.opentripplanner.routing.api.request.StreetMode;

class RequestModesMapper {

private static final Predicate<StreetMode> IS_BIKE = m -> m == StreetMode.BIKE;
private static final String accessModeKey = "accessMode";
private static final String egressModeKey = "egressMode";
private static final String directModeKey = "directMode";
Expand All @@ -15,22 +19,26 @@ class RequestModesMapper {
* Maps GraphQL Modes input type to RequestModes.
* <p>
* This only maps access, egress, direct & transfer modes. Transport modes are set using filters.
* Default modes are WALK for access, egress & transfer.
*/
static RequestModes mapRequestModes(Map<String, ?> modesInput) {
RequestModesBuilder mBuilder = RequestModes.of();

if (modesInput.containsKey(accessModeKey)) {
StreetMode accessMode = (StreetMode) modesInput.get(accessModeKey);
mBuilder.withAccessMode(accessMode);
mBuilder.withTransferMode(accessMode == StreetMode.BIKE ? StreetMode.BIKE : StreetMode.WALK);
}
if (modesInput.containsKey(egressModeKey)) {
mBuilder.withEgressMode((StreetMode) modesInput.get(egressModeKey));
}
// An unset directMode should overwrite the walk default, so we don't check for existence first.
mBuilder.withDirectMode((StreetMode) modesInput.get(directModeKey));
final StreetMode accessMode = (StreetMode) modesInput.get(accessModeKey);
ensureValueAndSet(accessMode, mBuilder::withAccessMode);
ensureValueAndSet((StreetMode) modesInput.get(egressModeKey), mBuilder::withEgressMode);
ensureValueAndSet((StreetMode) modesInput.get(directModeKey), mBuilder::withDirectMode);
Optional.ofNullable(accessMode).filter(IS_BIKE).ifPresent(mBuilder::withTransferMode);

return mBuilder.build();
}

/**
* Use the provided consumer to apply the StreetMode if it's non-null, otherwise apply NOT_SET.
*
* @param streetMode
* @param consumer
*/
private static void ensureValueAndSet(StreetMode streetMode, Consumer<StreetMode> consumer) {
consumer.accept(streetMode == null ? StreetMode.NOT_SET : streetMode);
}
}
37 changes: 29 additions & 8 deletions src/test/java/org/opentripplanner/apis/transmodel/mapping/RequestModesMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.opentripplanner.apis.transmodel.mapping;

import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.Map;
import org.junit.jupiter.api.Test;
Expand All @@ -9,23 +9,44 @@

class RequestModesMapperTest {

private static final RequestModes MODES_NOT_SET = RequestModes
.of()
.withAccessMode(StreetMode.NOT_SET)
.withEgressMode(StreetMode.NOT_SET)
.withDirectMode(StreetMode.NOT_SET)
.build();

@Test
void testMapRequestModesEmptyMapReturnsDefaults() {
Map<String, StreetMode> inputModes = Map.of();

RequestModes wantModes = RequestModes.of().withDirectMode(null).build();
RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);

assertEquals(MODES_NOT_SET, mappedModes);
}

@Test
void testMapRequestModesScooterRentalAccessSetReturnsDefaultsForOthers() {
Map<String, StreetMode> inputModes = Map.of("accessMode", StreetMode.SCOOTER_RENTAL);

RequestModes wantModes = MODES_NOT_SET
.copyOf()
.withAccessMode(StreetMode.SCOOTER_RENTAL)
.withTransferMode(StreetMode.WALK)
.withDirectMode(null)
.build();

RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);

assertEquals(wantModes, mappedModes);
}

@Test
void testMapRequestModesAccessSetReturnsDefaultsForOthers() {
void testMapRequestModesBikeAccessSetReturnsDefaultsForOthers() {
Map<String, StreetMode> inputModes = Map.of("accessMode", StreetMode.BIKE);

RequestModes wantModes = RequestModes
.of()
RequestModes wantModes = MODES_NOT_SET
.copyOf()
.withAccessMode(StreetMode.BIKE)
.withTransferMode(StreetMode.BIKE)
.withDirectMode(null)
Expand All @@ -40,8 +61,8 @@ void testMapRequestModesAccessSetReturnsDefaultsForOthers() {
void testMapRequestModesEgressSetReturnsDefaultsForOthers() {
Map<String, StreetMode> inputModes = Map.of("egressMode", StreetMode.CAR);

RequestModes wantModes = RequestModes
.of()
RequestModes wantModes = MODES_NOT_SET
.copyOf()
.withEgressMode(StreetMode.CAR)
.withDirectMode(null)
.build();
Expand All @@ -55,7 +76,7 @@ void testMapRequestModesEgressSetReturnsDefaultsForOthers() {
void testMapRequestModesDirectSetReturnsDefaultsForOthers() {
Map<String, StreetMode> inputModes = Map.of("directMode", StreetMode.CAR);

RequestModes wantModes = RequestModes.of().withDirectMode(StreetMode.CAR).build();
RequestModes wantModes = MODES_NOT_SET.copyOf().withDirectMode(StreetMode.CAR).build();

RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);

Expand Down
87 changes: 83 additions & 4 deletions src/test/java/org/opentripplanner/apis/transmodel/mapping/TripRequestMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@
import io.micrometer.core.instrument.Metrics;
import java.time.Duration;
import java.time.LocalDate;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -64,7 +68,6 @@ public class TripRequestMapperTest implements PlanTestConstants {

private static TransitModelForTest TEST_MODEL = TransitModelForTest.of();

static final TransmodelRequestContext context;
private static final Duration MAX_FLEXIBLE = Duration.ofMinutes(20);

private static final Function<StopLocation, String> STOP_TO_ID = s -> s.getId().toString();
Expand All @@ -76,8 +79,12 @@ public class TripRequestMapperTest implements PlanTestConstants {
private static final RegularStop stop2 = TEST_MODEL.stop("ST:stop2", 2, 1).build();
private static final RegularStop stop3 = TEST_MODEL.stop("ST:stop3", 3, 1).build();

private static final Graph graph = new Graph();
private static final DefaultTransitService transitService;

private TransmodelRequestContext context;

static {
var graph = new Graph();
var itinerary = newItinerary(Place.forStop(stop1), time("11:00"))
.bus(route1, 1, time("11:05"), time("11:20"), Place.forStop(stop2))
.bus(route2, 2, time("11:20"), time("11:40"), Place.forStop(stop3))
Expand All @@ -103,8 +110,12 @@ public class TripRequestMapperTest implements PlanTestConstants {

transitModel.updateCalendarServiceData(true, calendarServiceData, DataImportIssueStore.NOOP);
transitModel.index();
final var transitService = new DefaultTransitService(transitModel);
var defaultRequest = new RouteRequest();
transitService = new DefaultTransitService(transitModel);
}

@BeforeEach
void setup() {
final RouteRequest defaultRequest = new RouteRequest();

// Change defaults for FLEXIBLE to a lower value than the default 45m. This should restrict the
// input to be less than 20m, not 45m.
Expand Down Expand Up @@ -335,6 +346,74 @@ void testPassThroughPointsNoMatch() {
assertEquals("No match for F:XX:NonExisting.", ex.getMessage());
}

@Test
public void testNoModes() {
var req = TripRequestMapper.createRequest(executionContext(Map.of()));

assertEquals(StreetMode.WALK, req.journey().access().mode());
assertEquals(StreetMode.WALK, req.journey().egress().mode());
assertEquals(StreetMode.WALK, req.journey().direct().mode());
assertEquals(StreetMode.WALK, req.journey().transfer().mode());
}

@Test
public void testEmptyModes() {
Map<String, Object> arguments = Map.of("modes", Map.of());
var req = TripRequestMapper.createRequest(executionContext(arguments));

assertEquals(StreetMode.NOT_SET, req.journey().access().mode());
assertEquals(StreetMode.NOT_SET, req.journey().egress().mode());
assertEquals(StreetMode.NOT_SET, req.journey().direct().mode());
assertEquals(StreetMode.WALK, req.journey().transfer().mode());
}

@Test
public void testNullModes() {
HashMap<Object, Object> modes = new HashMap<>();
modes.put("accessMode", null);
modes.put("egressMode", null);
modes.put("directMode", null);
Map<String, Object> arguments = Map.of("modes", modes);
var req = TripRequestMapper.createRequest(executionContext(arguments));

assertEquals(StreetMode.NOT_SET, req.journey().access().mode());
assertEquals(StreetMode.NOT_SET, req.journey().egress().mode());
assertEquals(StreetMode.NOT_SET, req.journey().direct().mode());
assertEquals(StreetMode.WALK, req.journey().transfer().mode());
}

@Test
public void testExplicitModes() {
Map<String, Object> arguments = Map.of(
"modes",
Map.of(
"accessMode",
StreetMode.SCOOTER_RENTAL,
"egressMode",
StreetMode.BIKE_RENTAL,
"directMode",
StreetMode.BIKE_TO_PARK
)
);
var req = TripRequestMapper.createRequest(executionContext(arguments));

assertEquals(StreetMode.SCOOTER_RENTAL, req.journey().access().mode());
assertEquals(StreetMode.BIKE_RENTAL, req.journey().egress().mode());
assertEquals(StreetMode.BIKE_TO_PARK, req.journey().direct().mode());
assertEquals(StreetMode.WALK, req.journey().transfer().mode());
}

@Test
public void testExplicitModesBikeAccess() {
Map<String, Object> arguments = Map.of("modes", Map.of("accessMode", StreetMode.BIKE));
var req = TripRequestMapper.createRequest(executionContext(arguments));

assertEquals(StreetMode.BIKE, req.journey().access().mode());
assertEquals(StreetMode.NOT_SET, req.journey().egress().mode());
assertEquals(StreetMode.NOT_SET, req.journey().direct().mode());
assertEquals(StreetMode.BIKE, req.journey().transfer().mode());
}

private DataFetchingEnvironment executionContext(Map<String, Object> arguments) {
ExecutionInput executionInput = ExecutionInput
.newExecutionInput()
Expand Down