GTFS-RT Trip Updates > filter duplicate unique trip updates #TripModifications - #185
Open
mmathieum wants to merge 3 commits into
Open
GTFS-RT Trip Updates > filter duplicate unique trip updates #TripModifications#185mmathieum wants to merge 3 commits into
mmathieum wants to merge 3 commits into
Conversation
…fications The best practices suggest feed publishers to include duplicate trip updates w/ and w/o `ModifiedTripSelector` - #182
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a GTFS-RT TripUpdates deduplication step to reduce duplicate trip processing when publishers include multiple TripUpdates for the same trip (including TripModifications via ModifiedTripSelector), addressing issue #182.
Changes:
- Introduces
filterDuplicatesTrips()to pick a single TripUpdate per “duplicate” group, preferring non-modified trip updates and those with vehicle descriptors. - Integrates duplicate filtering into
GTFSRealTimeTripUpdatesProvider’s TripUpdates selection pipeline and tweaks related debug log messages / out-of-sync flagging. - Adds focused unit tests covering regular vs modified trip update duplicate scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProvider.kt | Applies duplicate filtering in the TripUpdates pipeline and adjusts match logging/out-of-sync behavior. |
| src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.kt | Adds the new TripUpdates duplicate-filtering helper. |
| src/test/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilterTests.kt | Adds tests validating duplicate filtering selection behavior. |
| src/test/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProviderTests.kt | Fixes TripDescriptor builder to assign the intended trip ID constant. |
Comment on lines
+14
to
+18
| this@filterDuplicatesTrips.groupBy { | ||
| it.optTrip?.optTripIdNotEmpty to ( | ||
| (it.optTrip?.optStartDate ?: it.optTrip?.optModifiedTrip?.optStartDate) to | ||
| (it.optTrip?.optStartTime ?: it.optTrip?.optModifiedTrip?.optStartTime)) | ||
| }.forEach { (_, gTripUpdates) -> |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesGTFS realtime trip updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GTFSRealTimeTripUpdatesProvider
participant match
participant filterDuplicatesTrips
participant CachedStatus
GTFSRealTimeTripUpdatesProvider->>match: evaluate each TripUpdate
match-->>GTFSRealTimeTripUpdatesProvider: retain matching updates
GTFSRealTimeTripUpdatesProvider->>filterDuplicatesTrips: pass matching updates
filterDuplicatesTrips-->>GTFSRealTimeTripUpdatesProvider: return representatives
GTFSRealTimeTripUpdatesProvider->>CachedStatus: map trip descriptors and updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.kt (2)
39-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing vehicle-descriptor tiebreaker when all duplicates carry ModifiedTrip.
When every duplicate in a group has
hasModifiedTrip() == true(rules 1 & 2 both yield nothing), the code falls straight to "pick 1st one in the list" without considering which one has aVehicledescriptor. This can silently discard a duplicate with valid vehicle telemetry in favor of an arbitrary earlier one lacking it (as tested only for the both-with-vehicle case intest_filterDuplicatesTrips_2ModifiedTrips).♻️ Suggested fix — add vehicle preference before the final fallback
// 2 - pick one w/o ModifiedTrip gTripUpdates .filter { it.optTrip?.hasModifiedTrip() == false } .takeIf { it.isNotEmpty() } ?.let { woModifiedTrip -> val picked = woModifiedTrip.first() MTLog.d(LOG_TAG, "filterDuplicatesTrips() > pick 1st out of ${woModifiedTrip.size} w/o ModifiedTrip: ${picked.toStringExt()}") add(picked) return@forEach } + // 3 - pick one w/ Vehicle descriptor (even if it has ModifiedTrip) + gTripUpdates + .filter { it.hasVehicle() } + .takeIf { it.isNotEmpty() } + ?.let { wVehicle -> + val picked = wVehicle.first() + MTLog.d(LOG_TAG, "filterDuplicatesTrips() > pick 1st out of ${wVehicle.size} w/ Vehicle: ${picked.toStringExt()}") + add(picked) + return@forEach + } // ELSE -> pick 1st one in the list🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.kt` around lines 39 - 53, Update the duplicate-selection flow in filterDuplicatesTrips by adding a vehicle-descriptor preference after the no-ModifiedTrip selection and before the final first-item fallback. When all candidates have ModifiedTrip, select a duplicate with a valid Vehicle descriptor if one exists; otherwise preserve the existing fallback to the first item.
14-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer a vehicle-aware tiebreaker for all
ModifiedTripduplicates
When every candidate in a group hasModifiedTrip, the final fallback still picks the first entry arbitrarily. If one of those carries a vehicle descriptor and another doesn't, the vehicle-bearing update can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.kt` around lines 14 - 17, Update the duplicate-selection logic around filterDuplicatesTrips so groups whose candidates all have ModifiedTrip use a vehicle-descriptor-aware tiebreaker before falling back to the first entry. Prefer the candidate with a vehicle descriptor, while preserving the existing trip ID, start date, and start time grouping behavior.src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProvider.kt (1)
257-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGood fix, but consider dedicated test coverage for
match()'s new out-of-sync gating.Gating
setTripIdsOutOfSync(true)ontd.hasRouteId()correctly avoids flagging "out of sync" for trip updates belonging to other routes that simply omitroute_id(previously any such trip-id miss across the entire feed would set the flag, since the route-mismatch short-circuit at Lines 235-241 only triggers whenroute_idis present). This is a meaningful improvement.No unit test in
GTFSRealTimeTripUpdatesProviderTests.ktdirectly exercisesmatch(), so this conditional (and the pre-existing unconditional behavior it replaces) has no regression coverage. Consider adding tests for: (1)td.hasRouteId()==true+ trip id miss →tripIdsOutOfSync=true; (2)td.hasRouteId()==false+ trip id miss →tripIdsOutOfSyncstaysfalse.Want me to draft these test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProvider.kt` around lines 257 - 267, Add focused unit tests in GTFSRealTimeTripUpdatesProviderTests for match(): verify a missing trip ID with td.hasRouteId() true sets tripIdsOutOfSync, while the same missing trip ID with no route_id leaves it false. Use the existing test fixtures and setup patterns, and assert both match()’s result and the out-of-sync state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.kt`:
- Around line 39-53: Update the duplicate-selection flow in
filterDuplicatesTrips by adding a vehicle-descriptor preference after the
no-ModifiedTrip selection and before the final first-item fallback. When all
candidates have ModifiedTrip, select a duplicate with a valid Vehicle descriptor
if one exists; otherwise preserve the existing fallback to the first item.
- Around line 14-17: Update the duplicate-selection logic around
filterDuplicatesTrips so groups whose candidates all have ModifiedTrip use a
vehicle-descriptor-aware tiebreaker before falling back to the first entry.
Prefer the candidate with a vehicle descriptor, while preserving the existing
trip ID, start date, and start time grouping behavior.
In
`@src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProvider.kt`:
- Around line 257-267: Add focused unit tests in
GTFSRealTimeTripUpdatesProviderTests for match(): verify a missing trip ID with
td.hasRouteId() true sets tripIdsOutOfSync, while the same missing trip ID with
no route_id leaves it false. Use the existing test fixtures and setup patterns,
and assert both match()’s result and the out-of-sync state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15136dbe-5882-4f4a-9516-06afd92d964f
📒 Files selected for processing (4)
src/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilter.ktsrc/main/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProvider.ktsrc/test/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesFilterTests.ktsrc/test/java/org/mtransit/android/commons/provider/status/GTFSRealTimeTripUpdatesProviderTests.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The best practices suggest feed publishers to include duplicate trip updates w/ and w/o
ModifiedTripSelector