GTFS Spec Updates by br648 · Pull Request #312 · conveyal/gtfs-lib

br648 · 2021-04-21T13:09:56Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR seeks to address the changes that have been made to the GTFS spec between Augusut 2018 and present. In particular, this PR is looking to enhance GTFS-lib by including conditionally required field checks which are now part of the GTFS spec. Initial work to determine the required updates can be reviewed here:

https://docs.google.com/document/d/1J_RlOx6wUml_fGhxX6pW1dr1-JKPMInia-73FTr3LJ8/edit

Current Breaking Changes

Routes -> continuous_pickup (New field)
Routes -> continuous_drop_off (New field)
Stop Times -> continuous_pickup (New field)
Stop Times -> continuous_drop_off (New field)
Stop -> platform_code (New field)
Stop -> wheelchair_boarding (Type change from String to Int)
Feed Info -> default_lang (New field)
Feed Info -> feed_contact_email (New field)
Feed Info -> feed_contact_url (New field)

… and test conditionally require

…onally required fields as each

…rence tracker

…from stops, for now, to minimiz

codecov-commenter · 2021-04-21T13:12:14Z

Codecov Report

Merging #312 (5e7f9d8) into dev (7ee0b08) will increase coverage by 0.47%.
The diff coverage is 70.40%.

@@             Coverage Diff              @@
##                dev     #312      +/-   ##
============================================
+ Coverage     66.15%   66.63%   +0.47%     
- Complexity      928      998      +70     
============================================
  Files           136      146      +10     
  Lines          6838     7217     +379     
  Branches        816      850      +34     
============================================
+ Hits           4524     4809     +285     
- Misses         2024     2109      +85     
- Partials        290      299       +9

Impacted Files	Coverage Δ
...java/com/conveyal/gtfs/loader/JdbcTableWriter.java	`60.17% <0.00%> (-0.18%)`	⬇️
...main/java/com/conveyal/gtfs/model/Attribution.java	`0.00% <0.00%> (ø)`
...main/java/com/conveyal/gtfs/model/Translation.java	`0.00% <0.00%> (ø)`
...rc/main/java/com/conveyal/gtfs/model/FeedInfo.java	`59.67% <60.00%> (+0.06%)`	⬆️
src/main/java/com/conveyal/gtfs/model/Stop.java	`70.00% <66.66%> (+4.42%)`	⬆️
...conveyal/gtfs/validator/NewTripTimesValidator.java	`78.48% <70.00%> (+4.56%)`	⬆️
.../java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java	`73.36% <75.00%> (+1.08%)`	⬆️
...rc/main/java/com/conveyal/gtfs/model/StopTime.java	`73.52% <75.00%> (+0.19%)`	⬆️
src/main/java/com/conveyal/gtfs/model/Route.java	`69.04% <80.00%> (+1.48%)`	⬆️
.../loader/conditions/AgencyHasMultipleRowsCheck.java	`81.25% <81.25%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ee0b08...5e7f9d8. Read the comment docs.

landonreed · 2021-04-21T15:29:55Z

.gitignore

 .idea/
 target/
+
+lambda$*.json


What's this for?

I think this is related to running the snapshot related unit tests under Windows. Instead of using:

src\test\resources\snapshots\com\conveyal\gtfs\graphql\GTFSGraphQLTest\canFetchAgencies-0.json

the following is created instead:

src\test\resources\snapshots\com\conveyal\gtfs\graphql\GTFSGraphQLTest\lambda$canFetchAgencies$7-0.json

This is repeated for all 22 snapshot unit tests. I think it is related to how the paths are interpreted in Windows and maybe similar to a fix that was put in place for otp-middleware.

For now, I have added this to git ignore so I don't accidentally commit them. Once I have addressed I will remove.

…tional checks

refactor: re-order stops/fare_rules for ref check

…Tracker and NewTripTimesValidat

…n field referential integrity f

… type.

src/main/java/com/conveyal/gtfs/loader/ConditionalCheckType.java

src/main/java/com/conveyal/gtfs/loader/ConditionalRequirement.java

src/main/java/com/conveyal/gtfs/loader/EntityPopulator.java

src/main/java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java

src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java

evansiroky

Please add tests for fetching GraphQL for the new tables as mentioned.

…attributions and translations

evansiroky · 2021-06-12T01:35:52Z

Assigning to @binh-dam-ibigroup since there have been a few commits since his approval.

binh-dam-ibigroup

LGTM. Please address the comment formatting and typo before merging. (My other code suggestions are not blocking.)

src/main/java/com/conveyal/gtfs/loader/conditions/ConditionalRequirement.java

src/main/java/com/conveyal/gtfs/loader/conditions/FieldInRangeCheck.java

src/main/java/com/conveyal/gtfs/loader/conditions/FieldIsEmptyCheck.java

src/main/java/com/conveyal/gtfs/loader/conditions/FieldInRangeCheck.java

… continuous_drop_off.

binh-dam-ibigroup

Please consider changes from #322 that should address ibi-group/datatools-server#387 (review).

Add stop_times linked fields continuous_pickup, continuous_drop_off.

binh-dam-ibigroup

Should be good now I think

br648 · 2021-08-02T16:24:42Z

@landonreed - TODO: add a BREAKING CHANGE commit.

landonreed · 2021-08-03T13:58:01Z

src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java

+        pattern_stop.drop_off_type = 3;
+        pattern_stop.pickup_type = 4;
+        pattern_stop.continuous_pickup = 5;
+        pattern_stop.continuous_drop_off = 6;


I don't think these invalid values should be allowed to be written to the database through JdbcTableWriter. But perhaps that's an issue for another day.

Rob Beer added 5 commits April 16, 2021 19:23

refactor(Conditionally required field checks): Initial work to define…

50e1146

… and test conditionally require

refactor(Check con req fields on data load): Now checking the conditi…

19ca48e

…onally required fields as each

refactor(ReferenceTracker.java): Addressed NPE

45e6589

refactor(Bug fixes and refactor): Moved conditional checks in to refe…

cd4fe91

…rence tracker

refactor(Updates to allow unit tests to pass): Removed platform_code …

d2899ad

…from stops, for now, to minimiz

refactor: re-order stops/fare_rules for ref check

481353b

landonreed mentioned this pull request Apr 21, 2021

refactor: re-order stops/fare_rules for ref check #313

Merged

landonreed added 2 commits April 21, 2021 10:44

refactor: remove ConditionallyRequiredForeignRefCheck.java

effb4b9

refactor: finish removing ConditionallyRequiredForeignRefCheck

e0881a8

landonreed reviewed Apr 21, 2021

View reviewed changes

landonreed and others added 7 commits April 21, 2021 14:58

refactor: modify conditional check structure

8620c49

refactor(ReferenceTracker.java): Updated logic to carry out all condi…

a8b8e62

…tional checks

Merge pull request #313 from conveyal/gtfs-spec-updates-ltr

79042ea

refactor: re-order stops/fare_rules for ref check

refactor: add (broken) WIP changes for more conditional checks

3f921e3

refactor(Added addtional conditional checks): New checks in Reference…

0b2698a

…Tracker and NewTripTimesValidat

refactor(Added foreign field reference check): Update to check foreig…

88219fa

…n field referential integrity f

refactor(GTFSTest.java): Fixed bug related to dropping DB schema

507d3f1

br648 requested a review from landonreed April 28, 2021 15:16

br648 assigned landonreed, evansiroky and binh-dam-ibigroup and unassigned landonreed Apr 28, 2021

refactor(ReferenceTracker.java): Update to log the correct GTFS error…

ed4e610

… type.