From 0a95a2d7516206fa011470d1c14e2104c0837308 Mon Sep 17 00:00:00 2001 From: Daniel Heppner Date: Tue, 24 Sep 2024 18:32:42 -0700 Subject: [PATCH 1/4] fix(orca-fares): remove fare attributes with no rules --- .../ext/fares/impl/DefaultFareServiceFactory.java | 2 +- .../opentripplanner/ext/fares/impl/OrcaFareFactory.java | 2 ++ .../org/opentripplanner/ext/fares/model/FareRuleSet.java | 9 +++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceFactory.java b/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceFactory.java index ee4c87924ea..d7a3a0425ec 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceFactory.java +++ b/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceFactory.java @@ -92,7 +92,7 @@ protected void fillFareRules( FareRuleSet fareRule = fareRuleSet.get(id); if (fareRule == null) { // Should never happen by design - LOG.error("Inexistant fare ID in fare rule: " + id); + LOG.error("Nonexistent fare ID in fare rule: " + id); continue; } String contains = rule.getContainsId(); diff --git a/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareFactory.java b/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareFactory.java index 34a03c1fc06..d48cad48450 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareFactory.java +++ b/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareFactory.java @@ -24,6 +24,8 @@ public FareService makeFareService() { @Override public void processGtfs(FareRulesData fareRuleService, OtpTransitService transitService) { fillFareRules(fareRuleService.fareAttributes(), fareRuleService.fareRules(), regularFareRules); + // ORCA agencies don't rely on fare attributes without rules, so let's remove them. + regularFareRules.entrySet().removeIf(entry -> !entry.getValue().hasRules()); } /** diff --git a/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java b/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java index 30119631216..97cc4e637d2 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java +++ b/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java @@ -40,6 +40,15 @@ public Set getRouteOriginDestinations() { return routeOriginDestinations; } + public boolean hasRules() { + return ( + !routes.isEmpty() || + !originDestinations.isEmpty() || + !routeOriginDestinations.isEmpty() || + !contains.isEmpty() + ); + } + public void addContains(String containsId) { contains.add(containsId); } From feedefe5b887543a88e38a105143274130f6e015 Mon Sep 17 00:00:00 2001 From: Daniel Heppner Date: Thu, 26 Sep 2024 13:43:58 -0700 Subject: [PATCH 2/4] add tests for FareRuleSet --- .../ext/fares/FareRuleSetTest.java | 294 ++++++++++++++++++ 1 file changed, 294 insertions(+) create mode 100644 src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java diff --git a/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java b/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java new file mode 100644 index 00000000000..ee1365856be --- /dev/null +++ b/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java @@ -0,0 +1,294 @@ +package org.opentripplanner.ext.fares; + +import static org.junit.jupiter.api.Assertions.*; + +import java.time.Duration; +import java.util.HashSet; +import java.util.Set; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.opentripplanner.ext.fares.model.FareAttribute; +import org.opentripplanner.ext.fares.model.FareRuleSet; +import org.opentripplanner.transit.model.basic.Money; +import org.opentripplanner.transit.model.framework.FeedScopedId; + +class FareRuleSetTest { + + private FareRuleSet fareRuleSet; + static final Money TWO_FIFTY = Money.usDollars(2.50f); + + @BeforeEach + void setUp() { + FeedScopedId id = new FeedScopedId("feed", "fare1"); + FareAttribute fareAttribute = FareAttribute + .of(id) + .setPrice(TWO_FIFTY) + .setPaymentMethod(1) + .setTransfers(1) + .setTransferDuration(7200) + .build(); + fareRuleSet = new FareRuleSet(fareAttribute); + } + + @Test + void testHasNoRules() { + assertFalse(fareRuleSet.hasRules()); + } + + @Test + void testAddOriginDestination() { + fareRuleSet.addOriginDestination("A", "B"); + assertTrue(fareRuleSet.hasRules()); + } + + @Test + void testAddRouteOriginDestination() { + fareRuleSet.addRouteOriginDestination("Route1", "A", "B"); + assertTrue(fareRuleSet.hasRules()); + assertEquals(1, fareRuleSet.getRouteOriginDestinations().size()); + } + + @Test + void testAddContains() { + fareRuleSet.addContains("Zone1"); + assertTrue(fareRuleSet.hasRules()); + assertEquals(1, fareRuleSet.getContains().size()); + } + + @Test + void testAddRoute() { + FeedScopedId routeId = new FeedScopedId("feed", "route1"); + fareRuleSet.addRoute(routeId); + assertTrue(fareRuleSet.hasRules()); + assertEquals(1, fareRuleSet.getRoutes().size()); + } + + @Test + void testMatchesWithNoRules() { + var routes = Set.of(new FeedScopedId("feed", "route1")); + var trips = Set.of(new FeedScopedId("feed", "trip1")); + var zones = Set.of("zone1"); + assertTrue( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + assertTrue( + fareRuleSet.matches( + "A", + "B", + zones, + routes, + trips, + 0, + Duration.ofMinutes(100), + Duration.ofMinutes(100) + ) + ); + } + + @Test + void testMatchesWithOriginDestination() { + fareRuleSet.addOriginDestination("A", "B"); + assertTrue( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + assertFalse( + fareRuleSet.matches( + "B", + "C", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + } + + @Test + void testMatchesWithContains() { + Set zones = new HashSet<>(); + zones.add("Zone1"); + zones.add("Zone2"); + fareRuleSet.addContains("Zone1"); + fareRuleSet.addContains("Zone2"); + assertTrue( + fareRuleSet.matches( + "A", + "B", + zones, + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + assertFalse( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + } + + @Test + void testMatchesWithRoutes() { + Set routes = new HashSet<>(); + FeedScopedId routeId = new FeedScopedId("feed", "route1"); + FeedScopedId otherRouteId = new FeedScopedId("feed", "route2"); + routes.add(routeId); + fareRuleSet.addRoute(routeId); + assertTrue( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + routes, + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + assertFalse( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + Set.of(otherRouteId), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ZERO + ) + ); + } + + @Test + void testMatchesWithTransfers() { + assertTrue( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 1, + Duration.ZERO, + Duration.ZERO + ) + ); + assertFalse( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 2, + Duration.ZERO, + Duration.ZERO + ) + ); + } + + @Test + void testMatchesWithTransferDuration() { + assertTrue( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ofSeconds(7000), + Duration.ZERO + ) + ); + assertFalse( + fareRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ofSeconds(8000), + Duration.ZERO + ) + ); + } + + @Test + void testMatchesWithJourneyDuration() { + FareAttribute journeyFare = FareAttribute + .of(new FeedScopedId("feed", "journey")) + .setPrice(Money.usDollars(3.00f)) + .setPaymentMethod(1) + .setJourneyDuration(7200) + .build(); + FareRuleSet journeyRuleSet = new FareRuleSet(journeyFare); + + assertTrue( + journeyRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ofSeconds(7000) + ) + ); + assertFalse( + journeyRuleSet.matches( + "A", + "B", + new HashSet<>(), + new HashSet<>(), + new HashSet<>(), + 0, + Duration.ZERO, + Duration.ofSeconds(8000) + ) + ); + } + + @Test + void testAgencyMethods() { + assertFalse(fareRuleSet.hasAgencyDefined()); + assertNull(fareRuleSet.getAgency()); + + FeedScopedId agencyId = new FeedScopedId("feed", "agency1"); + fareRuleSet.setAgency(agencyId); + assertTrue(fareRuleSet.hasAgencyDefined()); + assertEquals(agencyId, fareRuleSet.getAgency()); + } +} From 364cf4f54952a1ae0bf5c3a11a9015063d08ce99 Mon Sep 17 00:00:00 2001 From: Daniel Heppner Date: Fri, 27 Sep 2024 09:48:11 -0700 Subject: [PATCH 3/4] add Javadoc --- .../ext/fares/model/FareRuleSet.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java b/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java index 97cc4e637d2..b8fbb0f1fe9 100644 --- a/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java +++ b/src/ext/java/org/opentripplanner/ext/fares/model/FareRuleSet.java @@ -40,6 +40,10 @@ public Set getRouteOriginDestinations() { return routeOriginDestinations; } + /** + * Determine whether the FareRuleSet has any rules added. + * @return True if any rules have been added. + */ public boolean hasRules() { return ( !routes.isEmpty() || @@ -69,6 +73,19 @@ public FareAttribute getFareAttribute() { return fareAttribute; } + /** + * Determines whether the FareRuleSet matches against a set of itinerary parameters + * based on the added rules and fare attribute + * @param startZone Origin zone + * @param endZone End zone + * @param zonesVisited A set containing the names of zones visited on the fare + * @param routesVisited A set containing the route IDs visited + * @param tripsVisited [Not implemented] A set containing the trip IDs visited + * @param transfersUsed Number of transfers already used + * @param tripTime Time from beginning of first leg to beginning of current leg to be evaluated + * @param journeyTime Total journey time from beginning of first leg to end of current leg + * @return True if this FareAttribute should apply to this leg + */ public boolean matches( String startZone, String endZone, From 54223b1efec9c5d5294e37e7cc3a53e4ad46cccf Mon Sep 17 00:00:00 2001 From: Daniel Heppner Date: Fri, 27 Sep 2024 09:50:57 -0700 Subject: [PATCH 4/4] replace new hashset with set.of() --- .../ext/fares/FareRuleSetTest.java | 116 ++++-------------- 1 file changed, 22 insertions(+), 94 deletions(-) diff --git a/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java b/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java index ee1365856be..13d4f713634 100644 --- a/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/fares/FareRuleSetTest.java @@ -69,16 +69,7 @@ void testMatchesWithNoRules() { var trips = Set.of(new FeedScopedId("feed", "trip1")); var zones = Set.of("zone1"); assertTrue( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), Set.of(), Set.of(), 0, Duration.ZERO, Duration.ZERO) ); assertTrue( fareRuleSet.matches( @@ -98,28 +89,10 @@ void testMatchesWithNoRules() { void testMatchesWithOriginDestination() { fareRuleSet.addOriginDestination("A", "B"); assertTrue( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), Set.of(), Set.of(), 0, Duration.ZERO, Duration.ZERO) ); assertFalse( - fareRuleSet.matches( - "B", - "C", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("B", "C", Set.of(), Set.of(), Set.of(), 0, Duration.ZERO, Duration.ZERO) ); } @@ -131,28 +104,10 @@ void testMatchesWithContains() { fareRuleSet.addContains("Zone1"); fareRuleSet.addContains("Zone2"); assertTrue( - fareRuleSet.matches( - "A", - "B", - zones, - new HashSet<>(), - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", zones, Set.of(), Set.of(), 0, Duration.ZERO, Duration.ZERO) ); assertFalse( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), Set.of(), Set.of(), 0, Duration.ZERO, Duration.ZERO) ); } @@ -164,24 +119,15 @@ void testMatchesWithRoutes() { routes.add(routeId); fareRuleSet.addRoute(routeId); assertTrue( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - routes, - new HashSet<>(), - 0, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), routes, Set.of(), 0, Duration.ZERO, Duration.ZERO) ); assertFalse( fareRuleSet.matches( "A", "B", - new HashSet<>(), + Set.of(), Set.of(otherRouteId), - new HashSet<>(), + Set.of(), 0, Duration.ZERO, Duration.ZERO @@ -192,28 +138,10 @@ void testMatchesWithRoutes() { @Test void testMatchesWithTransfers() { assertTrue( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 1, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), Set.of(), Set.of(), 1, Duration.ZERO, Duration.ZERO) ); assertFalse( - fareRuleSet.matches( - "A", - "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), - 2, - Duration.ZERO, - Duration.ZERO - ) + fareRuleSet.matches("A", "B", Set.of(), Set.of(), Set.of(), 2, Duration.ZERO, Duration.ZERO) ); } @@ -223,9 +151,9 @@ void testMatchesWithTransferDuration() { fareRuleSet.matches( "A", "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), + Set.of(), + Set.of(), + Set.of(), 0, Duration.ofSeconds(7000), Duration.ZERO @@ -235,9 +163,9 @@ void testMatchesWithTransferDuration() { fareRuleSet.matches( "A", "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), + Set.of(), + Set.of(), + Set.of(), 0, Duration.ofSeconds(8000), Duration.ZERO @@ -259,9 +187,9 @@ void testMatchesWithJourneyDuration() { journeyRuleSet.matches( "A", "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), + Set.of(), + Set.of(), + Set.of(), 0, Duration.ZERO, Duration.ofSeconds(7000) @@ -271,9 +199,9 @@ void testMatchesWithJourneyDuration() { journeyRuleSet.matches( "A", "B", - new HashSet<>(), - new HashSet<>(), - new HashSet<>(), + Set.of(), + Set.of(), + Set.of(), 0, Duration.ZERO, Duration.ofSeconds(8000)