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: alter notifications constraint with ON DELETE CASCADE #211

Merged
merged 1 commit into from
Sep 26, 2023
Merged

fix: alter notifications constraint with ON DELETE CASCADE #211

merged 1 commit into from
Sep 26, 2023

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Sep 22, 2023

Description

This PR adds SQL migration file with changing the fk_notifications_client_id constraint to include ON DELETE CASCADE to fix the delete on table "clients" violates foreign key constraint "fk_notifications_client_id" error.

With this change removing records from the clients table will automatically remove related to the clients.id records in the notifications table.

Resolves #212

Test plan

Pre-requirements:

To test the sqlx migrations we should use the sqlx-cli tool:

cargo install sqlx-cli

Step 1:
To test the migration properly we should populate the table with different records including duplicated device_token records in the clients and notifications tables.
To mimic the real database behavior we will use the test migration SQL script with the data to be inserted into the database BEFORE the new migration SQL script.

Please download the script to the /migrations folder:

curl -o migrations/1690000000_TEST_populate_with_duplicates.sql https://gist.githubusercontent.com/geekbrother/72edfa9176263287c2e1626ae36df652/raw/e842759dc92bf362e0c52aca1d3590612408a192/populate_clients_with_duplicates.sql

Step 2:

Also, we should download the migration SQL script from #202 with removing duplicates and adding the unique constraint to the device_token to mimic removing records from the clients table and test the follow-up migration as well AFTER this PR migration script.

curl -o migrations/9000000000_TEST_delete_duplicates_add_constraint.sql https://gist.githubusercontent.com/geekbrother/89166e4221199058a7a55f1a7000a03a/raw/a48ba4b577078269a83f8cd3cc464a50b6be51d5/delete_duplicates.sql

After that, we should have the following migrations scripts in our /migrations folder:

1664117744_init.sql
1664117746_create-clients.sql
1664117751_create-notifications.sql
1667510128_add-tenant-id.sql
1675269994_add-sandbox-to-enum.sql
1690000000_TEST_populate_with_duplicates.sql   <--- Populating the database with the data BEFORE
1695381202_alter_notifications_constraint.sql   <--- Migration from this PR for altering the constraint
9000000000_TEST_delete_duplicates_add_constraint.sql  <--- Deleting duplicated records and test `ON DELETE CASCADE` AFTER

Step 3:

Start the Postgres docker container:

docker run \
  -p 5432:5432 \
  -e POSTGRES_HOST_AUTH_METHOD=trust \
  --name echo-server-pg \
  -d postgres

Step 4:

Now we can start the sqlx migration to test its correctness and results using the sqlx-cli:

sqlx migrate run --database-url postgres://postgres@localhost/postgres

The expected result is that all migration steps should be passed successfully:

➜  echo-server git:(fix/alter_notifications_client_id_constraint) ✗ sqlx migrate run --database-url postgres://postgres@localhost/postgres
Applied 1664117744/migrate init (1.861333ms)
Applied 1664117746/migrate create-clients (6.762958ms)
Applied 1664117751/migrate create-notifications (5.017792ms)
Applied 1667510128/migrate add-tenant-id (1.563875ms)
Applied 1675269994/migrate add-sandbox-to-enum (1.580292ms)
Applied 1690000000/migrate TEST populate with duplicates (2.883791ms)
Applied 1695381202/migrate alter notifications constraint (3.4805ms)
Applied 9000000000/migrate TEST delete duplicates (3.79875ms)

As an optional step, we can log in to Postgres and check for the duplicates:

SELECT device_token, COUNT(*)
FROM clients
GROUP BY device_token
HAVING COUNT(*) > 1;

The expected result is that there are no duplicates in the clients table at the end of the migration (but they were populated by the test database population migration script before).

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother self-assigned this Sep 22, 2023
@geekbrother geekbrother marked this pull request as ready for review September 22, 2023 20:34
@geekbrother geekbrother changed the title fix: alter notifications constraint fix: alter notifications constraint with ON DELETE CASCADE Sep 22, 2023
@geekbrother
Copy link
Contributor Author

cc @chris13524 please take a look as a blocking reviewer

@geekbrother geekbrother merged commit 3553b2e into WalletConnect:main Sep 26, 2023
6 of 8 checks passed
@geekbrother geekbrother deleted the fix/alter_notifications_client_id_constraint branch October 1, 2023 15: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.

fix: violation of the foreign key constraint "fk_notifications_client_id" error
3 participants