Fix ID generation hacks in GTFS mapping, improve FaresV2 spec conformance

application/src/ext-test/java/org/opentripplanner/ext/fares/FaresFilterTest.java

@@ -35,7 +35,7 @@ void shouldAddFare() {
     var fares = new ItineraryFare();
 
     var leg = i1.legs().get(1);
-    var fp = new FareProduct(id("fp"), "fare product", Money.euros(10.00f), null, null, null);
+    var fp = FareProduct.of(id("fp"), "fare product", Money.euros(10.00f)).build();
     fares.addFareProduct(leg, fp);
 
     var filter = new DecorateWithFare((FareService) itinerary -> fares);

application/src/ext-test/java/org/opentripplanner/ext/fares/impl/FareModelForTest.java

@@ -86,14 +86,11 @@ public class FareModelForTest {
     .setTransfers(1)
     .setAgency(OTHER_FEED_AGENCY.getId())
     .build();
-  public static final FareProduct FARE_PRODUCT = new FareProduct(
+  public static final FareProduct FARE_PRODUCT = FareProduct.of(
     id("fp"),
     "fare product",
-    Money.euros(10.00f),
-    null,
-    null,
-    null
-  );
+    Money.euros(10.00f)
+  ).build();
   public static final FareProductUse FARE_PRODUCT_USE = new FareProductUse(
     "c1a04702-1fb6-32d4-ba02-483bf68111ed",
     FARE_PRODUCT

application/src/ext-test/java/org/opentripplanner/ext/fares/impl/FreeTransferInNetworkTest.java

@@ -0,0 +1,64 @@
+package org.opentripplanner.ext.fares.impl;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.opentripplanner.model.plan.TestItineraryBuilder.newItinerary;
+import static org.opentripplanner.transit.model._data.TimetableRepositoryForTest.groupOfRoutes;
+import static org.opentripplanner.transit.model._data.TimetableRepositoryForTest.id;
+
+import com.google.common.collect.Multimaps;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.ext.fares.model.FareLegRule;
+import org.opentripplanner.ext.fares.model.FareTransferRule;
+import org.opentripplanner.model.fare.FareProduct;
+import org.opentripplanner.model.plan.PlanTestConstants;
+import org.opentripplanner.transit.model._data.TimetableRepositoryForTest;
+import org.opentripplanner.transit.model.basic.Money;
+import org.opentripplanner.transit.model.framework.FeedScopedId;
+import org.opentripplanner.transit.model.network.GroupOfRoutes;
+import org.opentripplanner.transit.model.network.Route;
+
+class FreeTransferInNetworkTest implements PlanTestConstants {
+
+  private static final GroupOfRoutes NETWORK = groupOfRoutes("n1").build();
+  private static final Route ROUTE = TimetableRepositoryForTest.route("r1")
+    .withGroupOfRoutes(List.of(NETWORK))
+    .build();
+  private static final FeedScopedId LEG_GROUP = id("leg-group1");
+
+  FareProduct regular = FareProduct.of(id("regular"), "regular", Money.euros(5)).build();
+
+  GtfsFaresV2Service service = new GtfsFaresV2Service(
+    List.of(
+      FareLegRule.of(id("6"), regular)
+        .withLegGroupId(LEG_GROUP)
+        .withNetworkId(NETWORK.getId())
+        .build()
+    ),
+    List.of(new FareTransferRule(id("transfer"), LEG_GROUP, LEG_GROUP, -1, null, List.of())),
+    Multimaps.forMap(Map.of())
+  );
+
+  @Test
+  void differentNetwork() {
+    var i1 = newItinerary(A, 0).bus(1, 0, 50, B).build();
+    var result = service.getProducts(i1);
+    assertEquals(Set.of(), result.itineraryProducts());
+  }
+
+  @Test
+  void singleLeg() {
+    var i1 = newItinerary(A, 0).bus(ROUTE, 1, 0, 50, B).build();
+    var result = service.getProducts(i1);
+    assertEquals(Set.of(regular), result.itineraryProducts());
+  }
+
+  @Test
+  void severalLegs() {
+    var i1 = newItinerary(A, 0).bus(ROUTE, 1, 0, 50, B).bus(ROUTE, 1, 0, 50, C).build();
+    var result = service.getProducts(i1);
+    assertEquals(Set.of(regular), result.itineraryProducts());
+  }
+}

application/src/ext-test/java/org/opentripplanner/ext/fares/impl/GtfsFaresV2ServiceTest.java

@@ -36,8 +36,8 @@ class GtfsFaresV2ServiceTest implements PlanTestConstants {
   FeedScopedId LEG_GROUP4 = id("leg-group4");
   FeedScopedId LEG_GROUP5 = id("leg-group5");
   int ID = 100;
-  String expressNetwork = "express";
-  String localNetwork = "local";
+  FeedScopedId expressNetwork = id("express");
+  FeedScopedId localNetwork = id("local");
 
   FareProduct single = FareProduct.of(
     new FeedScopedId(FEED_ID, "single"),
@@ -99,8 +99,8 @@ class GtfsFaresV2ServiceTest implements PlanTestConstants {
   Place OUTER_ZONE_STOP = Place.forStop(
     testModel.stop("outer city stop").withCoordinate(2, 2).build()
   );
-  String INNER_ZONE = "inner-zone";
-  String OUTER_ZONE = "outer-zone";
+  FeedScopedId INNER_ZONE = id("inner-zone");
+  FeedScopedId OUTER_ZONE = id("outer-zone");
 
   GtfsFaresV2Service service = new GtfsFaresV2Service(
     List.of(

application/src/ext-test/java/org/opentripplanner/ext/fares/model/FareProductTest.java

@@ -57,13 +57,10 @@ void instanceId(FareProduct fareProduct, ZonedDateTime startTime, String expecte
   }
 
   private static FareProduct fareProduct(Duration duration, RiderCategory cat, FareMedium medium) {
-    return new FareProduct(
-      new FeedScopedId("fares", "daypass"),
-      "day pass",
-      Money.euros(10),
-      duration,
-      cat,
-      medium
-    );
+    return FareProduct.of(new FeedScopedId("fares", "daypass"), "day pass", Money.euros(10))
+      .withValidity(duration)
+      .withCategory(cat)
+      .withMedium(medium)
+      .build();
   }
 }

application/src/ext/java/org/opentripplanner/ext/fares/impl/AtlantaFareService.java

@@ -403,14 +403,11 @@ public ItineraryFare calculateFaresForType(
     for (ATLTransfer transfer : transfers) {
       cost = cost.plus(transfer.getTotal());
     }
-    var fareProduct = new FareProduct(
+    var fareProduct = FareProduct.of(
       new FeedScopedId(FEED_ID, fareType.name()),
       fareType.name(),
-      cost,
-      null,
-      null,
-      null
-    );
+      cost
+    ).build();
     var fare = ItineraryFare.empty();
     fare.addItineraryProducts(List.of(fareProduct));
     return fare;

application/src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceFactory.java

@@ -38,7 +38,7 @@ public class DefaultFareServiceFactory implements FareServiceFactory {
   private final List<FareTransferRule> fareTransferRules = new ArrayList<>();
 
   // mapping the stop ids to area ids. one stop can be in several areas.
-  private final Multimap<FeedScopedId, String> stopAreas = ArrayListMultimap.create();
+  private final Multimap<FeedScopedId, FeedScopedId> stopAreas = ArrayListMultimap.create();
 
   @Override
   public FareService makeFareService() {

application/src/ext/java/org/opentripplanner/ext/fares/impl/GtfsFaresV2Service.java

@@ -21,6 +21,7 @@
 import org.opentripplanner.model.plan.Itinerary;
 import org.opentripplanner.model.plan.Leg;
 import org.opentripplanner.model.plan.ScheduledTransitLeg;
+import org.opentripplanner.transit.model.basic.Distance;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.slf4j.Logger;
@@ -31,15 +32,15 @@ public final class GtfsFaresV2Service implements Serializable {
   private static final Logger LOG = LoggerFactory.getLogger(GtfsFaresV2Service.class);
   private final List<FareLegRule> legRules;
   private final List<FareTransferRule> transferRules;
-  private final Multimap<FeedScopedId, String> stopAreas;
-  private final Set<String> networksWithRules;
-  private final Set<String> fromAreasWithRules;
-  private final Set<String> toAreasWithRules;
+  private final Multimap<FeedScopedId, FeedScopedId> stopAreas;
+  private final Set<FeedScopedId> networksWithRules;
+  private final Set<FeedScopedId> fromAreasWithRules;
+  private final Set<FeedScopedId> toAreasWithRules;
 
   public GtfsFaresV2Service(
     List<FareLegRule> legRules,
     List<FareTransferRule> fareTransferRules,
-    Multimap<FeedScopedId, String> stopAreas
+    Multimap<FeedScopedId, FeedScopedId> stopAreas
   ) {
     this.legRules = legRules;
     this.transferRules = fareTransferRules;
@@ -71,14 +72,14 @@ public ProductResult getProducts(Itinerary itinerary) {
     return new ProductResult(coveringItinerary, allLegProducts);
   }
 
-  private static Set<String> findAreasWithRules(
+  private static Set<FeedScopedId> findAreasWithRules(
     List<FareLegRule> legRules,
-    Function<FareLegRule, String> getArea
+    Function<FareLegRule, FeedScopedId> getArea
   ) {
     return legRules.stream().map(getArea).filter(Objects::nonNull).collect(Collectors.toSet());
   }
 
-  private static Set<String> findNetworksWithRules(Collection<FareLegRule> legRules) {
+  private static Set<FeedScopedId> findNetworksWithRules(Collection<FareLegRule> legRules) {
     return legRules
       .stream()
       .map(FareLegRule::networkId)
@@ -134,10 +135,7 @@ private boolean coversItineraryWithFreeTransfers(
 
     return (
       feedIdsInItinerary.size() == 1 &&
-      pwt
-        .transferRules()
-        .stream()
-        .anyMatch(r -> r.fareProducts().stream().anyMatch(fp -> fp.price().isZero()))
+      pwt.transferRules().stream().anyMatch(FareTransferRule::isFree)
     );
   }
 
@@ -209,7 +207,11 @@ private Optional<FareLegRule> getFareLegRuleByGroupId(FeedScopedId groupId) {
     return legRules.stream().filter(lr -> groupId.equals(lr.legGroupId())).findAny();
   }
 
-  private boolean matchesArea(StopLocation stop, String areaId, Set<String> areasWithRules) {
+  private boolean matchesArea(
+    StopLocation stop,
+    FeedScopedId areaId,
+    Set<FeedScopedId> areasWithRules
+  ) {
     var stopAreas = this.stopAreas.get(stop.getId());
     return (
       (isNull(areaId) && stopAreas.stream().noneMatch(areasWithRules::contains)) ||
@@ -226,7 +228,7 @@ private boolean matchesNetworkId(ScheduledTransitLeg leg, FareLegRule rule) {
       .getRoute()
       .getGroupsOfRoutes()
       .stream()
-      .map(group -> group.getId().getId())
+      .map(group -> group.getId())
       .filter(Objects::nonNull)
       .toList();
 
@@ -241,15 +243,15 @@ private boolean matchesDistance(ScheduledTransitLeg leg, FareLegRule rule) {
     // If no valid distance type is given, do not consider distances in fare computation
 
     FareDistance distance = rule.fareDistance();
-    if (distance instanceof FareDistance.Stops ruleDistance) {
+    if (distance instanceof FareDistance.Stops(int min, int max)) {
       var numStops = leg.getIntermediateStops().size();
-      return numStops >= ruleDistance.min() && ruleDistance.max() >= numStops;
-    } else if (rule.fareDistance() instanceof FareDistance.LinearDistance ruleDistance) {
-      var ruleMax = ruleDistance.max();
-      var ruleMin = ruleDistance.min();
+      return numStops >= min && max >= numStops;
+    } else if (
+      rule.fareDistance() instanceof FareDistance.LinearDistance(Distance min, Distance max)
+    ) {
       var legDistance = leg.getDirectDistanceMeters();
 
-      return legDistance > ruleMin.toMeters() && legDistance < ruleMax.toMeters();
+      return legDistance > min.toMeters() && legDistance < max.toMeters();
     } else return true;
   }
 

application/src/ext/java/org/opentripplanner/ext/fares/impl/HighestFareInFreeTransferWindowFareService.java

@@ -86,14 +86,11 @@ protected ItineraryFare calculateFaresForType(
       currentTransferWindowCost = Money.max(currentTransferWindowCost, rideCost.orElse(zero));
     }
     cost = cost.plus(currentTransferWindowCost);
-    var fp = new FareProduct(
+    var fp = FareProduct.of(
       new FeedScopedId("fares", fareType.name()),
       fareType.name(),
-      cost,
-      null,
-      null,
-      null
-    );
+      cost
+    ).build();
     var fare = ItineraryFare.empty();
     if (cost.greaterThan(zero)) {
       fare.addItineraryProducts(List.of(fp));

application/src/ext/java/org/opentripplanner/ext/fares/impl/OrcaFareService.java

@@ -535,18 +535,19 @@ private static void addLegFareProduct(
       medium = CASH_MEDIUM;
     }
     var duration = Duration.ZERO;
-    var fareProduct = new FareProduct(id, "rideCost", totalFare, duration, riderCategory, medium);
+    var fareProduct = FareProduct.of(id, "rideCost", totalFare)
+      .withValidity(duration)
+      .withCategory(riderCategory)
+      .withMedium(medium)
+      .build();
     itineraryFare.addFareProduct(leg, fareProduct);
     // If a transfer was used, then also add a transfer fare product.
     if (transferDiscount.isPositive()) {
-      var transferFareProduct = new FareProduct(
-        id,
-        "transfer",
-        transferDiscount,
-        duration,
-        riderCategory,
-        medium
-      );
+      var transferFareProduct = FareProduct.of(id, "transfer", transferDiscount)
+        .withValidity(duration)
+        .withCategory(riderCategory)
+        .withMedium(medium)
+        .build();
       itineraryFare.addFareProduct(leg, transferFareProduct);
     }
   }

application/src/ext/java/org/opentripplanner/ext/fares/model/FareLegRule.java

@@ -6,13 +6,14 @@
 import javax.annotation.Nullable;
 import org.opentripplanner.model.fare.FareProduct;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
+import org.opentripplanner.utils.collection.CollectionUtils;
 
 public record FareLegRule(
   FeedScopedId id,
   @Nullable FeedScopedId legGroupId,
-  @Nullable String networkId,
-  @Nullable String fromAreaId,
-  @Nullable String toAreaId,
+  @Nullable FeedScopedId networkId,
+  @Nullable FeedScopedId fromAreaId,
+  @Nullable FeedScopedId toAreaId,
   @Nullable FareDistance fareDistance,
   Collection<FareProduct> fareProducts
 ) {

application/src/ext/java/org/opentripplanner/ext/fares/model/FareLegRuleBuilder.java

@@ -12,10 +12,10 @@ public class FareLegRuleBuilder {
   private final FeedScopedId id;
   private final Collection<FareProduct> fareProducts;
   private FeedScopedId legGroupId;
-  private String networkId;
-  private String fromAreaId;
+  private FeedScopedId networkId;
+  private FeedScopedId fromAreaId;
+  private FeedScopedId toAreaId;
   private FareDistance fareDistance = null;
-  private String toAreaId;
 
   public FareLegRuleBuilder(FeedScopedId id, Collection<FareProduct> products) {
     this.id = id;
@@ -27,12 +27,12 @@ public FareLegRuleBuilder withLegGroupId(FeedScopedId legGroupId) {
     return this;
   }
 
-  public FareLegRuleBuilder withNetworkId(String networkId) {
+  public FareLegRuleBuilder withNetworkId(FeedScopedId networkId) {
     this.networkId = networkId;
     return this;
   }
 
-  public FareLegRuleBuilder withFromAreaId(String fromAreaId) {
+  public FareLegRuleBuilder withFromAreaId(FeedScopedId fromAreaId) {
     this.fromAreaId = fromAreaId;
     return this;
   }
@@ -42,7 +42,7 @@ public FareLegRuleBuilder withFareDistance(FareDistance fareDistance) {
     return this;
   }
 
-  public FareLegRuleBuilder withToAreaId(String toAreaId) {
+  public FareLegRuleBuilder withToAreaId(FeedScopedId toAreaId) {
     this.toAreaId = toAreaId;
     return this;
   }

application/src/ext/java/org/opentripplanner/ext/fares/model/FareTransferRule.java

@@ -2,6 +2,8 @@
 
 import java.time.Duration;
 import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
 import javax.annotation.Nullable;
 import org.opentripplanner.model.fare.FareProduct;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
@@ -14,7 +16,18 @@ public record FareTransferRule(
   @Nullable Duration timeLimit,
   Collection<FareProduct> fareProducts
 ) {
+  public FareTransferRule {
+    Objects.requireNonNull(id);
+    fareProducts = List.copyOf(fareProducts);
+  }
   public String feedId() {
     return id.getFeedId();
   }
+
+  /**
+   * Returns true if this rule contains a free transfer product or an empty list of fare products.
+   */
+  public boolean isFree() {
+    return fareProducts.isEmpty() || fareProducts.stream().anyMatch(p -> p.price().isZero());
+  }
 }