Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shared Stops Validator: Validate feed_id #557

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

miles-grant-ibigroup
Copy link
Contributor

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

Builds on #552, only validating the shared stop if the feed_id of the feed matches the feed id of the row being validated

@miles-grant-ibigroup miles-grant-ibigroup added the WIP Work in progress label Aug 24, 2023
@miles-grant-ibigroup miles-grant-ibigroup self-assigned this Aug 24, 2023
@miles-grant-ibigroup miles-grant-ibigroup added BLOCKED Awaiting GTFS-lib release Merge into dev once GTFS-lib has been released. labels Aug 24, 2023
@miles-grant-ibigroup miles-grant-ibigroup removed WIP Work in progress BLOCKED labels Sep 6, 2023
@miles-grant-ibigroup miles-grant-ibigroup marked this pull request as ready for review September 6, 2023 17:37
Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see an alternative to using JDBCFetcher (this is the first use of it in DT). I guess the validation cannot be done in GTFS lib because you need the project's shared stop config and this transcends all feed versions so it can't be included there?

@br648 br648 assigned miles-grant-ibigroup and unassigned br648 Sep 7, 2023
@miles-grant-ibigroup
Copy link
Contributor Author

I don't really see an alternative to using JDBCFetcher (this is the first use of it in DT). I guess the validation cannot be done in GTFS lib because you need the project's shared stop config and this transcends all feed versions so it can't be included there?

Yeah we'd do all the validation in gtfs-lib but we don't have access to the feed info table in there

@br648
Copy link
Contributor

br648 commented Sep 11, 2023

I don't really see an alternative to using JDBCFetcher (this is the first use of it in DT). I guess the validation cannot be done in GTFS lib because you need the project's shared stop config and this transcends all feed versions so it can't be included there?

Yeah we'd do all the validation in gtfs-lib but we don't have access to the feed info table in there

Feed Info is loaded by GTFS-Lib it's just not made available in Feed. This could be updated to include feed info. But if I understand this correctly, GTFS Lib doesn't have the shard stop config so you'd still have a similar issue?

@miles-grant-ibigroup miles-grant-ibigroup removed the Awaiting GTFS-lib release Merge into dev once GTFS-lib has been released. label Sep 11, 2023
Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve after discussing with Miles. This approach keeps GTFS-Lib clean.

Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. A nice-to-have would be to report the line number of the non-existing stop entity found.

// TODO: CHECK FEED ID (adjust the pre-build constructor to include feed_id)
if (!stopIds.contains(stopId)) {
// Make sure this error is only returned if we are inside the feed that is being checked
if (feedId.equals(sharedStopFeedId) && !stopIds.contains(stopId)) {
registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_ENTITY_DOES_NOT_EXIST, stopId));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have this error be reported for the current line (instead of the entire feed) of the shared stops file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared stops file is not in Postgres so the validator can't talk to it. the stopId links it to the stops file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of line numbers because the shared stops file is processed line by line using configReader.readRecord().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but unfortunately the validator doesn't know about those

@miles-grant-ibigroup miles-grant-ibigroup merged commit 0ab79bd into dev Sep 14, 2023
6 checks passed
@miles-grant-ibigroup miles-grant-ibigroup deleted the shared-stops-validator-feed-id branch September 14, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants