Skip to content

158 bug gtfs stops with no departures 1 #160

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

Closed
wants to merge 5 commits into from
Closed

158 bug gtfs stops with no departures 1 #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a7608c
ENH: 158 - Allow adding transfers to stops without departures in raptor.
clukas1 Dec 13, 2024
ba6ac3a
ENH: 158 - Add response dto objects for connections and isolines to a…
clukas1 Dec 13, 2024
7ddb2e8
TEST: 158 - Update tests for spring router.
clukas1 Dec 13, 2024
7cfb5e9
TEST: 158 - Updated expected transfer generation and stop inclusion i…
clukas1 Dec 13, 2024
b6d6134
FIX: 158 - Remove checks if stop has been added in when creating tran…
clukas1 Dec 13, 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
75 changes: 39 additions & 36 deletions src/main/java/ch/naviqore/app/controller/RoutingController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,15 @@
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.Nullable;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;

import java.time.LocalDateTime;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;

import static ch.naviqore.app.dto.DtoMapper.map;

Expand All @@ -42,9 +41,15 @@ public RoutingController(PublicTransitService service) {
this.service = service;
}

private static void handleConnectionRoutingException(ConnectionRoutingException e) {
private static ConnectionResponse handleConnectionRoutingException(ConnectionRoutingException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote against wrapping the responses in DTOs again, as it seems like reinventing the wheel.

Spring already provides built-in strategies for exception handling, and we are already using them. See: Error Handling for REST with Spring

I think the better approach is to have more speaking exceptions names and handle them in the already existing ch.naviqore.app.controllerGlobalExceptionHandler (@ControllerAdvice).

log.error("Connection routing exception", e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, e.getMessage());
return map(List.of(), e.getMessage(), MessageType.ERROR);
}

private static IsoLineResponse handleConnectionRoutingException(ConnectionRoutingException e, TimeType timeType,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same :)

boolean returnConnections) {
log.error("Connection routing exception", e);
return map(Map.of(), timeType, returnConnections, e.getMessage(), MessageType.ERROR);
}

@Operation(summary = "Get information about the routing", description = "Get all relevant information about the routing features supported by the service.")
Expand All @@ -62,21 +67,21 @@ public RoutingInfo getRoutingInfo() {
@ApiResponse(responseCode = "400", description = "Invalid input parameters", content = @Content(schema = @Schema()))
@ApiResponse(responseCode = "404", description = "StopID does not exist", content = @Content(schema = @Schema()))
@GetMapping("/connections")
public List<Connection> getConnections(@RequestParam(required = false) String sourceStopId,
@RequestParam(required = false) Double sourceLatitude,
@RequestParam(required = false) Double sourceLongitude,
@RequestParam(required = false) String targetStopId,
@RequestParam(required = false) Double targetLatitude,
@RequestParam(required = false) Double targetLongitude,
@RequestParam(required = false) LocalDateTime dateTime,
@RequestParam(required = false, defaultValue = "DEPARTURE") TimeType timeType,
@RequestParam(required = false) Integer maxWalkingDuration,
@RequestParam(required = false) Integer maxTransferNumber,
@RequestParam(required = false) Integer maxTravelTime,
@RequestParam(required = false, defaultValue = "0") int minTransferTime,
@RequestParam(required = false, defaultValue = "false") boolean wheelchairAccessible,
@RequestParam(required = false, defaultValue = "false") boolean bikeAllowed,
@RequestParam(required = false) EnumSet<TravelMode> travelModes) {
public ConnectionResponse getConnections(@RequestParam(required = false) String sourceStopId,
@RequestParam(required = false) Double sourceLatitude,
@RequestParam(required = false) Double sourceLongitude,
@RequestParam(required = false) String targetStopId,
@RequestParam(required = false) Double targetLatitude,
@RequestParam(required = false) Double targetLongitude,
@RequestParam(required = false) LocalDateTime dateTime,
@RequestParam(required = false, defaultValue = "DEPARTURE") TimeType timeType,
@RequestParam(required = false) Integer maxWalkingDuration,
@RequestParam(required = false) Integer maxTransferNumber,
@RequestParam(required = false) Integer maxTravelTime,
@RequestParam(required = false, defaultValue = "0") int minTransferTime,
@RequestParam(required = false, defaultValue = "false") boolean wheelchairAccessible,
@RequestParam(required = false, defaultValue = "false") boolean bikeAllowed,
@RequestParam(required = false) EnumSet<TravelMode> travelModes) {
// get coordinates if available
GeoCoordinate sourceCoordinate = Utils.getCoordinateIfAvailable(sourceStopId, sourceLatitude, sourceLongitude,
GlobalValidator.StopType.SOURCE);
Expand Down Expand Up @@ -106,8 +111,7 @@ public List<Connection> getConnections(@RequestParam(required = false) String so
return map(service.getConnections(sourceCoordinate, targetCoordinate, dateTime, map(timeType), config));
}
} catch (ConnectionRoutingException e) {
handleConnectionRoutingException(e);
return null;
return handleConnectionRoutingException(e);
}
}

Expand All @@ -116,19 +120,19 @@ public List<Connection> getConnections(@RequestParam(required = false) String so
@ApiResponse(responseCode = "400", description = "Invalid input parameters", content = @Content(schema = @Schema()))
@ApiResponse(responseCode = "404", description = "StopID does not exist", content = @Content(schema = @Schema()))
@GetMapping("/isolines")
public List<StopConnection> getIsolines(@RequestParam(required = false) String sourceStopId,
@RequestParam(required = false) Double sourceLatitude,
@RequestParam(required = false) Double sourceLongitude,
@RequestParam(required = false) LocalDateTime dateTime,
@RequestParam(required = false, defaultValue = "DEPARTURE") TimeType timeType,
@RequestParam(required = false) Integer maxWalkingDuration,
@RequestParam(required = false) Integer maxTransferNumber,
@RequestParam(required = false) Integer maxTravelTime,
@RequestParam(required = false, defaultValue = "0") int minTransferTime,
@RequestParam(required = false, defaultValue = "false") boolean wheelchairAccessible,
@RequestParam(required = false, defaultValue = "false") boolean bikeAllowed,
@RequestParam(required = false) EnumSet<TravelMode> travelModes,
@RequestParam(required = false, defaultValue = "false") boolean returnConnections) {
public IsoLineResponse getIsolines(@RequestParam(required = false) String sourceStopId,
@RequestParam(required = false) Double sourceLatitude,
@RequestParam(required = false) Double sourceLongitude,
@RequestParam(required = false) LocalDateTime dateTime,
@RequestParam(required = false, defaultValue = "DEPARTURE") TimeType timeType,
@RequestParam(required = false) Integer maxWalkingDuration,
@RequestParam(required = false) Integer maxTransferNumber,
@RequestParam(required = false) Integer maxTravelTime,
@RequestParam(required = false, defaultValue = "0") int minTransferTime,
@RequestParam(required = false, defaultValue = "false") boolean wheelchairAccessible,
@RequestParam(required = false, defaultValue = "false") boolean bikeAllowed,
@RequestParam(required = false) EnumSet<TravelMode> travelModes,
@RequestParam(required = false, defaultValue = "false") boolean returnConnections) {

// get stops or coordinates if available
GeoCoordinate sourceCoordinate = Utils.getCoordinateIfAvailable(sourceStopId, sourceLatitude, sourceLongitude,
Expand All @@ -150,8 +154,7 @@ public List<StopConnection> getIsolines(@RequestParam(required = false) String s
returnConnections);
}
} catch (ConnectionRoutingException e) {
handleConnectionRoutingException(e);
return null;
return handleConnectionRoutingException(e, timeType, returnConnections);
}
}

Expand Down
15 changes: 15 additions & 0 deletions src/main/java/ch/naviqore/app/dto/ConnectionResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package ch.naviqore.app.dto;

import lombok.*;

import java.util.List;

@RequiredArgsConstructor(access = AccessLevel.PACKAGE)
@EqualsAndHashCode
@ToString
@Getter
public class ConnectionResponse {
private final List<Connection> connections;
private final String message;
private final MessageType messageType;
}
19 changes: 14 additions & 5 deletions src/main/java/ch/naviqore/app/dto/DtoMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,17 +76,26 @@ public static Connection map(ch.naviqore.service.Connection connection) {
return new Connection(legs);
}

public static List<Connection> map(List<ch.naviqore.service.Connection> connections) {
return connections.stream().map(DtoMapper::map).toList();
public static ConnectionResponse map(List<ch.naviqore.service.Connection> connections, String message, MessageType messageType) {
return new ConnectionResponse( connections.stream().map(DtoMapper::map).toList(), message, messageType);
}

public static List<StopConnection> map(Map<ch.naviqore.service.Stop, ch.naviqore.service.Connection> connections,
TimeType timeType, boolean returnConnections) {
public static ConnectionResponse map(List<ch.naviqore.service.Connection> connections) {
return map( connections, "", MessageType.SUCCESS);
}

public static IsoLineResponse map(Map<ch.naviqore.service.Stop, ch.naviqore.service.Connection> connections,
TimeType timeType, boolean returnConnections, String message, MessageType messageType) {
List<StopConnection> arrivals = new ArrayList<>();
for (Map.Entry<ch.naviqore.service.Stop, ch.naviqore.service.Connection> entry : connections.entrySet()) {
arrivals.add(new StopConnection(entry.getKey(), entry.getValue(), map(timeType), returnConnections));
}
return arrivals;
return new IsoLineResponse( arrivals, message, messageType);
}

public static IsoLineResponse map(Map<ch.naviqore.service.Stop, ch.naviqore.service.Connection> connections,
TimeType timeType, boolean returnConnections) {
return map(connections, timeType, returnConnections, "", MessageType.SUCCESS);
}

public static ScheduleValidity map(ch.naviqore.service.Validity validity) {
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/ch/naviqore/app/dto/IsoLineResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package ch.naviqore.app.dto;

import lombok.*;

import java.util.List;

@RequiredArgsConstructor(access = AccessLevel.PACKAGE)
@EqualsAndHashCode
@ToString
@Getter
public class IsoLineResponse {
private final List<StopConnection> stopConnections;
private final String message;
private final MessageType messageType;
}
7 changes: 7 additions & 0 deletions src/main/java/ch/naviqore/app/dto/MessageType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package ch.naviqore.app.dto;

public enum MessageType {
SUCCESS,
WARNING,
ERROR,
}
65 changes: 38 additions & 27 deletions src/main/java/ch/naviqore/service/gtfs/raptor/convert/GtfsToRaptorConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
public class GtfsToRaptorConverter {

private final Set<String> addedStops = new HashSet<>();
private final Set<String> stopsForTransfers = new HashSet<>();
private final RaptorRouterBuilder builder;
private final GtfsRoutePartitioner partitioner;
private final List<TransferGenerator.Transfer> additionalTransfers;
Expand Down Expand Up @@ -65,8 +66,7 @@ private void addRoute(GtfsRoutePartitioner.SubRoute subRoute) {
List<String> stopIds = subRoute.getStopsSequence().stream().map(Stop::getId).toList();
for (String stopId : stopIds) {
if (!addedStops.contains(stopId)) {
builder.addStop(stopId);
addedStops.add(stopId);
this.addStop(stopId);
}
}

Expand Down Expand Up @@ -99,24 +99,46 @@ private void addRoute(GtfsRoutePartitioner.SubRoute subRoute) {
*/
private void processAllTransfers() {
addAdditionalTransfers();
processStopAndParentChildTransfers();
addGtfsTransfersWithPrecedence();
// It is possible that stops are added without departures in this loop, in that case this block will be
// re-run to ensure that transfers from those newly added stops are also included.
while (!stopsForTransfers.isEmpty()) {
Set<String> stopIterator = new HashSet<>(stopsForTransfers);
stopsForTransfers.clear();
processStopAndParentChildTransfers(stopIterator);
addGtfsTransfersWithPrecedence(stopIterator);
}
}

/**
* Adds all additional transfers.
*/
private void addAdditionalTransfers() {
for (TransferGenerator.Transfer transfer : additionalTransfers) {
builder.addTransfer(transfer.from().getId(), transfer.to().getId(), transfer.duration());
this.addTransfer(transfer.from().getId(), transfer.to().getId(), transfer.duration());
}
}

private void addStop(String stopId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still have the inconsistency between the GTFS schedule and the RAPTOR Router? Specifically, there could be stops in the schedule that the router doesn't recognize. I assume in that case, the router throws an exception when a route is requested from a stop with no departures in the GTFS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to add all stops to the router, it would simply result in no connections being returned, which accurately reflects the reality: the stop exists in the schedule, but there are no departures and, therefore, no connections.

Whether it makes semantic sense to include stops without departures in a schedule is another question entirely, but I believe that is not our responsibility to address.

builder.addStop(stopId);
addedStops.add(stopId);
stopsForTransfers.add(stopId);
}

private void addTransfer(String fromId, String toId, int duration) {
if (!addedStops.contains(fromId)) {
this.addStop(fromId);
}
if (!addedStops.contains(toId)) {
this.addStop(toId);
}
builder.addTransfer(fromId, toId, duration);
}

/**
* Processes transfers for each stop and handles parent-child relationships.
*/
private void processStopAndParentChildTransfers() {
for (String stopId : addedStops) {
private void processStopAndParentChildTransfers(Set<String> stopIterator) {
for (String stopId : stopIterator) {
Stop stop = schedule.getStops().get(stopId);
processParentAndChildTransfersForStop(stop);
}
Expand Down Expand Up @@ -162,7 +184,7 @@ private void processParentAndChildTransfersForStop(Stop stop) {
private void applyTransfersFromOtherStop(Stop consumerStop, Stop providerStop) {
Collection<TransferGenerator.Transfer> transfers = expandTransfersFromStop(providerStop);
for (TransferGenerator.Transfer transfer : transfers) {
builder.addTransfer(consumerStop.getId(), transfer.to().getId(), transfer.duration());
this.addTransfer(consumerStop.getId(), transfer.to().getId(), transfer.duration());
}
}

Expand All @@ -177,11 +199,7 @@ private void addToStopChildrenTransfers(Stop stop) {
if (stopTransfer.getTransferType() == TransferType.MINIMUM_TIME && stopTransfer.getMinTransferTime()
.isPresent()) {
for (Stop toChildStop : stopTransfer.getToStop().getChildren()) {
// only add new transfers if the to stop also has departures, else the raptor router does not care
// about this stop and the builder will throw an exception.
if (addedStops.contains(toChildStop.getId())) {
builder.addTransfer(stop.getId(), toChildStop.getId(), stopTransfer.getMinTransferTime().get());
}
this.addTransfer(stop.getId(), toChildStop.getId(), stopTransfer.getMinTransferTime().get());
}
}
}
Expand All @@ -190,16 +208,13 @@ private void addToStopChildrenTransfers(Stop stop) {
/**
* Adds transfers explicitly defined in the GTFS schedule, ensuring precedence over additional transfers.
*/
private void addGtfsTransfersWithPrecedence() {
for (String stopId : addedStops) {
private void addGtfsTransfersWithPrecedence(Set<String> stopIterator) {
for (String stopId : stopIterator) {
Stop stop = schedule.getStops().get(stopId);
for (Transfer transfer : stop.getTransfers()) {
// only add new transfers if the to stop also has departures, else the raptor router does not care about
// this stop and the builder will throw an exception.
if (transfer.getTransferType() == TransferType.MINIMUM_TIME && transfer.getMinTransferTime()
.isPresent() && addedStops.contains(transfer.getToStop().getId())) {
builder.addTransfer(stop.getId(), transfer.getToStop().getId(),
transfer.getMinTransferTime().get());
.isPresent()) {
this.addTransfer(stop.getId(), transfer.getToStop().getId(), transfer.getMinTransferTime().get());
}
}
}
Expand Down Expand Up @@ -229,14 +244,10 @@ private Collection<TransferGenerator.Transfer> expandTransfersFromStop(Stop stop
Stop toStop = transfer.getToStop();
// only add new transfers if the to stop also has departures, else the raptor router does not care about
// this stop and the builder will throw an exception.
if (addedStops.contains(toStop.getId())) {
otherTransfers.add(new TransferGenerator.Transfer(stop, toStop, transfer.getMinTransferTime().get()));
}
otherTransfers.add(new TransferGenerator.Transfer(stop, toStop, transfer.getMinTransferTime().get()));
for (Stop childToStop : toStop.getChildren()) {
if (addedStops.contains(childToStop.getId())) {
parentTransfers.put(childToStop,
new TransferGenerator.Transfer(stop, childToStop, transfer.getMinTransferTime().get()));
}
parentTransfers.put(childToStop,
new TransferGenerator.Transfer(stop, childToStop, transfer.getMinTransferTime().get()));
}
}

Expand Down
Loading
Loading