Merge remote-tracking branch 'upstream/dev-2.x' into renovate/jackson…

….version

docs/Changelog.md

@@ -42,6 +42,8 @@ based on merged pull requests. Search GitHub issues and pull requests for smalle
 - Add back walk-reluctance in Transmodel API [#5471](https://github.com/opentripplanner/OpenTripPlanner/pull/5471)
 - Make `feedId` required for realtime updaters [#5502](https://github.com/opentripplanner/OpenTripPlanner/pull/5502)
 - Fix serialization of `AtomicInteger` [#5508](https://github.com/opentripplanner/OpenTripPlanner/pull/5508)
+- Improve linking of fixed stops used by flex trips [#5503](https://github.com/opentripplanner/OpenTripPlanner/pull/5503)
+- Keep min transfer filter is not local to group-by-filters [#5436](https://github.com/opentripplanner/OpenTripPlanner/pull/5436)
 [](AUTOMATIC_CHANGELOG_PLACEHOLDER_DO_NOT_REMOVE)
 
 ## 2.4.0 (2023-09-13)

pom.xml

@@ -951,7 +951,7 @@
         <dependency>
             <groupId>org.apache.commons</groupId>
             <artifactId>commons-compress</artifactId>
-            <version>1.24.0</version>
+            <version>1.25.0</version>
             <scope>test</scope>
         </dependency>
     </dependencies>

src/ext-test/java/org/opentripplanner/ext/flex/FlexIntegrationTest.java

@@ -23,7 +23,7 @@
 import org.opentripplanner.framework.application.OTPFeature;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
 import org.opentripplanner.graph_builder.module.DirectTransferGenerator;
-import org.opentripplanner.graph_builder.module.StreetLinkerModule;
+import org.opentripplanner.graph_builder.module.TestStreetLinkerModule;
 import org.opentripplanner.gtfs.graphbuilder.GtfsBundle;
 import org.opentripplanner.gtfs.graphbuilder.GtfsModule;
 import org.opentripplanner.model.GenericLocation;
@@ -34,7 +34,6 @@
 import org.opentripplanner.routing.api.request.RouteRequest;
 import org.opentripplanner.routing.api.request.request.filter.AllowAllTransitFilter;
 import org.opentripplanner.routing.graph.Graph;
-import org.opentripplanner.test.support.ResourceLoader;
 import org.opentripplanner.transit.service.TransitModel;
 
 /**
@@ -186,7 +185,7 @@ private static void addGtfsToGraph(Graph graph, TransitModel transitModel, List<
     gtfsModule.buildGraph();
 
     // link stations to streets
-    StreetLinkerModule.linkStreetsForTestOnly(graph, transitModel);
+    TestStreetLinkerModule.link(graph, transitModel);
 
     // link flex locations to streets
     new AreaStopsToVerticesMapper(graph, transitModel).buildGraph();

src/ext/java/org/opentripplanner/ext/transmodelapi/model/framework/SystemNoticeType.java

@@ -28,15 +28,15 @@ public static GraphQLObjectType create() {
           .newFieldDefinition()
           .name("tag")
           .type(Scalars.GraphQLString)
-          .dataFetcher(env -> ((SystemNotice) env.getSource()).tag)
+          .dataFetcher(env -> ((SystemNotice) env.getSource()).tag())
           .build()
       )
       .field(
         GraphQLFieldDefinition
           .newFieldDefinition()
           .name("text")
           .type(Scalars.GraphQLString)
-          .dataFetcher(env -> ((SystemNotice) env.getSource()).text)
+          .dataFetcher(env -> ((SystemNotice) env.getSource()).text())
           .build()
       )
       .build();

src/main/java/org/opentripplanner/api/mapping/SystemNoticeMapper.java

@@ -24,6 +24,6 @@ public static List<ApiSystemNotice> mapSystemNotices(Collection<SystemNotice> do
   }
 
   public static ApiSystemNotice mapSystemNotice(SystemNotice domain) {
-    return new ApiSystemNotice(domain.tag, domain.text);
+    return new ApiSystemNotice(domain.tag(), domain.text());
   }
 }

src/main/java/org/opentripplanner/apis/gtfs/datafetchers/SystemNoticeImpl.java

@@ -9,12 +9,12 @@ public class SystemNoticeImpl implements GraphQLDataFetchers.GraphQLSystemNotice
 
   @Override
   public DataFetcher<String> tag() {
-    return environment -> getSource(environment).tag;
+    return environment -> getSource(environment).tag();
   }
 
   @Override
   public DataFetcher<String> text() {
-    return environment -> getSource(environment).text;
+    return environment -> getSource(environment).text();
   }
 
   private SystemNotice getSource(DataFetchingEnvironment environment) {

src/main/java/org/opentripplanner/framework/collection/CollectionUtils.java

@@ -0,0 +1,35 @@
+package org.opentripplanner.framework.collection;
+
+import java.util.Collection;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+
+public class CollectionUtils {
+
+  /**
+   * A null-safe version of toString() for a collections.
+   * <p>
+   * If the collection is {@code null} the given {@code nullText} is returned.
+   * <p>
+   * All elements are also converted to a sting using the {@code toString()} method or if
+   * {@code null} to the given {@code nullText}.
+   * <p>
+   * If the collection is a set, but not a SortedSet then the elements are sorted. This is done
+   * to return the elements in a deterministic manner, which is important if this is used in
+   * for example a unit-test.
+   * <p>
+   * The final result string examples: {@code "[]", "[a]", "[a, b, c]"}
+   */
+  public static <T> String toString(@Nullable Collection<T> c, String nullText) {
+    if (c == null) {
+      return nullText;
+    }
+    var stream = c.stream().map(it -> it == null ? nullText : it.toString());
+    if (c instanceof Set && !(c instanceof SortedSet<T>)) {
+      stream = stream.sorted();
+    }
+    return stream.collect(Collectors.joining(", ", "[", "]"));
+  }
+}

src/main/java/org/opentripplanner/graph_builder/module/StreetLinkerModule.java

@@ -4,6 +4,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
+import javax.annotation.Nonnull;
 import org.opentripplanner.framework.application.OTPFeature;
 import org.opentripplanner.framework.logging.ProgressTracker;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
@@ -13,10 +14,12 @@
 import org.opentripplanner.routing.linking.LinkingDirection;
 import org.opentripplanner.routing.vehicle_parking.VehicleParking;
 import org.opentripplanner.routing.vehicle_parking.VehicleParkingHelper;
+import org.opentripplanner.street.model.edge.Edge;
 import org.opentripplanner.street.model.edge.StreetTransitEntranceLink;
 import org.opentripplanner.street.model.edge.StreetTransitStopLink;
 import org.opentripplanner.street.model.edge.StreetVehicleParkingLink;
 import org.opentripplanner.street.model.edge.VehicleParkingEdge;
+import org.opentripplanner.street.model.vertex.StreetVertex;
 import org.opentripplanner.street.model.vertex.TransitEntranceVertex;
 import org.opentripplanner.street.model.vertex.TransitStopVertex;
 import org.opentripplanner.street.model.vertex.VehicleParkingEntranceVertex;
@@ -33,12 +36,14 @@
  * {@link GraphBuilderModule} plugin that links various
  * objects in the graph to the street network. It should be run after both the transit network and
  * street network are loaded. It links four things: transit stops, transit entrances, bike rental
- * stations, and bike parks. Therefore it should be run even when there's no GTFS data present to
+ * stations, and bike parks. Therefore, it should be run even when there's no GTFS data present to
  * make bike rental services and bike parks usable.
  */
 public class StreetLinkerModule implements GraphBuilderModule {
 
   private static final Logger LOG = LoggerFactory.getLogger(StreetLinkerModule.class);
+  private static final TraverseModeSet CAR_ONLY = new TraverseModeSet(TraverseMode.CAR);
+  private static final TraverseModeSet WALK_ONLY = new TraverseModeSet(TraverseMode.WALK);
   private final Graph graph;
   private final TransitModel transitModel;
   private final DataImportIssueStore issueStore;
@@ -56,11 +61,6 @@ public StreetLinkerModule(
     this.addExtraEdgesToAreas = addExtraEdgesToAreas;
   }
 
-  /** For test only */
-  public static void linkStreetsForTestOnly(Graph graph, TransitModel model) {
-    new StreetLinkerModule(graph, model, DataImportIssueStore.NOOP, false).buildGraph();
-  }
-
   @Override
   public void buildGraph() {
     transitModel.index();
@@ -103,48 +103,89 @@ public void linkTransitStops(Graph graph, TransitModel transitModel) {
     }
 
     for (TransitStopVertex tStop : vertices) {
-      // Stops with pathways do not need to be connected to the street network, since there are explicit entraces defined for that
+      // Stops with pathways do not need to be connected to the street network, since there are explicit entrances defined for that
       if (tStop.hasPathways()) {
         continue;
       }
-      // check if stop is already linked, to allow multiple linking cycles
-      if (tStop.getDegreeOut() + tStop.getDegreeIn() > 0) {
+      // check if stop is already linked, to allow multiple idempotent linking cycles
+      if (tStop.isConnectedToGraph()) {
         continue;
       }
-      TraverseModeSet modes = new TraverseModeSet(TraverseMode.WALK);
 
-      if (OTPFeature.FlexRouting.isOn()) {
-        // If regular stops are used for flex trips, they also need to be connected to car routable
-        // street edges.
-        if (stopLocationsUsedForFlexTrips.contains(tStop.getStop())) {
-          modes = new TraverseModeSet(TraverseMode.WALK, TraverseMode.CAR);
-        }
+      // ordinarily stops only need to be accessible by foot
+      StopLinkType linkType = StopLinkType.WALK_ONLY;
+
+      if (
+        OTPFeature.FlexRouting.isOn() && stopLocationsUsedForFlexTrips.contains(tStop.getStop())
+      ) {
+        linkType = StopLinkType.WALK_AND_CAR;
       }
 
-      graph
-        .getLinker()
-        .linkVertexPermanently(
-          tStop,
-          modes,
-          LinkingDirection.BOTH_WAYS,
-          (vertex, streetVertex) ->
-            List.of(
-              StreetTransitStopLink.createStreetTransitStopLink(
-                (TransitStopVertex) vertex,
-                streetVertex
-              ),
-              StreetTransitStopLink.createStreetTransitStopLink(
-                streetVertex,
-                (TransitStopVertex) vertex
-              )
-            )
-        );
+      linkStopToStreetNetwork(tStop, linkType);
+
       //noinspection Convert2MethodRef
       progress.step(m -> LOG.info(m));
     }
     LOG.info(progress.completeMessage());
   }
 
+  /**
+   * Link a stop to the nearest "relevant" edges.
+   * <p>
+   * These are mostly walk edges but if a stop is used by a flex pattern it also needs to be
+   * car-accessible. Therefore, flex stops are ensured to be connected to the car-accessible
+   * edge. This may lead to several links being created.
+   */
+  private void linkStopToStreetNetwork(TransitStopVertex tStop, StopLinkType linkType) {
+    graph
+      .getLinker()
+      .linkVertexPermanently(
+        tStop,
+        WALK_ONLY,
+        LinkingDirection.BOTH_WAYS,
+        (transitVertex, streetVertex) -> {
+          var linkEdges = createStopLinkEdges((TransitStopVertex) transitVertex, streetVertex);
+
+          if (linkType == StopLinkType.WALK_AND_CAR && !streetVertex.isConnectedToDriveableEdge()) {
+            linkToDriveableEdge(tStop);
+          }
+
+          return linkEdges;
+        }
+      );
+  }
+
+  /**
+   * If regular stops or group stops are used for flex trips, they also need to be connected to car
+   * routable street edges.
+   * <p>
+   * This does not apply to zones as street vertices store which zones they are part of.
+   *
+   * @see https://github.com/opentripplanner/OpenTripPlanner/issues/5498
+   */
+  private void linkToDriveableEdge(TransitStopVertex tStop) {
+    graph
+      .getLinker()
+      .linkVertexPermanently(
+        tStop,
+        CAR_ONLY,
+        LinkingDirection.BOTH_WAYS,
+        (transitVertex, streetVertex) ->
+          createStopLinkEdges((TransitStopVertex) transitVertex, streetVertex)
+      );
+  }
+
+  @Nonnull
+  private static List<Edge> createStopLinkEdges(
+    TransitStopVertex vertex,
+    StreetVertex streetVertex
+  ) {
+    return List.of(
+      StreetTransitStopLink.createStreetTransitStopLink(vertex, streetVertex),
+      StreetTransitStopLink.createStreetTransitStopLink(streetVertex, vertex)
+    );
+  }
+
   private static void linkVehicleParkingWithLinker(
     Graph graph,
     VehicleParkingEntranceVertex vehicleParkingVertex
@@ -274,7 +315,7 @@ private boolean vehicleParkingEntranceHasLinks(
   }
 
   /**
-   * Removes vehicle parking entrace vertex from graph.
+   * Removes vehicle parking entrance vertex from graph.
    *
    * @return vehicle parking for removal if the removed entrance was its only entrance.
    */
@@ -313,4 +354,17 @@ private VehicleParking removeVehicleParkingEntranceVertexFromGraph(
       return null;
     }
   }
+
+  private enum StopLinkType {
+    /**
+     * Only ensure that the link leads to a walkable edge.
+     * (The same edge may also be drivable but this is not guaranteed.)
+     */
+    WALK_ONLY,
+    /**
+     * Make sure that the stop is linked to an edge each that is walkable and drivable.
+     * This may lead to several links being created.
+     */
+    WALK_AND_CAR,
+  }
 }

src/main/java/org/opentripplanner/model/SystemNotice.java

@@ -19,18 +19,26 @@ public class SystemNotice {
   /**
    * An id or code identifying the notice. Use a descriptive tag like: 'transit-walking-filter'.
    */
-  public final String tag;
+  private final String tag;
 
   /**
    * An english text explaining why the element is tagged, and/or what the tag means.
    */
-  public final String text;
+  private final String text;
 
   public SystemNotice(String tag, String text) {
     this.tag = tag;
     this.text = text;
   }
 
+  public String tag() {
+    return tag;
+  }
+
+  public String text() {
+    return text;
+  }
+
   @Override
   public String toString() {
     return ToStringBuilder

src/main/java/org/opentripplanner/model/plan/Itinerary.java

@@ -6,6 +6,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
@@ -189,8 +190,8 @@ public void flagForDeletion(SystemNotice notice) {
   /**
    * Remove all deletion flags of this itinerary, in effect undeleting it from the result.
    */
-  public void removeDeletionFlags() {
-    systemNotices.clear();
+  public void removeDeletionFlags(Set<String> removeTags) {
+    systemNotices.removeIf(it -> removeTags.contains(it.tag()));
   }
 
   public boolean isFlaggedForDeletion() {
@@ -202,7 +203,7 @@ public boolean isFlaggedForDeletion() {
    * given {@code tag}.
    */
   public boolean hasSystemNoticeTag(String tag) {
-    return systemNotices.stream().map(n -> n.tag).anyMatch(tag::equals);
+    return systemNotices.stream().map(SystemNotice::tag).anyMatch(tag::equals);
   }
 
   public Itinerary withTimeShiftToStartAt(ZonedDateTime afterTime) {

src/main/java/org/opentripplanner/model/plan/ScheduledTransitLeg.java

@@ -8,6 +8,7 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -75,7 +76,7 @@ public class ScheduledTransitLeg implements TransitLeg {
     this.endTime = builder.endTime();
 
     this.serviceDate = builder.serviceDate();
-    this.zoneId = builder.zoneId();
+    this.zoneId = Objects.requireNonNull(builder.zoneId());
 
     this.tripOnServiceDate = builder.tripOnServiceDate();