Merge branch 'dev-2.x' of https://github.com/opentripplanner/OpenTripPlanner into group-area-from-designated-layer-builder

Author: eibakke

docs/Changelog.md

@@ -89,6 +89,7 @@ based on merged pull requests. Search GitHub issues and pull requests for smalle
 - Remove "fare" [#5645](https://github.com/opentripplanner/OpenTripPlanner/pull/5645)
 - Refactor GroupStopBuilder addLocation method [#5651](https://github.com/opentripplanner/OpenTripPlanner/pull/5651)
 - Remove `VehicleToStopHeuristics` [#5381](https://github.com/opentripplanner/OpenTripPlanner/pull/5381)
+- Set defaults of the modes WALK, even if one and not the others are set [#5675](https://github.com/opentripplanner/OpenTripPlanner/pull/5675)
 [](AUTOMATIC_CHANGELOG_PLACEHOLDER_DO_NOT_REMOVE)
 
 ## 2.4.0 (2023-09-13)

src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java

@@ -37,8 +37,8 @@ void changeNames() {
 
     filter.decorate(itinerary);
 
-    var updatedLeg = itinerary.getLegs().get(0);
-    assertEquals(STOP_D.getName(), updatedLeg.getFrom().name);
+    var updatedLeg = itinerary.getLegs().getFirst();
+    assertEquals(STOP_C.getName(), updatedLeg.getFrom().name);
     assertEquals(STOP_D.getName(), updatedLeg.getTo().name);
   }
 }

src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java

@@ -7,9 +7,9 @@
 import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryDecorator;
 
 /**
- * A decorating filter that checks if a transit leg contains any primary stops and if it does,
- * then replaces it with the secondary, agency-specific stop name. This is so that the in-vehicle
- * display matches what OTP returns as a board/alight stop name.
+ * A decorating filter that checks if a transit leg contains any consolidated stops and if it does,
+ * then replaces it with the appropriate, agency-specific stop name. This is so that the physical
+ * signage and in-vehicle display matches what OTP returns as a board/alight stop name.
  */
 public class DecorateConsolidatedStopNames implements ItineraryDecorator {
 
@@ -21,31 +21,45 @@ public DecorateConsolidatedStopNames(StopConsolidationService service) {
 
   @Override
   public void decorate(Itinerary itinerary) {
-    replacePrimaryNamesWithSecondary(itinerary);
+    replaceConsolidatedStops(itinerary);
   }
 
   /**
-   * If the itinerary has a from/to that is the primary stop of a {@link org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup}
-   * then we replace its name with the secondary name of the agency that is
-   * operating the route, so that the name in the result matches the name in the in-vehicle
-   * display.
+   * If the itinerary has a "from" stop that is the secondary stop of a
+   * {@link org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup}
+   * then we replace its name with the primary name of the agency that is
+   * operating the route, so that the name in the result matches the physical signage on the stop.
+   * <p>
+   * If the leg has a "to" stop that is a primary stop, then we don't want to show the stop that's on
+   * the signage but what is shown _inside_ the vehicle. That's why we use the agency-specific (aka
+   * secondary) stop.
+   * <p>
+   * This follows the somewhat idiosyncratic logic of the consolidated stops feature.
    */
-  private void replacePrimaryNamesWithSecondary(Itinerary i) {
+  private void replaceConsolidatedStops(Itinerary i) {
     i.transformTransitLegs(leg -> {
       if (leg instanceof ScheduledTransitLeg stl && needsToRenameStops(stl)) {
         var agency = leg.getAgency();
-        return new ConsolidatedStopLeg(
-          stl,
-          service.agencySpecificName(stl.getFrom().stop, agency),
-          service.agencySpecificName(stl.getTo().stop, agency)
-        );
+        // to show the name on the stop signage we use the primary stop's name
+        var from = service.primaryStop(stl.getFrom().stop.getId());
+        // to show the name that's on the display inside the vehicle we use the agency-specific name
+        var to = service.agencySpecificStop(stl.getTo().stop, agency);
+        return new ConsolidatedStopLeg(stl, from, to);
       } else {
         return leg;
       }
     });
   }
 
+  /**
+   * Figures out if the from/to stops are part of a consolidated stop group and therefore
+   * some stops need to be replaced.
+   * <p>
+   * Please consult the Javadoc of {@link DecorateConsolidatedStopNames#replaceConsolidatedStops(Itinerary)}
+   * for details of this idiosyncratic business logic and in particular why the logic is not the same
+   * for the from/to stops.
+   */
   private boolean needsToRenameStops(ScheduledTransitLeg stl) {
-    return (service.isPrimaryStop(stl.getFrom().stop) || service.isPrimaryStop(stl.getTo().stop));
+    return (service.isSecondaryStop(stl.getFrom().stop) || service.isPrimaryStop(stl.getTo().stop));
   }
 }

src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java

@@ -2,7 +2,6 @@
 
 import java.util.List;
 import org.opentripplanner.ext.stopconsolidation.model.StopReplacement;
-import org.opentripplanner.framework.i18n.I18NString;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.organization.Agency;
 import org.opentripplanner.transit.model.site.StopLocation;
@@ -24,13 +23,23 @@ public interface StopConsolidationService {
    */
   boolean isPrimaryStop(StopLocation stop);
 
+  /**
+   * Is the given stop a secondary stop as defined by the stop consolidation configuration?
+   */
+  boolean isSecondaryStop(StopLocation stop);
+
   /**
    * Are any stop consolidations defined?
    */
   boolean isActive();
 
   /**
-   * For a given primary stop look up the name as it was originally defined in the agency's feed.
+   * For a given primary stop look up secondary feed as it was originally defined in the agency's feed.
+   */
+  StopLocation agencySpecificStop(StopLocation stop, Agency agency);
+
+  /**
+   * For a given stop id return the primary stop if it is part of a consolidated stop group.
    */
-  I18NString agencySpecificName(StopLocation stop, Agency agency);
+  StopLocation primaryStop(FeedScopedId id);
 }

src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java

@@ -2,14 +2,15 @@
 
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Stream;
+import javax.annotation.Nonnull;
 import org.opentripplanner.ext.stopconsolidation.StopConsolidationRepository;
 import org.opentripplanner.ext.stopconsolidation.StopConsolidationService;
+import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup;
 import org.opentripplanner.ext.stopconsolidation.model.StopReplacement;
-import org.opentripplanner.framework.i18n.I18NString;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.organization.Agency;
-import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.service.TransitModel;
 import org.slf4j.Logger;
@@ -61,26 +62,46 @@ public boolean isPrimaryStop(StopLocation stop) {
     return repo.groups().stream().anyMatch(r -> r.primary().equals(stop.getId()));
   }
 
+  @Override
+  public boolean isSecondaryStop(StopLocation stop) {
+    return repo.groups().stream().anyMatch(r -> r.secondaries().contains(stop.getId()));
+  }
+
   @Override
   public boolean isActive() {
     return !repo.groups().isEmpty();
   }
 
   @Override
-  public I18NString agencySpecificName(StopLocation stop, Agency agency) {
+  public StopLocation agencySpecificStop(StopLocation stop, Agency agency) {
     if (agency.getId().getFeedId().equals(stop.getId().getFeedId())) {
-      return stop.getName();
+      return stop;
     } else {
-      return repo
-        .groups()
-        .stream()
-        .filter(r -> r.primary().equals(stop.getId()))
-        .flatMap(g -> g.secondaries().stream())
-        .filter(secondary -> secondary.getFeedId().equals(agency.getId().getFeedId()))
-        .findAny()
-        .map(id -> transitModel.getStopModel().getRegularStop(id))
-        .map(RegularStop::getName)
-        .orElseGet(stop::getName);
+      return findAgencySpecificStop(stop, agency).orElse(stop);
     }
   }
+
+  @Nonnull
+  private Optional<StopLocation> findAgencySpecificStop(StopLocation stop, Agency agency) {
+    return repo
+      .groups()
+      .stream()
+      .filter(r -> r.primary().equals(stop.getId()))
+      .flatMap(g -> g.secondaries().stream())
+      .filter(secondary -> secondary.getFeedId().equals(agency.getId().getFeedId()))
+      .findAny()
+      .map(id -> transitModel.getStopModel().getRegularStop(id));
+  }
+
+  @Override
+  public StopLocation primaryStop(FeedScopedId id) {
+    var primaryId = repo
+      .groups()
+      .stream()
+      .filter(g -> g.secondaries().contains(id))
+      .map(ConsolidatedStopGroup::primary)
+      .findAny()
+      .orElse(id);
+    return transitModel.getStopModel().getRegularStop(primaryId);
+  }
 }

src/ext/java/org/opentripplanner/ext/stopconsolidation/model/ConsolidatedStopLeg.java

@@ -1,28 +1,28 @@
 package org.opentripplanner.ext.stopconsolidation.model;
 
-import org.opentripplanner.framework.i18n.I18NString;
 import org.opentripplanner.model.plan.Place;
 import org.opentripplanner.model.plan.ScheduledTransitLeg;
 import org.opentripplanner.model.plan.ScheduledTransitLegBuilder;
+import org.opentripplanner.transit.model.site.StopLocation;
 
 public class ConsolidatedStopLeg extends ScheduledTransitLeg {
 
-  private final I18NString fromName;
-  private final I18NString toName;
+  private final StopLocation from;
+  private final StopLocation to;
 
-  public ConsolidatedStopLeg(ScheduledTransitLeg original, I18NString fromName, I18NString toName) {
+  public ConsolidatedStopLeg(ScheduledTransitLeg original, StopLocation from, StopLocation to) {
     super(new ScheduledTransitLegBuilder<>(original));
-    this.fromName = fromName;
-    this.toName = toName;
+    this.from = from;
+    this.to = to;
   }
 
   @Override
   public Place getFrom() {
-    return Place.forStop(super.getFrom().stop, fromName);
+    return Place.forStop(from);
   }
 
   @Override
   public Place getTo() {
-    return Place.forStop(super.getTo().stop, toName);
+    return Place.forStop(to);
   }
 }

src/main/java/org/opentripplanner/apis/transmodel/mapping/RequestModesMapper.java

@@ -7,23 +7,30 @@
 
 class RequestModesMapper {
 
+  private static final String accessModeKey = "accessMode";
+  private static final String egressModeKey = "egressMode";
+  private static final String directModeKey = "directMode";
+
   /**
-   * Maps a GraphQL Modes input type to a RequestModes.
-   *
-   * This only maps access, egress, direct & transfer.
-   * Transport modes are now part of filters.
-   * Only in case filters are not present we will use this mapping
+   * Maps GraphQL Modes input type to RequestModes.
+   * <p>
+   * This only maps access, egress, direct & transfer modes. Transport modes are set using filters.
+   * Default modes are WALK for access, egress, direct & transfer.
    */
-  @SuppressWarnings("unchecked")
   static RequestModes mapRequestModes(Map<String, ?> modesInput) {
-    StreetMode accessMode = (StreetMode) modesInput.get("accessMode");
-    RequestModesBuilder mBuilder = RequestModes
-      .of()
-      .withAccessMode(accessMode)
-      .withEgressMode((StreetMode) modesInput.get("egressMode"))
-      .withDirectMode((StreetMode) modesInput.get("directMode"));
+    RequestModesBuilder mBuilder = RequestModes.of();
 
-    mBuilder.withTransferMode(accessMode == StreetMode.BIKE ? StreetMode.BIKE : StreetMode.WALK);
+    if (modesInput.containsKey(accessModeKey)) {
+      StreetMode accessMode = (StreetMode) modesInput.get(accessModeKey);
+      mBuilder.withAccessMode(accessMode);
+      mBuilder.withTransferMode(accessMode == StreetMode.BIKE ? StreetMode.BIKE : StreetMode.WALK);
+    }
+    if (modesInput.containsKey(egressModeKey)) {
+      mBuilder.withEgressMode((StreetMode) modesInput.get(egressModeKey));
+    }
+    if (modesInput.containsKey(directModeKey)) {
+      mBuilder.withDirectMode((StreetMode) modesInput.get(directModeKey));
+    }
 
     return mBuilder.build();
   }

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

@@ -90,11 +90,7 @@ public static Place normal(Vertex vertex, I18NString name) {
   }
 
   public static Place forStop(StopLocation stop) {
-    return forStop(stop, stop.getName());
-  }
-
-  public static Place forStop(StopLocation stop, I18NString nameOverride) {
-    return new Place(nameOverride, stop.getCoordinate(), VertexType.TRANSIT, stop, null, null);
+    return new Place(stop.getName(), stop.getCoordinate(), VertexType.TRANSIT, stop, null, null);
   }
 
   public static Place forFlexStop(StopLocation stop, Vertex vertex) {

src/main/java/org/opentripplanner/transit/model/site/GroupStop.java

@@ -94,13 +94,12 @@ public Geometry getGeometry() {
 
   /**
    * Returns the geometry of the area that encompasses the bounds of this StopLocation group. If the
-   * group is defined only as a list of stops, this will return the same as getGeometry. If on the
-   * other hand the group is defined as an area and the stops are inferred from that area, then this
-   * will return the geometry of the area.
+   * group is defined as all the stops within an area, then this will return the geometry of the
+   * area. If the group is defined simply as a list of stops, this will return an empty optional.
    */
   @Override
   public Optional<? extends Geometry> getEncompassingAreaGeometry() {
-    return Optional.ofNullable(encompassingAreaGeometry).or(() -> Optional.of(geometry));
+    return Optional.ofNullable(encompassingAreaGeometry);
   }
 
   @Override

src/test/java/org/opentripplanner/apis/transmodel/mapping/RequestModesMapperTest.java

@@ -0,0 +1,57 @@
+package org.opentripplanner.apis.transmodel.mapping;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.routing.api.request.RequestModes;
+import org.opentripplanner.routing.api.request.StreetMode;
+
+class RequestModesMapperTest {
+
+  @Test
+  void testMapRequestModesEmptyMapReturnsDefaults() {
+    Map<String, StreetMode> inputModes = Map.of();
+
+    RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);
+
+    assertEquals(RequestModes.of().build(), mappedModes);
+  }
+
+  @Test
+  void testMapRequestModesAccessSetReturnsDefaultsForOthers() {
+    Map<String, StreetMode> inputModes = Map.of("accessMode", StreetMode.BIKE);
+
+    RequestModes wantModes = RequestModes
+      .of()
+      .withAccessMode(StreetMode.BIKE)
+      .withTransferMode(StreetMode.BIKE)
+      .build();
+
+    RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);
+
+    assertEquals(wantModes, mappedModes);
+  }
+
+  @Test
+  void testMapRequestModesEgressSetReturnsDefaultsForOthers() {
+    Map<String, StreetMode> inputModes = Map.of("egressMode", StreetMode.CAR);
+
+    RequestModes wantModes = RequestModes.of().withEgressMode(StreetMode.CAR).build();
+
+    RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);
+
+    assertEquals(wantModes, mappedModes);
+  }
+
+  @Test
+  void testMapRequestModesDirectSetReturnsDefaultsForOthers() {
+    Map<String, StreetMode> inputModes = Map.of("directMode", StreetMode.CAR);
+
+    RequestModes wantModes = RequestModes.of().withDirectMode(StreetMode.CAR).build();
+
+    RequestModes mappedModes = RequestModesMapper.mapRequestModes(inputModes);
+
+    assertEquals(wantModes, mappedModes);
+  }
+}