Merge pull request opentripplanner#7350 from entur/parse-not-validate-callwrapper

Refactor CallWrapper to parse-not-validate pattern

Author: vpaturet

application/src/main/java/org/opentripplanner/updater/trip/siri/AddedTripBuilder.java

@@ -76,7 +76,8 @@ class AddedTripBuilder {
     TransitEditorService transitService,
     DeduplicatorService deduplicator,
     EntityResolver entityResolver,
-    Function<Trip, FeedScopedId> getTripPatternId
+    Function<Trip, FeedScopedId> getTripPatternId,
+    List<CallWrapper> calls
   ) {
     this.deduplicator = deduplicator;
     // Verifying values required in SIRI Profile
@@ -104,7 +105,7 @@ class AddedTripBuilder {
       : lineRef;
     replacedRoute = entityResolver.resolveRoute(externalLineRef);
 
-    serviceDate = entityResolver.resolveServiceDate(estimatedVehicleJourney);
+    serviceDate = entityResolver.resolveServiceDate(estimatedVehicleJourney, calls);
 
     shortName = getFirstStringFromList(estimatedVehicleJourney.getPublishedLineNames());
 
@@ -117,7 +118,7 @@ class AddedTripBuilder {
     cancellation = TRUE.equals(estimatedVehicleJourney.isCancellation());
     headsign = getFirstStringFromList(estimatedVehicleJourney.getDestinationNames());
 
-    calls = CallWrapper.of(estimatedVehicleJourney);
+    this.calls = calls;
 
     this.transitService = transitService;
     this.entityResolver = entityResolver;

application/src/main/java/org/opentripplanner/updater/trip/siri/CallWrapper.java

@@ -2,8 +2,9 @@
 
 import java.time.ZonedDateTime;
 import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.List;
-import java.util.Optional;
+import org.opentripplanner.transit.model.framework.Result;
 import org.opentripplanner.updater.spi.UpdateError.UpdateErrorType;
 import org.opentripplanner.utils.lang.StringUtils;
 import uk.org.siri.siri21.ArrivalBoardingActivityEnumeration;
@@ -14,85 +15,99 @@
 import uk.org.siri.siri21.NaturalLanguageStringStructure;
 import uk.org.siri.siri21.OccupancyEnumeration;
 import uk.org.siri.siri21.RecordedCall;
+import uk.org.siri.siri21.StopPointRefStructure;
 
 /**
  * This class is a wrapper around either a {@link RecordedCall} or an {@link EstimatedCall}, making
  * it possible to iterate over both of the types at once.
+ * <p>
+ * Instances are created via the {@link #of(EstimatedVehicleJourney)} factory which validates and
+ * sorts calls during parsing, making invalid {@code CallWrapper} instances unrepresentable.
  */
 public interface CallWrapper {
-  static CallWrapper of(EstimatedCall estimatedCall) {
-    return new EstimatedCallWrapper(estimatedCall);
-  }
-
-  static CallWrapper of(RecordedCall recordedCall) {
-    return new RecordedCallWrapper(recordedCall);
-  }
-
-  static List<CallWrapper> of(EstimatedVehicleJourney estimatedVehicleJourney) {
+  /**
+   * Parse and validate all calls from an {@link EstimatedVehicleJourney}. Each call must have a
+   * non-empty stop point ref and exactly one of Order or VisitNumber. All calls must use the same
+   * strategy (all Order or all VisitNumber). The returned list is sorted by sort order.
+   *
+   * @return a successful sorted list of calls, or a failure with the appropriate error type
+   */
+  static Result<List<CallWrapper>, UpdateErrorType> of(
+    EstimatedVehicleJourney estimatedVehicleJourney
+  ) {
     List<CallWrapper> result = new ArrayList<>();
+    boolean hasOrderCalls = false;
+    boolean hasVisitNumberCalls = false;
 
     if (estimatedVehicleJourney.getRecordedCalls() != null) {
-      for (var recordedCall : estimatedVehicleJourney.getRecordedCalls().getRecordedCalls()) {
-        result.add(new RecordedCallWrapper(recordedCall));
+      for (var call : estimatedVehicleJourney.getRecordedCalls().getRecordedCalls()) {
+        var sortOrder = validateCall(
+          call.getStopPointRef(),
+          call.getOrder(),
+          call.getVisitNumber()
+        );
+        if (sortOrder.isFailure()) {
+          return sortOrder.toFailureResult();
+        }
+        hasOrderCalls |= call.getOrder() != null;
+        hasVisitNumberCalls |= call.getVisitNumber() != null;
+        result.add(new RecordedCallWrapper(call, sortOrder.successValue()));
       }
     }
 
     if (estimatedVehicleJourney.getEstimatedCalls() != null) {
-      for (var estimatedCall : estimatedVehicleJourney.getEstimatedCalls().getEstimatedCalls()) {
-        result.add(new EstimatedCallWrapper(estimatedCall));
+      for (var call : estimatedVehicleJourney.getEstimatedCalls().getEstimatedCalls()) {
+        var sortOrder = validateCall(
+          call.getStopPointRef(),
+          call.getOrder(),
+          call.getVisitNumber()
+        );
+        if (sortOrder.isFailure()) {
+          return sortOrder.toFailureResult();
+        }
+        hasOrderCalls |= call.getOrder() != null;
+        hasVisitNumberCalls |= call.getVisitNumber() != null;
+        result.add(new EstimatedCallWrapper(call, sortOrder.successValue()));
       }
     }
 
-    return List.copyOf(result);
-  }
+    // we reject messages that contain both Order and VisitNumber since we do not see any obvious
+    // use case that requires both, and making them mutually exclusive make the implementation
+    // simpler. We can relax this validation rule later if valid use cases are identified.
+    if (hasOrderCalls && hasVisitNumberCalls) {
+      return Result.failure(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
+    }
 
-  /**
-   * Validate that all calls have a non-empty stop point ref and either order or visit number (but
-   * not both). Also checks cross-call consistency: all calls must use the same strategy (all Order
-   * or all VisitNumber). A mix of Order and VisitNumber-only calls is rejected.
-   */
-  static Optional<UpdateErrorType> validateAll(List<CallWrapper> calls) {
-    var perCallError = calls
-      .stream()
-      .map(CallWrapper::validate)
-      .flatMap(Optional::stream)
-      .findFirst();
-    if (perCallError.isPresent()) {
-      return perCallError;
-    }
-    boolean anyHasOrder = calls.stream().anyMatch(CallWrapper::hasOrder);
-    boolean anyMissingOrder = calls.stream().anyMatch(c -> !c.hasOrder());
-    if (anyHasOrder && anyMissingOrder) {
-      return Optional.of(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
-    }
-    return Optional.empty();
+    result.sort(Comparator.comparingInt(CallWrapper::getSortOrder));
+
+    return Result.success(List.copyOf(result));
   }
 
   /**
-   * Validate that the call has a non-empty stop point ref and either order or visit number (but not
-   * both).
+   * Validate a single call's stop point ref and resolve its sort order from Order/VisitNumber.
    */
-  default Optional<UpdateErrorType> validate() {
-    if (StringUtils.hasNoValueOrNullAsString(getStopPointRef())) {
-      return Optional.of(UpdateErrorType.EMPTY_STOP_POINT_REF);
-    }
-    if (!hasOrder() && !hasVisitNumber()) {
-      return Optional.of(UpdateErrorType.MISSING_CALL_ORDER);
-    }
-    if (hasOrder() && hasVisitNumber()) {
-      return Optional.of(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
-    }
-    return Optional.empty();
+  private static Result<Integer, UpdateErrorType> validateCall(
+    StopPointRefStructure stopPointRef,
+    java.math.BigInteger order,
+    java.math.BigInteger visitNumber
+  ) {
+    var ref = stopPointRef != null ? stopPointRef.getValue() : null;
+    if (StringUtils.hasNoValueOrNullAsString(ref)) {
+      return Result.failure(UpdateErrorType.EMPTY_STOP_POINT_REF);
+    }
+    if (order == null && visitNumber == null) {
+      return Result.failure(UpdateErrorType.MISSING_CALL_ORDER);
+    }
+    if (order != null && visitNumber != null) {
+      return Result.failure(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
+    }
+    return Result.success(order != null ? order.intValueExact() : visitNumber.intValueExact());
   }
 
   String getStopPointRef();
 
-  boolean hasOrder();
-
-  boolean hasVisitNumber();
-
   /**
-   * Return the sort order of this call. Prefers Order if present, falls back to VisitNumber.
+   * Return the sort order of this call, resolved during parsing from either Order or VisitNumber.
    */
   int getSortOrder();
 
@@ -128,37 +143,21 @@ default boolean hasDeparted() {
   final class EstimatedCallWrapper implements CallWrapper {
 
     private final EstimatedCall call;
+    private final int sortOrder;
 
-    private EstimatedCallWrapper(EstimatedCall estimatedCall) {
+    private EstimatedCallWrapper(EstimatedCall estimatedCall, int sortOrder) {
       this.call = estimatedCall;
+      this.sortOrder = sortOrder;
     }
 
     @Override
     public String getStopPointRef() {
       return call.getStopPointRef() != null ? call.getStopPointRef().getValue() : null;
     }
 
-    @Override
-    public boolean hasOrder() {
-      return call.getOrder() != null;
-    }
-
-    @Override
-    public boolean hasVisitNumber() {
-      return call.getVisitNumber() != null;
-    }
-
-    /**
-     * Return the call order, either from the Order field or the VisitNumber field.
-     * Validation ensures that one of them is set.
-     * See {@link #validate()}
-     * @return
-     */
     @Override
     public int getSortOrder() {
-      return call.getOrder() != null
-        ? call.getOrder().intValueExact()
-        : call.getVisitNumber().intValueExact();
+      return sortOrder;
     }
 
     @Override
@@ -258,31 +257,21 @@ public boolean equals(Object obj) {
   final class RecordedCallWrapper implements CallWrapper {
 
     private final RecordedCall call;
+    private final int sortOrder;
 
-    private RecordedCallWrapper(RecordedCall estimatedCall) {
-      this.call = estimatedCall;
+    private RecordedCallWrapper(RecordedCall recordedCall, int sortOrder) {
+      this.call = recordedCall;
+      this.sortOrder = sortOrder;
     }
 
     @Override
     public String getStopPointRef() {
       return call.getStopPointRef() != null ? call.getStopPointRef().getValue() : null;
     }
 
-    @Override
-    public boolean hasOrder() {
-      return call.getOrder() != null;
-    }
-
-    @Override
-    public boolean hasVisitNumber() {
-      return call.getVisitNumber() != null;
-    }
-
     @Override
     public int getSortOrder() {
-      return call.getOrder() != null
-        ? call.getOrder().intValueExact()
-        : call.getVisitNumber().intValueExact();
+      return sortOrder;
     }
 
     @Override

application/src/main/java/org/opentripplanner/updater/trip/siri/EntityResolver.java

@@ -4,6 +4,7 @@
 import java.time.LocalDate;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeParseException;
+import java.util.List;
 import javax.annotation.Nullable;
 import org.opentripplanner.core.model.id.FeedScopedId;
 import org.opentripplanner.transit.model.network.Route;
@@ -214,7 +215,10 @@ public Operator resolveOperator(String operatorRef) {
   }
 
   @Nullable
-  public LocalDate resolveServiceDate(EstimatedVehicleJourney vehicleJourney) {
+  public LocalDate resolveServiceDate(
+    EstimatedVehicleJourney vehicleJourney,
+    List<CallWrapper> calls
+  ) {
     if (vehicleJourney.getFramedVehicleJourneyRef() != null) {
       var dataFrame = vehicleJourney.getFramedVehicleJourneyRef().getDataFrameRef();
       if (dataFrame != null) {
@@ -234,17 +238,18 @@ public LocalDate resolveServiceDate(EstimatedVehicleJourney vehicleJourney) {
       }
     }
 
-    var datetime = CallWrapper.of(vehicleJourney)
-      .stream()
-      .findFirst()
-      .map(CallWrapper::getAimedDepartureTime);
-    if (datetime.isEmpty()) {
+    if (calls.isEmpty()) {
+      return null;
+    }
+
+    var departureTime = calls.getFirst().getAimedDepartureTime();
+    if (departureTime == null) {
       return null;
     }
 
     var daysOffset = calculateDayOffset(vehicleJourney);
 
-    return datetime.get().toLocalDate().minusDays(daysOffset);
+    return departureTime.toLocalDate().minusDays(daysOffset);
   }
 
   /**

application/src/main/java/org/opentripplanner/updater/trip/siri/ExtraCallTripBuilder.java

@@ -52,21 +52,22 @@ class ExtraCallTripBuilder {
     DeduplicatorService deduplicator,
     EntityResolver entityResolver,
     Function<Trip, FeedScopedId> generateTripPatternId,
-    Trip trip
+    Trip trip,
+    List<CallWrapper> calls
   ) {
     this.trip = Objects.requireNonNull(trip);
 
     this.deduplicator = deduplicator;
     // DataSource of added trip
     dataSource = estimatedVehicleJourney.getDataSource();
 
-    serviceDate = entityResolver.resolveServiceDate(estimatedVehicleJourney);
+    serviceDate = entityResolver.resolveServiceDate(estimatedVehicleJourney, calls);
 
     isJourneyPredictionInaccurate = TRUE.equals(estimatedVehicleJourney.isPredictionInaccurate());
     occupancy = estimatedVehicleJourney.getOccupancy();
     cancellation = TRUE.equals(estimatedVehicleJourney.isCancellation());
 
-    calls = CallWrapper.of(estimatedVehicleJourney);
+    this.calls = calls;
 
     this.transitService = transitService;
     this.generateTripPatternId = generateTripPatternId;

application/src/main/java/org/opentripplanner/updater/trip/siri/ModifiedTripBuilder.java

@@ -55,15 +55,16 @@ public ModifiedTripBuilder(
     EstimatedVehicleJourney journey,
     LocalDate serviceDate,
     ZoneId zoneId,
-    EntityResolver entityResolver
+    EntityResolver entityResolver,
+    List<CallWrapper> calls
   ) {
     this.existingTripTimes = existingTripTimes;
     this.pattern = pattern;
     this.serviceDate = serviceDate;
     this.zoneId = zoneId;
     this.entityResolver = entityResolver;
 
-    calls = CallWrapper.of(journey);
+    this.calls = calls;
     cancellation = TRUE.equals(journey.isCancellation());
     predictionInaccurate = TRUE.equals(journey.isPredictionInaccurate());
     occupancy = journey.getOccupancy();

application/src/main/java/org/opentripplanner/updater/trip/siri/SiriFuzzyTripMatcher.java

@@ -62,12 +62,11 @@ public SiriFuzzyTripMatcher(TransitService transitService) {
    */
   public Result<TripAndPattern, UpdateError.UpdateErrorType> match(
     EstimatedVehicleJourney journey,
+    List<CallWrapper> calls,
     EntityResolver entityResolver,
     BiFunction<TripPattern, LocalDate, Timetable> getCurrentTimetable,
     BiFunction<FeedScopedId, LocalDate, TripPattern> getNewTripPatternForModifiedTrip
   ) {
-    List<CallWrapper> calls = CallWrapper.of(journey);
-
     if (calls.isEmpty()) {
       return Result.failure(NO_VALID_STOPS);
     }