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

Added evolution file to apply foreign key constraints to database #3692

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Jetpackjules
Copy link
Collaborator

@Jetpackjules Jetpackjules commented Oct 14, 2024

Resolves #3574

The issue was that missing foreign key constraints would allow for certain incorrect insertions or deletions (Like null), and "removing data wasn't cascading to all the tables you'd expect." I've added a DB conf evolution that should manually add all the proper foreign key constraints and fix the issue.

BEFORE:

image

AFTER:

image

Testing instructions

To check if the fix worked, check foreign key constraints in the relevant database with:
Run these lines in a separate WSL terminal in the root directory of the project:

docker exec -it projectsidewalk-db bash

psql -U postgres -d sidewalk

SET search_path TO DATABASE_USER_HERE;

SELECT conname AS constraint_name, conrelid::regclass AS table_name, pg_get_constraintdef(oid) AS definition
FROM pg_constraint
WHERE contype = 'f'
AND connamespace = 'sidewalk_walla_walla'::regnamespace;

(The above should output a table listing all foreign key constraints)

Things to check before submitting the PR
  • I've included before/after screenshots above.
  • I've asked for and included translations for any user facing text that was added or modified.
  • I've updated any logging. Clicks, keyboard presses, and other user interactions should be logged. If you're not sure how (or if you need to update the logging), ask Mikey. Then make sure the documentation on this wiki page is up to date for the logs you added/updated.
  • I've tested on mobile (only needed for the validation page).

@Jetpackjules Jetpackjules marked this pull request as ready for review October 15, 2024 00:13
@misaugstad misaugstad self-requested a review October 18, 2024 22:55
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

@Jetpackjules these are small things to clean up, but I'll throw them to you now so that you can change them before I get a chance to fully review/test this!

  1. I don't see the comments as adding anything, really. It's basically as verbose as the SQL itself, and the SQL is pretty self explanatory, so I think that we can just remove these comments.
  2. Let's reduce the length of the file slightly by combining the last two lines of each:
    ALTER TABLE audit_task
        ADD CONSTRAINT fk_audit_task_amt_assignment
            FOREIGN KEY (amt_assignment_id) REFERENCES amt_assignment(amt_assignment_id);
    
  3. And for the Down section, let's just make each of those one line!
  4. For the Downs section, technically they should be in reverse order compared to the Ups... If the Downs are "undoing" the Ups, then we go in reverse order! Doesn't have any practical implications for this file, but we should stick to it because it sometimes matters!

I changed the evolution number to fix the conflict and did the simple requested reformatting.
@Jetpackjules
Copy link
Collaborator Author

Thanks for the preliminary feedback, I've implemented the changes!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Another couple things to change before I test!

conf/evolutions/default/252.sql Outdated Show resolved Hide resolved
conf/evolutions/default/252.sql Outdated Show resolved Hide resolved
Used default constraint names and fixed downs by specifying table
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.

Some tables in the db are missing foreign key constraints
2 participants