diff --git a/docs/Changelog.md b/docs/Changelog.md index 979e6579ba2..113120b2857 100644 --- a/docs/Changelog.md +++ b/docs/Changelog.md @@ -17,6 +17,8 @@ based on merged pull requests. Search GitHub issues and pull requests for smalle - Fix check for OSM relation members not being present in the extract [#5379](https://github.com/opentripplanner/OpenTripPlanner/pull/5379) - Add a configurable limit for the search window [#5293](https://github.com/opentripplanner/OpenTripPlanner/pull/5293) - Fix fare calculation for combined interlined legs [#5408](https://github.com/opentripplanner/OpenTripPlanner/pull/5408) +- Fix board slack list mapping in Transmodel API [#5420](https://github.com/opentripplanner/OpenTripPlanner/pull/5420) +- Fix flexible quay querying in Transmodel API [#5417](https://github.com/opentripplanner/OpenTripPlanner/pull/5417) [](AUTOMATIC_CHANGELOG_PLACEHOLDER_DO_NOT_REMOVE) ## 2.4.0 (2023-09-13) diff --git a/pom.xml b/pom.xml index d0e418ae73b..7a44cc72aa4 100644 --- a/pom.xml +++ b/pom.xml @@ -60,7 +60,7 @@ 29.2 2.48.1 - 2.15.2 + 2.15.3 3.1.3 5.10.0 1.11.5 diff --git a/src/ext-test/java/org/opentripplanner/ext/fares/impl/OrcaFareServiceTest.java b/src/ext-test/java/org/opentripplanner/ext/fares/impl/OrcaFareServiceTest.java index f505d001bc4..1f880dcab71 100644 --- a/src/ext-test/java/org/opentripplanner/ext/fares/impl/OrcaFareServiceTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/fares/impl/OrcaFareServiceTest.java @@ -75,9 +75,9 @@ public static void setUpClass() { * types. */ private static void calculateFare(List legs, FareType fareType, Money expectedPrice) { - ItineraryFares fare = new ItineraryFares(); - orcaFareService.populateFare(fare, USD, fareType, legs, null); - assertEquals(expectedPrice, fare.getFare(fareType)); + var itinerary = new Itinerary(legs); + var itineraryFares = orcaFareService.calculateFares(itinerary); + assertEquals(expectedPrice, itineraryFares.getFare(fareType)); } private static void assertLegFareEquals( @@ -642,25 +642,27 @@ private static Itinerary createItinerary( String firstStopName, String lastStopName ) { + // Use the agency ID as feed ID to make sure that we have a new feed ID for each different agency + // This tests to make sure we are calculating transfers across feeds correctly. Agency agency = Agency - .of(new FeedScopedId(FEED_ID, agencyId)) + .of(new FeedScopedId(agencyId, agencyId)) .withName(agencyId) .withTimezone(ZoneIds.NEW_YORK.getId()) .build(); // Set up stops RegularStop firstStop = RegularStop - .of(new FeedScopedId(FEED_ID, "1")) + .of(new FeedScopedId(agencyId, "1")) .withCoordinate(new WgsCoordinate(1, 1)) .withName(new NonLocalizedString(firstStopName)) .build(); RegularStop lastStop = RegularStop - .of(new FeedScopedId(FEED_ID, "2")) + .of(new FeedScopedId(agencyId, "2")) .withCoordinate(new WgsCoordinate(1, 2)) .withName(new NonLocalizedString(lastStopName)) .build(); - FeedScopedId routeFeedScopeId = new FeedScopedId(FEED_ID, routeId); + FeedScopedId routeFeedScopeId = new FeedScopedId(agencyId, routeId); NonLocalizedString longName = null; if (routeLongName != null) { longName = new NonLocalizedString(routeLongName); diff --git a/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java b/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java index fcdcfe05f2d..19ff992f3c7 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java +++ b/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java @@ -90,6 +90,15 @@ public Map> getFareRulesPerType() { return fareRulesPerType; } + /** + * Takes a legs and returns a map of their agency's feed id and all corresponding legs. + */ + protected Map> fareLegsByFeed(List fareLegs) { + return fareLegs + .stream() + .collect(Collectors.groupingBy(leg -> leg.getAgency().getId().getFeedId())); + } + @Override public ItineraryFares calculateFares(Itinerary itinerary) { var fareLegs = itinerary @@ -105,9 +114,7 @@ public ItineraryFares calculateFares(Itinerary itinerary) { if (fareLegs.isEmpty()) { return null; } - var fareLegsByFeed = fareLegs - .stream() - .collect(Collectors.groupingBy(leg -> leg.getAgency().getId().getFeedId())); + var fareLegsByFeed = fareLegsByFeed(fareLegs); ItineraryFares fare = ItineraryFares.empty(); boolean hasFare = false; diff --git a/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareService.java b/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareService.java index 724de3e225f..32aa8b75b45 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareService.java +++ b/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareService.java @@ -613,6 +613,15 @@ protected Collection fareRulesForFeed(FareType fareType, String fee return fareRulesPerType.get(fareType); } + /** + * Disables functionality grouping legs by their feed. + * This ensures we can calculate transfers between agencies/feeds. + */ + @Override + protected Map> fareLegsByFeed(List fareLegs) { + return Map.of(FEED_ID, fareLegs); + } + /** * Check if trip falls within the transfer time window. */ diff --git a/src/ext/java/org/opentripplanner/ext/transmodelapi/mapping/preferences/TransitPreferencesMapper.java b/src/ext/java/org/opentripplanner/ext/transmodelapi/mapping/preferences/TransitPreferencesMapper.java index 5a950327b54..d7fe4e254bc 100644 --- a/src/ext/java/org/opentripplanner/ext/transmodelapi/mapping/preferences/TransitPreferencesMapper.java +++ b/src/ext/java/org/opentripplanner/ext/transmodelapi/mapping/preferences/TransitPreferencesMapper.java @@ -20,7 +20,7 @@ public static void mapTransitPreferences( callWith.argument("boardSlackDefault", builder::withDefaultSec); callWith.argument( "boardSlackList", - (Integer v) -> TransportModeSlack.mapIntoDomain(builder, v) + (Object v) -> TransportModeSlack.mapIntoDomain(builder, v) ); }); transit.withAlightSlack(builder -> { diff --git a/src/ext/java/org/opentripplanner/ext/transmodelapi/model/stop/QuayType.java b/src/ext/java/org/opentripplanner/ext/transmodelapi/model/stop/QuayType.java index b68bc528f6c..e056fb725d6 100644 --- a/src/ext/java/org/opentripplanner/ext/transmodelapi/model/stop/QuayType.java +++ b/src/ext/java/org/opentripplanner/ext/transmodelapi/model/stop/QuayType.java @@ -15,20 +15,18 @@ import java.time.Instant; import java.time.ZoneId; import java.util.Collection; -import java.util.Locale; import java.util.Objects; import java.util.Optional; -import java.util.stream.Collectors; import org.opentripplanner.ext.transmodelapi.model.EnumTypes; import org.opentripplanner.ext.transmodelapi.model.plan.JourneyWhiteListed; import org.opentripplanner.ext.transmodelapi.model.scalars.GeoJSONCoordinatesScalar; import org.opentripplanner.ext.transmodelapi.support.GqlUtil; import org.opentripplanner.framework.graphql.GraphQLUtils; -import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.model.TripTimeOnDate; import org.opentripplanner.routing.stoptimes.ArrivalDeparture; import org.opentripplanner.transit.model.basic.Accessibility; import org.opentripplanner.transit.model.basic.TransitMode; +import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.model.site.AreaStop; import org.opentripplanner.transit.model.site.GroupStop; import org.opentripplanner.transit.model.site.RegularStop; @@ -184,15 +182,15 @@ public static GraphQLObjectType create( .withDirective(gqlUtil.timingData) .description("List of lines servicing this quay") .type(new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(lineType)))) - .dataFetcher(environment -> { - return GqlUtil + .dataFetcher(environment -> + GqlUtil .getTransitService(environment) .getPatternsForStop(environment.getSource(), true) .stream() - .map(pattern -> pattern.getRoute()) + .map(TripPattern::getRoute) .distinct() - .collect(Collectors.toList()); - }) + .toList() + ) .build() ) .field( @@ -202,11 +200,9 @@ public static GraphQLObjectType create( .withDirective(gqlUtil.timingData) .description("List of journey patterns servicing this quay") .type(new GraphQLNonNull(new GraphQLList(journeyPatternType))) - .dataFetcher(environment -> { - return GqlUtil - .getTransitService(environment) - .getPatternsForStop(environment.getSource(), true); - }) + .dataFetcher(environment -> + GqlUtil.getTransitService(environment).getPatternsForStop(environment.getSource(), true) + ) .build() ) .field( @@ -317,7 +313,7 @@ public static GraphQLObjectType create( ); Integer timeRangeInput = environment.getArgument("timeRange"); Duration timeRange = Duration.ofSeconds(timeRangeInput.longValue()); - RegularStop stop = environment.getSource(); + StopLocation stop = environment.getSource(); JourneyWhiteListed whiteListed = new JourneyWhiteListed(environment); Collection transitModes = environment.getArgument("whiteListedModes"); @@ -343,7 +339,7 @@ public static GraphQLObjectType create( .sorted(TripTimeOnDate.compareByDeparture()) .distinct() .limit(numberOfDepartures) - .collect(Collectors.toList()); + .toList(); }) .build() ) @@ -353,12 +349,12 @@ public static GraphQLObjectType create( .name("situations") .description("Get all situations active for the quay.") .type(new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(ptSituationElementType)))) - .dataFetcher(env -> { - return GqlUtil + .dataFetcher(env -> + GqlUtil .getTransitService(env) .getTransitAlertService() - .getStopAlerts(((StopLocation) env.getSource()).getId()); - }) + .getStopAlerts(((StopLocation) env.getSource()).getId()) + ) .build() ) .field( diff --git a/src/main/java/org/opentripplanner/model/plan/legreference/LegReference.java b/src/main/java/org/opentripplanner/model/plan/legreference/LegReference.java index 983d3c9ca88..cbac4abe6a5 100644 --- a/src/main/java/org/opentripplanner/model/plan/legreference/LegReference.java +++ b/src/main/java/org/opentripplanner/model/plan/legreference/LegReference.java @@ -1,5 +1,6 @@ package org.opentripplanner.model.plan.legreference; +import javax.annotation.Nullable; import org.opentripplanner.model.plan.Leg; import org.opentripplanner.transit.service.TransitService; @@ -7,5 +8,6 @@ * Marker interface for various types of leg references */ public interface LegReference { + @Nullable Leg getLeg(TransitService transitService); } diff --git a/src/main/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReference.java b/src/main/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReference.java index a54945413c7..e5e2ab695a4 100644 --- a/src/main/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReference.java +++ b/src/main/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReference.java @@ -2,6 +2,7 @@ import java.time.LocalDate; import java.time.ZoneId; +import javax.annotation.Nullable; import org.opentripplanner.framework.time.ServiceDateUtils; import org.opentripplanner.model.Timetable; import org.opentripplanner.model.plan.ScheduledTransitLeg; @@ -11,10 +12,12 @@ import org.opentripplanner.transit.model.timetable.Trip; import org.opentripplanner.transit.model.timetable.TripTimes; import org.opentripplanner.transit.service.TransitService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A reference which can be used to rebuild an exact copy of a {@link ScheduledTransitLeg} using the - * {@Link RoutingService} + * {@link org.opentripplanner.routing.api.RoutingService} */ public record ScheduledTransitLegReference( FeedScopedId tripId, @@ -23,30 +26,71 @@ public record ScheduledTransitLegReference( int toStopPositionInPattern ) implements LegReference { + private static final Logger LOG = LoggerFactory.getLogger(ScheduledTransitLegReference.class); + + /** + * Reconstruct a scheduled transit leg from this scheduled transit leg reference. + * Since the transit model could have been modified between the time the reference is created + * and the time the transit leg is reconstructed (either because new planned data have been + * rolled out, or because a realtime update has modified a trip), + * it may not be possible to reconstruct the leg. + * In this case the method returns null. + */ @Override + @Nullable public ScheduledTransitLeg getLeg(TransitService transitService) { Trip trip = transitService.getTripForId(tripId); - if (trip == null) { + LOG.info("Invalid transit leg reference: trip {} not found", tripId); return null; } TripPattern tripPattern = transitService.getPatternForTrip(trip, serviceDate); - - // no matching pattern found anywhere if (tripPattern == null) { + LOG.info( + "Invalid transit leg reference: trip pattern not found for trip {} and service date {} ", + tripId, + serviceDate + ); return null; } - Timetable timetable = transitService.getTimetableForTripPattern(tripPattern, serviceDate); + int numStops = tripPattern.numberOfStops(); + if (fromStopPositionInPattern >= numStops || toStopPositionInPattern >= numStops) { + LOG.info( + "Invalid transit leg reference: boarding stop {} or alighting stop {} is out of range" + + " in trip {} and service date {} ({} stops in trip pattern) ", + fromStopPositionInPattern, + toStopPositionInPattern, + tripId, + serviceDate, + numStops + ); + return null; + } + Timetable timetable = transitService.getTimetableForTripPattern(tripPattern, serviceDate); TripTimes tripTimes = timetable.getTripTimes(trip); + if (tripTimes == null) { + LOG.info( + "Invalid transit leg reference: trip times not found for trip {} and service date {} ", + tripId, + serviceDate + ); + return null; + } + if ( !transitService .getServiceCodesRunningForDate(serviceDate) .contains(tripTimes.getServiceCode()) ) { + LOG.info( + "Invalid transit leg reference: the trip {} does not run on service date {} ", + tripId, + serviceDate + ); return null; } diff --git a/src/main/java/org/opentripplanner/transit/service/StopModel.java b/src/main/java/org/opentripplanner/transit/service/StopModel.java index 110260c1cde..9dc0c792c5b 100644 --- a/src/main/java/org/opentripplanner/transit/service/StopModel.java +++ b/src/main/java/org/opentripplanner/transit/service/StopModel.java @@ -248,6 +248,9 @@ private void reindex() { @Nullable @SafeVarargs private static V getById(FeedScopedId id, Map... maps) { + if (id == null) { + return null; + } for (Map map : maps) { V v = map.get(id); if (v != null) { diff --git a/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java b/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java new file mode 100644 index 00000000000..ed54ac1bd44 --- /dev/null +++ b/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java @@ -0,0 +1,128 @@ +package org.opentripplanner.model.plan.legreference; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.opentripplanner.transit.model._data.TransitModelForTest.id; + +import java.time.LocalDate; +import java.util.List; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore; +import org.opentripplanner.model.Timetable; +import org.opentripplanner.model.calendar.CalendarServiceData; +import org.opentripplanner.model.plan.PlanTestConstants; +import org.opentripplanner.model.plan.ScheduledTransitLeg; +import org.opentripplanner.transit.model._data.TransitModelForTest; +import org.opentripplanner.transit.model.framework.Deduplicator; +import org.opentripplanner.transit.model.framework.FeedScopedId; +import org.opentripplanner.transit.model.network.TripPattern; +import org.opentripplanner.transit.model.timetable.Trip; +import org.opentripplanner.transit.model.timetable.TripTimes; +import org.opentripplanner.transit.service.DefaultTransitService; +import org.opentripplanner.transit.service.StopModel; +import org.opentripplanner.transit.service.TransitModel; +import org.opentripplanner.transit.service.TransitService; + +class ScheduledTransitLegReferenceTest { + + private static final int SERVICE_CODE = 555; + private static final LocalDate SERVICE_DATE = LocalDate.of(2023, 1, 1); + private static final int NUMBER_OF_STOPS = 3; + private static TransitService transitService; + private static FeedScopedId tripId; + + @BeforeAll + static void buildTransitService() { + // build transit data + TripPattern tripPattern = TransitModelForTest + .tripPattern("1", TransitModelForTest.route(id("1")).build()) + .withStopPattern(TransitModelForTest.stopPattern(NUMBER_OF_STOPS)) + .build(); + Timetable timetable = tripPattern.getScheduledTimetable(); + Trip trip = TransitModelForTest.trip("1").build(); + tripId = trip.getId(); + TripTimes tripTimes = new TripTimes( + trip, + TransitModelForTest.stopTimesEvery5Minutes(5, trip, PlanTestConstants.T11_00), + new Deduplicator() + ); + tripTimes.setServiceCode(SERVICE_CODE); + timetable.addTripTimes(tripTimes); + + // build transit model + TransitModel transitModel = new TransitModel(StopModel.of().build(), new Deduplicator()); + transitModel.addTripPattern(tripPattern.getId(), tripPattern); + transitModel.getServiceCodes().put(tripPattern.getId(), SERVICE_CODE); + CalendarServiceData calendarServiceData = new CalendarServiceData(); + calendarServiceData.putServiceDatesForServiceId(tripPattern.getId(), List.of(SERVICE_DATE)); + transitModel.updateCalendarServiceData(true, calendarServiceData, DataImportIssueStore.NOOP); + transitModel.index(); + + // build transit service + transitService = new DefaultTransitService(transitModel); + } + + @Test + void getLegFromReference() { + int boardAtStop = 0; + int alightAtStop = 1; + ScheduledTransitLegReference scheduledTransitLegReference = new ScheduledTransitLegReference( + tripId, + SERVICE_DATE, + boardAtStop, + alightAtStop + ); + ScheduledTransitLeg leg = scheduledTransitLegReference.getLeg(transitService); + assertNotNull(leg); + assertEquals(tripId, leg.getTrip().getId()); + assertEquals(SERVICE_DATE, leg.getServiceDate()); + assertEquals(boardAtStop, leg.getBoardStopPosInPattern()); + assertEquals(alightAtStop, leg.getAlightStopPosInPattern()); + } + + @Test + void getLegFromReferenceUnknownTrip() { + ScheduledTransitLegReference scheduledTransitLegReference = new ScheduledTransitLegReference( + FeedScopedId.ofNullable("XXX", "YYY"), + SERVICE_DATE, + 0, + 1 + ); + assertNull(scheduledTransitLegReference.getLeg(transitService)); + } + + @Test + void getLegFromReferenceInvalidServiceDate() { + ScheduledTransitLegReference scheduledTransitLegReference = new ScheduledTransitLegReference( + tripId, + LocalDate.EPOCH, + 0, + 1 + ); + assertNull(scheduledTransitLegReference.getLeg(transitService)); + } + + @Test + void getLegFromReferenceInvalidBoardingStop() { + ScheduledTransitLegReference scheduledTransitLegReference = new ScheduledTransitLegReference( + tripId, + SERVICE_DATE, + NUMBER_OF_STOPS, + 1 + ); + assertNull(scheduledTransitLegReference.getLeg(transitService)); + } + + @Test + void getLegFromReferenceInvalidAlightingStop() { + ScheduledTransitLegReference scheduledTransitLegReference = new ScheduledTransitLegReference( + tripId, + SERVICE_DATE, + 0, + NUMBER_OF_STOPS + ); + assertNull(scheduledTransitLegReference.getLeg(transitService)); + } +} diff --git a/src/test/java/org/opentripplanner/transit/service/StopModelTest.java b/src/test/java/org/opentripplanner/transit/service/StopModelTest.java index 94625a4a224..228ff55b70a 100644 --- a/src/test/java/org/opentripplanner/transit/service/StopModelTest.java +++ b/src/test/java/org/opentripplanner/transit/service/StopModelTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; @@ -132,4 +133,10 @@ void testGroupOfStations() { assertEquals(COOR_B, m.getCoordinateById(ID)); assertFalse(m.hasAreaStops()); } + + @Test + void testNullStopLocationId() { + var m = StopModel.of().build(); + assertNull(m.getStopLocation(null)); + } }