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

fix: Drop DuplicateFareRuleZoneIdFieldsValidator and replace with multi-column @PrimaryKey for fare_rules.txt #1297

Merged
merged 3 commits into from
Nov 22, 2022

Conversation

bdferris-v2
Copy link
Collaborator

@bdferris-v2 bdferris-v2 commented Nov 21, 2022

Closes #1296.

Per discussion in #1296, DuplicateFareRuleZoneIdFieldsValidator says the combination of route_id, origin_id, destination_id, and contains_id must be unique in fare_rules.txt, when the spec says fare_id should be included (aka the primary key is all fields). This PR drops the custom validator in favor of proper @PrimaryKey annotations for fare_rules.txt.

Per the acceptance tests, there are a number of feeds (~60) that trigger the existingduplicate_fare_rule_zone_id_fields notice. Spot checking a few feeds, many of them are indeed specifying the same combination of fare rules with different fare ids. The corresponding fare attributes often have a different price, probably indicating that the producer is attempting to model some other aspect of their fare system (different fare products? different rider categories?) despite the fact that this is not really supported in Fares v1. While it's maybe not obvious how feed consumers should productively use this data, other than showing the cheapest fare, the spec technically allows it, so the validator should as well.

The acceptance test shows that a number of feeds (~10) newly trigger a duplicate_key notice. These are all feeds that really due have a duplicate combination of all fields in fare_rules.txt. However, each of these feeds were already triggering a duplicate_fare_rule_zone_id_fields notice, so there are no newly-invalidated feeds from this change.

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues

@bdferris-v2 bdferris-v2 marked this pull request as ready for review November 22, 2022 05:04
@github-actions
Copy link
Contributor

❌ Invalid acceptance test.
New Errors: 10 out of 1374 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 66 out of 1374 datasets (~5%) are invalid due to code change, which is above the provided threshold of 1%.
0 out of 1374 sources (~0 %) are corrupted.
Commit: f6f136e
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

@isabelle-dr
Copy link
Contributor

Thanks for working on this @bdferris-v2!
This makes sense.
Following our process for the acceptance test results, I would consider that this change isn't generating additional errors for the reasons you just explained.

Copy link
Contributor

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

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

Can you remove the outdated rule from the documentation, please?
Then we are good to merge this PR :)

@bdferris-v2
Copy link
Collaborator Author

RULES.md has been updated.

Copy link
Contributor

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

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

Thanks! I think we are good to merge without an additional code review here.

@github-actions
Copy link
Contributor

❌ Invalid acceptance test.
New Errors: 10 out of 1372 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 66 out of 1372 datasets (~5%) are invalid due to code change, which is above the provided threshold of 1%.
1 out of 1373 sources (~0 %) are corrupted.
Corrupted sources:
de-unknown-ulmer-eisenbahnfreunde-gtfs-1081
Commit: 6fe8d06
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

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.

False positives for duplicate_fare_rule_zone_id_fields in fare_rules.txt
2 participants