Skip to content

Commit

Permalink
Fix #255 - E047 - Allow vehicle to be assigned to multiple trips in s…
Browse files Browse the repository at this point in the history 
…ame block (#313)

The same vehicle should be able to be assigned to more than one trip if the trips are part of the same block (i.e., the vehicle will serve one trip and then the next trip)

* Add unit test assertion that shows incorrect error logged for E047 (prior to this PR), as described in #255
* Remove BiMap and replace with another pair of HashMaps
* Add check for same vehicle serving multiple trips with the same block for E047
* Also fix error in comment that mistakenly referenced E036
* Clean up left-over references to HashBiMaps, but keep TODO comments for where new rules should be inserted
  • Loading branch information
@barbeau
barbeau committed Nov 22, 2017
1 parent d9fe899 commit f20eda2
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 75 deletions.
2 changes: 1 addition & 1 deletion gtfs-realtime-validator-lib/pom.xml
View file
Expand Up @@ -37,7 +37,7 @@
<artifactId>commons-cli</artifactId>
<version>1.4</version>
</dependency>
<!-- For Bidirectional HashMap (i.e., BiMap) (see E047) -->
<!-- For ordering of stop_time_updates (see E002) and @Immutable (See ViewMessageDetailsModel) -->
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
94 changes: 45 additions & 49 deletions .../java/edu/usf/cutr/gtfsrtvalidator/lib/validation/rules/CrossFeedDescriptorValidator.java
View file
Expand Up @@ -17,8 +17,6 @@

package edu.usf.cutr.gtfsrtvalidator.lib.validation.rules;

import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.transit.realtime.GtfsRealtime;
import edu.usf.cutr.gtfsrtvalidator.lib.model.MessageLogModel;
import edu.usf.cutr.gtfsrtvalidator.lib.model.OccurrenceModel;
Expand All @@ -28,6 +26,7 @@
import edu.usf.cutr.gtfsrtvalidator.lib.validation.interfaces.FeedEntityValidator;
import org.apache.commons.lang3.StringUtils;
import org.onebusaway.gtfs.impl.GtfsDaoImpl;
import org.onebusaway.gtfs.model.Trip;
import org.slf4j.LoggerFactory;

import java.util.*;
Expand Down Expand Up @@ -56,14 +55,23 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsDaoImpl g
List<OccurrenceModel> w003List = new ArrayList<>();
List<OccurrenceModel> e047List = new ArrayList<>();

/*
Create inverse maps, so we can efficiently check if a trip_id in TripUpdates is in VehiclePositions, and if
vehicle_id in VehiclePositions is in TripUpdates.
*/

// key is trip_id from TripUpdates feed, value is vehicle.id
BiMap<String, String> tripUpdates = HashBiMap.create();
HashMap<String, String> tripUpdatesTripIdToVehicleId = new HashMap<>();
// key is vehicle.id from TripUpdates feed, value is trip_id
HashMap<String, String> tripUpdatesVehicleIdToTripId = new HashMap<>();
// A set of trips (key = trip_id) that don't have any vehicle.ids
Set<String> tripsWithoutVehicles = new HashSet<>();
int tripUpdateCount = 0;

// key is vehicle_id from VehiclePositions feed, value is trip_id
BiMap<String, String> vehiclePositions = HashBiMap.create();
HashMap<String, String> vehiclePositionsVehicleIdToTripId = new HashMap<>();
// key is trip_id from VehiclePositions feed, value is vehicle_id
HashMap<String, String> vehiclePositionsTripIdToVehicleId = new HashMap<>();
// A set of vehicles (key = vehicle.id) that don't have any trip_ids
Set<String> vehiclesWithoutTrips = new HashSet<>();
int vehiclePositionCount = 0;
Expand All @@ -78,17 +86,13 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsDaoImpl g
vehicleId = entity.getTripUpdate().getVehicle().getId();
}
if (StringUtils.isEmpty(vehicleId)) {
// Trip does not have a vehicle.id - add it to the set (it can't exist in HashBiMap)
// Trip does not have a vehicle.id - add it to the set
tripsWithoutVehicles.add(tripId);
} else {
// Trip has a vehicle.id - add it to the HashBiMap
try {
tripUpdates.put(tripId, vehicleId);
} catch (IllegalArgumentException e) {
// TODO - Maybe log this as error under new rule? - see https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/33
// However, there are legitimate cases that will end up here - see https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/255
_log.error("Error adding trip_id " + tripId + " -> vehicle_id " + vehicleId + " to TripUpdates HashBiMap. TripUpdate exists twice in feed, or more than one TripUpdate is assigned to the same vehicle. " + e);
}
// Trip has a vehicle.id - add it to the HashMap
tripUpdatesTripIdToVehicleId.put(tripId, vehicleId);
tripUpdatesVehicleIdToTripId.put(vehicleId, tripId);
// TODO - New rule - check that there is at most one TripUpdate entity per scheduled trip_id - see https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/33
}

}
Expand All @@ -100,16 +104,13 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsDaoImpl g
tripId = entity.getVehicle().getTrip().getTripId();
}
if (StringUtils.isEmpty(tripId)) {
// Vehicle does not have a trip_id - add it to the set (it can't exist in HashBiMap)
// Vehicle does not have a trip_id - add it to the set
vehiclesWithoutTrips.add(vehicleId);
} else {
// Vehicle has a trip_id - add it to the HashBiMap
try {
vehiclePositions.put(vehicleId, tripId);
} catch (IllegalArgumentException e) {
// TODO - We should log this as error under new rule - see https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/38
_log.error("Error adding vehicle.id " + vehicleId + " -> trip_id " + tripId + " to VehiclePositions HashBiMap. Vehicle exists twice in feed, or more than one vehicle is assigned to same trip. " + e);
}
// Vehicle has a trip_id - add it to the HashMap
vehiclePositionsVehicleIdToTripId.put(vehicleId, tripId);
vehiclePositionsTripIdToVehicleId.put(tripId, vehicleId);
// TODO - New rule - check that there is at most one vehicle assigned each trip - see https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/38
}
}
}
Expand All @@ -120,56 +121,43 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsDaoImpl g
return errors;
}

/**
* Create inverse maps, so we can efficiently check if a trip_id in TripUpdates is in VehiclePositions, and if
* vehicle_id in VehiclePositions is in TripUpdates.
*
* tripUpdatesInverse - A map of vehicle_ids to trip_ids, from the TripUpdates feed
* vehiclePositionsInverse - A map of trip_ids to vehicle_ids, from the VehiclePositions feed
*
* Note that we still need to check vehiclesWithoutTrips and tripsWithoutVehicles, as these trips/vehicles can't exist in HashBiMaps.
* See https://github.com/CUTR-at-USF/gtfs-realtime-validator/issues/241#issuecomment-313194304.
*/
BiMap<String, String> tripUpdatesInverse = tripUpdates.inverse();
BiMap<String, String> vehiclePositionsInverse = vehiclePositions.inverse();

// Check all trips that contained a vehicle
for (Map.Entry<String, String> trip : tripUpdates.entrySet()) {
if (!vehiclePositionsInverse.containsKey(trip.getKey())) {
for (Map.Entry<String, String> trip : tripUpdatesTripIdToVehicleId.entrySet()) {
if (!vehiclePositionsTripIdToVehicleId.containsKey(trip.getKey())) {
// W003 - TripUpdates feed has a trip_id that's not in VehiclePositions feed
RuleUtils.addOccurrence(W003, "trip_id " + trip.getKey() + " is in TripUpdates but not in VehiclePositions feed", w003List, _log);
}
if (!vehiclePositions.containsKey(trip.getValue()) && !vehiclesWithoutTrips.contains(trip.getValue())) {
if (!vehiclePositionsVehicleIdToTripId.containsKey(trip.getValue()) && !vehiclesWithoutTrips.contains(trip.getValue())) {
// W003 - TripUpdates feed has a vehicle_id that's not in VehiclePositions feed
RuleUtils.addOccurrence(W003, "vehicle_id " + trip.getValue() + " is in TripUpdates but not in VehiclePositions feed", w003List, _log);
}
checkE047TripUpdates(trip, vehiclePositionsInverse, e047List);
checkE047TripUpdates(trip, vehiclePositionsTripIdToVehicleId, e047List);
}

// Check all vehicles that contained a trip
for (Map.Entry<String, String> vehiclePosition : vehiclePositions.entrySet()) {
if (!tripUpdatesInverse.containsKey(vehiclePosition.getKey())) {
for (Map.Entry<String, String> vehiclePosition : vehiclePositionsVehicleIdToTripId.entrySet()) {
if (!tripUpdatesVehicleIdToTripId.containsKey(vehiclePosition.getKey())) {
// W003 - VehiclePositions has a vehicle_id that's not in TripUpdates feed
RuleUtils.addOccurrence(W003, "vehicle_id " + vehiclePosition.getKey() + " is in VehiclePositions but not in TripUpdates feed", w003List, _log);
}
if (!tripUpdates.containsKey(vehiclePosition.getValue()) && !tripsWithoutVehicles.contains(vehiclePosition.getValue())) {
if (!tripUpdatesTripIdToVehicleId.containsKey(vehiclePosition.getValue()) && !tripsWithoutVehicles.contains(vehiclePosition.getValue())) {
// W003 - VehiclePositions has a trip_id that's not in the TripUpdates feed
RuleUtils.addOccurrence(W003, "trip_id " + vehiclePosition.getValue() + " is in VehiclePositions but not in TripUpdates feed", w003List, _log);
}
checkE047VehiclePositions(vehiclePosition, tripUpdatesInverse, e047List);
checkE047VehiclePositions(vehiclePosition, tripUpdatesVehicleIdToTripId, gtfsMetadata, e047List);
}

// Check all trips that did NOT contain a vehicle
for (String trip_id : tripsWithoutVehicles) {
if (!vehiclePositionsInverse.containsKey(trip_id)) {
if (!vehiclePositionsTripIdToVehicleId.containsKey(trip_id)) {
// W003 - TripUpdates feed has a trip_id that's not in VehiclePositions feed
RuleUtils.addOccurrence(W003, "trip_id " + trip_id + " is in TripUpdates but not in VehiclePositions feed", w003List, _log);
}
}

// Check all vehicles that did NOT contain a trip
for (String vehicle_id : vehiclesWithoutTrips) {
if (!tripUpdatesInverse.containsKey(vehicle_id)) {
if (!tripUpdatesVehicleIdToTripId.containsKey(vehicle_id)) {
// W003 - VehiclePositions has a vehicle_id that's not in TripUpdates feed
RuleUtils.addOccurrence(W003, "vehicle_id " + vehicle_id + " is in VehiclePositions but not in TripUpdates feed", w003List, _log);
}
Expand Down Expand Up @@ -205,13 +193,13 @@ private boolean hasVehicleId(GtfsRealtime.VehiclePosition vehiclePosition) {
}

/**
* Checks E036 - "VehiclePosition and TripUpdate ID pairing mismatch" for TripUpdates, and adds a found error to the provided error list.
* Checks E047 - "VehiclePosition and TripUpdate ID pairing mismatch" for TripUpdates, and adds a found error to the provided error list.
*
* @param trip A map entry of a trip_id to a vehicle_id that represents a trip in TripUpdates and a vehicle_id that the trip contains
* @param vehiclePositionsInverse An inverse map of a VehiclePositions feed, with keys that represent the trip_ids that each VehiclePosition contains, and the value being the VehiclePosition vehicle.id that contains the trip_id
* @param errors the list to add the errors to
*/
private void checkE047TripUpdates(Map.Entry<String, String> trip, BiMap<String, String> vehiclePositionsInverse, List<OccurrenceModel> errors) {
private void checkE047TripUpdates(Map.Entry<String, String> trip, HashMap<String, String> vehiclePositionsInverse, List<OccurrenceModel> errors) {
String vehiclePositionsVehicleId = vehiclePositionsInverse.get(trip.getKey());
String tripUpdatesVehicleId = trip.getValue();
if (!StringUtils.isEmpty(vehiclePositionsVehicleId)) {
Expand All @@ -222,18 +210,26 @@ private void checkE047TripUpdates(Map.Entry<String, String> trip, BiMap<String,
}

/**
* Checks E036 - "VehiclePosition and TripUpdate ID pairing mismatch" for VehiclePositions, and adds a found error to the provided error list.
* Checks E047 - "VehiclePosition and TripUpdate ID pairing mismatch" for VehiclePositions, and adds a found error to the provided error list.
*
* @param vehicle A map entry of a vehicle_id to a trip_id that represents a vehicle in VehiclePositions and the trip_id that the vehicle contains
* @param tripUpdatesInverse An inverse map of a TripUpdates feed, with keys that represent the vehicle_ids that each TripUpdate contains, and the value being the TripUpdate trip_id that contains the vehicle.id
* @param errors the list to add the errors to
*/
private void checkE047VehiclePositions(Map.Entry<String, String> vehicle, BiMap<String, String> tripUpdatesInverse, List<OccurrenceModel> errors) {
private void checkE047VehiclePositions(Map.Entry<String, String> vehicle, HashMap<String, String> tripUpdatesInverse, GtfsMetadata gtfsMetadata, List<OccurrenceModel> errors) {
String tripUpdatesTripId = tripUpdatesInverse.get(vehicle.getKey());
String vehiclePositionsTripId = vehicle.getValue();
if (!StringUtils.isEmpty(tripUpdatesTripId)) {
if (!vehiclePositionsTripId.equals(tripUpdatesTripId)) {
RuleUtils.addOccurrence(E047, "trip_id " + vehicle.getValue() + " and vehicle_id " + vehicle.getKey() + " pairing in VehiclePositions does not match trip_id " + tripUpdatesTripId + " and vehicle_id " + vehicle.getKey() + " pairing in TripUpdates feed", errors, _log);
// Log E047 if either trip_id is missing from GTFS, if the block_id is missing for either trip (block_id is an optional field) or if the two trips aren't in the same block
Trip tripA = gtfsMetadata.getTrips().get(vehiclePositionsTripId);
Trip tripB = gtfsMetadata.getTrips().get(tripUpdatesTripId);
if (tripA == null || tripB == null ||
StringUtils.isEmpty(tripA.getBlockId()) || StringUtils.isEmpty(tripB.getBlockId()) ||
!tripA.getBlockId().equals(tripB.getBlockId())) {
// E036 - "VehiclePosition and TripUpdate ID pairing mismatch" for VehiclePositions
RuleUtils.addOccurrence(E047, "trip_id " + vehicle.getValue() + " and vehicle_id " + vehicle.getKey() + " pairing in VehiclePositions does not match trip_id " + tripUpdatesTripId + " and vehicle_id " + vehicle.getKey() + " pairing in TripUpdates feed and trip block_ids aren't the same", errors, _log);
}
}
}
}
Expand Down

0 comments on commit f20eda2

Please sign in to comment.