fix(MergeLineContext): Partially remove unused service_ids.

binh-dam-ibigroup · binh-dam-ibigroup · commit 6ea596804f6c · 2021-11-24T00:21:38.000-05:00
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java b/src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java
@@ -86,7 +86,7 @@ public class MergeFeedsJob extends FeedSourceJob {
     @JsonIgnore @BsonIgnore
     public Set<String> serviceIdsToCloneRenameAndExtend = new HashSet<>();
     @JsonIgnore @BsonIgnore
-    public Set<String> serviceIdsToTerminateEarly = new HashSet<>();
+    public Set<String> serviceIdsFromActiveFeedToTerminateEarly = new HashSet<>();
 
     private List<TripAndCalendars> sharedConsistentTripAndCalendarIds = new ArrayList<>();
 
@@ -437,7 +437,7 @@ private void determineMergeStrategy() {
 
             // Build the set of calendars to be shortened to the day before the future feed start date
             // from trips in the active feed but not in the future feed.
-            serviceIdsToTerminateEarly.addAll(
+            serviceIdsFromActiveFeedToTerminateEarly.addAll(
                 getActiveServiceIds(feedMergeContext.getActiveTripIdsNotInFutureFeed())
             );
 
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/CalendarMergeLineContext.java b/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/CalendarMergeLineContext.java
@@ -38,19 +38,20 @@ public void afterRowWrite() throws IOException {
 
     private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldContext) throws IOException {
         boolean shouldSkipRecord = false;
+        String key = getTableScopedValue(table, getIdScope(), keyValue);
+
         if (isHandlingActiveFeed()) {
             LocalDate startDate = getCsvDate("start_date");
-            // If a service_id from the active calendar has both the
-            // start_date and end_date in the future, the service will be
-            // excluded from the merged file. Records in trips,
-            // calendar_dates, and calendar_attributes referencing this
-            // service_id shall also be removed/ignored. Stop_time records
-            // for the ignored trips shall also be removed.
             if (!startDate.isBefore(feedMergeContext.future.getFeedFirstDate())) {
+                // If a service_id from the active calendar has both the
+                // start_date and end_date in the future, the service will be
+                // excluded from the merged file. Records in trips,
+                // calendar_dates, and calendar_attributes referencing this
+                // service_id shall also be removed/ignored. Stop_time records
+                // for the ignored trips shall also be removed.
                 LOG.warn(
-                    "Skipping calendar entry {} because it operates fully within the time span of future feed.",
+                    "Skipping active calendar entry {} because it operates fully within the time span of future feed.",
                     keyValue);
-                String key = getTableScopedValue(table, getIdScope(), keyValue);
                 mergeFeedsResult.skippedIds.add(key);
                 shouldSkipRecord = true;
             } else {
@@ -68,7 +69,7 @@ private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldC
                 boolean activeAndFutureTripIdsAreDisjoint = job.sharedTripIdsWithConsistentSignature.isEmpty();
                 if (activeAndFutureTripIdsAreDisjoint) {
                     futureStartDate = feedMergeContext.futureFirstCalendarStartDate;
-                } else if (job.serviceIdsToTerminateEarly.contains(keyValue)) {
+                } else if (job.serviceIdsFromActiveFeedToTerminateEarly.contains(keyValue)) {
                     futureStartDate = feedMergeContext.future.getFeedFirstDate();
                 }
                 // In other cases not covered above, new calendar entry is already flagged for insertion
@@ -85,10 +86,20 @@ private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldC
                         .format(GTFS_DATE_FORMATTER));
                 }
             }
+/*        } else {
+            // If handling the future feed, the MTC revised feed merge logic is as follows:
+            // - Calendar entries from the future feed will be inserted as is in the merged feed.
+            // so no additional processing needed here, unless the calendar entry is no longer used.
+            if (job.serviceIdsFromFutureFeedToRemove.contains(keyValue)) {
+                LOG.warn(
+                    "Skipping future calendar entry {} because it will become unused in the merged feed.",
+                    keyValue);
+                mergeFeedsResult.skippedIds.add(key);
+                shouldSkipRecord = true;
+            }
+
+ */
         }
-        // If handling the future feed, the MTC revised feed merge logic is as follows:
-        // - Calendar entries from the future feed will be inserted as is in the merged feed.
-        // so no additional processing needed here.
 
 
         // If any service_id in the active feed matches with the future
@@ -98,8 +109,7 @@ private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldC
         //  duplicates? I think we would need to consider the
         //  service_id:exception_type:date as the unique key and include any
         //  all entries as long as they are unique on this key.
-
-        if (hasDuplicateError(idErrors)) {
+        if (isHandlingActiveFeed() && hasDuplicateError(idErrors)) {
             // Modify service_id and ensure that referencing trips
             // have service_id updated.
             updateAndRemapOutput(fieldContext);
@@ -108,7 +118,11 @@ private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldC
         // Track service ID because we want to avoid removing trips that may reference this
         // service_id when the service_id is used by calendar_dates that operate in the valid
         // date range, i.e., before the future feed's first date.
-        if (!shouldSkipRecord && fieldContext.nameEquals(SERVICE_ID)) mergeFeedsResult.serviceIds.add(fieldContext.getValueToWrite());
+        //
+        // If service is going to be cloned, add to the output service ids.
+        if (!shouldSkipRecord && fieldContext.nameEquals(SERVICE_ID)) {
+            mergeFeedsResult.serviceIds.add(fieldContext.getValueToWrite());
+        }
 
         return !shouldSkipRecord;
     }
@@ -119,25 +133,27 @@ private boolean checkCalendarIds(Set<NewGTFSError> idErrors, FieldContext fieldC
      * @throws IOException
      */
     public void addClonedServiceId() throws IOException {
-        String originalServiceId = getRowValues()[keyFieldIndex];
-        if (job.serviceIdsToCloneRenameAndExtend.contains(originalServiceId)) {
-            // FIXME: Do we need to worry about calendar_dates?
-            String[] clonedValues = getRowValues().clone();
-            String newServiceId = clonedValues[keyFieldIndex] = String.join(":", getIdScope(), originalServiceId);
-            // Modify start date only (preserve the end date on the future calendar entry).
-            int startDateIndex = Table.CALENDAR.getFieldIndex("start_date");
-            clonedValues[startDateIndex] = feedMergeContext.active.feed.calendars.get(originalServiceId).start_date
-                .format(GTFS_DATE_FORMATTER);
-            referenceTracker.checkReferencesAndUniqueness(
-                keyValue,
-                getLineNumber(),
-                table.fields[0],
-                newServiceId,
-                table,
-                keyField,
-                table.getOrderFieldName()
-            );
-            writeValuesToTable(clonedValues, true);
+        if (isHandlingFutureFeed()) {
+            String originalServiceId = keyValue;
+            if (job.serviceIdsToCloneRenameAndExtend.contains(originalServiceId)) {
+                // FIXME: Do we need to worry about calendar_dates?
+                String[] clonedValues = getRowValues().clone();
+                String newServiceId = clonedValues[keyFieldIndex] = String.join(":", getIdScope(), originalServiceId);
+                // Modify start date only (preserve the end date from the future calendar entry).
+                int startDateIndex = Table.CALENDAR.getFieldIndex("start_date");
+                clonedValues[startDateIndex] = feedMergeContext.active.feed.calendars.get(originalServiceId).start_date
+                    .format(GTFS_DATE_FORMATTER);
+                referenceTracker.checkReferencesAndUniqueness(
+                    keyValue,
+                    getLineNumber(),
+                    table.fields[0],
+                    newServiceId,
+                    table,
+                    keyField,
+                    table.getOrderFieldName()
+                );
+                writeValuesToTable(clonedValues, true);
+            }
         }
     }
 }
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedContext.java b/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedContext.java
@@ -38,8 +38,6 @@ public FeedContext(FeedToMerge givenFeedToMerge) throws IOException {
 
     public LocalDate getFeedFirstDate() { return feedFirstDate; }
 
-    public void setFeedFirstDate(LocalDate firstDate) { feedFirstDate = firstDate; }
-
     public String getNewAgencyId() {
         return newAgencyId;
     }
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedMergeContext.java b/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedMergeContext.java
@@ -32,7 +32,7 @@ public FeedMergeContext(Set<FeedVersion> feedVersions, Auth0UserProfile owner) t
         future = new FeedContext(futureFeedToMerge);
 
         // Determine whether service and trip IDs are exact matches.
-        serviceIdsMatch = activeFeedToMerge.serviceIds.equals(futureFeedToMerge.serviceIds);
+        serviceIdsMatch = activeFeedToMerge.serviceIdsInUse.equals(futureFeedToMerge.serviceIdsInUse);
         tripIdsMatch = active.tripIds.equals(future.tripIds);
         sharedTripIds = Sets.intersection(active.tripIds, future.tripIds);
 
@@ -63,9 +63,16 @@ public boolean areActiveAndFutureTripIdsDisjoint() {
     }
 
     /**
-     * Obtains the active trip ids found in the active feed, but not in the future feed.
+     * Obtains the trip ids found in the active feed, but not in the future feed.
      */
     public Sets.SetView<String> getActiveTripIdsNotInFutureFeed() {
         return Sets.difference(active.tripIds, future.tripIds);
     }
+
+    /**
+     * Obtains the trip ids found in the future feed, but not in the active feed.
+     */
+    public Sets.SetView<String> getFutureTripIdsNotInActiveFeed() {
+        return Sets.difference(future.tripIds, active.tripIds);
+    }
 }
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedToMerge.java b/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/FeedToMerge.java
@@ -10,6 +10,7 @@
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.stream.Collectors;
 import java.util.zip.ZipFile;
 
 import static com.conveyal.datatools.manager.utils.MergeFeedUtils.getIdsForTable;
@@ -23,6 +24,7 @@ public class FeedToMerge implements Closeable {
     public ZipFile zipFile;
     public SetMultimap<Table, String> idsForTable = HashMultimap.create();
     public Set<String> serviceIds = new HashSet<>();
+    public Set<String> serviceIdsInUse;
     private static final Set<Table> tablesToCheck = Sets.newHashSet(Table.TRIPS, Table.CALENDAR, Table.CALENDAR_DATES);
 
     public FeedToMerge(FeedVersion version) throws IOException {
@@ -37,6 +39,18 @@ public void collectTripAndServiceIds() throws IOException {
         }
         serviceIds.addAll(idsForTable.get(Table.CALENDAR));
         serviceIds.addAll(idsForTable.get(Table.CALENDAR_DATES));
+
+        serviceIdsInUse = getServiceIdsInUse(idsForTable.get(Table.TRIPS));
+    }
+
+    /**
+     * Obtains the service ids corresponding to the provided trip ids.
+     * FIXME: Duplicate of MergeFeedsJob.
+     */
+    private Set<String> getServiceIdsInUse(Set<String> tripIds) {
+        return tripIds.stream()
+            .map(tripId -> version.retrieveFeed().trips.get(tripId).service_id)
+            .collect(Collectors.toSet());
     }
 
     public void close() throws IOException {
diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/MergeLineContext.java b/src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/MergeLineContext.java
@@ -144,6 +144,14 @@ public void startNewFeed(int feedIndex) throws IOException {
             // If there is no agency_id for agency table, create one and ensure that
             // route#agency_id gets set.
         }
+
+        if (handlingFutureFeed) {
+            mergeFeedsResult.serviceIds.addAll(
+                job.serviceIdsToCloneRenameAndExtend.stream().map(
+                    id -> String.join(":", idScope, id)
+                ).collect(Collectors.toSet())
+            );
+        }
     }
 
     public boolean shouldSkipFile() {
@@ -395,6 +403,24 @@ public boolean updateAgencyIdIfNeeded(FieldContext fieldContext) {
         return true;
     }
 
+    public boolean updateServiceIdsIfNeeded(FieldContext fieldContext) {
+        String fieldValue = fieldContext.getValue();
+        if (table.name.equals(Table.TRIPS.name) &&
+            fieldContext.nameEquals(SERVICE_ID) &&
+            job.serviceIdsToCloneRenameAndExtend.contains(fieldValue) &&
+            job.mergeType.equals(SERVICE_PERIOD)
+        ) {
+            // Future trip ids not in the active feed will not get the service id remapped,
+            // they will use the service id as defined in the future feed instead.
+            if (!(handlingFutureFeed && feedMergeContext.getFutureTripIdsNotInActiveFeed().contains(keyValue))) {
+                String newServiceId = String.join(":", idScope, fieldValue);
+                LOG.info("Updating {}#service_id to (auto-generated) {} for ID {}", table.name, newServiceId, keyValue);
+                fieldContext.setValueToWrite(newServiceId);
+            }
+        }
+        return true;
+    }
+
     public boolean storeRowAndStopValues() {
         String newLine = String.join(",", rowValues);
         switch (table.name) {
@@ -527,9 +553,10 @@ public boolean constructRowValues() throws IOException {
                     updateAndRemapOutput(fieldContext);
                 }
 
+                updateServiceIdsIfNeeded(fieldContext);
+
                 // Store values for key fields that have been encountered and update any key values that need modification due
                 // to conflicts.
-                // This method can change skipRecord.
                 if (!checkFieldsForMergeConflicts(getIdErrors(fieldContext), fieldContext)) {
                     skipRecord = true;
                     break;
diff --git a/src/test/java/com/conveyal/datatools/manager/jobs/MergeFeedsJobTest.java b/src/test/java/com/conveyal/datatools/manager/jobs/MergeFeedsJobTest.java
@@ -387,16 +387,31 @@ void mergeMTCShouldHandleMatchingTripIdsWithSameSignature() throws SQLException
 
         // - calendar table
         // expect a total of 4 records in calendar table:
-        // - 1 from the active feed (common_id start date is changed to one day before first start_date in future feed)
-        //   (the other one is unused and is discarded)
-        // - 2 from the future feed
-        // - 1 cloned for the matching trip id present in both active and future feeds
-        //   (from MergeFeedsJob#serviceIdsToCloneAndRename).
+        // - common_id from the active feed (but start date is changed to one day before first start_date in future feed),
+        // - common_id from the future feed (because of one future trip not in the active feed),
+        // - common_id cloned and extended for the matching trip id present in both active and future feeds
+        //   (from MergeFeedsJob#serviceIdsToCloneAndRename),
+        // - only_calendar_id used in the future feed.
         assertThatSqlCountQueryYieldsExpectedCount(
             String.format("SELECT count(*) FROM %s.calendar", mergedNamespace),
             4
         );
 
+        // Out of all trips from the input datasets, expect 4 trips in merged output.
+        // 1 trip from active feed that is not in the future feed,
+        // 1 trip in both the active and future feeds, with the same signature (same stop times),
+        // 2 trips from the future feed not in the active feed.
+        assertThatSqlCountQueryYieldsExpectedCount(
+            String.format("SELECT count(*) FROM %s.trips", mergedNamespace),
+            4
+        );
+
+        // There should be no unused service ids.
+        assertThatSqlCountQueryYieldsExpectedCount(
+            String.format("SELECT count(*) FROM %s.errors where error_type = 'SERVICE_UNUSED'", mergedNamespace),
+            0
+        );
+
         // expect that 2 calendars (1 common_id extended from future and 1 Fake_Transit1:common_id from active) have
         // start_date pinned to start date of active feed.
         assertThatSqlCountQueryYieldsExpectedCount(
@@ -415,14 +430,6 @@ void mergeMTCShouldHandleMatchingTripIdsWithSameSignature() throws SQLException
             String.format("SELECT count(*) FROM %s.calendar where start_date = '20170918' and end_date='20170919'", mergedNamespace),
             1
         );
-        // Out of all trips from the input datasets, expect 4 trips in merged output.
-        // 1 trip from active feed that is not in the future feed,
-        // 1 trip in both the active and future feeds, with the same signature (same stop times),
-        // 2 trips from the future feed not in the active feed.
-        assertThatSqlCountQueryYieldsExpectedCount(
-            String.format("SELECT count(*) FROM %s.trips", mergedNamespace),
-            4
-        );
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -38,8 +38,6 @@ public FeedContext(FeedToMerge givenFeedToMerge) throws IOException {`
`38`	`38`
`39`	`39`	`public LocalDate getFeedFirstDate() { return feedFirstDate; }`
`40`	`40`
`41`		`- public void setFeedFirstDate(LocalDate firstDate) { feedFirstDate = firstDate; }`
`42`		`-`
`43`	`41`	`public String getNewAgencyId() {`
`44`	`42`	`return newAgencyId;`
`45`	`43`	`}`