Fix consolidation stop returning original stop ID.

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

@@ -57,7 +57,9 @@ void changeNames() {
 
     var updatedLeg = itinerary.getLegs().getFirst();
     assertEquals(STOP_C.getName(), updatedLeg.getFrom().name);
+    assertEquals(STOP_C.getId(), updatedLeg.getFrom().stop.getId());
     assertEquals(STOP_D.getName(), updatedLeg.getTo().name);
+    assertEquals(STOP_C.getId(), updatedLeg.getTo().stop.getId());
 
     // Check that the fares were carried over
     assertEquals(fpu, updatedLeg.fareProducts());

application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModuleTest.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.assertTrue;
 import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.PATTERN;
 import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.STOP_A;
 import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.STOP_B;
@@ -29,6 +30,7 @@ void replacePatterns() {
     var modifiedPattern = transitModel.getTripPatternForId(PATTERN.getId());
     assertFalse(modifiedPattern.getRoutingTripPattern().getPattern().sameAs(PATTERN));
     assertFalse(modifiedPattern.sameAs(PATTERN));
+    assertTrue(modifiedPattern.getScheduledTimetable().getPattern().sameAs(modifiedPattern));
 
     var modifiedStop = modifiedPattern
       .getRoutingTripPattern()

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

@@ -31,8 +31,9 @@ public void decorate(Itinerary itinerary) {
    * 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.
+   * the signage but what is shown _inside_ the vehicle. However, we still want to use the primary ID
+   * for stop information and alternatives legs. So we copy the agency-specific (aka secondary) stop
+   * name to the primary stop object.
    * <p>
    * This follows the somewhat idiosyncratic logic of the consolidated stops feature.
    */

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

@@ -8,8 +8,13 @@
 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.AreaStop;
+import org.opentripplanner.transit.model.site.GroupStop;
+import org.opentripplanner.transit.model.site.RegularStop;
+import org.opentripplanner.transit.model.site.RegularStopBuilder;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.service.TransitModel;
 import org.slf4j.Logger;
@@ -93,7 +98,24 @@ private Optional<StopLocation> findAgencySpecificStop(StopLocation stop, Agency
       .flatMap(g -> g.secondaries().stream())
       .filter(secondary -> secondary.getFeedId().equals(agency.getId().getFeedId()))
       .findAny()
-      .map(id -> transitModel.getStopModel().getRegularStop(id));
+      .map(id -> copyStopWithName(stop, transitModel.getStopModel().getRegularStop(id).getName()));
+  }
+
+  private StopLocation copyStopWithName(StopLocation primaryStop, I18NString agencySpecificName) {
+    if (primaryStop instanceof RegularStop) {
+      return ((RegularStop) primaryStop).copy().withName(agencySpecificName).build();
+    }
+
+    if (primaryStop instanceof AreaStop) {
+      return ((AreaStop) primaryStop).copy().withName(agencySpecificName).build();
+    }
+
+    if (primaryStop instanceof GroupStop) {
+      return ((GroupStop) primaryStop).copy().withName(agencySpecificName).build();
+    }
+
+    // should not happen unless new stop types are added
+    throw new IllegalArgumentException("Unknown stop location type: " + primaryStop);
   }
 
   @Override

application/src/main/java/org/opentripplanner/model/Timetable.java

@@ -20,6 +20,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import javax.annotation.Nullable;
 import org.opentripplanner.framework.time.ServiceDateUtils;
@@ -483,4 +484,13 @@ private static TripTimes getRepresentativeTripTimes(
       return null;
     }
   }
+
+  public boolean sameAs(Timetable other) {
+    return (
+      Objects.equals(pattern, other.pattern) &&
+      Objects.equals(tripTimes, other.tripTimes) &&
+      Objects.equals(frequencyEntries, other.frequencyEntries) &&
+      Objects.equals(serviceDate, other.serviceDate)
+    );
+  }
 }

application/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java

@@ -128,17 +128,7 @@ public final class TripPattern
     this.netexSubMode = requireNonNull(builder.getNetexSubmode());
     this.containsMultipleModes = builder.getContainsMultipleModes();
 
-    if (builder.getScheduledTimetable() != null) {
-      if (builder.getScheduledTimetableBuilder() != null) {
-        throw new IllegalArgumentException(
-          "Cannot provide both scheduled timetable and scheduled timetable builder"
-        );
-      }
-      this.scheduledTimetable = builder.getScheduledTimetable();
-    } else {
-      this.scheduledTimetable =
-        builder.getScheduledTimetableBuilder().withTripPattern(this).build();
-    }
+    this.scheduledTimetable = builder.getScheduledTimetableBuilder().withTripPattern(this).build();
 
     this.originalTripPattern = builder.getOriginalTripPattern();
 
@@ -469,7 +459,7 @@ public boolean sameAs(TripPattern other) {
       Objects.equals(this.containsMultipleModes, other.containsMultipleModes) &&
       Objects.equals(this.name, other.name) &&
       Objects.equals(this.stopPattern, other.stopPattern) &&
-      Objects.equals(this.scheduledTimetable, other.scheduledTimetable)
+      this.scheduledTimetable.sameAs(other.scheduledTimetable)
     );
   }
 

application/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java

@@ -28,7 +28,6 @@ public final class TripPatternBuilder
   private SubMode netexSubMode;
   private boolean containsMultipleModes;
   private StopPattern stopPattern;
-  private Timetable scheduledTimetable;
   private TimetableBuilder scheduledTimetableBuilder;
   private String name;
 
@@ -50,7 +49,7 @@ public final class TripPatternBuilder
     this.netexSubMode = original.getNetexSubmode();
     this.containsMultipleModes = original.getContainsMultipleModes();
     this.stopPattern = original.getStopPattern();
-    this.scheduledTimetable = original.getScheduledTimetable();
+    this.scheduledTimetableBuilder = original.getScheduledTimetable().copyOf();
     this.createdByRealtimeUpdate = original.isCreatedByRealtimeUpdater();
     this.originalTripPattern = original.getOriginalTripPattern();
     this.hopGeometries =
@@ -93,24 +92,13 @@ public TripPatternBuilder withStopPattern(StopPattern stopPattern) {
   }
 
   public TripPatternBuilder withScheduledTimeTable(Timetable scheduledTimetable) {
-    if (scheduledTimetableBuilder != null) {
-      throw new IllegalStateException(
-        "Cannot set scheduled Timetable after scheduled Timetable builder is created"
-      );
-    }
-    this.scheduledTimetable = scheduledTimetable;
+    this.scheduledTimetableBuilder = scheduledTimetable.copyOf();
     return this;
   }
 
   public TripPatternBuilder withScheduledTimeTableBuilder(
     UnaryOperator<TimetableBuilder> producer
   ) {
-    // create a builder for the scheduled timetable only if it needs to be modified.
-    // otherwise reuse the existing timetable
-    if (scheduledTimetableBuilder == null) {
-      scheduledTimetableBuilder = scheduledTimetable.copyOf();
-      scheduledTimetable = null;
-    }
     producer.apply(scheduledTimetableBuilder);
     return this;
   }
@@ -141,9 +129,6 @@ public int transitReluctanceFactorIndex() {
   }
 
   public Direction getDirection() {
-    if (scheduledTimetable != null) {
-      return scheduledTimetable.getDirection();
-    }
     return scheduledTimetableBuilder.getDirection();
   }
 
@@ -172,10 +157,6 @@ public StopPattern getStopPattern() {
     return stopPattern;
   }
 
-  public Timetable getScheduledTimetable() {
-    return scheduledTimetable;
-  }
-
   public TimetableBuilder getScheduledTimetableBuilder() {
     return scheduledTimetableBuilder;
   }

application/src/test/java/org/opentripplanner/transit/model/network/TripPatternTest.java

@@ -68,6 +68,9 @@ void copy() {
     assertEquals(subject.getHopGeometry(0), copy.getHopGeometry(0));
     assertEquals(subject.getHopGeometry(1), copy.getHopGeometry(1));
     assertEquals(HOP_GEOMETRIES.get(1), copy.getHopGeometry(1));
+
+    // The linked timetable should link back to the new pattern
+    assertSame(copy, copy.getScheduledTimetable().getPattern());
   }
 
   @Test