MTC Field length validation #276

binh-dam-ibigroup · 2020-04-02T17:40:30Z

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
[na] The description lists all applicable issues this PR seeks to resolve
[na] The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR adds the following (in pursuit of ibi-group/datatools-ui#544):

Ability to specify custom validator classes for method GTFS.validate() and related methods.
A custom validator MTCValidator for custom validation of 511 MTC guidelines on GTFS field length.
An entry in NewGTFSErrorType for field values that are too long.
Unit tests and a database loading/validation test case that uses MTCValidator.

Some modifications regarding passing additional FeedValidator classes are included. BREAKING CHANGE: Change in the type of optional arguments for GTFS.validate and Feed.validate

…th callback approach.

…h Java's BiFunction equivalent.

codecov-io · 2020-04-02T21:14:54Z

Codecov Report

Merging #276 into dev will increase coverage by 0.28%.
The diff coverage is 96.92%.

@@             Coverage Diff              @@
##                dev     #276      +/-   ##
============================================
+ Coverage     64.99%   65.28%   +0.28%     
- Complexity      866      879      +13     
============================================
  Files           134      135       +1     
  Lines          6725     6743      +18     
  Branches        789      794       +5     
============================================
+ Hits           4371     4402      +31     
+ Misses         2055     2040      -15     
- Partials        299      301       +2

Impacted Files	Coverage Δ	Complexity Δ
...conveyal/gtfs/validator/ReversedTripValidator.java	`8.77% <ø> (+0.43%)`	`3.00 <0.00> (ø)`
src/main/java/com/conveyal/gtfs/loader/Feed.java	`74.13% <66.66%> (-1.73%)`	`7.00 <0.00> (+1.00)`	⬇️
...java/com/conveyal/gtfs/validator/MTCValidator.java	`95.00% <95.00%> (ø)`	`10.00 <10.00> (?)`
src/main/java/com/conveyal/gtfs/GTFS.java	`55.55% <100.00%> (ø)`	`12.00 <0.00> (ø)`
...java/com/conveyal/gtfs/error/NewGTFSErrorType.java	`100.00% <100.00%> (ø)`	`2.00 <0.00> (ø)`
src/main/java/com/conveyal/gtfs/model/Agency.java	`71.42% <0.00%> (+2.04%)`	`2.00% <0.00%> (+1.00%)`
...java/com/conveyal/gtfs/loader/EntityPopulator.java	`91.22% <0.00%> (+6.43%)`	`27.00% <0.00%> (+1.00%)`

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 9b4d6e6...12ff728. Read the comment docs.

evansiroky · 2020-04-03T17:56:28Z

Unassigning myself until #277 gets merged.

… method overrides.

evansiroky · 2020-04-03T22:57:58Z

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

@@ -65,44 +58,51 @@ public Feed (DataSource dataSource, String tablePrefix) {
        routes = new JDBCTableReader(Table.ROUTES, dataSource, tablePrefix, EntityPopulator.ROUTE);
        stops = new JDBCTableReader(Table.STOPS, dataSource, tablePrefix, EntityPopulator.STOP);
        trips = new JDBCTableReader(Table.TRIPS, dataSource, tablePrefix, EntityPopulator.TRIP);
-//        shapePoints = new JDBCTableReader(Table.SHAPES, dataSource, tablePrefix, EntityPopulator.SHAPE_POINT);


I realize this has been commented out for a while since @landonreed committed 9244ff0. But why? Should we add it back?

Perhaps we should. It might be worth revisiting #130.

src/main/java/com/conveyal/gtfs/validator/MTCValidator.java

src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java

evansiroky

Would like to have a method renamed as indicated. Also curious what's up with the shape stuff being commented out or deleted.

src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java

src/main/java/com/conveyal/gtfs/validator/MTCValidator.java

…st comments.

…gtfs-lib into field-length-validator

binh-dam-ibigroup · 2020-04-06T16:28:01Z

PR requests addressed, and the tests are passing now... Please review.

evansiroky · 2020-04-06T19:27:19Z

src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java

@@ -11,6 +11,7 @@
    URL_FORMAT(Priority.MEDIUM, "URL format should be <scheme>://<authority><path>?<query>#<fragment>"),
    LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."),
    ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."),
+    // The error type below is an MTC-specific requirement.


Let's organize these into sections instead of having an individual comment for potentially each error type.

Did you have anything in mind for reorganizing?

I'd say one section for errors that apply during all types of validation and another section for MTC-specific errors.

How about this? https://github.com/conveyal/gtfs-lib/pull/276/files#diff-8946752ed48c4dd733a37388eec80ac8

evansiroky

Just one comment about a comment. Good to go otherwise.

evansiroky

Love the alphabetization!

landonreed

Nice work! FYI I'm going to update the PR description to ref the issue in the Data Tools repo.

landonreed · 2020-04-13T19:29:19Z

🎉 This PR is included in version 6.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

landonreed and others added 5 commits April 1, 2020 13:57

refactor(Feed): allow additional validators

b85a922

test(MTC): Set up test files for MTC field length validation.

8b72831

Some modifications regarding passing additional FeedValidator classes are included. BREAKING CHANGE: Change in the type of optional arguments for GTFS.validate and Feed.validate

feat(MTCValidator): Implement MTC validator and complete test.

eeb6b32

docs: Refine comments and error messages.

3f42515

refactor(FieldLengthError): Remove unused class.

4058c9b

binh-dam-ibigroup requested review from landonreed and evansiroky April 2, 2020 17:40

refactor: Replace calling reflection on FeedValidator constructors wi…

d423983

…th callback approach.

binh-dam-ibigroup assigned landonreed and evansiroky Apr 2, 2020

refactor(Feed): Replace callback interface to create MTCValidator wit…

9b94a0e

…h Java's BiFunction equivalent.

landonreed added 4 commits April 2, 2020 17:39

refactor(validate): remove list from custom validator interface

13696e5

refactor(validator): rename validator creator

df84840

refactor(validator): refactor Feed class and add javadoc

d449b12

refactor(validator): fix npe

b6aea3b

landonreed mentioned this pull request Apr 3, 2020

Field length validator ltr 2 #277

Merged

evansiroky removed their assignment Apr 3, 2020

landonreed and others added 3 commits April 3, 2020 14:22

Merge branch 'field-length-validator' into field-length-validator-ltr-2

19db9c2

refactor: fix build/imports

fd249da

refactor(GTFSTest): Use FeedValidatorCreator... syntax to remove test…

28b1dda

… method overrides.

binh-dam-ibigroup assigned evansiroky Apr 3, 2020

evansiroky reviewed Apr 3, 2020

View reviewed changes

src/main/java/com/conveyal/gtfs/validator/MTCValidator.java Outdated Show resolved Hide resolved

evansiroky reviewed Apr 3, 2020

View reviewed changes

src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java Show resolved Hide resolved

evansiroky suggested changes Apr 3, 2020

View reviewed changes

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky Apr 3, 2020

landonreed reviewed Apr 6, 2020

View reviewed changes

src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java Outdated Show resolved Hide resolved

landonreed reviewed Apr 6, 2020

View reviewed changes

src/main/java/com/conveyal/gtfs/validator/MTCValidator.java Outdated Show resolved Hide resolved

landonreed reviewed Apr 6, 2020

View reviewed changes

src/main/java/com/conveyal/gtfs/validator/MTCValidator.java Outdated Show resolved Hide resolved

binh-dam-ibigroup added 4 commits April 6, 2020 10:34

refactor(MTCValidator): Rename validation method + add overload, adju…

707a119

…st comments.

fix(tests): Fix tests

7e97461

refactor(MTCValidator): Rename validateFieldLength; Add null check!

be1fc6d

Merge branch 'field-length-validator' of https://github.com/conveyal/…

ffd601a

…gtfs-lib into field-length-validator

binh-dam-ibigroup requested review from landonreed and evansiroky April 6, 2020 16:28

binh-dam-ibigroup assigned evansiroky Apr 6, 2020

evansiroky reviewed Apr 6, 2020

View reviewed changes

evansiroky approved these changes Apr 6, 2020

View reviewed changes

evansiroky removed their assignment Apr 6, 2020

refactor(NewGTFSErrorType): Reorganize error types per PR comment.

12ff728

binh-dam-ibigroup assigned evansiroky Apr 7, 2020

evansiroky approved these changes Apr 7, 2020

View reviewed changes

evansiroky removed their assignment Apr 7, 2020

landonreed approved these changes Apr 7, 2020

View reviewed changes

binh-dam-ibigroup mentioned this pull request Apr 10, 2020

Add call to MTCValidator ibi-group/datatools-server#297

Merged

2 tasks

landonreed merged commit 8e1de67 into dev Apr 13, 2020

landonreed deleted the field-length-validator branch April 13, 2020 14:35

landonreed mentioned this pull request Apr 13, 2020

Release #278

Merged

7 tasks

landonreed added the released label Apr 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MTC Field length validation #276

MTC Field length validation #276

binh-dam-ibigroup commented Apr 2, 2020 •

edited by landonreed

Loading

codecov-io commented Apr 2, 2020 •

edited

Loading

evansiroky commented Apr 3, 2020

evansiroky Apr 3, 2020

landonreed Apr 6, 2020

evansiroky left a comment •

edited

Loading

binh-dam-ibigroup commented Apr 6, 2020

evansiroky Apr 6, 2020

binh-dam-ibigroup Apr 6, 2020

evansiroky Apr 6, 2020

binh-dam-ibigroup Apr 7, 2020

evansiroky left a comment

evansiroky left a comment

landonreed left a comment •

edited

Loading

landonreed commented Apr 13, 2020

MTC Field length validation #276

MTC Field length validation #276

Conversation

binh-dam-ibigroup commented Apr 2, 2020 • edited by landonreed Loading

Checklist

Description

codecov-io commented Apr 2, 2020 • edited Loading

Codecov Report

evansiroky commented Apr 3, 2020

evansiroky Apr 3, 2020

Choose a reason for hiding this comment

landonreed Apr 6, 2020

Choose a reason for hiding this comment

evansiroky left a comment • edited Loading

Choose a reason for hiding this comment

binh-dam-ibigroup commented Apr 6, 2020

evansiroky Apr 6, 2020

Choose a reason for hiding this comment

binh-dam-ibigroup Apr 6, 2020

Choose a reason for hiding this comment

evansiroky Apr 6, 2020

Choose a reason for hiding this comment

binh-dam-ibigroup Apr 7, 2020

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

landonreed left a comment • edited Loading

Choose a reason for hiding this comment

landonreed commented Apr 13, 2020

binh-dam-ibigroup commented Apr 2, 2020 •

edited by landonreed

Loading

codecov-io commented Apr 2, 2020 •

edited

Loading

evansiroky left a comment •

edited

Loading

landonreed left a comment •

edited

Loading