Merge pull request opentripplanner#7407 from Skanetrafiken/relax-visit-number-validation

Relax the visit number validation logic

Author: habrahamsson-skanetrafiken

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

@@ -27,16 +27,17 @@
 public interface CallWrapper {
   /**
    * 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.
+   * non-empty stop point ref and at least one of Order or VisitNumber (Order is preferred when both
+   * are present). 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 List<CallWrapper> of(EstimatedVehicleJourney estimatedVehicleJourney)
     throws UpdateException {
     List<CallWrapper> result = new ArrayList<>();
-    boolean hasOrderCalls = false;
-    boolean hasVisitNumberCalls = false;
+    boolean hasCallWithOrder = false;
+    boolean hasCallWithoutOrder = false;
 
     if (estimatedVehicleJourney.getRecordedCalls() != null) {
       for (var call : estimatedVehicleJourney.getRecordedCalls().getRecordedCalls()) {
@@ -45,8 +46,9 @@ static List<CallWrapper> of(EstimatedVehicleJourney estimatedVehicleJourney)
           call.getOrder(),
           call.getVisitNumber()
         );
-        hasOrderCalls |= call.getOrder() != null;
-        hasVisitNumberCalls |= call.getVisitNumber() != null;
+        var hasOrder = call.getOrder() != null;
+        hasCallWithOrder |= hasOrder;
+        hasCallWithoutOrder |= !hasOrder;
         result.add(new RecordedCallWrapper(call, sortOrder));
       }
     }
@@ -58,16 +60,14 @@ static List<CallWrapper> of(EstimatedVehicleJourney estimatedVehicleJourney)
           call.getOrder(),
           call.getVisitNumber()
         );
-        hasOrderCalls |= call.getOrder() != null;
-        hasVisitNumberCalls |= call.getVisitNumber() != null;
+        var hasOrder = call.getOrder() != null;
+        hasCallWithOrder |= hasOrder;
+        hasCallWithoutOrder |= !hasOrder;
         result.add(new EstimatedCallWrapper(call, sortOrder));
       }
     }
 
-    // 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) {
+    if (hasCallWithOrder && hasCallWithoutOrder) {
       throw UpdateException.of(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
     }
 
@@ -90,9 +90,6 @@ private static int validateCall(
     if (order == null && visitNumber == null) {
       throw UpdateException.of(UpdateErrorType.MISSING_CALL_ORDER);
     }
-    if (order != null && visitNumber != null) {
-      throw UpdateException.of(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER);
-    }
     return order != null ? order.intValueExact() : visitNumber.intValueExact();
   }
 

application/src/test/java/org/opentripplanner/updater/trip/siri/CallWrapperParsingTest.java

@@ -58,13 +58,18 @@ void rejectMissingOrderAndVisitNumber() {
   }
 
   @Test
-  void rejectBothOrderAndVisitNumber() {
+  void preferOrderBeforeVisitNumber() {
     var journey = new EstimatedVehicleJourney();
     var estimatedCalls = new EstimatedVehicleJourney.EstimatedCalls();
-    estimatedCalls.getEstimatedCalls().add(estimatedCall("STOP_A", 1, 1));
+    estimatedCalls.getEstimatedCalls().add(estimatedCall("STOP_A", 1, 3));
+    estimatedCalls.getEstimatedCalls().add(estimatedCall("STOP_B", 2, 6));
     journey.setEstimatedCalls(estimatedCalls);
 
-    assertFailure(UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER, () -> CallWrapper.of(journey));
+    var calls = CallWrapper.of(journey);
+
+    assertEquals(2, calls.size());
+    assertEquals(1, calls.get(0).getSortOrder());
+    assertEquals(2, calls.get(1).getSortOrder());
   }
 
   @Test

application/src/test/java/org/opentripplanner/updater/trip/siri/moduletests/rejection/MissingCallOrderTest.java

@@ -1,5 +1,6 @@
 package org.opentripplanner.updater.trip.siri.moduletests.rejection;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.opentripplanner.updater.spi.UpdateErrorType.MISSING_CALL_ORDER;
 import static org.opentripplanner.updater.spi.UpdateErrorType.MIXED_CALL_ORDER_AND_VISIT_NUMBER;
 import static org.opentripplanner.updater.spi.UpdateResultAssertions.assertFailure;
@@ -107,7 +108,7 @@ void acceptVisitNumberAsFallback() {
   }
 
   @Test
-  void rejectMixedOrderAndVisitNumber() {
+  void preferOrderToVisitNumber() {
     var env = ENV_BUILDER.addTrip(TRIP_INPUT).build();
     var siri = SiriTestHelper.of(env);
 
@@ -120,14 +121,20 @@ void rejectMixedOrderAndVisitNumber() {
           .withVisitNumber(1)
           .departAimedExpected("00:00:11", "00:00:15")
           .call(STOP_B)
+          .withVisitNumber(3)
           .departAimedExpected("00:00:21", "00:00:25")
           .call(STOP_C)
+          .withVisitNumber(2)
           .arriveAimedExpected("00:00:40", "00:00:45")
       )
       .buildEstimatedTimetableDeliveries();
 
     var result = siri.applyEstimatedTimetable(updates);
-    assertFailure(MIXED_CALL_ORDER_AND_VISIT_NUMBER, result);
+    assertSuccess(result);
+    assertEquals(
+      "UPDATED | A 0:00:15 0:00:15 | B 0:00:20 0:00:25 | C 0:00:45 0:00:45",
+      env.tripData(TRIP_1_ID).showTimetable()
+    );
   }
 
   @Test