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

tweak: Drop external foreign key constraints during GTFS import #1030

Merged
merged 8 commits into from
Nov 1, 2024

Conversation

jzimbel-mbta
Copy link
Member

@jzimbel-mbta jzimbel-mbta commented Oct 31, 2024

Summary of changes

Asana Ticket: No ticket, followup to GTFS import feature

In order for TRUNCATE all_the_gtfs_tables to be possible, the tables need to have no dependent foreign key constraints originating from outside the set of tables being truncated.

This achieves that by temporarily dropping such constraints, and then re-adding them after the GTFS tables have been repopulated.

Implementation notes

I wanted to make sure any Arrow -> GTFS foreign key constraints added in the future would be handled automatically by this logic.

I felt it would be too cumbersome for developers to have to remember to update code in two different places only when adding foreign keys satisfying these specific conditions.

As a result the code relies on some introspection tools provided by Postgres and is a bit heavy on custom queries, but I think it's still relatively straightforward, and worth it for the ease of use it provides.

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@jzimbel-mbta jzimbel-mbta requested review from a team and Whoops and removed request for a team October 31, 2024 15:20
Copy link
Collaborator

@Whoops Whoops 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 have a problem with the implementation as written, but how hard would it be to write tests for this functionality? If it's not to much effort, I think it'd be worthwhile to have.

@jzimbel-mbta
Copy link
Member Author

I don't have a problem with the implementation as written, but how hard would it be to write tests for this functionality? If it's not to much effort, I think it'd be worthwhile to have.

@Whoops Sounds good, would testing the functions in ForeignKeyConstraint be enough?
Or would you also like to see tests on Arrow.Gtfs?

The latter is a bit more challenging in its current state, since it's hardcoded to act on the GTFS tables specifically, but I should be able to tweak it to be more testable.

@Whoops
Copy link
Collaborator

Whoops commented Oct 31, 2024

I think ForeignKeyConstraint is sufficient. If you did try to cover GTFS I'd suggest doing integration tests that import a feed and check on the state of the world afterward, but that may be more work than it's worth.

@jzimbel-mbta jzimbel-mbta requested a review from Whoops November 1, 2024 13:56
@jzimbel-mbta jzimbel-mbta merged commit ff9581a into master Nov 1, 2024
10 checks passed
@jzimbel-mbta jzimbel-mbta deleted the jz-drop-external-fkeys-during-gtfs-import branch November 1, 2024 17:17
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.

2 participants