From 76ed09efa266da5e16d13b32d1c3f5a99b41d5c6 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Mon, 19 Jun 2023 14:52:58 +0100 Subject: [PATCH 01/19] feat(Exception based scheduling with calendar dates): Update to allow calednar dates to define trip --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 14 +++--- .../gtfs/graphql/GraphQLGtfsSchema.java | 19 ++++++++ .../gtfs/loader/JdbcGTFSFeedConverter.java | 2 + .../gtfs/loader/JdbcGtfsExporter.java | 29 +++++++----- .../gtfs/loader/JdbcGtfsSnapshotter.java | 30 +++++++------ .../java/com/conveyal/gtfs/loader/Table.java | 2 +- .../com/conveyal/gtfs/model/CalendarDate.java | 45 +++++++++++-------- .../java/com/conveyal/gtfs/model/Entity.java | 19 ++++++++ .../java/com/conveyal/gtfs/model/Trip.java | 2 +- .../gtfs/graphql/GTFSGraphQLTest.java | 8 ++++ .../resources/fake-agency/calendar_dates.txt | 3 +- .../resources/graphql/feedCalendarDates.txt | 10 +++++ 12 files changed, 131 insertions(+), 52 deletions(-) create mode 100644 src/test/resources/graphql/feedCalendarDates.txt diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index 8e6de45e2..ba2cd4fa5 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -84,6 +84,10 @@ public class GTFSFeed implements Cloneable, Closeable { /* A service is a calendar entry and all calendar_dates that modify that calendar entry. */ public final BTreeMap services; + /* A calendar date specifies a single date that a service runs. These dates are wholly separate to calendar dates + * that are related to a calendar and defined in 'services'. */ + public final Map calendarDates; + /* A place to accumulate errors while the feed is loaded. Tolerate as many errors as possible and keep on loading. */ public final NavigableSet errors; @@ -153,21 +157,18 @@ else if (feedId == null || feedId.isEmpty()) { new Agency.Loader(this).loadTable(zip); - // calendars and calendar dates are joined into services. This means a lot of manipulating service objects as - // they are loaded; since mapdb keys/values are immutable, load them in memory then copy them to MapDB once - // we're done loading them + // calendars and calendar dates are joined into services, where there is a match on service id. This means a lot + // of manipulating service objects as they are loaded; since mapdb keys/values are immutable, load them in + // memory then copy them to MapDB once we're done loading them. Map serviceTable = new HashMap<>(); new Calendar.Loader(this, serviceTable).loadTable(zip); new CalendarDate.Loader(this, serviceTable).loadTable(zip); this.services.putAll(serviceTable); - serviceTable = null; // free memory - // Same deal Map fares = new HashMap<>(); new FareAttribute.Loader(this, fares).loadTable(zip); new FareRule.Loader(this, fares).loadTable(zip); this.fares.putAll(fares); - fares = null; // free memory new Route.Loader(this).loadTable(zip); new ShapePoint.Loader(this).loadTable(zip); @@ -627,6 +628,7 @@ private GTFSFeed (DB db) { this.db = db; agency = db.getTreeMap("agency"); + calendarDates = db.getTreeMap("calendar_dates"); feedInfo = db.getTreeMap("feed_info"); routes = db.getTreeMap("routes"); trips = db.getTreeMap("trips"); diff --git a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java index fe5d901ce..975a792a0 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java +++ b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java @@ -90,6 +90,14 @@ public class GraphQLGtfsSchema { .field(MapFetcher.field("description")) .build(); + // Represents rows from calendar_dates.txt + public static final GraphQLObjectType calendarDatesType = newObject() + .name("calendar_dates") + .field(MapFetcher.field("id", GraphQLInt)) + .field(MapFetcher.field("service_id")) + .field(MapFetcher.field("date")) + .field(MapFetcher.field("exception_type", GraphQLInt)) + .build(); private static final GraphQLScalarType stringList = new GraphQLScalarType("StringList", "List of Strings", new StringCoercing()); // Represents GTFS Editor service exceptions. @@ -715,6 +723,17 @@ public class GraphQLGtfsSchema { .dataFetcher(new JDBCFetcher("calendar")) .build() ) + .field(newFieldDefinition() + .name("calendar_dates") + .type(new GraphQLList(GraphQLGtfsSchema.calendarDatesType)) + .argument(stringArg("namespace")) // FIXME maybe these nested namespace arguments are not doing anything. + .argument(multiStringArg("service_id")) + .argument(intArg(ID_ARG)) + .argument(intArg(LIMIT_ARG)) + .argument(intArg(OFFSET_ARG)) + .dataFetcher(new JDBCFetcher("calendar_dates")) + .build() + ) .field(newFieldDefinition() .name("fares") .type(new GraphQLList(GraphQLGtfsSchema.fareType)) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java index 01990b24a..b91b066e0 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java @@ -92,6 +92,8 @@ public FeedLoadResult loadTables () { .map(service -> service.calendar_dates.values()) .flatMap(Collection::stream) .collect(Collectors.toList()); + // Combine calendar dates and calendar dates derived from services. + calendarDates.addAll(new ArrayList<>(gtfsFeed.calendarDates.values())); List fareAttributes = gtfsFeed.fares.values() .stream() .map(fare -> fare.fare_attribute) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 61f6bb8cf..404477d22 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -11,7 +11,6 @@ import org.postgresql.core.BaseConnection; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - import javax.sql.DataSource; import java.io.File; import java.io.FileOutputStream; @@ -109,6 +108,7 @@ public FeedLoadResult exportTables() { } else { result.calendar = export(Table.CALENDAR, connection); } + boolean exportCalendarDatesDirectFromTable = true; if (fromEditor) { // Export schedule exceptions in place of calendar dates if exporting a feed/schema that represents an editor snapshot. GTFSFeed feed = new GTFSFeed(); @@ -165,22 +165,27 @@ public FeedLoadResult exportTables() { feed.services.put(cal.service_id, service); } if (calendarDateCount == 0) { - LOG.info("No calendar dates found. Skipping table."); + LOG.info("No calendar dates found within scheduled exceptions."); } else { + // In this scenario all values in the calendar dates table need to be combined with calendar + // dates derived from scheduled exceptions and then written to file. + exportCalendarDatesDirectFromTable = false; + JDBCTableReader calendarDatesReader = new JDBCTableReader( + Table.CALENDAR_DATES, + dataSource, + feedIdToExport + ".", + EntityPopulator.CALENDAR_DATE + ); + Iterable calendarDates = calendarDatesReader.getAll(); + for (CalendarDate calendarDate : calendarDates) { + feed.calendarDates.put(calendarDate.service_id, calendarDate); + } LOG.info("Writing {} calendar dates from schedule exceptions", calendarDateCount); new CalendarDate.Writer(feed).writeTable(zipOutputStream); } - } else { - // No calendar records exist, export calendar_dates as is and hope for the best. - // This situation will occur in at least 2 scenarios: - // 1. A GTFS has been loaded into the editor that had only the calendar_dates.txt file - // and no further edits were made before exporting to a snapshot - // 2. A new GTFS has been created from scratch and calendar information has yet to be added. - // This will result in an invalid GTFS, but it was what the user wanted so ¯\_(ツ)_/¯ - result.calendarDates = export(Table.CALENDAR_DATES, connection); } - } else { - // Otherwise, simply export the calendar dates as they were loaded in. + } + if (exportCalendarDatesDirectFromTable) { result.calendarDates = export(Table.CALENDAR_DATES, connection); } result.fareAttributes = export(Table.FARE_ATTRIBUTES, connection); diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 3b36a9b8e..87c356b29 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -220,6 +220,19 @@ private TableLoadResult createScheduleExceptionsTable() { scheduleExceptionsStatement ); + // Fetch all entries in the calendar table to generate set of serviceIds that exist in the calendar + // table. + JDBCTableReader calendarReader = new JDBCTableReader( + Table.CALENDAR, + dataSource, + feedIdToSnapshot + ".", + EntityPopulator.CALENDAR + ); + Set calendarServiceIds = new HashSet<>(); + for (Calendar calendar : calendarReader.getAll()) { + calendarServiceIds.add(calendar.service_id); + } + JDBCTableReader calendarDatesReader = new JDBCTableReader( Table.CALENDAR_DATES, dataSource, @@ -240,6 +253,10 @@ private TableLoadResult createScheduleExceptionsTable() { LOG.warn("Encountered calendar date record with null value for date field. Skipping."); continue; } + if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { + LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); + continue; + } String date = calendarDate.date.format(DateTimeFormatter.BASIC_ISO_DATE); if (calendarDate.exception_type == 1) { addedServiceForDate.put(date, calendarDate.service_id); @@ -278,19 +295,6 @@ private TableLoadResult createScheduleExceptionsTable() { } scheduleExceptionsTracker.executeRemaining(); - // fetch all entries in the calendar table to generate set of serviceIds that exist in the calendar - // table. - JDBCTableReader calendarReader = new JDBCTableReader( - Table.CALENDAR, - dataSource, - feedIdToSnapshot + ".", - EntityPopulator.CALENDAR - ); - Set calendarServiceIds = new HashSet<>(); - for (Calendar calendar : calendarReader.getAll()) { - calendarServiceIds.add(calendar.service_id); - } - // For service_ids that only existed in the calendar_dates table, insert auto-generated, "blank" // (no days of week specified) calendar entries. sql = String.format( diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index b3344c5c5..21ec4d8b9 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -159,7 +159,7 @@ public Table (String name, Class entityClass, Requirement requ ); public static final Table CALENDAR_DATES = new Table("calendar_dates", CalendarDate.class, OPTIONAL, - new StringField("service_id", REQUIRED).isReferenceTo(CALENDAR), + new StringField("service_id", REQUIRED), new DateField("date", REQUIRED), new IntegerField("exception_type", REQUIRED, 1, 2) ).keyFieldIsNotUnique() diff --git a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java index b737e62ff..4ce088e80 100644 --- a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java +++ b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java @@ -70,20 +70,27 @@ protected boolean isRequired() { @Override public void loadOneRow() throws IOException { - /* Calendars and Fares are special: they are stored as joined tables rather than simple maps. */ - String service_id = getStringField("service_id", true); - Service service = services.computeIfAbsent(service_id, Service::new); + /* Calendars and calendar dates that are matched on service id are special: they are stored as joined tables + rather than simple maps. */ + String serviceId = getStringField("service_id", true); LocalDate date = getDateField("date", true); - if (service.calendar_dates.containsKey(date)) { - feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)")); + CalendarDate cd = new CalendarDate(); + cd.id = row + 1; // offset line number by 1 to account for 0-based row index + cd.service_id = serviceId; + cd.date = date; + cd.exception_type = getIntField("exception_type", true, 1, 2); + cd.feed = feed; + if (services.containsKey(serviceId)) { + // Calendar date is related to calendar. + Service service = services.computeIfAbsent(serviceId, Service::new); + if (service.calendar_dates.containsKey(date)) { + feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)")); + } else { + service.calendar_dates.put(date, cd); + } } else { - CalendarDate cd = new CalendarDate(); - cd.id = row + 1; // offset line number by 1 to account for 0-based row index - cd.service_id = service_id; - cd.date = date; - cd.exception_type = getIntField("exception_type", true, 1, 2); - cd.feed = feed; - service.calendar_dates.put(date, cd); + // Calendar date is a stand-alone date that is not related to a calendar. + if (cd.service_id != null) feed.calendarDates.put(cd.service_id, cd); } } } @@ -108,13 +115,15 @@ protected void writeOneRow(CalendarDate d) throws IOException { @Override protected Iterator iterator() { + // Combine all calendar dates references. This is only used to write to table/file. Iterator serviceIterator = feed.services.values().iterator(); - return Iterators.concat(Iterators.transform(serviceIterator, new Function> () { - @Override - public Iterator apply(Service service) { - return service.calendar_dates.values().iterator(); - } - })); + Iterator calendarDatesFromServices = Iterators.concat( + Iterators.transform(serviceIterator, service -> service.calendar_dates.values().iterator()) + ); + Iterator calendarDatesIterator = feed.calendarDates.values().iterator(); + return Iterators.concat( + calendarDatesIterator, calendarDatesFromServices + ); } } } diff --git a/src/main/java/com/conveyal/gtfs/model/Entity.java b/src/main/java/com/conveyal/gtfs/model/Entity.java index 29657b6e6..bb154234b 100644 --- a/src/main/java/com/conveyal/gtfs/model/Entity.java +++ b/src/main/java/com/conveyal/gtfs/model/Entity.java @@ -268,6 +268,25 @@ protected V getRefField(String column, boolean required, Map target return val; } + /** + * Used to check referential integrity in two targets. E.g. a service id defined in a trip could either be in + * services or calendar dates. + */ + protected void getRefField(String column, boolean required, Map target1, Map target2) throws IOException { + String str = getFieldCheckRequired(column, required); + if (str != null) { + V val = target1.get(str); + J val2 = target2.get(str); + String transitId = column + ":" + str; + if (!feed.transitIds.contains(transitId)) { + feed.transitIds.add(transitId); + if (val == null && val2 == null) { + feed.errors.add(new ReferentialIntegrityError(tableName, row, column, str)); + } + } + } + } + protected abstract boolean isRequired(); /** Implemented by subclasses to read one row, produce one GTFS entity, and store that entity in a map. */ diff --git a/src/main/java/com/conveyal/gtfs/model/Trip.java b/src/main/java/com/conveyal/gtfs/model/Trip.java index 416854974..5f8f0f4a1 100644 --- a/src/main/java/com/conveyal/gtfs/model/Trip.java +++ b/src/main/java/com/conveyal/gtfs/model/Trip.java @@ -85,7 +85,7 @@ public void loadOneRow() throws IOException { Routes because they would be serialized into the MapDB. */ // TODO confirm existence of shape ID - getRefField("service_id", true, feed.services); + getRefField("service_id", true, feed.services, feed.calendarDates); getRefField("route_id", true, feed.routes); } diff --git a/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java b/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java index 240683306..fc0d72cb5 100644 --- a/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java +++ b/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java @@ -164,6 +164,14 @@ public void canFetchCalendars() { }); } + /** Tests that the calendar dates of a feed can be fetched. */ + @Test + void canFetchCalendarDates() { + assertTimeout(Duration.ofMillis(TEST_TIMEOUT), () -> { + MatcherAssert.assertThat(queryGraphQL("feedCalendarDates.txt"), matchesSnapshot()); + }); + } + /** Tests that the fares of a feed can be fetched. */ @Test public void canFetchFares() { diff --git a/src/test/resources/fake-agency/calendar_dates.txt b/src/test/resources/fake-agency/calendar_dates.txt index 403ee2bbe..377866c8e 100755 --- a/src/test/resources/fake-agency/calendar_dates.txt +++ b/src/test/resources/fake-agency/calendar_dates.txt @@ -1,2 +1,3 @@ service_id,date,exception_type -04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 \ No newline at end of file +04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 +exception-in-own-right,20230619,2 \ No newline at end of file diff --git a/src/test/resources/graphql/feedCalendarDates.txt b/src/test/resources/graphql/feedCalendarDates.txt new file mode 100644 index 000000000..c2dd8b7a4 --- /dev/null +++ b/src/test/resources/graphql/feedCalendarDates.txt @@ -0,0 +1,10 @@ +query ($namespace: String) { + feed(namespace: $namespace) { + feed_version + calendar_dates { + service_id + date + exception_type + } + } +} \ No newline at end of file From dfed0a3915beded7edd34319dd5edbf4722bdaa6 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Wed, 21 Jun 2023 11:13:29 +0100 Subject: [PATCH 02/19] refactor(ServiceValidator.java): Update to exclude calendar dates that are not related to calendars --- .../gtfs/validator/ServiceValidator.java | 7 +++++++ .../GTFSGraphQLTest/canFetchCalendarDates-0.json | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index 83036fe4a..3b621dd80 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -120,8 +120,11 @@ public void complete(ValidationResult validationResult) { private void validateServiceInfo(ValidationResult validationResult) { LOG.info("Merging calendars and calendar_dates..."); + Set calendarServiceIds = new HashSet<>(); + // First handle the calendar entries, which define repeating weekly schedules. for (Calendar calendar : feed.calendars) { + calendarServiceIds.add(calendar.service_id); // Validate that calendars apply to at least one day of the week. if (!isCalendarUsedDuringWeek(calendar)) { if (errorStorage != null) registerError(calendar, SERVICE_WITHOUT_DAYS_OF_WEEK); @@ -151,6 +154,10 @@ private void validateServiceInfo(ValidationResult validationResult) { // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. for (CalendarDate calendarDate : feed.calendarDates) { + if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { + LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); + continue; + } ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); if (calendarDate.exception_type == 1) { // Service added, add to set for this date. diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json new file mode 100644 index 000000000..51c43fd59 --- /dev/null +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json @@ -0,0 +1,16 @@ +{ + "data" : { + "feed" : { + "calendar_dates" : [ { + "date" : "20170916", + "exception_type" : 2, + "service_id" : "04100312-8fe1-46a5-a9f2-556f39478f57" + }, { + "date" : "20230619", + "exception_type" : 2, + "service_id" : "exception-in-own-right" + } ], + "feed_version" : "1.0" + } + } +} \ No newline at end of file From 3b99138bdb3da3620403af4880addb44d475d1ba Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Wed, 21 Jun 2023 14:51:29 +0100 Subject: [PATCH 03/19] refactor(Removed trips and calendar date service id references from services): --- .../gtfs/validator/ServiceValidator.java | 61 +++++++++++-------- src/test/java/com/conveyal/gtfs/GTFSTest.java | 26 ++------ 2 files changed, 43 insertions(+), 44 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index 3b621dd80..bc0df3dc8 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -62,6 +62,8 @@ public class ServiceValidator extends TripValidator { private Map dateInfoForDate = new HashMap<>(); + private Set calendarServiceIds = new HashSet<>(); + public ServiceValidator(Feed feed, SQLErrorStorage errorStorage) { super(feed, errorStorage); } @@ -92,15 +94,27 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< // ERR return; } + + // Create a list of calendar service ids. We are only interested in creating services for calendar and related + // calendar dates. + for (Calendar calendar : feed.calendars) { + calendarServiceIds.add(calendar.service_id); + } + // Get the map from modes to service durations in seconds for this trip's service ID. // Create a new empty map if it doesn't yet exist. - ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(trip.service_id, ServiceInfo::new); - if (route != null) { - // Increment the service duration for this trip's transport mode and service ID. - serviceInfo.durationByRouteType.adjustOrPutValue(route.route_type, tripDurationSeconds, tripDurationSeconds); + if (calendarServiceIds.contains(trip.service_id)) { + // Trip contains calendar service id, therefore we are interested in creating service info for this service id. + ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(trip.service_id, ServiceInfo::new); + if (route != null) { + // Increment the service duration for this trip's transport mode and service ID. + serviceInfo.durationByRouteType.adjustOrPutValue(route.route_type, tripDurationSeconds, tripDurationSeconds); + } + // Record which trips occur on each service_id. + serviceInfo.tripIds.add(trip.trip_id); + LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); } - // Record which trips occur on each service_id. - serviceInfo.tripIds.add(trip.trip_id); + // TODO validate mode codes } @@ -120,11 +134,8 @@ public void complete(ValidationResult validationResult) { private void validateServiceInfo(ValidationResult validationResult) { LOG.info("Merging calendars and calendar_dates..."); - Set calendarServiceIds = new HashSet<>(); - // First handle the calendar entries, which define repeating weekly schedules. for (Calendar calendar : feed.calendars) { - calendarServiceIds.add(calendar.service_id); // Validate that calendars apply to at least one day of the week. if (!isCalendarUsedDuringWeek(calendar)) { if (errorStorage != null) registerError(calendar, SERVICE_WITHOUT_DAYS_OF_WEEK); @@ -152,22 +163,24 @@ private void validateServiceInfo(ValidationResult validationResult) { } } + // Next remove any calendar date entries from serviceInfoForServiceId which + // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. - for (CalendarDate calendarDate : feed.calendarDates) { - if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { - LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); - continue; - } - ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); - if (calendarDate.exception_type == 1) { - // Service added, add to set for this date. - serviceInfo.datesActive.add(calendarDate.date); - } else if (calendarDate.exception_type == 2) { - // Service removed, remove from Set for this date. - serviceInfo.datesActive.remove(calendarDate.date); - } - // Otherwise exception_type is out of range. This should already have been caught during the loading phase. - } +// for (CalendarDate calendarDate : feed.calendarDates) { +// if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { +// LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); +// continue; +// } +// ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); +// if (calendarDate.exception_type == 1) { +// // Service added, add to set for this date. +// serviceInfo.datesActive.add(calendarDate.date); +// } else if (calendarDate.exception_type == 2) { +// // Service removed, remove from Set for this date. +// serviceInfo.datesActive.remove(calendarDate.date); +// } +// // Otherwise exception_type is out of range. This should already have been caught during the loading phase. +// } /* A view that is similar to ServiceInfo class, but doesn't deal well with missing IDs in either subquery: diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 19b0413d8..d9706e220 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -111,8 +111,7 @@ public void canLoadAndExportSimpleAgency() { new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), - new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) + new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")) ); assertThat( runIntegrationTestOnFolder( @@ -143,7 +142,6 @@ public void canLoadFeedWithBadDates () { new ErrorExpectation(NewGTFSErrorType.DATE_FORMAT), new ErrorExpectation(NewGTFSErrorType.DATE_FORMAT), new ErrorExpectation(NewGTFSErrorType.DATE_FORMAT), - new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), new ErrorExpectation(NewGTFSErrorType.DATE_FORMAT), new ErrorExpectation(NewGTFSErrorType.DATE_FORMAT), // The below "wrong number of fields" errors are for empty new lines @@ -158,8 +156,8 @@ public void canLoadFeedWithBadDates () { new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), new ErrorExpectation(NewGTFSErrorType.TRIP_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), - new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) + new ErrorExpectation(NewGTFSErrorType.TRIP_NEVER_ACTIVE), + new ErrorExpectation(NewGTFSErrorType.NO_SERVICE) ); assertThat( "Integration test passes", @@ -282,8 +280,7 @@ public void canLoadAndExportSimpleAgencyInSubDirectory() { new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED), - new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) + new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED) ); assertThat( runIntegrationTestOnZipFile(zipFileName, nullValue(), fakeAgencyPersistenceExpectations, errorExpectations), @@ -359,7 +356,8 @@ public void canLoadAndExportSimpleAgencyWithOnlyCalendarDates() { ErrorExpectation[] errorExpectations = ErrorExpectation.list( new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), + new ErrorExpectation(NewGTFSErrorType.NO_SERVICE) ); assertThat( runIntegrationTestOnFolder( @@ -414,18 +412,6 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new RecordExpectation("exception_type", 2) } ), - // calendar-dates.txt-only expectation - new PersistenceExpectation( - "calendar", - new RecordExpectation[]{ - new RecordExpectation( - "service_id", "only-in-calendar-dates-txt" - ), - new RecordExpectation("start_date", 20170916), - new RecordExpectation("end_date", 20170916) - }, - true - ), new PersistenceExpectation( "calendar_dates", new RecordExpectation[]{ From 33445fdab6d7478a92b29b5d48ec947849022ac9 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 22 Jun 2023 10:20:09 +0100 Subject: [PATCH 04/19] refactor(Added multi foreign reference functionality and addressed failing unit tests): --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 1 + .../java/com/conveyal/gtfs/loader/Field.java | 7 +- .../conveyal/gtfs/loader/JdbcTableWriter.java | 184 +++++++++++------- .../gtfs/loader/ReferenceTracker.java | 46 ++++- .../java/com/conveyal/gtfs/loader/Table.java | 3 +- src/test/java/com/conveyal/gtfs/GTFSTest.java | 2 +- .../GTFSGraphQLTest/canFetchErrors-0.json | 13 -- .../canFetchFeedRowCounts-0.json | 4 +- 8 files changed, 156 insertions(+), 104 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 477c034dc..575d7d4aa 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -32,6 +32,7 @@ public enum NewGTFSErrorType { MISSING_ARRIVAL_OR_DEPARTURE(Priority.MEDIUM, "First and last stop times are required to have both an arrival and departure time."), MISSING_COLUMN(Priority.MEDIUM, "A required column was missing from a table."), MISSING_FIELD(Priority.MEDIUM, "A required field was missing or empty in a particular row."), + MISSING_FOREIGN_TABLE_REFERENCE(Priority.HIGH, "This line references an ID that must exist in a single foreign table."), MISSING_SHAPE(Priority.MEDIUM, "???"), MISSING_TABLE(Priority.MEDIUM, "This table is required by the GTFS specification but is missing."), MULTIPLE_SHAPES_FOR_PATTERN(Priority.MEDIUM, "Multiple shapes found for a single unique sequence of stops (i.e, trip pattern)."), diff --git a/src/main/java/com/conveyal/gtfs/loader/Field.java b/src/main/java/com/conveyal/gtfs/loader/Field.java index 52cda789c..13c2c74a9 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Field.java +++ b/src/main/java/com/conveyal/gtfs/loader/Field.java @@ -8,6 +8,7 @@ import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.SQLType; +import java.util.HashSet; import java.util.Set; /** @@ -46,7 +47,7 @@ public abstract class Field { * Indicates that this field acts as a foreign key to this referenced table. This is used when checking referential * integrity when loading a feed. * */ - public Table referenceTable = null; + public HashSet referenceTables = new HashSet<>(); private boolean shouldBeIndexed; private boolean emptyValuePermitted; private boolean isConditionallyRequired; @@ -138,7 +139,7 @@ public boolean isRequired () { * a many-to-many reference. */ public boolean isForeignReference () { - return this.referenceTable != null; + return !this.referenceTables.isEmpty(); } /** @@ -181,7 +182,7 @@ public boolean shouldBeIndexed() { * @return this same Field instance */ public Field isReferenceTo(Table table) { - this.referenceTable = table; + referenceTables.add(table); return this; } diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index ffd669ed1..99e7f1ae8 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -594,6 +594,7 @@ private String updateChildTable( boolean hasOrderField = orderFieldName != null; int previousOrder = -1; TIntSet orderValues = new TIntHashSet(); + Multimap> foreignReferencesPerTable = HashMultimap.create(); Multimap referencesPerTable = HashMultimap.create(); int cumulativeTravelTime = 0; for (JsonNode entityNode : subEntities) { @@ -602,19 +603,9 @@ private String updateChildTable( // Always override the key field (shape_id for shapes, pattern_id for patterns) regardless of the entity's // actual value. subEntity.put(keyField.name, keyValue); - // Check any references the sub entity might have. For example, this checks that stop_id values on - // pattern_stops refer to entities that actually exist in the stops table. NOTE: This skips the "specTable", - // i.e., for pattern stops it will not check pattern_id references. This is enforced above with the put key - // field statement above. - for (Field field : subTable.specFields()) { - if (field.referenceTable != null && !field.referenceTable.name.equals(specTable.name)) { - JsonNode refValueNode = subEntity.get(field.name); - // Skip over references that are null but not required (e.g., route_id in fare_rules). - if (refValueNode.isNull() && !field.isRequired()) continue; - String refValue = refValueNode.asText(); - referencesPerTable.put(field.referenceTable, refValue); - } - } + + checkTableReferences(foreignReferencesPerTable, referencesPerTable, specTable, subTable, subEntity); + // Insert new sub-entity. if (entityCount == 0) { // If handling first iteration, create the prepared statement (later iterations will add to batch). @@ -691,6 +682,41 @@ private String updateChildTable( return keyValue; } + /** + * Check any references the sub entity might have. For example, this checks that stop_id values on + * pattern_stops refer to entities that actually exist in the stops table. NOTE: This skips the "specTable", + * i.e., for pattern stops it will not check pattern_id references. This is enforced above with the put key + * field statement above. + */ + private void checkTableReferences( + Multimap> foreignReferencesPerTable, + Multimap referencesPerTable, + Table specTable, + Table subTable, + ObjectNode subEntity + ) { + for (Field field : subTable.specFields()) { + if (field.referenceTables.isEmpty()) continue; + Multimap foreignReferences = HashMultimap.create(); + for (Table referenceTable : field.referenceTables) { + if (!referenceTable.name.equals(specTable.name)) { + JsonNode refValueNode = subEntity.get(field.name); + // Skip over references that are null but not required (e.g., route_id in fare_rules). + if (refValueNode.isNull() && !field.isRequired()) continue; + String refValue = refValueNode.asText(); + if (field.referenceTables.size() == 1) { + referencesPerTable.put(referenceTable, refValue); + } else { + foreignReferences.put(referenceTable, refValue); + } + } + } + if (!foreignReferences.isEmpty()) { + foreignReferencesPerTable.put(subTable, foreignReferences); + } + } + } + /** * Delete existing sub-entities for given key value for when an update to the parent entity is made (i.e., the parent * entity is not being newly created). Examples of sub-entities include stop times for trips, pattern stops for a @@ -1439,15 +1465,19 @@ private static Set
getReferencingTables(Table table) { // which could have unexpected behaviour. referencingTables.add(gtfsTable); } - if (field.isForeignReference() && field.referenceTable.name.equals(table.name)) { - // If any of the table's fields are foreign references to the specified table, add to the return set. - referencingTables.add(gtfsTable); + if (field.isForeignReference()) { + for (Table refTable : field.referenceTables) { + if (refTable.name.equals(table.name)) { + referencingTables.add(gtfsTable); + } + } } } } return referencingTables; } + /** * For a given integer ID, return the value for the specified field name for that entity. */ @@ -1560,71 +1590,75 @@ private void updateReferencingTables( } else { // General deletion for (Field field : referencingTable.editorFields()) { - if (field.isForeignReference() && field.referenceTable.name.equals(table.name)) { - // Get statement to update or delete entities that reference the key value. - PreparedStatement updateStatement = getUpdateReferencesStatement(sqlMethod, refTableName, field, keyValue, newKeyValue); - LOG.info(updateStatement.toString()); - result = updateStatement.executeUpdate(); - if (result > 0) { - // FIXME: is this where a delete hook should go? (E.g., CalendarController subclass would override - // deleteEntityHook). - if (sqlMethod.equals(SqlMethod.DELETE)) { - ArrayList patternAndRouteIds = new ArrayList<>(); - // Check for restrictions on delete. - if (table.isCascadeDeleteRestricted()) { - // The entity must not have any referencing entities in order to delete it. - connection.rollback(); - if (entityClass.getSimpleName().equals("Stop")) { - String patternStopLookup = String.format( - "select distinct p.id, r.id " + - "from %s.pattern_stops ps " + - "inner join " + - "%s.patterns p " + - "on p.pattern_id = ps.pattern_id " + - "inner join " + - "%s.routes r " + - "on p.route_id = r.route_id " + - "where %s = '%s'", - namespace, - namespace, - namespace, - keyField.name, - keyValue - ); - PreparedStatement patternStopSelectStatement = connection.prepareStatement(patternStopLookup); - if (patternStopSelectStatement.execute()) { - ResultSet resultSet = patternStopSelectStatement.getResultSet(); - while (resultSet.next()) { - patternAndRouteIds.add( - "{" + resultSet.getString(1) + "-" + resultSet.getString(2) + "}" + if (field.isForeignReference()) { + for (Table refTable : field.referenceTables) { + if (refTable.name.equals(table.name)) { + // Get statement to update or delete entities that reference the key value. + PreparedStatement updateStatement = getUpdateReferencesStatement(sqlMethod, refTableName, field, keyValue, newKeyValue); + LOG.info(updateStatement.toString()); + result = updateStatement.executeUpdate(); + if (result > 0) { + // FIXME: is this where a delete hook should go? (E.g., CalendarController subclass would override + // deleteEntityHook). + if (sqlMethod.equals(SqlMethod.DELETE)) { + ArrayList patternAndRouteIds = new ArrayList<>(); + // Check for restrictions on delete. + if (table.isCascadeDeleteRestricted()) { + // The entity must not have any referencing entities in order to delete it. + connection.rollback(); + if (entityClass.getSimpleName().equals("Stop")) { + String patternStopLookup = String.format( + "select distinct p.id, r.id " + + "from %s.pattern_stops ps " + + "inner join " + + "%s.patterns p " + + "on p.pattern_id = ps.pattern_id " + + "inner join " + + "%s.routes r " + + "on p.route_id = r.route_id " + + "where %s = '%s'", + namespace, + namespace, + namespace, + keyField.name, + keyValue ); + PreparedStatement patternStopSelectStatement = connection.prepareStatement(patternStopLookup); + if (patternStopSelectStatement.execute()) { + ResultSet resultSet = patternStopSelectStatement.getResultSet(); + while (resultSet.next()) { + patternAndRouteIds.add( + "{" + resultSet.getString(1) + "-" + resultSet.getString(2) + "}" + ); + } + } } + String message = String.format( + "Cannot delete %s %s=%s. %d %s reference this %s.", + entityClass.getSimpleName(), + keyField.name, + keyValue, + result, + referencingTable.name, + entityClass.getSimpleName() + ); + if (patternAndRouteIds.size() > 0) { + // Append referenced patterns data to the end of the error. + message = String.format( + "%s\nReferenced patterns: [%s]", + message, + StringUtils.join(patternAndRouteIds, ",") + ); + } + LOG.warn(message); + throw new SQLException(message); } } - String message = String.format( - "Cannot delete %s %s=%s. %d %s reference this %s.", - entityClass.getSimpleName(), - keyField.name, - keyValue, - result, - referencingTable.name, - entityClass.getSimpleName() - ); - if (patternAndRouteIds.size() > 0) { - // Append referenced patterns data to the end of the error. - message = String.format( - "%s\nReferenced patterns: [%s]", - message, - StringUtils.join(patternAndRouteIds, ",") - ); - } - LOG.warn(message); - throw new SQLException(message); + LOG.info("{} reference(s) in {} {}D!", result, refTableName, sqlMethod); + } else { + LOG.info("No references in {} found!", refTableName); } } - LOG.info("{} reference(s) in {} {}D!", result, refTableName, sqlMethod); - } else { - LOG.info("No references in {} found!", refTableName); } } } diff --git a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java index acd1178c7..cbc69cafb 100644 --- a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java +++ b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java @@ -1,6 +1,7 @@ package com.conveyal.gtfs.loader; import com.conveyal.gtfs.error.NewGTFSError; +import com.conveyal.gtfs.error.NewGTFSErrorType; import com.conveyal.gtfs.loader.conditions.ConditionalRequirement; import com.google.common.collect.HashMultimap; @@ -8,8 +9,10 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import static com.conveyal.gtfs.error.NewGTFSErrorType.DUPLICATE_ID; +import static com.conveyal.gtfs.error.NewGTFSErrorType.MISSING_FOREIGN_TABLE_REFERENCE; import static com.conveyal.gtfs.error.NewGTFSErrorType.REFERENTIAL_INTEGRITY; /** @@ -79,15 +82,30 @@ public Set checkReferencesAndUniqueness(String keyValue, int lineN // First, handle referential integrity check. boolean isOrderField = field.name.equals(orderField); if (field.isForeignReference()) { - // Check referential integrity if the field is a foreign reference. Note: the - // reference table must be loaded before the table/value being currently checked. - String referenceField = field.referenceTable.getKeyFieldName(); - String referenceTransitId = String.join(":", referenceField, value); - - if (!this.transitIds.contains(referenceTransitId)) { - // If the reference tracker does not contain + // Check foreign references. If the foreign reference is present in one of the tables, there is no + // need to check the remainder. If no matching foreign reference is found, flag integrity error. + // Note: The reference table must be loaded before the table/value being currently checked. + boolean hasMatchingReference = false; + TreeSet badValues = new TreeSet<>(); + for (Table referenceTable : field.referenceTables) { + String referenceField = referenceTable.getKeyFieldName(); + if (checkReference(referenceField, value, badValues)) { + hasMatchingReference = true; + break; + } + } + if (!hasMatchingReference) { + // If the reference tracker does not contain a match. + NewGTFSErrorType errorType = (field.referenceTables.size() > 1) + ? MISSING_FOREIGN_TABLE_REFERENCE + : REFERENTIAL_INTEGRITY; NewGTFSError referentialIntegrityError = NewGTFSError - .forLine(table, lineNumber, REFERENTIAL_INTEGRITY, referenceTransitId) + .forLine( + table, + lineNumber, + errorType, + String.join(", ", badValues) + ) .setEntityId(keyValue); // If the field is an order field, set the sequence for the new error. if (isOrderField) referentialIntegrityError.setSequence(value); @@ -148,6 +166,18 @@ public Set checkReferencesAndUniqueness(String keyValue, int lineN return errors; } + /** + * Check that a reference is valid. + */ + private boolean checkReference(String referenceField, String reference, TreeSet badValues) { + String referenceTransitId = String.join(":", referenceField, reference); + if (this.transitIds.contains(referenceTransitId)) { + return true; + } else { + badValues.add(referenceTransitId); + } + return false; + } /** * Work through each conditionally required check assigned to fields within a table. First check the reference field diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index 21ec4d8b9..9a31b5629 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -336,9 +336,8 @@ public Table (String name, Class entityClass, Requirement requ public static final Table TRIPS = new Table("trips", Trip.class, REQUIRED, new StringField("trip_id", REQUIRED), new StringField("route_id", REQUIRED).isReferenceTo(ROUTES).indexThisColumn(), - // FIXME: Should this also optionally reference CALENDAR_DATES? // FIXME: Do we need an index on service_id - new StringField("service_id", REQUIRED).isReferenceTo(CALENDAR), + new StringField("service_id", REQUIRED).isReferenceTo(CALENDAR).isReferenceTo(CALENDAR_DATES), new StringField("trip_headsign", OPTIONAL), new StringField("trip_short_name", OPTIONAL), new ShortField("direction_id", OPTIONAL, 1), diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index d9706e220..970c7f110 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -151,7 +151,7 @@ public void canLoadFeedWithBadDates () { new ErrorExpectation(NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS), new ErrorExpectation(NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS), new ErrorExpectation(NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS), - new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), + new ErrorExpectation(NewGTFSErrorType.MISSING_FOREIGN_TABLE_REFERENCE), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json index 743035e50..f9037e8cb 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json @@ -2,11 +2,6 @@ "data" : { "feed" : { "error_counts" : [ { - "count" : 1, - "message" : "No service_ids were active on a date within the range of dates with defined service.", - "priority" : "MEDIUM", - "type" : "DATE_NO_SERVICE" - }, { "count" : 1, "message" : "All travel times in the feed are rounded to the minute, which may cause unexpected results in routing applications where travel times are zero.", "priority" : "LOW", @@ -72,14 +67,6 @@ "error_id" : 4, "error_type" : "STOP_UNUSED", "line_number" : 6 - }, { - "bad_value" : "20170916", - "entity_id" : null, - "entity_sequence" : null, - "entity_type" : null, - "error_id" : 5, - "error_type" : "DATE_NO_SERVICE", - "line_number" : null } ], "feed_version" : "1.0" } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json index 4f2b230be..98983d54e 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json @@ -5,8 +5,8 @@ "row_counts" : { "agency" : 1, "calendar" : 1, - "calendar_dates" : 1, - "errors" : 6, + "calendar_dates" : 2, + "errors" : 5, "routes" : 1, "stop_times" : 4, "stops" : 5, From 9ea7d34b2481d72e52ef8491817b254964414335 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 22 Jun 2023 15:59:49 +0100 Subject: [PATCH 05/19] refactor(Bug fixes and unit test updates): --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 2 +- .../gtfs/loader/JdbcGtfsExporter.java | 8 +++- .../gtfs/validator/ServiceValidator.java | 35 ++++++++------- .../java/com/conveyal/gtfs/GTFSFeedTest.java | 12 +++++- src/test/java/com/conveyal/gtfs/GTFSTest.java | 43 ++++++++++++++++--- .../calendar_dates.txt | 4 +- .../routes.txt | 1 + .../stop_times.txt | 4 ++ .../trips.txt | 4 +- .../GTFSGraphQLTest/canFetchErrors-0.json | 13 ++++++ .../canFetchFeedRowCounts-0.json | 2 +- 11 files changed, 95 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index ba2cd4fa5..49277a2fb 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -157,7 +157,7 @@ else if (feedId == null || feedId.isEmpty()) { new Agency.Loader(this).loadTable(zip); - // calendars and calendar dates are joined into services, where there is a match on service id. This means a lot + // Calendars and calendar dates are joined into services, where there is a match on service id. This means a lot // of manipulating service objects as they are loaded; since mapdb keys/values are immutable, load them in // memory then copy them to MapDB once we're done loading them. Map serviceTable = new HashMap<>(); diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 404477d22..3e09d0881 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -168,7 +168,8 @@ public FeedLoadResult exportTables() { LOG.info("No calendar dates found within scheduled exceptions."); } else { // In this scenario all values in the calendar dates table need to be combined with calendar - // dates derived from scheduled exceptions and then written to file. + // dates derived from scheduled exceptions and then written to file. Service calendar dates and + // calendar dates are merged in CalendarDate.iterator() for export. exportCalendarDatesDirectFromTable = false; JDBCTableReader calendarDatesReader = new JDBCTableReader( Table.CALENDAR_DATES, @@ -178,7 +179,10 @@ public FeedLoadResult exportTables() { ); Iterable calendarDates = calendarDatesReader.getAll(); for (CalendarDate calendarDate : calendarDates) { - feed.calendarDates.put(calendarDate.service_id, calendarDate); + if (!feed.services.containsKey(calendarDate.service_id)) { + // Only include the calendar date if not already derived from a scheduled exception. + feed.calendarDates.put(calendarDate.service_id, calendarDate); + } } LOG.info("Writing {} calendar dates from schedule exceptions", calendarDateCount); new CalendarDate.Writer(feed).writeTable(zipOutputStream); diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index bc0df3dc8..931a54d04 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -163,24 +163,23 @@ private void validateServiceInfo(ValidationResult validationResult) { } } - // Next remove any calendar date entries from serviceInfoForServiceId which - - // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. -// for (CalendarDate calendarDate : feed.calendarDates) { -// if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { -// LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); -// continue; -// } -// ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); -// if (calendarDate.exception_type == 1) { -// // Service added, add to set for this date. -// serviceInfo.datesActive.add(calendarDate.date); -// } else if (calendarDate.exception_type == 2) { -// // Service removed, remove from Set for this date. -// serviceInfo.datesActive.remove(calendarDate.date); -// } -// // Otherwise exception_type is out of range. This should already have been caught during the loading phase. -// } + // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. A calendar date + // must be related (via a service id) to a calendar to qualify. + for (CalendarDate calendarDate : feed.calendarDates) { + if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { + LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); + continue; + } + ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); + if (calendarDate.exception_type == 1) { + // Service added, add to set for this date. + serviceInfo.datesActive.add(calendarDate.date); + } else if (calendarDate.exception_type == 2) { + // Service removed, remove from Set for this date. + serviceInfo.datesActive.remove(calendarDate.date); + } + // Otherwise exception_type is out of range. This should already have been caught during the loading phase. + } /* A view that is similar to ServiceInfo class, but doesn't deal well with missing IDs in either subquery: diff --git a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java index 305c1bb2d..414d31757 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java @@ -62,10 +62,10 @@ public static void setUpClass() { } /** - * Make sure a roundtrip of loading a GTFS zip file and then writing another zip file can be performed. + * Make sure a round-trip of loading a GTFS zip file and then writing another zip file can be performed. */ @Test - public void canDoRoundtripLoadAndWriteToZipFile() throws IOException { + public void canDoRoundTripLoadAndWriteToZipFile() throws IOException { // create a temp file for this test File outZip = File.createTempFile("fake-agency-output", ".zip"); @@ -97,6 +97,14 @@ public void canDoRoundtripLoadAndWriteToZipFile() throws IOException { new DataExpectation("end_date", "20170917") } ), + new FileTestCase( + "calendar_dates.txt", + new DataExpectation[]{ + new DataExpectation("service_id", "exception-in-own-right"), + new DataExpectation("date", "20230619"), + new DataExpectation("exception_type", "2") + } + ), new FileTestCase( "routes.txt", new DataExpectation[]{ diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 970c7f110..7adc1db08 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -111,7 +111,8 @@ public void canLoadAndExportSimpleAgency() { new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")) + new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), + new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( runIntegrationTestOnFolder( @@ -153,11 +154,7 @@ public void canLoadFeedWithBadDates () { new ErrorExpectation(NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS), new ErrorExpectation(NewGTFSErrorType.MISSING_FOREIGN_TABLE_REFERENCE), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.TRIP_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.TRIP_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.NO_SERVICE) + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) ); assertThat( "Integration test passes", @@ -280,7 +277,8 @@ public void canLoadAndExportSimpleAgencyInSubDirectory() { new ErrorExpectation(NewGTFSErrorType.REFERENTIAL_INTEGRITY), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED) + new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED), + new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( runIntegrationTestOnZipFile(zipFileName, nullValue(), fakeAgencyPersistenceExpectations, errorExpectations), @@ -422,6 +420,26 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new RecordExpectation("exception_type", 1) } ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "calendar-dates-txt-exception-one" + ), + new RecordExpectation("date", 20230622), + new RecordExpectation("exception_type", 1) + } + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "calendar-dates-txt-exception-two" + ), + new RecordExpectation("date", 20230622), + new RecordExpectation("exception_type", 1) + } + ), new PersistenceExpectation( "stop_times", new RecordExpectation[]{ @@ -498,6 +516,7 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { ErrorExpectation[] errorExpectations = ErrorExpectation.list( new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), + new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) ); assertThat( @@ -1206,6 +1225,16 @@ private void assertThatPersistenceExpectationRecordWasFound( new RecordExpectation("exception_type", 2) } ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "exception-in-own-right" + ), + new RecordExpectation("date", 20230619), + new RecordExpectation("exception_type", 2) + } + ), new PersistenceExpectation( "fare_attributes", new RecordExpectation[]{ diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt index ea060f019..e49dd9313 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt @@ -1,3 +1,5 @@ service_id,date,exception_type in-both-calendar-txt-and-calendar-dates,20170920,2 -only-in-calendar-dates-txt,20170916,1 \ No newline at end of file +only-in-calendar-dates-txt,20170916,1 +calendar-dates-txt-exception-one,20230622,1 +calendar-dates-txt-exception-two,20230622,1 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt index 35ea7aa67..b13480efa 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt @@ -1,2 +1,3 @@ agency_id,route_id,route_short_name,route_long_name,route_desc,route_type,route_url,route_color,route_text_color,route_branding_url 1,1,1,Route 1,,3,,7CE6E7,FFFFFF, +1,2,2,Route 2,,3,,7CE6E7,FFFFFF, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt index fe0a9ad12..12ad079e6 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt @@ -5,3 +5,7 @@ non-frequency-trip-2,08:00:00,08:00:00,4u6g,1,,0,0,0.0000000, non-frequency-trip-2,08:01:00,08:01:00,johv,2,,0,0,341.4491961, frequency-trip,09:00:00,09:00:00,4u6g,1,,0,0,0.0000000, frequency-trip,09:01:00,09:01:00,johv,2,,0,0,341.4491961, +exception-trip-1,09:00:00,09:00:00,4u6g,1,,0,0,0.0000000, +exception-trip-1,09:01:00,09:01:00,johv,2,,0,0,341.4491961, +exception-trip-2,09:00:00,09:00:00,4u6g,1,,0,0,0.0000000, +exception-trip-2,09:01:00,09:01:00,johv,2,,0,0,341.4491961, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt index 077253974..a0c194b3f 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt @@ -1,4 +1,6 @@ route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bikes_allowed,wheelchair_accessible,service_id 1,non-frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-dates-txt 1,non-frequency-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-txt -1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,in-both-calendar-txt-and-calendar-dates \ No newline at end of file +1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,in-both-calendar-txt-and-calendar-dates +2,exception-trip-1,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-exception-one +2,exception-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-exception-two \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json index f9037e8cb..743035e50 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json @@ -2,6 +2,11 @@ "data" : { "feed" : { "error_counts" : [ { + "count" : 1, + "message" : "No service_ids were active on a date within the range of dates with defined service.", + "priority" : "MEDIUM", + "type" : "DATE_NO_SERVICE" + }, { "count" : 1, "message" : "All travel times in the feed are rounded to the minute, which may cause unexpected results in routing applications where travel times are zero.", "priority" : "LOW", @@ -67,6 +72,14 @@ "error_id" : 4, "error_type" : "STOP_UNUSED", "line_number" : 6 + }, { + "bad_value" : "20170916", + "entity_id" : null, + "entity_sequence" : null, + "entity_type" : null, + "error_id" : 5, + "error_type" : "DATE_NO_SERVICE", + "line_number" : null } ], "feed_version" : "1.0" } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json index 98983d54e..080545c85 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json @@ -6,7 +6,7 @@ "agency" : 1, "calendar" : 1, "calendar_dates" : 2, - "errors" : 5, + "errors" : 6, "routes" : 1, "stop_times" : 4, "stops" : 5, From e1e67bfc48c70829e645a5fdae2451ef299a8caf Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Tue, 27 Jun 2023 17:10:20 +0100 Subject: [PATCH 06/19] refactor(Rescoped to use servicing instead of using calendar dates): --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 5 - .../gtfs/graphql/GraphQLGtfsSchema.java | 12 --- .../gtfs/loader/JdbcGTFSFeedConverter.java | 2 - .../gtfs/loader/JdbcGtfsExporter.java | 96 ++++++++++++------- .../gtfs/loader/JdbcGtfsSnapshotter.java | 24 +++-- .../java/com/conveyal/gtfs/loader/Table.java | 1 - .../com/conveyal/gtfs/model/CalendarDate.java | 40 +++----- .../java/com/conveyal/gtfs/model/Entity.java | 19 ---- .../gtfs/model/ScheduleException.java | 8 +- .../java/com/conveyal/gtfs/model/Trip.java | 2 +- .../gtfs/validator/ServiceValidator.java | 41 +++----- src/test/java/com/conveyal/gtfs/GTFSTest.java | 37 +++---- .../gtfs/graphql/GTFSGraphQLTest.java | 8 -- .../calendar_dates.txt | 4 +- .../resources/graphql/feedCalendarDates.txt | 10 -- src/test/resources/graphql/feedRowCounts.txt | 1 - .../canFetchCalendarDates-0.json | 16 ---- .../GTFSGraphQLTest/canFetchErrors-0.json | 28 +++++- .../canFetchFeedRowCounts-0.json | 3 +- 19 files changed, 143 insertions(+), 214 deletions(-) delete mode 100644 src/test/resources/graphql/feedCalendarDates.txt delete mode 100644 src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index 49277a2fb..14a0e052a 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -84,10 +84,6 @@ public class GTFSFeed implements Cloneable, Closeable { /* A service is a calendar entry and all calendar_dates that modify that calendar entry. */ public final BTreeMap services; - /* A calendar date specifies a single date that a service runs. These dates are wholly separate to calendar dates - * that are related to a calendar and defined in 'services'. */ - public final Map calendarDates; - /* A place to accumulate errors while the feed is loaded. Tolerate as many errors as possible and keep on loading. */ public final NavigableSet errors; @@ -628,7 +624,6 @@ private GTFSFeed (DB db) { this.db = db; agency = db.getTreeMap("agency"); - calendarDates = db.getTreeMap("calendar_dates"); feedInfo = db.getTreeMap("feed_info"); routes = db.getTreeMap("routes"); trips = db.getTreeMap("trips"); diff --git a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java index 975a792a0..f3eef36f9 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java +++ b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java @@ -486,7 +486,6 @@ public class GraphQLGtfsSchema { .field(RowCountFetcher.field("stop_times")) .field(RowCountFetcher.field("agency")) .field(RowCountFetcher.field("calendar")) - .field(RowCountFetcher.field("calendar_dates")) .field(RowCountFetcher.field("errors")) .build(); @@ -723,17 +722,6 @@ public class GraphQLGtfsSchema { .dataFetcher(new JDBCFetcher("calendar")) .build() ) - .field(newFieldDefinition() - .name("calendar_dates") - .type(new GraphQLList(GraphQLGtfsSchema.calendarDatesType)) - .argument(stringArg("namespace")) // FIXME maybe these nested namespace arguments are not doing anything. - .argument(multiStringArg("service_id")) - .argument(intArg(ID_ARG)) - .argument(intArg(LIMIT_ARG)) - .argument(intArg(OFFSET_ARG)) - .dataFetcher(new JDBCFetcher("calendar_dates")) - .build() - ) .field(newFieldDefinition() .name("fares") .type(new GraphQLList(GraphQLGtfsSchema.fareType)) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java index b91b066e0..01990b24a 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGTFSFeedConverter.java @@ -92,8 +92,6 @@ public FeedLoadResult loadTables () { .map(service -> service.calendar_dates.values()) .flatMap(Collection::stream) .collect(Collectors.toList()); - // Combine calendar dates and calendar dates derived from services. - calendarDates.addAll(new ArrayList<>(gtfsFeed.calendarDates.values())); List fareAttributes = gtfsFeed.fares.values() .stream() .map(fare -> fare.fare_attribute) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 3e09d0881..462495e70 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -11,6 +11,7 @@ import org.postgresql.core.BaseConnection; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import javax.sql.DataSource; import java.io.File; import java.io.FileOutputStream; @@ -27,8 +28,10 @@ import java.time.LocalDate; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; @@ -108,40 +111,66 @@ public FeedLoadResult exportTables() { } else { result.calendar = export(Table.CALENDAR, connection); } - boolean exportCalendarDatesDirectFromTable = true; if (fromEditor) { // Export schedule exceptions in place of calendar dates if exporting a feed/schema that represents an editor snapshot. GTFSFeed feed = new GTFSFeed(); // FIXME: The below table readers should probably just share a connection with the exporter. JDBCTableReader exceptionsReader = - new JDBCTableReader(Table.SCHEDULE_EXCEPTIONS, dataSource, feedIdToExport + ".", - EntityPopulator.SCHEDULE_EXCEPTION); + new JDBCTableReader(Table.SCHEDULE_EXCEPTIONS, dataSource, feedIdToExport + ".", + EntityPopulator.SCHEDULE_EXCEPTION); JDBCTableReader calendarsReader = - new JDBCTableReader(Table.CALENDAR, dataSource, feedIdToExport + ".", - EntityPopulator.CALENDAR); + new JDBCTableReader(Table.CALENDAR, dataSource, feedIdToExport + ".", + EntityPopulator.CALENDAR); Iterable calendars = calendarsReader.getAll(); + Set calendarServiceIds = new HashSet<>(); + for (Calendar calendar : calendarsReader.getAll()) { + calendarServiceIds.add(calendar.service_id); + } Iterable exceptionsIterator = exceptionsReader.getAll(); List exceptions = new ArrayList<>(); - // FIXME: Doing this causes the connection to stay open, but it is closed in the finalizer so it should - // not be a big problem. - for (ScheduleException exception : exceptionsIterator) { - exceptions.add(exception); + List calendarDateServices = new ArrayList<>(); + for (ScheduleException ex : exceptionsIterator) { + if (ex.exemplar.equals(ScheduleException.ExemplarServiceDescriptor.SWAP) && + (ex.addedService.stream().noneMatch(calendarServiceIds::contains) && + ex.removedService.stream().noneMatch(calendarServiceIds::contains))) { + calendarDateServices.add(ex); + } else { + exceptions.add(ex); + } + } + + int calendarDateCount = 0; + // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one + // date and either one entry in add service or remove service. Must likely, only one entry ever in add + // service. + for (ScheduleException ex : calendarDateServices) { + // Only ever expecting one date here. + LocalDate date = ex.dates.get(0); + CalendarDate calendarDate = new CalendarDate(); + calendarDate.date = date; + if (!ex.addedService.isEmpty()) { + calendarDate.service_id = ex.addedService.get(0); + } else if (!ex.removedService.isEmpty()) { + calendarDate.service_id = ex.removedService.get(0); + } else { + calendarDate.service_id = ex.name; + } + // It is assumed for a calendar date service that only dates where the service is available are defined. + calendarDate.exception_type = 1; + Service service = new Service(calendarDate.service_id); + service.calendar_dates.put(date, calendarDate); + feed.services.put(calendarDate.service_id, service); + calendarDateCount++; } + // check whether the feed is organized in a format with the calendars.txt file if (calendarsReader.getRowCount() > 0) { // feed does have calendars.txt file, continue export with strategy of matching exceptions // to calendar to output calendar_dates.txt - int calendarDateCount = 0; for (Calendar cal : calendars) { Service service = new Service(cal.service_id); service.calendar = cal; for (ScheduleException ex : exceptions) { - if (ex.exemplar.equals(ScheduleException.ExemplarServiceDescriptor.SWAP) && - (!ex.addedService.contains(cal.service_id) && !ex.removedService.contains(cal.service_id))) { - // Skip swap exception if cal is not referenced by added or removed service. - // This is not technically necessary, but the output is cleaner/more intelligible. - continue; - } for (LocalDate date : ex.dates) { if (date.isBefore(cal.start_date) || date.isAfter(cal.end_date)) { @@ -156,7 +185,7 @@ public FeedLoadResult exportTables() { LOG.info("Adding exception {} (type={}) for calendar {} on date {}", ex.name, calendarDate.exception_type, cal.service_id, date.toString()); if (service.calendar_dates.containsKey(date)) - throw new IllegalArgumentException("Duplicate schedule exceptions on " + date.toString()); + throw new IllegalArgumentException("Duplicate schedule exceptions on " + date); service.calendar_dates.put(date, calendarDate); calendarDateCount += 1; @@ -165,31 +194,24 @@ public FeedLoadResult exportTables() { feed.services.put(cal.service_id, service); } if (calendarDateCount == 0) { - LOG.info("No calendar dates found within scheduled exceptions."); + LOG.info("No calendar dates found. Skipping table."); } else { - // In this scenario all values in the calendar dates table need to be combined with calendar - // dates derived from scheduled exceptions and then written to file. Service calendar dates and - // calendar dates are merged in CalendarDate.iterator() for export. - exportCalendarDatesDirectFromTable = false; - JDBCTableReader calendarDatesReader = new JDBCTableReader( - Table.CALENDAR_DATES, - dataSource, - feedIdToExport + ".", - EntityPopulator.CALENDAR_DATE - ); - Iterable calendarDates = calendarDatesReader.getAll(); - for (CalendarDate calendarDate : calendarDates) { - if (!feed.services.containsKey(calendarDate.service_id)) { - // Only include the calendar date if not already derived from a scheduled exception. - feed.calendarDates.put(calendarDate.service_id, calendarDate); - } - } LOG.info("Writing {} calendar dates from schedule exceptions", calendarDateCount); new CalendarDate.Writer(feed).writeTable(zipOutputStream); } } - } - if (exportCalendarDatesDirectFromTable) { + + if (calendarsReader.getRowCount() == 0 && calendarDateServices.isEmpty()) { + // No calendar or calendar date service records exist, export calendar_dates as is and hope for the best. + // This situation will occur in at least 2 scenarios: + // 1. A GTFS has been loaded into the editor that had only the calendar_dates.txt file + // and no further edits were made before exporting to a snapshot + // 2. A new GTFS has been created from scratch and calendar information has yet to be added. + // This will result in an invalid GTFS, but it was what the user wanted so ¯\_(ツ)_/¯ + result.calendarDates = export(Table.CALENDAR_DATES, connection); + } + } else { + // Otherwise, simply export the calendar dates as they were loaded in. result.calendarDates = export(Table.CALENDAR_DATES, connection); } result.fareAttributes = export(Table.FARE_ATTRIBUTES, connection); diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 87c356b29..9ba7fb807 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -248,29 +248,27 @@ private TableLoadResult createScheduleExceptionsTable() { Multimap removedServiceForDate = HashMultimap.create(); Multimap addedServiceForDate = HashMultimap.create(); for (CalendarDate calendarDate : calendarDates) { - // Skip any null dates + // Skip any null dates. if (calendarDate.date == null) { LOG.warn("Encountered calendar date record with null value for date field. Skipping."); continue; } - if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { - LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); - continue; - } String date = calendarDate.date.format(DateTimeFormatter.BASIC_ISO_DATE); if (calendarDate.exception_type == 1) { addedServiceForDate.put(date, calendarDate.service_id); // create (if needed) and extend range of dummy calendar that would need to be created if we are // copying from a feed that doesn't have the calendar.txt file - Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); - calendar.service_id = calendarDate.service_id; - if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { - calendar.start_date = calendarDate.date; - } - if (calendar.end_date == null || calendar.end_date.isBefore(calendarDate.date)) { - calendar.end_date = calendarDate.date; + if (calendarDate.service_id != null && calendarServiceIds.contains(calendarDate.service_id)) { + Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); + calendar.service_id = calendarDate.service_id; + if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { + calendar.start_date = calendarDate.date; + } + if (calendar.end_date == null || calendar.end_date.isBefore(calendarDate.date)) { + calendar.end_date = calendarDate.date; + } + dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); } - dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); } else { removedServiceForDate.put(date, calendarDate.service_id); } diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index 9a31b5629..0f558102c 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -150,7 +150,6 @@ public Table (String name, Class entityClass, Requirement requ public static final Table SCHEDULE_EXCEPTIONS = new Table("schedule_exceptions", ScheduleException.class, EDITOR, new StringField("name", REQUIRED), // FIXME: This makes name the key field... - // FIXME: Change to DateListField new DateListField("dates", REQUIRED), new ShortField("exemplar", REQUIRED, 9), new StringListField("custom_schedule", OPTIONAL).isReferenceTo(CALENDAR), diff --git a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java index 4ce088e80..8153e7544 100644 --- a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java +++ b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java @@ -70,27 +70,20 @@ protected boolean isRequired() { @Override public void loadOneRow() throws IOException { - /* Calendars and calendar dates that are matched on service id are special: they are stored as joined tables - rather than simple maps. */ - String serviceId = getStringField("service_id", true); + /* Calendars and Fares are special: they are stored as joined tables rather than simple maps. */ + String service_id = getStringField("service_id", true); + Service service = services.computeIfAbsent(service_id, Service::new); LocalDate date = getDateField("date", true); - CalendarDate cd = new CalendarDate(); - cd.id = row + 1; // offset line number by 1 to account for 0-based row index - cd.service_id = serviceId; - cd.date = date; - cd.exception_type = getIntField("exception_type", true, 1, 2); - cd.feed = feed; - if (services.containsKey(serviceId)) { - // Calendar date is related to calendar. - Service service = services.computeIfAbsent(serviceId, Service::new); - if (service.calendar_dates.containsKey(date)) { - feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)")); - } else { - service.calendar_dates.put(date, cd); - } + if (service.calendar_dates.containsKey(date)) { + feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)")); } else { - // Calendar date is a stand-alone date that is not related to a calendar. - if (cd.service_id != null) feed.calendarDates.put(cd.service_id, cd); + CalendarDate cd = new CalendarDate(); + cd.id = row + 1; // offset line number by 1 to account for 0-based row index + cd.service_id = service_id; + cd.date = date; + cd.exception_type = getIntField("exception_type", true, 1, 2); + cd.feed = feed; + service.calendar_dates.put(date, cd); } } } @@ -115,15 +108,8 @@ protected void writeOneRow(CalendarDate d) throws IOException { @Override protected Iterator iterator() { - // Combine all calendar dates references. This is only used to write to table/file. Iterator serviceIterator = feed.services.values().iterator(); - Iterator calendarDatesFromServices = Iterators.concat( - Iterators.transform(serviceIterator, service -> service.calendar_dates.values().iterator()) - ); - Iterator calendarDatesIterator = feed.calendarDates.values().iterator(); - return Iterators.concat( - calendarDatesIterator, calendarDatesFromServices - ); + return Iterators.concat(Iterators.transform(serviceIterator, service -> service.calendar_dates.values().iterator())); } } } diff --git a/src/main/java/com/conveyal/gtfs/model/Entity.java b/src/main/java/com/conveyal/gtfs/model/Entity.java index bb154234b..29657b6e6 100644 --- a/src/main/java/com/conveyal/gtfs/model/Entity.java +++ b/src/main/java/com/conveyal/gtfs/model/Entity.java @@ -268,25 +268,6 @@ protected V getRefField(String column, boolean required, Map target return val; } - /** - * Used to check referential integrity in two targets. E.g. a service id defined in a trip could either be in - * services or calendar dates. - */ - protected void getRefField(String column, boolean required, Map target1, Map target2) throws IOException { - String str = getFieldCheckRequired(column, required); - if (str != null) { - V val = target1.get(str); - J val2 = target2.get(str); - String transitId = column + ":" + str; - if (!feed.transitIds.contains(transitId)) { - feed.transitIds.add(transitId); - if (val == null && val2 == null) { - feed.errors.add(new ReferentialIntegrityError(tableName, row, column, str)); - } - } - } - } - protected abstract boolean isRequired(); /** Implemented by subclasses to read one row, produce one GTFS entity, and store that entity in a map. */ diff --git a/src/main/java/com/conveyal/gtfs/model/ScheduleException.java b/src/main/java/com/conveyal/gtfs/model/ScheduleException.java index 932875ccc..79f0220fc 100644 --- a/src/main/java/com/conveyal/gtfs/model/ScheduleException.java +++ b/src/main/java/com/conveyal/gtfs/model/ScheduleException.java @@ -41,7 +41,7 @@ public class ScheduleException extends Entity { @Override public void setStatementParameters(PreparedStatement statement, boolean setDefaultId) throws SQLException { - // FIXME + // Require } public boolean serviceRunsOn(Calendar calendar) { @@ -73,6 +73,8 @@ public boolean serviceRunsOn(Calendar calendar) { if (removedService != null && removedService.contains(calendar.service_id)) { return false; } + case CALENDAR_DATE_SERVICE: + return false; default: // can't actually happen, but java requires a default with a return here return false; @@ -84,7 +86,7 @@ public boolean serviceRunsOn(Calendar calendar) { * For example, run Sunday service on Presidents' Day, or no service on New Year's Day. */ public enum ExemplarServiceDescriptor { - MONDAY(0), TUESDAY(1), WEDNESDAY(2), THURSDAY(3), FRIDAY(4), SATURDAY(5), SUNDAY(6), NO_SERVICE(7), CUSTOM(8), SWAP(9), MISSING(-1); + MONDAY(0), TUESDAY(1), WEDNESDAY(2), THURSDAY(3), FRIDAY(4), SATURDAY(5), SUNDAY(6), NO_SERVICE(7), CUSTOM(8), SWAP(9), CALENDAR_DATE_SERVICE(10), MISSING(-1); private final int value; @@ -119,6 +121,8 @@ public static ExemplarServiceDescriptor exemplarFromInt (int value) { return ExemplarServiceDescriptor.CUSTOM; case 9: return ExemplarServiceDescriptor.SWAP; + case 10: + return ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE; default: return ExemplarServiceDescriptor.MISSING; } diff --git a/src/main/java/com/conveyal/gtfs/model/Trip.java b/src/main/java/com/conveyal/gtfs/model/Trip.java index 5f8f0f4a1..416854974 100644 --- a/src/main/java/com/conveyal/gtfs/model/Trip.java +++ b/src/main/java/com/conveyal/gtfs/model/Trip.java @@ -85,7 +85,7 @@ public void loadOneRow() throws IOException { Routes because they would be serialized into the MapDB. */ // TODO confirm existence of shape ID - getRefField("service_id", true, feed.services, feed.calendarDates); + getRefField("service_id", true, feed.services); getRefField("route_id", true, feed.routes); } diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index 931a54d04..96c7f5886 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -62,8 +62,6 @@ public class ServiceValidator extends TripValidator { private Map dateInfoForDate = new HashMap<>(); - private Set calendarServiceIds = new HashSet<>(); - public ServiceValidator(Feed feed, SQLErrorStorage errorStorage) { super(feed, errorStorage); } @@ -94,27 +92,15 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< // ERR return; } - - // Create a list of calendar service ids. We are only interested in creating services for calendar and related - // calendar dates. - for (Calendar calendar : feed.calendars) { - calendarServiceIds.add(calendar.service_id); - } - // Get the map from modes to service durations in seconds for this trip's service ID. // Create a new empty map if it doesn't yet exist. - if (calendarServiceIds.contains(trip.service_id)) { - // Trip contains calendar service id, therefore we are interested in creating service info for this service id. - ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(trip.service_id, ServiceInfo::new); - if (route != null) { - // Increment the service duration for this trip's transport mode and service ID. - serviceInfo.durationByRouteType.adjustOrPutValue(route.route_type, tripDurationSeconds, tripDurationSeconds); - } - // Record which trips occur on each service_id. - serviceInfo.tripIds.add(trip.trip_id); - LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); + ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(trip.service_id, ServiceInfo::new); + if (route != null) { + // Increment the service duration for this trip's transport mode and service ID. + serviceInfo.durationByRouteType.adjustOrPutValue(route.route_type, tripDurationSeconds, tripDurationSeconds); } - + // Record which trips occur on each service_id. + serviceInfo.tripIds.add(trip.trip_id); // TODO validate mode codes } @@ -163,13 +149,8 @@ private void validateServiceInfo(ValidationResult validationResult) { } } - // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. A calendar date - // must be related (via a service id) to a calendar to qualify. + // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. for (CalendarDate calendarDate : feed.calendarDates) { - if (calendarDate.service_id != null && !calendarServiceIds.contains(calendarDate.service_id)) { - LOG.warn("Encountered calendar date that is not joined by service id to a calendar. Skipping."); - continue; - } ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); if (calendarDate.exception_type == 1) { // Service added, add to set for this date. @@ -202,8 +183,8 @@ select durations.service_id, duration_seconds, days_active from ( for (String tripId : serviceInfo.tripIds) { registerError( NewGTFSError.forTable(Table.TRIPS, NewGTFSErrorType.TRIP_NEVER_ACTIVE) - .setEntityId(tripId) - .setBadValue(tripId)); + .setEntityId(tripId) + .setBadValue(tripId)); } } if (serviceInfo.tripIds.isEmpty()) { @@ -266,7 +247,7 @@ select durations.service_id, duration_seconds, days_active from ( // Check for low or zero service, which seems to happen even when services are defined. // This will also catch cases where dateInfo was null and the new instance contains no service. registerError(NewGTFSError.forFeed(NewGTFSErrorType.DATE_NO_SERVICE, - DateField.GTFS_DATE_FORMATTER.format(date))); + DateField.GTFS_DATE_FORMATTER.format(date))); } } } @@ -339,7 +320,7 @@ select durations.service_id, duration_seconds, days_active from ( String serviceDurationsTableName = feed.tablePrefix + "service_durations"; sql = String.format("create table %s (service_id varchar, route_type integer, " + - "duration_seconds integer, primary key (service_id, route_type))", serviceDurationsTableName); + "duration_seconds integer, primary key (service_id, route_type))", serviceDurationsTableName); LOG.info(sql); statement.execute(sql); sql = String.format("insert into %s values (?, ?, ?)", serviceDurationsTableName); diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 7adc1db08..4e6a48448 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -112,6 +112,8 @@ public void canLoadAndExportSimpleAgency() { new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), + new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), + new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( @@ -154,7 +156,11 @@ public void canLoadFeedWithBadDates () { new ErrorExpectation(NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS), new ErrorExpectation(NewGTFSErrorType.MISSING_FOREIGN_TABLE_REFERENCE), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), + new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), + new ErrorExpectation(NewGTFSErrorType.TRIP_NEVER_ACTIVE), + new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), + new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( "Integration test passes", @@ -278,6 +284,8 @@ public void canLoadAndExportSimpleAgencyInSubDirectory() { new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED), + new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), + new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( @@ -354,8 +362,7 @@ public void canLoadAndExportSimpleAgencyWithOnlyCalendarDates() { ErrorExpectation[] errorExpectations = ErrorExpectation.list( new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.NO_SERVICE) + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) ); assertThat( runIntegrationTestOnFolder( @@ -426,7 +433,7 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new RecordExpectation( "service_id", "calendar-dates-txt-exception-one" ), - new RecordExpectation("date", 20230622), + new RecordExpectation("date", 20170917), new RecordExpectation("exception_type", 1) } ), @@ -436,7 +443,7 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new RecordExpectation( "service_id", "calendar-dates-txt-exception-two" ), - new RecordExpectation("date", 20230622), + new RecordExpectation("date", 20170918), new RecordExpectation("exception_type", 1) } ), @@ -1215,26 +1222,6 @@ private void assertThatPersistenceExpectationRecordWasFound( new RecordExpectation("end_date", "20170917") } ), - new PersistenceExpectation( - "calendar_dates", - new RecordExpectation[]{ - new RecordExpectation( - "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" - ), - new RecordExpectation("date", 20170916), - new RecordExpectation("exception_type", 2) - } - ), - new PersistenceExpectation( - "calendar_dates", - new RecordExpectation[]{ - new RecordExpectation( - "service_id", "exception-in-own-right" - ), - new RecordExpectation("date", 20230619), - new RecordExpectation("exception_type", 2) - } - ), new PersistenceExpectation( "fare_attributes", new RecordExpectation[]{ diff --git a/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java b/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java index fc0d72cb5..240683306 100644 --- a/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java +++ b/src/test/java/com/conveyal/gtfs/graphql/GTFSGraphQLTest.java @@ -164,14 +164,6 @@ public void canFetchCalendars() { }); } - /** Tests that the calendar dates of a feed can be fetched. */ - @Test - void canFetchCalendarDates() { - assertTimeout(Duration.ofMillis(TEST_TIMEOUT), () -> { - MatcherAssert.assertThat(queryGraphQL("feedCalendarDates.txt"), matchesSnapshot()); - }); - } - /** Tests that the fares of a feed can be fetched. */ @Test public void canFetchFares() { diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt index e49dd9313..32f45f510 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt @@ -1,5 +1,5 @@ service_id,date,exception_type in-both-calendar-txt-and-calendar-dates,20170920,2 only-in-calendar-dates-txt,20170916,1 -calendar-dates-txt-exception-one,20230622,1 -calendar-dates-txt-exception-two,20230622,1 \ No newline at end of file +calendar-dates-txt-exception-one,20170917,1 +calendar-dates-txt-exception-two,20170918,1 \ No newline at end of file diff --git a/src/test/resources/graphql/feedCalendarDates.txt b/src/test/resources/graphql/feedCalendarDates.txt deleted file mode 100644 index c2dd8b7a4..000000000 --- a/src/test/resources/graphql/feedCalendarDates.txt +++ /dev/null @@ -1,10 +0,0 @@ -query ($namespace: String) { - feed(namespace: $namespace) { - feed_version - calendar_dates { - service_id - date - exception_type - } - } -} \ No newline at end of file diff --git a/src/test/resources/graphql/feedRowCounts.txt b/src/test/resources/graphql/feedRowCounts.txt index 116395f6f..74ec31965 100644 --- a/src/test/resources/graphql/feedRowCounts.txt +++ b/src/test/resources/graphql/feedRowCounts.txt @@ -5,7 +5,6 @@ query($namespace: String) { row_counts { agency calendar - calendar_dates errors routes stops diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json deleted file mode 100644 index 51c43fd59..000000000 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchCalendarDates-0.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "data" : { - "feed" : { - "calendar_dates" : [ { - "date" : "20170916", - "exception_type" : 2, - "service_id" : "04100312-8fe1-46a5-a9f2-556f39478f57" - }, { - "date" : "20230619", - "exception_type" : 2, - "service_id" : "exception-in-own-right" - } ], - "feed_version" : "1.0" - } - } -} \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json index 743035e50..883ad21fd 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json @@ -26,6 +26,16 @@ "message" : "The long name of a route should complement the short name, not include it.", "priority" : "LOW", "type" : "ROUTE_LONG_NAME_CONTAINS_SHORT_NAME" + }, { + "count" : 1, + "message" : "A service code was defined, but is never active on any date.", + "priority" : "MEDIUM", + "type" : "SERVICE_NEVER_ACTIVE" + }, { + "count" : 1, + "message" : "A service code was defined, but is never referenced by any trips.", + "priority" : "MEDIUM", + "type" : "SERVICE_UNUSED" }, { "count" : 1, "message" : "This stop is not referenced by any trips.", @@ -73,11 +83,27 @@ "error_type" : "STOP_UNUSED", "line_number" : 6 }, { - "bad_value" : "20170916", + "bad_value" : "exception-in-own-right", "entity_id" : null, "entity_sequence" : null, "entity_type" : null, "error_id" : 5, + "error_type" : "SERVICE_NEVER_ACTIVE", + "line_number" : null + }, { + "bad_value" : "exception-in-own-right", + "entity_id" : null, + "entity_sequence" : null, + "entity_type" : null, + "error_id" : 6, + "error_type" : "SERVICE_UNUSED", + "line_number" : null + }, { + "bad_value" : "20170916", + "entity_id" : null, + "entity_sequence" : null, + "entity_type" : null, + "error_id" : 7, "error_type" : "DATE_NO_SERVICE", "line_number" : null } ], diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json index 080545c85..9fbfafe83 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json @@ -5,8 +5,7 @@ "row_counts" : { "agency" : 1, "calendar" : 1, - "calendar_dates" : 2, - "errors" : 6, + "errors" : 8, "routes" : 1, "stop_times" : 4, "stops" : 5, From d96f4b4b8e83f97ea348d0df455283fd9afdc13c Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Wed, 28 Jun 2023 13:51:47 +0100 Subject: [PATCH 07/19] refactor(jbdcGtfsExporter): Bug fixes --- .../gtfs/loader/JdbcGtfsExporter.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 462495e70..b95161cdf 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -141,7 +141,7 @@ public FeedLoadResult exportTables() { int calendarDateCount = 0; // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one - // date and either one entry in add service or remove service. Must likely, only one entry ever in add + // date and either one entry in add service or remove service. Most likely, only one entry ever in add // service. for (ScheduleException ex : calendarDateServices) { // Only ever expecting one date here. @@ -150,15 +150,17 @@ public FeedLoadResult exportTables() { calendarDate.date = date; if (!ex.addedService.isEmpty()) { calendarDate.service_id = ex.addedService.get(0); + calendarDate.exception_type = 1; } else if (!ex.removedService.isEmpty()) { calendarDate.service_id = ex.removedService.get(0); + calendarDate.exception_type = 2; } else { calendarDate.service_id = ex.name; } - // It is assumed for a calendar date service that only dates where the service is available are defined. - calendarDate.exception_type = 1; Service service = new Service(calendarDate.service_id); service.calendar_dates.put(date, calendarDate); + // If the calendar dates provided contain duplicates (e.g. two or more identical service ids that are + // NOT associated with a calendar) only the first entry will persist export. feed.services.put(calendarDate.service_id, service); calendarDateCount++; } @@ -193,12 +195,12 @@ public FeedLoadResult exportTables() { } feed.services.put(cal.service_id, service); } - if (calendarDateCount == 0) { - LOG.info("No calendar dates found. Skipping table."); - } else { - LOG.info("Writing {} calendar dates from schedule exceptions", calendarDateCount); - new CalendarDate.Writer(feed).writeTable(zipOutputStream); - } + } + if (calendarDateCount == 0) { + LOG.info("No calendar dates found. Skipping table."); + } else { + LOG.info("Writing {} calendar dates from schedule exceptions", calendarDateCount); + new CalendarDate.Writer(feed).writeTable(zipOutputStream); } if (calendarsReader.getRowCount() == 0 && calendarDateServices.isEmpty()) { From fc79dfea94aa81e36ba5634ba16c19f601b59e9a Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 10:41:05 +0100 Subject: [PATCH 08/19] refactor(Updats to better handle calendar date services): --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 61 ++++++++++--------- .../gtfs/graphql/GraphQLGtfsSchema.java | 9 --- .../gtfs/loader/JdbcGtfsExporter.java | 30 ++++----- .../gtfs/loader/JdbcGtfsSnapshotter.java | 60 ++++++++++++------ .../conveyal/gtfs/loader/JdbcTableWriter.java | 7 +-- .../gtfs/model/ScheduleException.java | 2 +- src/test/java/com/conveyal/gtfs/GTFSTest.java | 10 ++- 7 files changed, 98 insertions(+), 81 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index 14a0e052a..41ef84f5e 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -153,18 +153,21 @@ else if (feedId == null || feedId.isEmpty()) { new Agency.Loader(this).loadTable(zip); - // Calendars and calendar dates are joined into services, where there is a match on service id. This means a lot - // of manipulating service objects as they are loaded; since mapdb keys/values are immutable, load them in - // memory then copy them to MapDB once we're done loading them. + // calendars and calendar dates are joined into services. This means a lot of manipulating service objects as + // they are loaded; since mapdb keys/values are immutable, load them in memory then copy them to MapDB once + // we're done loading them Map serviceTable = new HashMap<>(); new Calendar.Loader(this, serviceTable).loadTable(zip); new CalendarDate.Loader(this, serviceTable).loadTable(zip); this.services.putAll(serviceTable); + serviceTable = null; // free memory + // Same deal Map fares = new HashMap<>(); new FareAttribute.Loader(this, fares).loadTable(zip); new FareRule.Loader(this, fares).loadTable(zip); this.fares.putAll(fares); + fares = null; // free memory new Route.Loader(this).loadTable(zip); new ShapePoint.Loader(this).loadTable(zip); @@ -294,10 +297,10 @@ public FeedInfo getFeedInfo () { */ public Iterable getOrderedStopTimesForTrip (String trip_id) { Map tripStopTimes = - stop_times.subMap( - Fun.t2(trip_id, null), - Fun.t2(trip_id, Fun.HI) - ); + stop_times.subMap( + Fun.t2(trip_id, null), + Fun.t2(trip_id, Fun.HI) + ); return tripStopTimes.values(); } @@ -358,7 +361,7 @@ public void findPatterns () { } Map patternObjects = patternFinder.createPatternObjects(this.stops, null); this.patterns.putAll(patternObjects.values().stream() - .collect(Collectors.toMap(Pattern::getId, pattern -> pattern))); + .collect(Collectors.toMap(Pattern::getId, pattern -> pattern))); } /** @@ -367,8 +370,8 @@ public void findPatterns () { public Iterable getInterpolatedStopTimesForTrip (String trip_id) throws FirstAndLastStopsDoNotHaveTimes { // clone stop times so as not to modify base GTFS structures StopTime[] stopTimes = StreamSupport.stream(getOrderedStopTimesForTrip(trip_id).spliterator(), false) - .map(st -> st.clone()) - .toArray(i -> new StopTime[i]); + .map(st -> st.clone()) + .toArray(i -> new StopTime[i]); // avoid having to make sure that the array has length below. if (stopTimes.length == 0) return Collections.emptyList(); @@ -446,8 +449,8 @@ public Collection getFrequencies (String trip_id) { // IntelliJ tells me all these casts are unnecessary, and that's also my feeling, but the code won't compile // without them return (List) frequencies.subSet(new Fun.Tuple2(trip_id, null), new Fun.Tuple2(trip_id, Fun.HI)).stream() - .map(t2 -> ((Tuple2) t2).b) - .collect(Collectors.toList()); + .map(t2 -> ((Tuple2) t2).b) + .collect(Collectors.toList()); } public List getOrderedStopListForTrip (String trip_id) { @@ -515,8 +518,8 @@ public LineString getTripGeometry(String trip_id){ /** Get the length of a trip in meters. */ public double getTripDistance (String trip_id, boolean straightLine) { return straightLine - ? GeoUtils.getDistance(this.getStraightLineForStops(trip_id)) - : GeoUtils.getDistance(this.getTripGeometry(trip_id)); + ? GeoUtils.getDistance(this.getStraightLineForStops(trip_id)) + : GeoUtils.getDistance(this.getTripGeometry(trip_id)); } /** Get trip speed (using trip shape if available) in meters per second. */ @@ -547,7 +550,7 @@ public Polygon getConvexHull() { if (this.convexHull == null) { synchronized (this) { List coordinates = this.stops.values().stream().map( - stop -> new Coordinate(stop.stop_lon, stop.stop_lat) + stop -> new Coordinate(stop.stop_lon, stop.stop_lat) ).collect(Collectors.toList()); Coordinate[] coords = coordinates.toArray(new Coordinate[coordinates.size()]); ConvexHull convexHull = new ConvexHull(coords, gf); @@ -588,13 +591,13 @@ public class FirstAndLastStopsDoNotHaveTimes extends Exception { public GTFSFeed () { // calls to this must be first operation in constructor - why, Java? this(DBMaker.newTempFileDB() - .transactionDisable() - .mmapFileEnable() - .asyncWriteEnable() - .deleteFilesAfterClose() - .compressionEnable() - // .cacheSize(1024 * 1024) this bloats memory consumption - .make()); // TODO db.close(); + .transactionDisable() + .mmapFileEnable() + .asyncWriteEnable() + .deleteFilesAfterClose() + .compressionEnable() + // .cacheSize(1024 * 1024) this bloats memory consumption + .make()); // TODO db.close(); } /** Create a GTFS feed connected to a particular DB, which will be created if it does not exist. */ @@ -607,12 +610,12 @@ private static DB constructDB(String dbFile) { try{ DBMaker dbMaker = DBMaker.newFileDB(new File(dbFile)); db = dbMaker - .transactionDisable() - .mmapFileEnable() - .asyncWriteEnable() - .compressionEnable() + .transactionDisable() + .mmapFileEnable() + .asyncWriteEnable() + .compressionEnable() // .cacheSize(1024 * 1024) this bloats memory consumption - .make(); + .make(); return db; } catch (ExecutionError | IOError | Exception e) { LOG.error("Could not construct db from file.", e); @@ -643,8 +646,8 @@ private GTFSFeed (DB db) { // use Java serialization because MapDB serialization is very slow with JTS as they have a lot of references. // nothing else contains JTS objects patterns = db.createTreeMap("patterns") - .valueSerializer(Serializer.JAVA) - .makeOrGet(); + .valueSerializer(Serializer.JAVA) + .makeOrGet(); tripPatternMap = db.getTreeMap("patternForTrip"); diff --git a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java index f3eef36f9..7085f78e0 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java +++ b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java @@ -89,15 +89,6 @@ public class GraphQLGtfsSchema { // FIXME: Description is an editor-specific field .field(MapFetcher.field("description")) .build(); - - // Represents rows from calendar_dates.txt - public static final GraphQLObjectType calendarDatesType = newObject() - .name("calendar_dates") - .field(MapFetcher.field("id", GraphQLInt)) - .field(MapFetcher.field("service_id")) - .field(MapFetcher.field("date")) - .field(MapFetcher.field("exception_type", GraphQLInt)) - .build(); private static final GraphQLScalarType stringList = new GraphQLScalarType("StringList", "List of Strings", new StringCoercing()); // Represents GTFS Editor service exceptions. diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index b95161cdf..ed0a9eee6 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -127,15 +127,14 @@ public FeedLoadResult exportTables() { calendarServiceIds.add(calendar.service_id); } Iterable exceptionsIterator = exceptionsReader.getAll(); - List exceptions = new ArrayList<>(); - List calendarDateServices = new ArrayList<>(); + List calendarExceptions = new ArrayList<>(); + List calendarDateExceptions = new ArrayList<>(); + // Separate distinct calendar date exceptions from those associated with calendars. for (ScheduleException ex : exceptionsIterator) { - if (ex.exemplar.equals(ScheduleException.ExemplarServiceDescriptor.SWAP) && - (ex.addedService.stream().noneMatch(calendarServiceIds::contains) && - ex.removedService.stream().noneMatch(calendarServiceIds::contains))) { - calendarDateServices.add(ex); + if (ex.exemplar.equals(ScheduleException.ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE)) { + calendarDateExceptions.add(ex); } else { - exceptions.add(ex); + calendarExceptions.add(ex); } } @@ -143,20 +142,13 @@ public FeedLoadResult exportTables() { // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one // date and either one entry in add service or remove service. Most likely, only one entry ever in add // service. - for (ScheduleException ex : calendarDateServices) { + for (ScheduleException ex : calendarDateExceptions) { // Only ever expecting one date here. LocalDate date = ex.dates.get(0); CalendarDate calendarDate = new CalendarDate(); calendarDate.date = date; - if (!ex.addedService.isEmpty()) { - calendarDate.service_id = ex.addedService.get(0); - calendarDate.exception_type = 1; - } else if (!ex.removedService.isEmpty()) { - calendarDate.service_id = ex.removedService.get(0); - calendarDate.exception_type = 2; - } else { - calendarDate.service_id = ex.name; - } + calendarDate.service_id = ex.name; + calendarDate.exception_type = 1; Service service = new Service(calendarDate.service_id); service.calendar_dates.put(date, calendarDate); // If the calendar dates provided contain duplicates (e.g. two or more identical service ids that are @@ -172,7 +164,7 @@ public FeedLoadResult exportTables() { for (Calendar cal : calendars) { Service service = new Service(cal.service_id); service.calendar = cal; - for (ScheduleException ex : exceptions) { + for (ScheduleException ex : calendarExceptions) { for (LocalDate date : ex.dates) { if (date.isBefore(cal.start_date) || date.isAfter(cal.end_date)) { @@ -203,7 +195,7 @@ public FeedLoadResult exportTables() { new CalendarDate.Writer(feed).writeTable(zipOutputStream); } - if (calendarsReader.getRowCount() == 0 && calendarDateServices.isEmpty()) { + if (calendarsReader.getRowCount() == 0 && calendarDateExceptions.isEmpty()) { // No calendar or calendar date service records exist, export calendar_dates as is and hope for the best. // This situation will occur in at least 2 scenarios: // 1. A GTFS has been loaded into the editor that had only the calendar_dates.txt file diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 9ba7fb807..71d46240f 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -2,6 +2,7 @@ import com.conveyal.gtfs.model.Calendar; import com.conveyal.gtfs.model.CalendarDate; +import com.conveyal.gtfs.model.ScheduleException; import com.google.common.collect.HashMultimap; import com.google.common.collect.Multimap; import com.google.common.collect.Sets; @@ -210,15 +211,6 @@ private TableLoadResult createScheduleExceptionsTable() { tablePrefix.replace(".", ""), true ); - String sql = String.format( - "insert into %s (name, dates, exemplar, added_service, removed_service) values (?, ?, ?, ?, ?)", - scheduleExceptionsTableName - ); - PreparedStatement scheduleExceptionsStatement = connection.prepareStatement(sql); - final BatchTracker scheduleExceptionsTracker = new BatchTracker( - "schedule_exceptions", - scheduleExceptionsStatement - ); // Fetch all entries in the calendar table to generate set of serviceIds that exist in the calendar // table. @@ -244,9 +236,10 @@ private TableLoadResult createScheduleExceptionsTable() { // Keep track of calendars by service id in case we need to add dummy calendar entries. Map dummyCalendarsByServiceId = new HashMap<>(); - // Iterate through calendar dates to build up to get maps from exceptions to their dates. + // Iterate through calendar dates to build up appropriate service dates. Multimap removedServiceForDate = HashMultimap.create(); Multimap addedServiceForDate = HashMultimap.create(); + HashMap calendarDateService = new HashMap<>(); for (CalendarDate calendarDate : calendarDates) { // Skip any null dates. if (calendarDate.date == null) { @@ -254,11 +247,12 @@ private TableLoadResult createScheduleExceptionsTable() { continue; } String date = calendarDate.date.format(DateTimeFormatter.BASIC_ISO_DATE); - if (calendarDate.exception_type == 1) { - addedServiceForDate.put(date, calendarDate.service_id); - // create (if needed) and extend range of dummy calendar that would need to be created if we are - // copying from a feed that doesn't have the calendar.txt file - if (calendarDate.service_id != null && calendarServiceIds.contains(calendarDate.service_id)) { + if (calendarDate.service_id != null && calendarServiceIds.contains(calendarDate.service_id)) { + // Calendar date is related to a calendar. + if (calendarDate.exception_type == 1) { + addedServiceForDate.put(date, calendarDate.service_id); + // create (if needed) and extend range of dummy calendar that would need to be created if we are + // copying from a feed that doesn't have the calendar.txt file Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); calendar.service_id = calendarDate.service_id; if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { @@ -268,11 +262,25 @@ private TableLoadResult createScheduleExceptionsTable() { calendar.end_date = calendarDate.date; } dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); + } else { + removedServiceForDate.put(date, calendarDate.service_id); } - } else { - removedServiceForDate.put(date, calendarDate.service_id); + } else if (!calendarServiceIds.contains(calendarDate.service_id)) { + // Calendar date is unique. + calendarDateService.put(date, calendarDate.service_id); } } + + String sql = String.format( + "insert into %s (name, dates, exemplar, added_service, removed_service) values (?, ?, ?, ?, ?)", + scheduleExceptionsTableName + ); + PreparedStatement scheduleExceptionsStatement = connection.prepareStatement(sql); + final BatchTracker scheduleExceptionsTracker = new BatchTracker( + "schedule_exceptions", + scheduleExceptionsStatement + ); + // Iterate through dates with added or removed service and add to database. // For usability and simplicity of code, don't attempt to find all dates with similar // added and removed services, but simply create an entry for each found date. @@ -280,7 +288,7 @@ private TableLoadResult createScheduleExceptionsTable() { scheduleExceptionsStatement.setString(1, date); String[] dates = {date}; scheduleExceptionsStatement.setArray(2, connection.createArrayOf("text", dates)); - scheduleExceptionsStatement.setInt(3, 9); // FIXME use better static type + scheduleExceptionsStatement.setInt(3, ScheduleException.ExemplarServiceDescriptor.SWAP.getValue()); scheduleExceptionsStatement.setArray( 4, connection.createArrayOf("text", addedServiceForDate.get(date).toArray()) @@ -291,6 +299,22 @@ private TableLoadResult createScheduleExceptionsTable() { ); scheduleExceptionsTracker.addBatch(); } + for (Map.Entry entry : calendarDateService.entrySet()) { + String serviceId = entry.getValue(); + scheduleExceptionsStatement.setString(1, serviceId); + String[] dates = {entry.getKey()}; + scheduleExceptionsStatement.setArray(2, connection.createArrayOf("text", dates)); + scheduleExceptionsStatement.setInt(3, ScheduleException.ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE.getValue()); + scheduleExceptionsStatement.setArray( + 4, + connection.createArrayOf("text", new String[] {}) + ); + scheduleExceptionsStatement.setArray( + 5, + connection.createArrayOf("text", new String[] {}) + ); + scheduleExceptionsTracker.addBatch(); + } scheduleExceptionsTracker.executeRemaining(); // For service_ids that only existed in the calendar_dates table, insert auto-generated, "blank" diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 99e7f1ae8..2790acf3a 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -683,10 +683,9 @@ private String updateChildTable( } /** - * Check any references the sub entity might have. For example, this checks that stop_id values on - * pattern_stops refer to entities that actually exist in the stops table. NOTE: This skips the "specTable", - * i.e., for pattern stops it will not check pattern_id references. This is enforced above with the put key - * field statement above. + * Check any references the sub entity might have. For example, this checks that a service_id defined in a trip + * refers to a calendar or calendar date. NOTE: This skips the "specTable", i.e., for pattern stops it will not + * check pattern_id references. This is enforced above with the put key field statement above. */ private void checkTableReferences( Multimap> foreignReferencesPerTable, diff --git a/src/main/java/com/conveyal/gtfs/model/ScheduleException.java b/src/main/java/com/conveyal/gtfs/model/ScheduleException.java index 79f0220fc..e88c04658 100644 --- a/src/main/java/com/conveyal/gtfs/model/ScheduleException.java +++ b/src/main/java/com/conveyal/gtfs/model/ScheduleException.java @@ -41,7 +41,7 @@ public class ScheduleException extends Entity { @Override public void setStatementParameters(PreparedStatement statement, boolean setDefaultId) throws SQLException { - // Require + // FIXME } public boolean serviceRunsOn(Calendar calendar) { diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 4e6a48448..65da01bef 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -131,13 +131,21 @@ public void canLoadAndExportSimpleAgency() { * Tests that a GTFS feed with bad date values in calendars.txt and calendar_dates.txt can pass the integration test. */ @Test - public void canLoadFeedWithBadDates () { + void canLoadFeedWithBadDates () { PersistenceExpectation[] expectations = PersistenceExpectation.list( new PersistenceExpectation( "calendar", new RecordExpectation[]{ new RecordExpectation("start_date", null) } + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation("service_id", "123_ID_NOT_EXISTS"), + new RecordExpectation("date", "20190301"), + new RecordExpectation("exception_type", "1") + } ) ); ErrorExpectation[] errorExpectations = ErrorExpectation.list( From 00147c03c2b7639a1d4cae0adfaa40d3621760b3 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 10:47:07 +0100 Subject: [PATCH 09/19] refactor(GraphQLGtfsSchema.java): Restored to original state --- src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java index 7085f78e0..fe5d901ce 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java +++ b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java @@ -89,6 +89,7 @@ public class GraphQLGtfsSchema { // FIXME: Description is an editor-specific field .field(MapFetcher.field("description")) .build(); + private static final GraphQLScalarType stringList = new GraphQLScalarType("StringList", "List of Strings", new StringCoercing()); // Represents GTFS Editor service exceptions. @@ -477,6 +478,7 @@ public class GraphQLGtfsSchema { .field(RowCountFetcher.field("stop_times")) .field(RowCountFetcher.field("agency")) .field(RowCountFetcher.field("calendar")) + .field(RowCountFetcher.field("calendar_dates")) .field(RowCountFetcher.field("errors")) .build(); From d7aaa57a101a9c492d2be30c856e1ec42026b30c Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 11:34:32 +0100 Subject: [PATCH 10/19] refactor(Renamed calendar date service ids in tests): --- src/test/java/com/conveyal/gtfs/GTFSFeedTest.java | 2 +- src/test/java/com/conveyal/gtfs/GTFSTest.java | 4 ++-- .../calendar_dates.txt | 4 ++-- .../fake-agency-mixture-of-calendar-definitions/trips.txt | 4 ++-- src/test/resources/fake-agency/calendar_dates.txt | 2 +- .../gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java index 414d31757..3cb09655c 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java @@ -100,7 +100,7 @@ public void canDoRoundTripLoadAndWriteToZipFile() throws IOException { new FileTestCase( "calendar_dates.txt", new DataExpectation[]{ - new DataExpectation("service_id", "exception-in-own-right"), + new DataExpectation("service_id", "calendar-date-service"), new DataExpectation("date", "20230619"), new DataExpectation("exception_type", "2") } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 65da01bef..70c60fa8f 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -439,7 +439,7 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { "calendar_dates", new RecordExpectation[]{ new RecordExpectation( - "service_id", "calendar-dates-txt-exception-one" + "service_id", "calendar-dates-txt-service-one" ), new RecordExpectation("date", 20170917), new RecordExpectation("exception_type", 1) @@ -449,7 +449,7 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { "calendar_dates", new RecordExpectation[]{ new RecordExpectation( - "service_id", "calendar-dates-txt-exception-two" + "service_id", "calendar-dates-txt-service-two" ), new RecordExpectation("date", 20170918), new RecordExpectation("exception_type", 1) diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt index 32f45f510..fc4825b41 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt @@ -1,5 +1,5 @@ service_id,date,exception_type in-both-calendar-txt-and-calendar-dates,20170920,2 only-in-calendar-dates-txt,20170916,1 -calendar-dates-txt-exception-one,20170917,1 -calendar-dates-txt-exception-two,20170918,1 \ No newline at end of file +calendar-dates-txt-service-one,20170917,1 +calendar-dates-txt-service-two,20170918,1 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt index a0c194b3f..221642959 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt @@ -2,5 +2,5 @@ route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bi 1,non-frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-dates-txt 1,non-frequency-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-txt 1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,in-both-calendar-txt-and-calendar-dates -2,exception-trip-1,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-exception-one -2,exception-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-exception-two \ No newline at end of file +2,exception-trip-1,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-service-one +2,exception-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-dates-txt-service-two \ No newline at end of file diff --git a/src/test/resources/fake-agency/calendar_dates.txt b/src/test/resources/fake-agency/calendar_dates.txt index 377866c8e..ea29d18fd 100755 --- a/src/test/resources/fake-agency/calendar_dates.txt +++ b/src/test/resources/fake-agency/calendar_dates.txt @@ -1,3 +1,3 @@ service_id,date,exception_type 04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 -exception-in-own-right,20230619,2 \ No newline at end of file +calendar-date-service,20230619,2 \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json index 883ad21fd..d17c52324 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json @@ -83,7 +83,7 @@ "error_type" : "STOP_UNUSED", "line_number" : 6 }, { - "bad_value" : "exception-in-own-right", + "bad_value" : "calendar-date-service", "entity_id" : null, "entity_sequence" : null, "entity_type" : null, @@ -91,7 +91,7 @@ "error_type" : "SERVICE_NEVER_ACTIVE", "line_number" : null }, { - "bad_value" : "exception-in-own-right", + "bad_value" : "calendar-date-service", "entity_id" : null, "entity_sequence" : null, "entity_type" : null, From 0255a380ecf4a3c73257190b2f04a024ee4edbe1 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 12:41:12 +0100 Subject: [PATCH 11/19] refactor(Various tweaks to test data): --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 52 +++++++++---------- .../gtfs/loader/JdbcGtfsExporter.java | 3 +- .../gtfs/loader/JdbcGtfsSnapshotter.java | 10 ++-- .../resources/fake-agency/calendar_dates.txt | 2 +- src/test/resources/fake-agency/trips.txt | 3 +- .../GTFSGraphQLTest/canFetchServices-0.json | 16 +++++- 6 files changed, 49 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index 41ef84f5e..8e6de45e2 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -297,10 +297,10 @@ public FeedInfo getFeedInfo () { */ public Iterable getOrderedStopTimesForTrip (String trip_id) { Map tripStopTimes = - stop_times.subMap( - Fun.t2(trip_id, null), - Fun.t2(trip_id, Fun.HI) - ); + stop_times.subMap( + Fun.t2(trip_id, null), + Fun.t2(trip_id, Fun.HI) + ); return tripStopTimes.values(); } @@ -361,7 +361,7 @@ public void findPatterns () { } Map patternObjects = patternFinder.createPatternObjects(this.stops, null); this.patterns.putAll(patternObjects.values().stream() - .collect(Collectors.toMap(Pattern::getId, pattern -> pattern))); + .collect(Collectors.toMap(Pattern::getId, pattern -> pattern))); } /** @@ -370,8 +370,8 @@ public void findPatterns () { public Iterable getInterpolatedStopTimesForTrip (String trip_id) throws FirstAndLastStopsDoNotHaveTimes { // clone stop times so as not to modify base GTFS structures StopTime[] stopTimes = StreamSupport.stream(getOrderedStopTimesForTrip(trip_id).spliterator(), false) - .map(st -> st.clone()) - .toArray(i -> new StopTime[i]); + .map(st -> st.clone()) + .toArray(i -> new StopTime[i]); // avoid having to make sure that the array has length below. if (stopTimes.length == 0) return Collections.emptyList(); @@ -449,8 +449,8 @@ public Collection getFrequencies (String trip_id) { // IntelliJ tells me all these casts are unnecessary, and that's also my feeling, but the code won't compile // without them return (List) frequencies.subSet(new Fun.Tuple2(trip_id, null), new Fun.Tuple2(trip_id, Fun.HI)).stream() - .map(t2 -> ((Tuple2) t2).b) - .collect(Collectors.toList()); + .map(t2 -> ((Tuple2) t2).b) + .collect(Collectors.toList()); } public List getOrderedStopListForTrip (String trip_id) { @@ -518,8 +518,8 @@ public LineString getTripGeometry(String trip_id){ /** Get the length of a trip in meters. */ public double getTripDistance (String trip_id, boolean straightLine) { return straightLine - ? GeoUtils.getDistance(this.getStraightLineForStops(trip_id)) - : GeoUtils.getDistance(this.getTripGeometry(trip_id)); + ? GeoUtils.getDistance(this.getStraightLineForStops(trip_id)) + : GeoUtils.getDistance(this.getTripGeometry(trip_id)); } /** Get trip speed (using trip shape if available) in meters per second. */ @@ -550,7 +550,7 @@ public Polygon getConvexHull() { if (this.convexHull == null) { synchronized (this) { List coordinates = this.stops.values().stream().map( - stop -> new Coordinate(stop.stop_lon, stop.stop_lat) + stop -> new Coordinate(stop.stop_lon, stop.stop_lat) ).collect(Collectors.toList()); Coordinate[] coords = coordinates.toArray(new Coordinate[coordinates.size()]); ConvexHull convexHull = new ConvexHull(coords, gf); @@ -591,13 +591,13 @@ public class FirstAndLastStopsDoNotHaveTimes extends Exception { public GTFSFeed () { // calls to this must be first operation in constructor - why, Java? this(DBMaker.newTempFileDB() - .transactionDisable() - .mmapFileEnable() - .asyncWriteEnable() - .deleteFilesAfterClose() - .compressionEnable() - // .cacheSize(1024 * 1024) this bloats memory consumption - .make()); // TODO db.close(); + .transactionDisable() + .mmapFileEnable() + .asyncWriteEnable() + .deleteFilesAfterClose() + .compressionEnable() + // .cacheSize(1024 * 1024) this bloats memory consumption + .make()); // TODO db.close(); } /** Create a GTFS feed connected to a particular DB, which will be created if it does not exist. */ @@ -610,12 +610,12 @@ private static DB constructDB(String dbFile) { try{ DBMaker dbMaker = DBMaker.newFileDB(new File(dbFile)); db = dbMaker - .transactionDisable() - .mmapFileEnable() - .asyncWriteEnable() - .compressionEnable() + .transactionDisable() + .mmapFileEnable() + .asyncWriteEnable() + .compressionEnable() // .cacheSize(1024 * 1024) this bloats memory consumption - .make(); + .make(); return db; } catch (ExecutionError | IOError | Exception e) { LOG.error("Could not construct db from file.", e); @@ -646,8 +646,8 @@ private GTFSFeed (DB db) { // use Java serialization because MapDB serialization is very slow with JTS as they have a lot of references. // nothing else contains JTS objects patterns = db.createTreeMap("patterns") - .valueSerializer(Serializer.JAVA) - .makeOrGet(); + .valueSerializer(Serializer.JAVA) + .makeOrGet(); tripPatternMap = db.getTreeMap("patternForTrip"); diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index ed0a9eee6..8e1b08878 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -140,8 +140,7 @@ public FeedLoadResult exportTables() { int calendarDateCount = 0; // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one - // date and either one entry in add service or remove service. Most likely, only one entry ever in add - // service. + // date which will be exported as a service on the specified date. for (ScheduleException ex : calendarDateExceptions) { // Only ever expecting one date here. LocalDate date = ex.dates.get(0); diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 71d46240f..5468af7e8 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -241,13 +241,13 @@ private TableLoadResult createScheduleExceptionsTable() { Multimap addedServiceForDate = HashMultimap.create(); HashMap calendarDateService = new HashMap<>(); for (CalendarDate calendarDate : calendarDates) { - // Skip any null dates. - if (calendarDate.date == null) { - LOG.warn("Encountered calendar date record with null value for date field. Skipping."); + // Skip any null dates or service ids. + if (calendarDate.date == null || calendarDate.service_id == null) { + LOG.warn("Encountered calendar date record with null value for date/service_id field. Skipping."); continue; } String date = calendarDate.date.format(DateTimeFormatter.BASIC_ISO_DATE); - if (calendarDate.service_id != null && calendarServiceIds.contains(calendarDate.service_id)) { + if (calendarServiceIds.contains(calendarDate.service_id)) { // Calendar date is related to a calendar. if (calendarDate.exception_type == 1) { addedServiceForDate.put(date, calendarDate.service_id); @@ -265,7 +265,7 @@ private TableLoadResult createScheduleExceptionsTable() { } else { removedServiceForDate.put(date, calendarDate.service_id); } - } else if (!calendarServiceIds.contains(calendarDate.service_id)) { + } else { // Calendar date is unique. calendarDateService.put(date, calendarDate.service_id); } diff --git a/src/test/resources/fake-agency/calendar_dates.txt b/src/test/resources/fake-agency/calendar_dates.txt index ea29d18fd..8ae192325 100755 --- a/src/test/resources/fake-agency/calendar_dates.txt +++ b/src/test/resources/fake-agency/calendar_dates.txt @@ -1,3 +1,3 @@ service_id,date,exception_type 04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 -calendar-date-service,20230619,2 \ No newline at end of file +calendar-date-service,20230619,1 \ No newline at end of file diff --git a/src/test/resources/fake-agency/trips.txt b/src/test/resources/fake-agency/trips.txt index eab14b86d..8dbe316b2 100755 --- a/src/test/resources/fake-agency/trips.txt +++ b/src/test/resources/fake-agency/trips.txt @@ -1,3 +1,4 @@ route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bikes_allowed,wheelchair_accessible,service_id 1,a30277f8-e50a-4a85-9141-b1e0da9d429d,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 -1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 \ No newline at end of file +1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 +1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-date-service \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json index 26fc242ed..0d0e8e439 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json @@ -4,9 +4,9 @@ "feed_version" : "1.0", "services" : [ { "dates" : [ "20170915", "20170917" ], - "duration_seconds" : "1800", + "duration_seconds" : "60", "durations" : [ { - "duration_seconds" : 1800, + "duration_seconds" : 60, "route_type" : 3 } ], "n_days_active" : "2", @@ -16,6 +16,18 @@ }, { "trip_id" : "frequency-trip" } ] + }, { + "dates" : [ "20230619" ], + "duration_seconds" : "1740", + "durations" : [ { + "duration_seconds" : 1740, + "route_type" : 3 + } ], + "n_days_active" : "1", + "service_id" : "calendar-date-service", + "trips" : [ { + "trip_id" : "frequency-trip" + } ] } ] } } From 275e62ba842d5ffdbc211cbe521eff280dd11079 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 14:35:03 +0100 Subject: [PATCH 12/19] refactor(Updates to test results to match updated test data): --- .../java/com/conveyal/gtfs/GTFSFeedTest.java | 4 +- src/test/java/com/conveyal/gtfs/GTFSTest.java | 4 - .../resources/fake-agency/calendar_dates.txt | 2 +- src/test/resources/fake-agency/stop_times.txt | 2 + src/test/resources/fake-agency/trips.txt | 2 +- .../GTFSGraphQLTest/canFetchErrors-0.json | 28 +------ .../canFetchFeedRowCounts-0.json | 6 +- .../GTFSGraphQLTest/canFetchPatterns-0.json | 14 ++-- .../GTFSGraphQLTest/canFetchRoutes-0.json | 4 +- .../GTFSGraphQLTest/canFetchServices-0.json | 8 +- .../GTFSGraphQLTest/canFetchStopTimes-0.json | 24 +++++- .../GTFSGraphQLTest/canFetchStops-0.json | 12 ++- .../GTFSGraphQLTest/canFetchTrips-0.json | 73 +++++++++++++++++++ ...nSanitizeSQLInjectionSentAsKeyValue-0.json | 4 +- 14 files changed, 134 insertions(+), 53 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java index 3cb09655c..18f843d4c 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSFeedTest.java @@ -101,8 +101,8 @@ public void canDoRoundTripLoadAndWriteToZipFile() throws IOException { "calendar_dates.txt", new DataExpectation[]{ new DataExpectation("service_id", "calendar-date-service"), - new DataExpectation("date", "20230619"), - new DataExpectation("exception_type", "2") + new DataExpectation("date", "20170917"), + new DataExpectation("exception_type", "1") } ), new FileTestCase( diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 70c60fa8f..3754b8d73 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -112,8 +112,6 @@ public void canLoadAndExportSimpleAgency() { new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), - new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( @@ -292,8 +290,6 @@ public void canLoadAndExportSimpleAgencyInSubDirectory() { new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED), - new ErrorExpectation(NewGTFSErrorType.SERVICE_NEVER_ACTIVE), - new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED), new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) ); assertThat( diff --git a/src/test/resources/fake-agency/calendar_dates.txt b/src/test/resources/fake-agency/calendar_dates.txt index 8ae192325..5d0a31806 100755 --- a/src/test/resources/fake-agency/calendar_dates.txt +++ b/src/test/resources/fake-agency/calendar_dates.txt @@ -1,3 +1,3 @@ service_id,date,exception_type 04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 -calendar-date-service,20230619,1 \ No newline at end of file +calendar-date-service,20170917,1 \ No newline at end of file diff --git a/src/test/resources/fake-agency/stop_times.txt b/src/test/resources/fake-agency/stop_times.txt index 5d8793689..1cfefaa49 100755 --- a/src/test/resources/fake-agency/stop_times.txt +++ b/src/test/resources/fake-agency/stop_times.txt @@ -3,3 +3,5 @@ a30277f8-e50a-4a85-9141-b1e0da9d429d,07:00:00,07:00:00,4u6g,1,Test stop headsign a30277f8-e50a-4a85-9141-b1e0da9d429d,07:01:00,07:01:00,johv,2,Test stop headsign 2,0,0,341.4491961, frequency-trip,08:00:00,08:00:00,4u6g,1,Test stop headsign frequency trip,0,0,0.0000000, frequency-trip,08:29:00,08:29:00,1234,2,Test stop headsign frequency trip 2,0,0,341.4491961, +calendar-date-trip,08:00:00,08:00:00,4u6g,1,Test stop headsign calendar date trip,0,0,0.0000000, +calendar-date-trip,08:29:00,08:29:00,1234,2,Test stop headsign calendar date trip 2,0,0,341.4491961, \ No newline at end of file diff --git a/src/test/resources/fake-agency/trips.txt b/src/test/resources/fake-agency/trips.txt index 8dbe316b2..982c01e0f 100755 --- a/src/test/resources/fake-agency/trips.txt +++ b/src/test/resources/fake-agency/trips.txt @@ -1,4 +1,4 @@ route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bikes_allowed,wheelchair_accessible,service_id 1,a30277f8-e50a-4a85-9141-b1e0da9d429d,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 -1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-date-service \ No newline at end of file +1,calendar-date-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,calendar-date-service \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json index d17c52324..743035e50 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors-0.json @@ -26,16 +26,6 @@ "message" : "The long name of a route should complement the short name, not include it.", "priority" : "LOW", "type" : "ROUTE_LONG_NAME_CONTAINS_SHORT_NAME" - }, { - "count" : 1, - "message" : "A service code was defined, but is never active on any date.", - "priority" : "MEDIUM", - "type" : "SERVICE_NEVER_ACTIVE" - }, { - "count" : 1, - "message" : "A service code was defined, but is never referenced by any trips.", - "priority" : "MEDIUM", - "type" : "SERVICE_UNUSED" }, { "count" : 1, "message" : "This stop is not referenced by any trips.", @@ -82,28 +72,12 @@ "error_id" : 4, "error_type" : "STOP_UNUSED", "line_number" : 6 - }, { - "bad_value" : "calendar-date-service", - "entity_id" : null, - "entity_sequence" : null, - "entity_type" : null, - "error_id" : 5, - "error_type" : "SERVICE_NEVER_ACTIVE", - "line_number" : null - }, { - "bad_value" : "calendar-date-service", - "entity_id" : null, - "entity_sequence" : null, - "entity_type" : null, - "error_id" : 6, - "error_type" : "SERVICE_UNUSED", - "line_number" : null }, { "bad_value" : "20170916", "entity_id" : null, "entity_sequence" : null, "entity_type" : null, - "error_id" : 7, + "error_id" : 5, "error_type" : "DATE_NO_SERVICE", "line_number" : null } ], diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json index 9fbfafe83..a8f58d527 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts-0.json @@ -5,11 +5,11 @@ "row_counts" : { "agency" : 1, "calendar" : 1, - "errors" : 8, + "errors" : 6, "routes" : 1, - "stop_times" : 4, + "stop_times" : 6, "stops" : 5, - "trips" : 2 + "trips" : 3 }, "snapshot_of" : null } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchPatterns-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchPatterns-0.json index 3732092fe..5c45497fd 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchPatterns-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchPatterns-0.json @@ -14,11 +14,11 @@ "id" : 1, "pattern_id" : "1", "pickup_type" : 0, - "stop_headsign" : "Test stop headsign", "shape_dist_traveled" : 0.0, "stop" : [ { "stop_id" : "4u6g" } ], + "stop_headsign" : "Test stop headsign", "stop_id" : "4u6g", "stop_sequence" : 0, "timepoint" : null @@ -29,11 +29,11 @@ "id" : 2, "pattern_id" : "1", "pickup_type" : 0, - "stop_headsign" : "Test stop headsign 2", "shape_dist_traveled" : 341.4491961, "stop" : [ { "stop_id" : "johv" } ], + "stop_headsign" : "Test stop headsign 2", "stop_id" : "johv", "stop_sequence" : 1, "timepoint" : null @@ -106,7 +106,7 @@ }, { "direction_id" : 0, "id" : 2, - "name" : "2 stops from Butler Ln to Child Stop (1 trips)", + "name" : "2 stops from Butler Ln to Child Stop (2 trips)", "pattern_id" : "2", "pattern_stops" : [ { "default_dwell_time" : 0, @@ -115,11 +115,11 @@ "id" : 3, "pattern_id" : "2", "pickup_type" : 0, - "stop_headsign" : "Test stop headsign frequency trip", "shape_dist_traveled" : 0.0, "stop" : [ { "stop_id" : "4u6g" } ], + "stop_headsign" : "Test stop headsign calendar date trip", "stop_id" : "4u6g", "stop_sequence" : 0, "timepoint" : null @@ -130,11 +130,11 @@ "id" : 4, "pattern_id" : "2", "pickup_type" : 0, - "stop_headsign" : "Test stop headsign frequency trip 2", "shape_dist_traveled" : 341.4491961, "stop" : [ { "stop_id" : "1234" } ], + "stop_headsign" : "Test stop headsign calendar date trip 2", "stop_id" : "1234", "stop_sequence" : 1, "timepoint" : null @@ -199,9 +199,11 @@ }, { "stop_id" : "1234" } ], - "trip_count" : 1, + "trip_count" : 2, "trips" : [ { "trip_id" : "frequency-trip" + }, { + "trip_id" : "calendar-date-trip" } ], "use_frequency" : null } ] diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchRoutes-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchRoutes-0.json index 4792aa782..1c1a10c26 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchRoutes-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchRoutes-0.json @@ -30,11 +30,13 @@ }, { "stop_id" : "1234" } ], - "trip_count" : 2, + "trip_count" : 3, "trips" : [ { "trip_id" : "a30277f8-e50a-4a85-9141-b1e0da9d429d" }, { "trip_id" : "frequency-trip" + }, { + "trip_id" : "calendar-date-trip" } ], "wheelchair_accessible" : null } ] diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json index 0d0e8e439..9911140b9 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchServices-0.json @@ -4,9 +4,9 @@ "feed_version" : "1.0", "services" : [ { "dates" : [ "20170915", "20170917" ], - "duration_seconds" : "60", + "duration_seconds" : "1800", "durations" : [ { - "duration_seconds" : 60, + "duration_seconds" : 1800, "route_type" : 3 } ], "n_days_active" : "2", @@ -17,7 +17,7 @@ "trip_id" : "frequency-trip" } ] }, { - "dates" : [ "20230619" ], + "dates" : [ "20170917" ], "duration_seconds" : "1740", "durations" : [ { "duration_seconds" : 1740, @@ -26,7 +26,7 @@ "n_days_active" : "1", "service_id" : "calendar-date-service", "trips" : [ { - "trip_id" : "frequency-trip" + "trip_id" : "calendar-date-trip" } ] } ] } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStopTimes-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStopTimes-0.json index 370176de9..08f52fc97 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStopTimes-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStopTimes-0.json @@ -41,11 +41,33 @@ "drop_off_type" : 0, "pickup_type" : 0, "shape_dist_traveled" : 341.4491961, - "stop_headsign" : "Test stop headsign frequency trip", + "stop_headsign" : "Test stop headsign frequency trip 2", "stop_id" : "1234", "stop_sequence" : 2, "timepoint" : null, "trip_id" : "frequency-trip" + }, { + "arrival_time" : 28800, + "departure_time" : 28800, + "drop_off_type" : 0, + "pickup_type" : 0, + "shape_dist_traveled" : 0.0, + "stop_headsign" : "Test stop headsign calendar date trip", + "stop_id" : "4u6g", + "stop_sequence" : 1, + "timepoint" : null, + "trip_id" : "calendar-date-trip" + }, { + "arrival_time" : 30540, + "departure_time" : 30540, + "drop_off_type" : 0, + "pickup_type" : 0, + "shape_dist_traveled" : 341.4491961, + "stop_headsign" : "Test stop headsign calendar date trip 2", + "stop_id" : "1234", + "stop_sequence" : 2, + "timepoint" : null, + "trip_id" : "calendar-date-trip" } ] } } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStops-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStops-0.json index 0594014cd..15a993f2d 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStops-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchStops-0.json @@ -20,7 +20,7 @@ "stop_lat" : 37.0612132, "stop_lon" : -122.0074332, "stop_name" : "Butler Ln", - "stop_time_count" : 2, + "stop_time_count" : 3, "stop_times" : [ { "stop_id" : "4u6g", "stop_sequence" : 1, @@ -29,6 +29,10 @@ "stop_id" : "4u6g", "stop_sequence" : 1, "trip_id" : "frequency-trip" + }, { + "stop_id" : "4u6g", + "stop_sequence" : 1, + "trip_id" : "calendar-date-trip" } ], "stop_timezone" : null, "stop_url" : null, @@ -94,11 +98,15 @@ "stop_lat" : 37.06662, "stop_lon" : -122.07772, "stop_name" : "Child Stop", - "stop_time_count" : 1, + "stop_time_count" : 2, "stop_times" : [ { "stop_id" : "1234", "stop_sequence" : 2, "trip_id" : "frequency-trip" + }, { + "stop_id" : "1234", + "stop_sequence" : 2, + "trip_id" : "calendar-date-trip" } ], "stop_timezone" : null, "stop_url" : null, diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchTrips-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchTrips-0.json index deb41d06b..6e31418a8 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchTrips-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchTrips-0.json @@ -150,6 +150,79 @@ "trip_id" : "frequency-trip", "trip_short_name" : null, "wheelchair_accessible" : 0 + }, { + "bikes_allowed" : 0, + "block_id" : null, + "direction_id" : 0, + "frequencies" : [ ], + "id" : 4, + "pattern_id" : "2", + "route_id" : "1", + "service_id" : "calendar-date-service", + "shape" : [ { + "point_type" : null, + "shape_dist_traveled" : 0.0, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.0612132, + "shape_pt_lon" : -122.0074332, + "shape_pt_sequence" : 1 + }, { + "point_type" : null, + "shape_dist_traveled" : 7.4997067, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.061172, + "shape_pt_lon" : -122.0075, + "shape_pt_sequence" : 2 + }, { + "point_type" : null, + "shape_dist_traveled" : 33.8739075, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.061359, + "shape_pt_lon" : -122.007683, + "shape_pt_sequence" : 3 + }, { + "point_type" : null, + "shape_dist_traveled" : 109.0402932, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.060878, + "shape_pt_lon" : -122.008278, + "shape_pt_sequence" : 4 + }, { + "point_type" : null, + "shape_dist_traveled" : 184.6078298, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.060359, + "shape_pt_lon" : -122.008828, + "shape_pt_sequence" : 5 + }, { + "point_type" : null, + "shape_dist_traveled" : 265.8053023, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.059761, + "shape_pt_lon" : -122.009354, + "shape_pt_sequence" : 6 + }, { + "point_type" : null, + "shape_dist_traveled" : 357.8617018, + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "shape_pt_lat" : 37.059066, + "shape_pt_lon" : -122.009919, + "shape_pt_sequence" : 7 + } ], + "shape_id" : "5820f377-f947-4728-ac29-ac0102cbc34e", + "stop_times" : [ { + "stop_id" : "4u6g", + "stop_sequence" : 1, + "trip_id" : "calendar-date-trip" + }, { + "stop_id" : "1234", + "stop_sequence" : 2, + "trip_id" : "calendar-date-trip" + } ], + "trip_headsign" : null, + "trip_id" : "calendar-date-trip", + "trip_short_name" : null, + "wheelchair_accessible" : 0 } ] } } diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canSanitizeSQLInjectionSentAsKeyValue-0.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canSanitizeSQLInjectionSentAsKeyValue-0.json index 9b1e74973..04c98ecdc 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canSanitizeSQLInjectionSentAsKeyValue-0.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canSanitizeSQLInjectionSentAsKeyValue-0.json @@ -30,11 +30,13 @@ }, { "stop_id" : "1234" } ], - "trip_count" : 2, + "trip_count" : 3, "trips" : [ { "trip_id" : "a30277f8-e50a-4a85-9141-b1e0da9d429d" }, { "trip_id" : "frequency-trip" + }, { + "trip_id" : "calendar-date-trip" } ], "wheelchair_accessible" : null } ] From ba747df9a48a4caa74a414d815967071977276e2 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 29 Jun 2023 15:13:32 +0100 Subject: [PATCH 13/19] refactor(Field.java): Changed referenceTable to a linked hash set so ordering can be used in MergeLi --- src/main/java/com/conveyal/gtfs/loader/Field.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/Field.java b/src/main/java/com/conveyal/gtfs/loader/Field.java index 13c2c74a9..3531fdaa5 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Field.java +++ b/src/main/java/com/conveyal/gtfs/loader/Field.java @@ -8,7 +8,7 @@ import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.SQLType; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.Set; /** @@ -47,7 +47,7 @@ public abstract class Field { * Indicates that this field acts as a foreign key to this referenced table. This is used when checking referential * integrity when loading a feed. * */ - public HashSet
referenceTables = new HashSet<>(); + public Set
referenceTables = new LinkedHashSet<>(); private boolean shouldBeIndexed; private boolean emptyValuePermitted; private boolean isConditionallyRequired; From 59b568a5e2c003b82c3bf89cf4cd503e6d3cdac9 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 7 Jul 2023 10:04:01 +0100 Subject: [PATCH 14/19] refactor(JdbcGtfsExporter.java): Update to tie exceptions to a calendar based on service id --- .../com/conveyal/gtfs/loader/JdbcGtfsExporter.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 8e1b08878..64292716a 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -32,6 +32,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; @@ -163,7 +164,12 @@ public FeedLoadResult exportTables() { for (Calendar cal : calendars) { Service service = new Service(cal.service_id); service.calendar = cal; - for (ScheduleException ex : calendarExceptions) { + List exceptionsForCalendar = calendarExceptions.stream().filter(ex -> + ex.addedService.contains(cal.service_id) || + ex.removedService.contains(cal.service_id) || + ex.customSchedule.contains(cal.service_id) + ).collect(Collectors.toList()); + for (ScheduleException ex : exceptionsForCalendar) { for (LocalDate date : ex.dates) { if (date.isBefore(cal.start_date) || date.isAfter(cal.end_date)) { @@ -175,7 +181,7 @@ public FeedLoadResult exportTables() { calendarDate.date = date; calendarDate.service_id = cal.service_id; calendarDate.exception_type = ex.serviceRunsOn(cal) ? 1 : 2; - LOG.info("Adding exception {} (type={}) for calendar {} on date {}", ex.name, calendarDate.exception_type, cal.service_id, date.toString()); + LOG.info("Adding exception {} (type={}) for calendar {} on date {}", ex.name, calendarDate.exception_type, cal.service_id, date); if (service.calendar_dates.containsKey(date)) throw new IllegalArgumentException("Duplicate schedule exceptions on " + date); From 49a722e161a52cae98fba9cb23995dae82fd01e4 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Tue, 8 Aug 2023 14:20:35 +0100 Subject: [PATCH 15/19] refactor(Addressed PR feedback): Refactored inline with feedback --- .../conveyal/gtfs/loader/JDBCTableReader.java | 13 +++ .../gtfs/loader/JdbcGtfsExporter.java | 15 ++- .../gtfs/loader/JdbcGtfsSnapshotter.java | 102 ++++++++++-------- .../conveyal/gtfs/loader/JdbcTableWriter.java | 1 - .../gtfs/loader/ReferenceTracker.java | 27 ++--- 5 files changed, 94 insertions(+), 64 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JDBCTableReader.java b/src/main/java/com/conveyal/gtfs/loader/JDBCTableReader.java index 6f4fa07c2..0eaa80331 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JDBCTableReader.java +++ b/src/main/java/com/conveyal/gtfs/loader/JDBCTableReader.java @@ -1,5 +1,6 @@ package com.conveyal.gtfs.loader; +import com.conveyal.gtfs.model.Calendar; import com.conveyal.gtfs.model.Entity; import com.conveyal.gtfs.storage.StorageException; import gnu.trove.map.TObjectIntMap; @@ -146,6 +147,18 @@ public int getRowCount() { } } + /** + * Provide reader for calendar table. + */ + public static JDBCTableReader getCalendarTableReader(DataSource dataSource, String tablePrefix) { + return new JDBCTableReader( + Table.CALENDAR, + dataSource, + tablePrefix + ".", + EntityPopulator.CALENDAR + ); + } + private class EntityIterator implements Iterator { private Connection connection; // Will remain open for the duration of the iteration. diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index b517472e1..2688a5e7c 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -130,12 +130,10 @@ public FeedLoadResult exportTables() { JDBCTableReader exceptionsReader = new JDBCTableReader(Table.SCHEDULE_EXCEPTIONS, dataSource, feedIdToExport + ".", EntityPopulator.SCHEDULE_EXCEPTION); - JDBCTableReader calendarsReader = - new JDBCTableReader(Table.CALENDAR, dataSource, feedIdToExport + ".", - EntityPopulator.CALENDAR); - Iterable calendars = calendarsReader.getAll(); + JDBCTableReader calendarReader = JDBCTableReader.getCalendarTableReader(dataSource, feedIdToExport); + Iterable calendars = calendarReader.getAll(); Set calendarServiceIds = new HashSet<>(); - for (Calendar calendar : calendarsReader.getAll()) { + for (Calendar calendar : calendarReader.getAll()) { calendarServiceIds.add(calendar.service_id); } Iterable exceptionsIterator = exceptionsReader.getAll(); @@ -150,7 +148,7 @@ public FeedLoadResult exportTables() { } } - int calendarDateCount = 0; + int calendarDateCount = calendarDateExceptions.size(); // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one // date which will be exported as a service on the specified date. for (ScheduleException ex : calendarDateExceptions) { @@ -165,11 +163,10 @@ public FeedLoadResult exportTables() { // If the calendar dates provided contain duplicates (e.g. two or more identical service ids that are // NOT associated with a calendar) only the first entry will persist export. feed.services.put(calendarDate.service_id, service); - calendarDateCount++; } // check whether the feed is organized in a format with the calendars.txt file - if (calendarsReader.getRowCount() > 0) { + if (calendarReader.getRowCount() > 0) { // feed does have calendars.txt file, continue export with strategy of matching exceptions // to calendar to output calendar_dates.txt for (Calendar cal : calendars) { @@ -208,7 +205,7 @@ public FeedLoadResult exportTables() { new CalendarDate.Writer(feed).writeTable(zipOutputStream); } - if (calendarsReader.getRowCount() == 0 && calendarDateExceptions.isEmpty()) { + if (calendarReader.getRowCount() == 0 && calendarDateExceptions.isEmpty()) { // No calendar or calendar date service records exist, export calendar_dates as is and hope for the best. // This situation will occur in at least 2 scenarios: // 1. A GTFS has been loaded into the editor that had only the calendar_dates.txt file diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 5468af7e8..982d81263 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -214,12 +214,7 @@ private TableLoadResult createScheduleExceptionsTable() { // Fetch all entries in the calendar table to generate set of serviceIds that exist in the calendar // table. - JDBCTableReader calendarReader = new JDBCTableReader( - Table.CALENDAR, - dataSource, - feedIdToSnapshot + ".", - EntityPopulator.CALENDAR - ); + JDBCTableReader calendarReader = JDBCTableReader.getCalendarTableReader(dataSource, feedIdToSnapshot); Set calendarServiceIds = new HashSet<>(); for (Calendar calendar : calendarReader.getAll()) { calendarServiceIds.add(calendar.service_id); @@ -251,17 +246,7 @@ private TableLoadResult createScheduleExceptionsTable() { // Calendar date is related to a calendar. if (calendarDate.exception_type == 1) { addedServiceForDate.put(date, calendarDate.service_id); - // create (if needed) and extend range of dummy calendar that would need to be created if we are - // copying from a feed that doesn't have the calendar.txt file - Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); - calendar.service_id = calendarDate.service_id; - if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { - calendar.start_date = calendarDate.date; - } - if (calendar.end_date == null || calendar.end_date.isBefore(calendarDate.date)) { - calendar.end_date = calendarDate.date; - } - dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); + extendDummyCalendarRange(dummyCalendarsByServiceId, calendarDate); } else { removedServiceForDate.put(date, calendarDate.service_id); } @@ -285,35 +270,26 @@ private TableLoadResult createScheduleExceptionsTable() { // For usability and simplicity of code, don't attempt to find all dates with similar // added and removed services, but simply create an entry for each found date. for (String date : Sets.union(removedServiceForDate.keySet(), addedServiceForDate.keySet())) { - scheduleExceptionsStatement.setString(1, date); - String[] dates = {date}; - scheduleExceptionsStatement.setArray(2, connection.createArrayOf("text", dates)); - scheduleExceptionsStatement.setInt(3, ScheduleException.ExemplarServiceDescriptor.SWAP.getValue()); - scheduleExceptionsStatement.setArray( - 4, - connection.createArrayOf("text", addedServiceForDate.get(date).toArray()) - ); - scheduleExceptionsStatement.setArray( - 5, - connection.createArrayOf("text", removedServiceForDate.get(date).toArray()) + createScheduledExceptionStatement( + scheduleExceptionsStatement, + scheduleExceptionsTracker, + date, + new String[] {date}, + ScheduleException.ExemplarServiceDescriptor.SWAP, + addedServiceForDate.get(date).toArray(), + removedServiceForDate.get(date).toArray() ); - scheduleExceptionsTracker.addBatch(); } for (Map.Entry entry : calendarDateService.entrySet()) { - String serviceId = entry.getValue(); - scheduleExceptionsStatement.setString(1, serviceId); - String[] dates = {entry.getKey()}; - scheduleExceptionsStatement.setArray(2, connection.createArrayOf("text", dates)); - scheduleExceptionsStatement.setInt(3, ScheduleException.ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE.getValue()); - scheduleExceptionsStatement.setArray( - 4, - connection.createArrayOf("text", new String[] {}) - ); - scheduleExceptionsStatement.setArray( - 5, - connection.createArrayOf("text", new String[] {}) + createScheduledExceptionStatement( + scheduleExceptionsStatement, + scheduleExceptionsTracker, + entry.getValue(), + new String[] {entry.getKey()}, + ScheduleException.ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE, + new String[] {}, + new String[] {} ); - scheduleExceptionsTracker.addBatch(); } scheduleExceptionsTracker.executeRemaining(); @@ -368,6 +344,48 @@ private TableLoadResult createScheduleExceptionsTable() { } } + /** + * Create (if needed) and extend range of dummy calendars that would need to be created if we are copying from a + * feed that doesn't have the calendar.txt file. + */ + private void extendDummyCalendarRange(Map dummyCalendarsByServiceId, CalendarDate calendarDate) { + Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); + calendar.service_id = calendarDate.service_id; + if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { + calendar.start_date = calendarDate.date; + } + if (calendar.end_date == null || calendar.end_date.isBefore(calendarDate.date)) { + calendar.end_date = calendarDate.date; + } + dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); + } + + /** + * Populate schedule exception statement and add to batch tracker. + */ + private void createScheduledExceptionStatement( + PreparedStatement scheduleExceptionsStatement, + BatchTracker scheduleExceptionsTracker, + String name, + String[] dates, + ScheduleException.ExemplarServiceDescriptor exemplarServiceDescriptor, + Object[] addedServicesForDate, + Object[] removedServicesForDate + ) throws SQLException { + scheduleExceptionsStatement.setString(1, name); + scheduleExceptionsStatement.setArray(2, connection.createArrayOf("text", dates)); + scheduleExceptionsStatement.setInt(3, exemplarServiceDescriptor.getValue()); + scheduleExceptionsStatement.setArray( + 4, + connection.createArrayOf("text", addedServicesForDate) + ); + scheduleExceptionsStatement.setArray( + 5, + connection.createArrayOf("text", removedServicesForDate) + ); + scheduleExceptionsTracker.addBatch(); + } + /** * Helper method to determine if a table exists within a namespace. */ diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index ace4d025a..eac393091 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -1479,7 +1479,6 @@ private static Set
getReferencingTables(Table table) { return referencingTables; } - /** * For a given integer ID, return the value for the specified field name for that entity. */ diff --git a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java index cbc69cafb..32b3d57c7 100644 --- a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java +++ b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java @@ -82,19 +82,8 @@ public Set checkReferencesAndUniqueness(String keyValue, int lineN // First, handle referential integrity check. boolean isOrderField = field.name.equals(orderField); if (field.isForeignReference()) { - // Check foreign references. If the foreign reference is present in one of the tables, there is no - // need to check the remainder. If no matching foreign reference is found, flag integrity error. - // Note: The reference table must be loaded before the table/value being currently checked. - boolean hasMatchingReference = false; TreeSet badValues = new TreeSet<>(); - for (Table referenceTable : field.referenceTables) { - String referenceField = referenceTable.getKeyFieldName(); - if (checkReference(referenceField, value, badValues)) { - hasMatchingReference = true; - break; - } - } - if (!hasMatchingReference) { + if (!hasMatchingReference(field, value, badValues)) { // If the reference tracker does not contain a match. NewGTFSErrorType errorType = (field.referenceTables.size() > 1) ? MISSING_FOREIGN_TABLE_REFERENCE @@ -166,6 +155,20 @@ public Set checkReferencesAndUniqueness(String keyValue, int lineN return errors; } + /** + * Check foreign references. If the foreign reference is present in one of the tables, there is no + * need to check the remainder. If no matching foreign reference is found, flag integrity error. + * Note: The reference table must be loaded before the table/value being currently checked. + */ + private boolean hasMatchingReference(Field field, String value, TreeSet badValues) { + for (Table referenceTable : field.referenceTables) { + if (checkReference(referenceTable.getKeyFieldName(), value, badValues)) { + return true; + } + } + return false; + } + /** * Check that a reference is valid. */ From b00499a44bb59ec4607dfc062ce548cb98279ea1 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Thu, 24 Aug 2023 08:19:51 +0100 Subject: [PATCH 16/19] Update src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java Co-authored-by: Philip Cline <63798641+philip-cline@users.noreply.github.com> --- src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 2688a5e7c..98d6096da 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -149,7 +149,7 @@ public FeedLoadResult exportTables() { } int calendarDateCount = calendarDateExceptions.size(); - // Extract calendar date services, convert to calendar date and add to the feed. We are expect only one + // Extract calendar date services, convert to calendar date and add to the feed. We are expecting only one // date which will be exported as a service on the specified date. for (ScheduleException ex : calendarDateExceptions) { // Only ever expecting one date here. From 1b6cc2f8745c79ec3e739371cca8d5a183f52193 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Tue, 29 Aug 2023 14:13:53 +0100 Subject: [PATCH 17/19] refactor(To allow for unique schedule exception names): Combine schedule exceptions for a single ser --- .../gtfs/loader/JdbcGtfsExporter.java | 44 +++++++++---------- .../gtfs/loader/JdbcGtfsSnapshotter.java | 34 ++++++++++---- .../com/conveyal/gtfs/model/Calendar.java | 7 +-- src/test/java/com/conveyal/gtfs/GTFSTest.java | 23 +++++++++- .../calendar_dates.txt | 4 +- 5 files changed, 74 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java index 98d6096da..9276cfb89 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsExporter.java @@ -23,6 +23,7 @@ import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.sql.Connection; import java.sql.SQLException; import java.time.LocalDate; @@ -93,7 +94,7 @@ public FeedLoadResult exportTables() { FeedLoadResult result = new FeedLoadResult(); try { - zipOutputStream = new ZipOutputStream(new FileOutputStream(outFile)); + zipOutputStream = new ZipOutputStream(Files.newOutputStream(Paths.get(outFile))); long startTime = System.currentTimeMillis(); // We get a single connection object and share it across several different methods. // This ensures that actions taken in one method are visible to all subsequent SQL statements. @@ -126,16 +127,14 @@ public FeedLoadResult exportTables() { if (fromEditor) { // Export schedule exceptions in place of calendar dates if exporting a feed/schema that represents an editor snapshot. GTFSFeed feed = new GTFSFeed(); - // FIXME: The below table readers should probably just share a connection with the exporter. - JDBCTableReader exceptionsReader = - new JDBCTableReader(Table.SCHEDULE_EXCEPTIONS, dataSource, feedIdToExport + ".", - EntityPopulator.SCHEDULE_EXCEPTION); + JDBCTableReader exceptionsReader =new JDBCTableReader( + Table.SCHEDULE_EXCEPTIONS, + dataSource, + feedIdToExport + ".", + EntityPopulator.SCHEDULE_EXCEPTION + ); JDBCTableReader calendarReader = JDBCTableReader.getCalendarTableReader(dataSource, feedIdToExport); Iterable calendars = calendarReader.getAll(); - Set calendarServiceIds = new HashSet<>(); - for (Calendar calendar : calendarReader.getAll()) { - calendarServiceIds.add(calendar.service_id); - } Iterable exceptionsIterator = exceptionsReader.getAll(); List calendarExceptions = new ArrayList<>(); List calendarDateExceptions = new ArrayList<>(); @@ -149,20 +148,21 @@ public FeedLoadResult exportTables() { } int calendarDateCount = calendarDateExceptions.size(); - // Extract calendar date services, convert to calendar date and add to the feed. We are expecting only one - // date which will be exported as a service on the specified date. + // Extract calendar date services, convert to calendar date and add to the feed. for (ScheduleException ex : calendarDateExceptions) { - // Only ever expecting one date here. - LocalDate date = ex.dates.get(0); - CalendarDate calendarDate = new CalendarDate(); - calendarDate.date = date; - calendarDate.service_id = ex.name; - calendarDate.exception_type = 1; - Service service = new Service(calendarDate.service_id); - service.calendar_dates.put(date, calendarDate); - // If the calendar dates provided contain duplicates (e.g. two or more identical service ids that are - // NOT associated with a calendar) only the first entry will persist export. - feed.services.put(calendarDate.service_id, service); + for (LocalDate date : ex.dates) { + String serviceId = ex.customSchedule.get(0); + CalendarDate calendarDate = new CalendarDate(); + calendarDate.date = date; + calendarDate.service_id = serviceId; + calendarDate.exception_type = 1; + Service service = new Service(serviceId); + service.calendar_dates.put(date, calendarDate); + // If the calendar dates provided contain duplicates (e.g. two or more identical service ids + // that are NOT associated with a calendar) only the first entry would persist export. To + // resolve this a unique key consisting of service id and date is used. + feed.services.put(String.format("%s-%s", calendarDate.service_id, calendarDate.date), service); + } } // check whether the feed is organized in a format with the calendars.txt file diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 982d81263..a4e05ecf4 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -234,7 +234,7 @@ private TableLoadResult createScheduleExceptionsTable() { // Iterate through calendar dates to build up appropriate service dates. Multimap removedServiceForDate = HashMultimap.create(); Multimap addedServiceForDate = HashMultimap.create(); - HashMap calendarDateService = new HashMap<>(); + HashMap> calendarDateService = new HashMap<>(); for (CalendarDate calendarDate : calendarDates) { // Skip any null dates or service ids. if (calendarDate.date == null || calendarDate.service_id == null) { @@ -251,13 +251,20 @@ private TableLoadResult createScheduleExceptionsTable() { removedServiceForDate.put(date, calendarDate.service_id); } } else { - // Calendar date is unique. - calendarDateService.put(date, calendarDate.service_id); + // Calendar date is not related to a calendar. Group calendar dates by service id. + if (calendarDateService.containsKey(calendarDate.service_id)) { + calendarDateService.get(calendarDate.service_id).add(date); + } else { + Set dates = new HashSet<>(); + dates.add(date); + calendarDateService.put(calendarDate.service_id, dates); + } + } } String sql = String.format( - "insert into %s (name, dates, exemplar, added_service, removed_service) values (?, ?, ?, ?, ?)", + "insert into %s (name, dates, exemplar, custom_schedule, added_service, removed_service) values (?, ?, ?, ?, ?, ?)", scheduleExceptionsTableName ); PreparedStatement scheduleExceptionsStatement = connection.prepareStatement(sql); @@ -276,17 +283,23 @@ private TableLoadResult createScheduleExceptionsTable() { date, new String[] {date}, ScheduleException.ExemplarServiceDescriptor.SWAP, + new String[] {}, addedServiceForDate.get(date).toArray(), removedServiceForDate.get(date).toArray() ); } - for (Map.Entry entry : calendarDateService.entrySet()) { + + for (Map.Entry> entry : calendarDateService.entrySet()) { + String serviceId = entry.getKey(); + String[] dates = entry.getValue().toArray(new String[0]); createScheduledExceptionStatement( scheduleExceptionsStatement, scheduleExceptionsTracker, - entry.getValue(), - new String[] {entry.getKey()}, + // Unique-ish schedule name that shouldn't conflict with existing service ids. + String.format("%s-%s", serviceId, dates[0]), + dates, ScheduleException.ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE, + new String[] {serviceId}, new String[] {}, new String[] {} ); @@ -369,6 +382,7 @@ private void createScheduledExceptionStatement( String name, String[] dates, ScheduleException.ExemplarServiceDescriptor exemplarServiceDescriptor, + Object[] customSchedule, Object[] addedServicesForDate, Object[] removedServicesForDate ) throws SQLException { @@ -377,10 +391,14 @@ private void createScheduledExceptionStatement( scheduleExceptionsStatement.setInt(3, exemplarServiceDescriptor.getValue()); scheduleExceptionsStatement.setArray( 4, - connection.createArrayOf("text", addedServicesForDate) + connection.createArrayOf("text", customSchedule) ); scheduleExceptionsStatement.setArray( 5, + connection.createArrayOf("text", addedServicesForDate) + ); + scheduleExceptionsStatement.setArray( + 6, connection.createArrayOf("text", removedServicesForDate) ); scheduleExceptionsTracker.addBatch(); diff --git a/src/main/java/com/conveyal/gtfs/model/Calendar.java b/src/main/java/com/conveyal/gtfs/model/Calendar.java index 22c6029b9..097ca0eaf 100644 --- a/src/main/java/com/conveyal/gtfs/model/Calendar.java +++ b/src/main/java/com/conveyal/gtfs/model/Calendar.java @@ -143,12 +143,7 @@ protected void writeOneRow(Calendar c) throws IOException { @Override protected Iterator iterator() { // wrap an iterator over services - Iterator calIt = Iterators.transform(feed.services.values().iterator(), new Function () { - @Override - public Calendar apply (Service s) { - return s.calendar; - } - }); + Iterator calIt = Iterators.transform(feed.services.values().iterator(), s -> s.calendar); // not every service has a calendar (e.g. TriMet has no calendars, just calendar dates). // This is legal GTFS, so skip services with no calendar diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 3754b8d73..9f97029ca 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -451,6 +451,26 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new RecordExpectation("exception_type", 1) } ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "calendar-dates-txt-service-three" + ), + new RecordExpectation("date", 20170917), + new RecordExpectation("exception_type", 1) + } + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "calendar-dates-txt-service-three" + ), + new RecordExpectation("date", 20170918), + new RecordExpectation("exception_type", 1) + } + ), new PersistenceExpectation( "stop_times", new RecordExpectation[]{ @@ -528,7 +548,8 @@ public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), + new ErrorExpectation(NewGTFSErrorType.SERVICE_UNUSED) ); assertThat( runIntegrationTestOnFolder( diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt index fc4825b41..6e4f013d1 100755 --- a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt @@ -2,4 +2,6 @@ service_id,date,exception_type in-both-calendar-txt-and-calendar-dates,20170920,2 only-in-calendar-dates-txt,20170916,1 calendar-dates-txt-service-one,20170917,1 -calendar-dates-txt-service-two,20170918,1 \ No newline at end of file +calendar-dates-txt-service-two,20170918,1 +calendar-dates-txt-service-three,20170917,1 +calendar-dates-txt-service-three,20170918,1 \ No newline at end of file From 3ec26e653d7bf2eb28b23953fb3ce398329de834 Mon Sep 17 00:00:00 2001 From: "philip.cline" Date: Fri, 10 Nov 2023 16:39:23 -0500 Subject: [PATCH 18/19] refactor(TableWriter): allow update of trips service_id based on exception based service --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 137 ++++++++++++------ .../java/com/conveyal/gtfs/loader/Table.java | 2 +- 2 files changed, 94 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index f5edde63b..24820e137 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -2,6 +2,7 @@ import com.conveyal.gtfs.model.Entity; import com.conveyal.gtfs.model.PatternStop; +import com.conveyal.gtfs.model.ScheduleException.ExemplarServiceDescriptor; import com.conveyal.gtfs.model.Shape; import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.storage.StorageException; @@ -1328,6 +1329,58 @@ private static long handleStatementExecution(PreparedStatement statement, boolea } } + private void checkUniqueIdsAndUpdateReferencingTables( + TIntSet uniqueIds, + Integer id, + String namespace, + Table table, + String keyValue, + Boolean isCreating, + Field keyField + ) throws SQLException { + int size = uniqueIds.size(); + if (size == 0 || (size == 1 && id != null && uniqueIds.contains(id))) { + // OK. + if (size == 0 && !isCreating) { + // FIXME: Need to update referencing tables because entity has changed ID. + // Entity key value is being changed to an entirely new one. If there are entities that + // reference this value, we need to update them. + updateReferencingTables(namespace, table, id, keyValue, keyField); + } + } else { + // Conflict. The different conflict conditions are outlined below. + if (size == 1) { + // There was one match found. + if (isCreating) { + // Under no circumstance should a new entity have a conflict with existing key field. + throw new SQLException( + String.format("New %s's %s value (%s) conflicts with an existing record in table.", + table.entityClass.getSimpleName(), + keyField.name, + keyValue) + ); + } + if (!uniqueIds.contains(id)) { + // There are two circumstances we could encounter here. + // 1. The key value for this entity has been updated to match some other entity's key value (conflict). + // 2. The int ID provided in the request parameter does not match any rows in the table. + throw new SQLException("Key field must be unique and request parameter ID must exist."); + } + } else if (size > 1) { + // FIXME: Handle edge case where original data set contains duplicate values for key field and this is an + // attempt to rectify bad data. + String message = String.format( + "%d %s entities shares the same key field (%s=%s)! Key field must be unique.", + size, + table.name, + keyField.name, + keyValue); + LOG.error(message); + throw new SQLException(message); + } + } + } + /** * Checks for modification of GTFS key field (e.g., stop_id, route_id) in supplied JSON object and ensures * both uniqueness and that referencing tables are appropriately updated. @@ -1369,46 +1422,15 @@ private void ensureReferentialIntegrity( String keyValue = jsonObject.get(keyField).asText(); // If updating key field, check that there is no ID conflict on value (e.g., stop_id or route_id) TIntSet uniqueIds = getIdsForCondition(tableName, keyField, keyValue, connection); - int size = uniqueIds.size(); - if (size == 0 || (size == 1 && id != null && uniqueIds.contains(id))) { - // OK. - if (size == 0 && !isCreating) { - // FIXME: Need to update referencing tables because entity has changed ID. - // Entity key value is being changed to an entirely new one. If there are entities that - // reference this value, we need to update them. - updateReferencingTables(namespace, table, id, keyValue); - } - } else { - // Conflict. The different conflict conditions are outlined below. - if (size == 1) { - // There was one match found. - if (isCreating) { - // Under no circumstance should a new entity have a conflict with existing key field. - throw new SQLException( - String.format("New %s's %s value (%s) conflicts with an existing record in table.", - table.entityClass.getSimpleName(), - keyField, - keyValue) - ); - } - if (!uniqueIds.contains(id)) { - // There are two circumstances we could encounter here. - // 1. The key value for this entity has been updated to match some other entity's key value (conflict). - // 2. The int ID provided in the request parameter does not match any rows in the table. - throw new SQLException("Key field must be unique and request parameter ID must exist."); - } - } else if (size > 1) { - // FIXME: Handle edge case where original data set contains duplicate values for key field and this is an - // attempt to rectify bad data. - String message = String.format( - "%d %s entities shares the same key field (%s=%s)! Key field must be unique.", - size, - table.name, - keyField, - keyValue); - LOG.error(message); - throw new SQLException(message); - } + checkUniqueIdsAndUpdateReferencingTables(uniqueIds, id, namespace, table, keyValue, isCreating, table.getFieldForName(table.getKeyFieldName())); + + // Special case for schedule_exceptions where for exception type 10, service_id is also a key + if (table.name.equals("schedule_exceptions") && jsonObject.has("exemplar") && jsonObject.get("exemplar").asInt() == ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE.getValue()) { + String calendarDateServiceKey = "custom_schedule"; + Field calendarDateServiceKeyField = table.getFieldForName(calendarDateServiceKey); + String calendarDateServiceKeyVal = jsonObject.get(calendarDateServiceKey).asText(); + TIntSet calendarDateServiceUniqueIds = getIdsForCondition (tableName, calendarDateServiceKey, calendarDateServiceKeyVal, connection); + checkUniqueIdsAndUpdateReferencingTables(calendarDateServiceUniqueIds, id, namespace, table, calendarDateServiceKeyVal, isCreating, calendarDateServiceKeyField); } } @@ -1434,7 +1456,13 @@ private static TIntSet getIdsForCondition( String keyValue, Connection connection ) throws SQLException { - String idCheckSql = String.format("select id from %s where %s = ?", tableName, keyField); + String idCheckSql = ""; + // The custom_schedule field of an exception based service contains an array and requires an "any" query + if (keyField == "custom_schedule") { + idCheckSql = String.format("select id from %s where ? = any (%s)", tableName, keyField); + } else { + idCheckSql = String.format("select id from %s where %s = ?", tableName, keyField); + } // Create statement for counting rows selected PreparedStatement statement = connection.prepareStatement(idCheckSql); statement.setString(1, keyValue); @@ -1563,16 +1591,18 @@ private void updateReferencingTables( String namespace, Table table, int id, - String newKeyValue + String newKeyValue, + Field keyField ) throws SQLException { - Field keyField = table.getFieldForName(table.getKeyFieldName()); Class entityClass = table.getEntityClass(); // Determine method (update vs. delete) depending on presence of newKeyValue field. SqlMethod sqlMethod = newKeyValue != null ? SqlMethod.UPDATE : SqlMethod.DELETE; Set
referencingTables = getReferencingTables(table); // If there are no referencing tables, there is no need to update any values (e.g., . if (referencingTables.size() == 0) return; - String keyValue = getValueForId(id, keyField.name, namespace, table, connection); + // Exception based service contains a single service ID in custom_schedule + String sqlKeyFieldName = keyField.name == "custom_schedule" ? "custom_schedule[1]" : keyField.name; + String keyValue = getValueForId(id, sqlKeyFieldName, namespace, table, connection); if (keyValue == null) { // FIXME: should we still check referencing tables for null value? LOG.warn("Entity {} to {} has null value for {}. Skipping references check.", id, sqlMethod, keyField); @@ -1677,6 +1707,25 @@ private void updateReferencingTables( } } + /** + * Traditional method signature for updateReferencingTables, updating exception based service requires + * passing the keyField. + * @param namespace + * @param table + * @param id + * @param newKeyValue + * @throws SQLException + */ + private void updateReferencingTables( + String namespace, + Table table, + int id, + String newKeyValue + ) throws SQLException { + Field keyField = table.getFieldForName(table.getKeyFieldName()); + updateReferencingTables(namespace, table, id, newKeyValue, keyField); + } + /** * To prevent orphaned descendants, delete them before joining references are deleted. For the relationship * route -> pattern -> pattern stop, delete pattern stop before deleting the joining pattern. diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index c70653e77..2ec188339 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -340,7 +340,7 @@ public Table (String name, Class entityClass, Requirement requ new StringField("trip_id", REQUIRED), new StringField("route_id", REQUIRED).isReferenceTo(ROUTES).indexThisColumn(), // FIXME: Do we need an index on service_id - new StringField("service_id", REQUIRED).isReferenceTo(CALENDAR).isReferenceTo(CALENDAR_DATES), + new StringField("service_id", REQUIRED).isReferenceTo(CALENDAR).isReferenceTo(CALENDAR_DATES).isReferenceTo(SCHEDULE_EXCEPTIONS), new StringField("trip_headsign", OPTIONAL), new StringField("trip_short_name", OPTIONAL), new ShortField("direction_id", OPTIONAL, 1), From 3a2417a420cab5ba796b994f699d1742790185f4 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Tue, 14 Nov 2023 13:51:13 +0000 Subject: [PATCH 19/19] refactor(JdbcTableWriter.java): Formatting updates --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 24820e137..8484e80c3 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -1422,15 +1422,34 @@ private void ensureReferentialIntegrity( String keyValue = jsonObject.get(keyField).asText(); // If updating key field, check that there is no ID conflict on value (e.g., stop_id or route_id) TIntSet uniqueIds = getIdsForCondition(tableName, keyField, keyValue, connection); - checkUniqueIdsAndUpdateReferencingTables(uniqueIds, id, namespace, table, keyValue, isCreating, table.getFieldForName(table.getKeyFieldName())); + checkUniqueIdsAndUpdateReferencingTables( + uniqueIds, + id, + namespace, + table, + keyValue, + isCreating, + table.getFieldForName(table.getKeyFieldName()) + ); - // Special case for schedule_exceptions where for exception type 10, service_id is also a key - if (table.name.equals("schedule_exceptions") && jsonObject.has("exemplar") && jsonObject.get("exemplar").asInt() == ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE.getValue()) { + if (table.name.equals("schedule_exceptions") && + jsonObject.has("exemplar") && + jsonObject.get("exemplar").asInt() == ExemplarServiceDescriptor.CALENDAR_DATE_SERVICE.getValue() + ) { + // Special case for schedule_exceptions where for exception type 10 and service_id is also a key. String calendarDateServiceKey = "custom_schedule"; Field calendarDateServiceKeyField = table.getFieldForName(calendarDateServiceKey); String calendarDateServiceKeyVal = jsonObject.get(calendarDateServiceKey).asText(); - TIntSet calendarDateServiceUniqueIds = getIdsForCondition (tableName, calendarDateServiceKey, calendarDateServiceKeyVal, connection); - checkUniqueIdsAndUpdateReferencingTables(calendarDateServiceUniqueIds, id, namespace, table, calendarDateServiceKeyVal, isCreating, calendarDateServiceKeyField); + TIntSet calendarDateServiceUniqueIds = getIdsForCondition(tableName, calendarDateServiceKey, calendarDateServiceKeyVal, connection); + checkUniqueIdsAndUpdateReferencingTables( + calendarDateServiceUniqueIds, + id, + namespace, + table, + calendarDateServiceKeyVal, + isCreating, + calendarDateServiceKeyField + ); } } @@ -1456,9 +1475,9 @@ private static TIntSet getIdsForCondition( String keyValue, Connection connection ) throws SQLException { - String idCheckSql = ""; - // The custom_schedule field of an exception based service contains an array and requires an "any" query - if (keyField == "custom_schedule") { + String idCheckSql; + if (keyField.equals("custom_schedule")) { + // The custom_schedule field of an exception based service contains an array and requires an "any" query. idCheckSql = String.format("select id from %s where ? = any (%s)", tableName, keyField); } else { idCheckSql = String.format("select id from %s where %s = ?", tableName, keyField);