Avoid duplicate service IDs during GTFS merge

onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/strategies/AbstractCollectionEntityMergeStrategy.java

@@ -14,6 +14,7 @@
 package org.onebusaway.gtfs_merge.strategies;
 
 import java.io.Serializable;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Set;
@@ -55,8 +56,13 @@ public AbstractCollectionEntityMergeStrategy(String keyDescription) {
 
   @Override
   public void merge(GtfsMergeContext context) {
-    for (KEY key : getKeys(context.getSource())) {
-      processKey(context, key);
+    Collection<KEY> sourceKeys = new ArrayList<>(getKeys(context.getSource()));
+    Set<String> sourceOriginalRawIds = new HashSet<>();
+    for (KEY key : sourceKeys) {
+      sourceOriginalRawIds.add(getRawKey(key));
+    }
+    for (KEY key : sourceKeys) {
+      processKey(context, key, sourceOriginalRawIds);
     }
   }
 
@@ -66,8 +72,10 @@ public void merge(GtfsMergeContext context) {
    *
    * @param context
    * @param key the identifier of the current entity collection to process
+   * @param sourceOriginalRawIds all raw ids originally present in the current source feed, reserved
+   *     for the duration of this strategy merge
    */
-  private void processKey(GtfsMergeContext context, KEY key) {
+  private void processKey(GtfsMergeContext context, KEY key, Set<String> sourceOriginalRawIds) {
     KEY duplicate = getDuplicate(context, key);
     if (duplicate != null) {
       logDuplicateKey(key);
@@ -83,7 +91,7 @@ private void processKey(GtfsMergeContext context, KEY key) {
      * avoid duplication.
      */
     if (context.getEntityForRawId(rawKey) != null) {
-      KEY newKey = getRenamedKey(context, key);
+      KEY newKey = getNextAvailableRenamedKey(context, key, sourceOriginalRawIds);
       renameKey(context, key, newKey);
       key = newKey;
       rawKey = getRawKey(key);
@@ -94,6 +102,43 @@ private void processKey(GtfsMergeContext context, KEY key) {
     saveElementsForKey(context, key);
   }
 
+  /**
+   * Finds a renamed id for the specified key that is safe to use: one that doesn't collide with an
+   * id already present in the merged output feed, and doesn't collide with any raw id originally
+   * present in the current source feed. Those original ids remain reserved for the duration of this
+   * strategy merge. Candidates are generated by repeatedly applying {@link
+   * #getRenamedKey(GtfsMergeContext, Serializable)} without mutating the source feed; the caller is
+   * responsible for actually applying the rename once a safe candidate is found.
+   *
+   * @param context
+   * @param key the original, colliding key
+   * @param sourceOriginalRawIds the set of raw ids originally present in the current source feed
+   * @return a renamed key that is safe to use
+   */
+  private KEY getNextAvailableRenamedKey(
+      GtfsMergeContext context, KEY key, Set<String> sourceOriginalRawIds) {
+    String previousRawKey = getRawKey(key);
+    KEY candidate = key;
+    while (true) {
+      KEY renamed = getRenamedKey(context, candidate);
+      String renamedRawKey = getRawKey(renamed);
+      if (renamedRawKey.equals(previousRawKey)) {
+        throw new IllegalStateException(
+            "renaming key="
+                + candidate
+                + " produced no change to raw id="
+                + renamedRawKey
+                + "; check merge prefix configuration");
+      }
+      candidate = renamed;
+      previousRawKey = renamedRawKey;
+      if (!sourceOriginalRawIds.contains(renamedRawKey)
+          && context.getEntityForRawId(renamedRawKey) == null) {
+        return candidate;
+      }
+    }
+  }
+
   /**
    * An entity-specific method to determine the set of unique identifiers used by collection
    * entities in the specified GTFS feed.

onebusaway-gtfs-merge/src/test/java/org/onebusaway/gtfs_merge/GtfsMergerTest.java

@@ -14,13 +14,18 @@
 package org.onebusaway.gtfs_merge;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.File;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -628,6 +633,107 @@ public void testLocationTypeMismatch_PlatformFeedFirstStationFeedSecond() throws
     assertTrue(foundStopTime, "expected at least one merged stop_time");
   }
 
+  /**
+   * Reproduces issue 142: after a raw service_id collision forces a rename, the renamed id must not
+   * collide with an id that already exists in the merged output feed (or in another,
+   * not-yet-processed key from the same source feed). Round-trips the merged feed through a
+   * writer/reader cycle to make sure the on-disk result is unambiguous.
+   */
+  @Test
+  public void testServiceIdRenameAvoidsCollisionWithExistingRawId() throws IOException {
+    // "fresh" input (lowest priority, processed second): a single, previously-unmerged
+    // service calendar whose raw id "T0" collides with an id in the higher-priority input.
+    _oldGtfs.putAgencies(1);
+    _oldGtfs.putRoutes(1);
+    _oldGtfs.putStops(3);
+    _oldGtfs.putCalendars(1, "mask=1111100", "service_id=T0");
+    _oldGtfs.putCalendarDates("T0=20120704");
+    _oldGtfs.putTrips(1, "r0", "T0", "trip_id=fresh-trip");
+    _oldGtfs.putStopTimes("fresh-trip", "s0,s1,s2");
+
+    // "already merged" input (highest priority, processed first): already contains both the
+    // raw id "T0" and a previously-renamed raw id "a-T0" as two logically distinct calendars.
+    _newGtfs.putAgencies(1);
+    _newGtfs.putRoutes(1);
+    _newGtfs.putStops(3);
+    _newGtfs.putCalendars(2, "mask=0000011,1010101", "service_id=T0,a-T0");
+    _newGtfs.putCalendarDates("T0=20120705", "a-T0=20120706");
+    _newGtfs.putTrips(2, "r0,r0", "T0,a-T0", "trip_id=target-t0-trip,target-a-t0-trip");
+    _newGtfs.putStopTimes("target-t0-trip,target-a-t0-trip", "s0,s1,s2");
+
+    ServiceCalendarMergeStrategy strategy = new ServiceCalendarMergeStrategy();
+    strategy.setDuplicateDetectionStrategy(EDuplicateDetectionStrategy.NONE);
+    strategy.setDuplicateRenamingStrategy(EDuplicateRenamingStrategy.CONTEXT);
+    _merger.setServiceCalendarStrategy(strategy);
+
+    GtfsRelationalDao dao = merge();
+
+    Set<String> serviceIds = new HashSet<>();
+    Map<String, String> serviceIdByCalendarMask = new HashMap<>();
+    for (ServiceCalendar calendar : dao.getAllCalendars()) {
+      String serviceId = calendar.getServiceId().getId();
+      serviceIds.add(serviceId);
+      serviceIdByCalendarMask.put(getCalendarMask(calendar), serviceId);
+    }
+    assertEquals(3, dao.getAllCalendars().size(), "expected three logical calendars");
+    assertEquals(3, serviceIds.size(), "expected three distinct raw service ids");
+    assertEquals(3, serviceIdByCalendarMask.size(), "expected three distinct calendar masks");
+    assertEquals(
+        "T0",
+        serviceIdByCalendarMask.get("0000011"),
+        "higher-priority T0 calendar must remain unchanged");
+    assertEquals(
+        "a-T0",
+        serviceIdByCalendarMask.get("1010101"),
+        "higher-priority a-T0 calendar must remain unchanged");
+
+    String freshId = serviceIdByCalendarMask.get("1111100");
+    assertNotNull(freshId, "expected the fresh calendar to receive a third, unused id");
+    assertTrue(!freshId.equals("T0") && !freshId.equals("a-T0"));
+
+    Map<String, String> serviceIdByTripId = new HashMap<>();
+    for (Trip trip : dao.getAllTrips()) {
+      serviceIdByTripId.put(trip.getId().getId(), trip.getServiceId().getId());
+    }
+    assertEquals(3, dao.getAllTrips().size(), "expected one trip for each logical calendar");
+    assertEquals(freshId, serviceIdByTripId.get("fresh-trip"));
+    assertEquals("T0", serviceIdByTripId.get("target-t0-trip"));
+    assertEquals("a-T0", serviceIdByTripId.get("target-a-t0-trip"));
+
+    Map<String, String> serviceIdByExceptionDate = new HashMap<>();
+    for (ServiceCalendarDate date : dao.getAllCalendarDates()) {
+      serviceIdByExceptionDate.put(date.getDate().getAsString(), date.getServiceId().getId());
+    }
+    assertEquals(
+        3, dao.getAllCalendarDates().size(), "expected one date for each logical calendar");
+    assertEquals(3, serviceIdByExceptionDate.size(), "expected three distinct exception dates");
+    assertEquals(freshId, serviceIdByExceptionDate.get("20120704"));
+    assertEquals("T0", serviceIdByExceptionDate.get("20120705"));
+    assertEquals("a-T0", serviceIdByExceptionDate.get("20120706"));
+
+    // no ambiguity in the round-tripped output: exactly one calendar per raw service id
+    for (String id : serviceIds) {
+      int count = 0;
+      for (ServiceCalendar calendar : dao.getAllCalendars()) {
+        if (id.equals(calendar.getServiceId().getId())) {
+          count++;
+        }
+      }
+      assertEquals(1, count, "expected exactly one calendar for service id=" + id);
+    }
+  }
+
+  private String getCalendarMask(ServiceCalendar calendar) {
+    return ""
+        + calendar.getMonday()
+        + calendar.getTuesday()
+        + calendar.getWednesday()
+        + calendar.getThursday()
+        + calendar.getFriday()
+        + calendar.getSaturday()
+        + calendar.getSunday();
+  }
+
   private GtfsRelationalDao merge() throws IOException {
     List<File> paths = new ArrayList<>();
     paths.add(_oldGtfs.getPath());