Page MenuHomePhabricator

[sql/keyserver] added multipleStatements to fix fatal error
ClosedPublic

Authored by ashoat on Nov 2 2022, 11:18 AM.
Tags
None
Referenced Files
F3519580: D5521.id18034.diff
Sun, Dec 22, 10:40 PM
F3519474: D5521.id18035.diff
Sun, Dec 22, 10:14 PM
F3518157: D5521.diff
Sun, Dec 22, 7:07 PM
Unknown Object (File)
Thu, Dec 19, 8:43 PM
Unknown Object (File)
Nov 21 2024, 5:14 AM
Unknown Object (File)
Nov 21 2024, 5:14 AM
Unknown Object (File)
Nov 21 2024, 5:14 AM
Unknown Object (File)
Nov 21 2024, 5:14 AM

Details

Summary

fixes fatal error from landing https://phab.comm.dev/D5494

Test Plan

Ran yarn dev in keyserver and:

  1. Confirmed migration 7 ran by checking the logs for the following:
[NODEM] (node:90015) DB version: 6
[NODEM] (node:90015) migration 7 succeeded.
  1. Confirmed that the MySQL tables looks as expected by inspecting with the mysql CLI utility

Tested the above for three distinct scenarios:

  1. Somebody has a broken setup due to the issues introduced in the previous diff (ran broken migration 6)
  2. Somebody has never run any migration 6 and still has the columns
  3. Somebody has never run any migration 6 and DOESN'T have these columns

To test all of these scenarios, I ran the following queries to undo the migration:

ALTER TABLE users ADD COLUMN public_key char(116) DEFAULT NULL, MODIFY hash char(60) COLLATE utf8mb4_bin NOT NULL;
ALTER TABLE sessions ADD COLUMN public_key char(116) DEFAULT NULL;
UPDATE metadata SET data = 5 WHERE name = "db_version";

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Based on chat correspondence it seems like your understanding of the MariaDB migrations code is incomplete. Can you brush up on it and then update the test plan here to clarify exactly how you are making sure your code is being run? A simple console.log next to your await dbQuery line would probably be enough to verify if it's being called or not...

By the way, you still need to make sure your queries are idempotent. Please also update your test plan to:

  1. Cover testing the scenario where somebody has a broken setup due to the issues introduced in your previous diff (ran broken migration 6)
  2. Cover testing the scenario where somebody has never run any migration 6 and doesn't have these columns
  3. Cover testing the scenario where somebody has never run any migration 6 and DOES have these columns (presumably this has happened to you, since you introduced these columns manually via a script before running the migration)

By the way, you might need to bump the migration version here if you want your new query to run in the "has already run broken migration 6" scenario... (not sure if the migration version got bumped in that scenario or not)

ashoat requested changes to this revision.Nov 2 2022, 12:13 PM

(Passing back with requests on test plan above)

This revision now requires changes to proceed.Nov 2 2022, 12:13 PM

i definitely need to brush up on my migration understanding - in the process of running those tests. will update diff & rerequest shortly

separated second query into separate migration

ashoat requested changes to this revision.Nov 2 2022, 2:31 PM

Please update test plan as requested above

keyserver/src/database/migration-config.js
61 ↗(On Diff #18035)

Indents look weird, can we undo this change?

This revision now requires changes to proceed.Nov 2 2022, 2:31 PM
keyserver/src/database/migration-config.js
61 ↗(On Diff #18035)

this is prettier's auto-formatting

tested locally, and things seem to be running smoothly again

keyserver/src/database/migration-config.js
61 ↗(On Diff #18035)

While it's true that Prettier messed up the formatting due to the introduction of a second parameter to dbQuery, it's not true that Prettier insists on this broken indentation. See my upcoming update

ashoat edited reviewers, added: derek; removed: ashoat.
This revision is now accepted and ready to land.Nov 3 2022, 11:39 AM
ashoat added a subscriber: przemek.

The queries are not idempotent, so somebody who fixed up the migration to run successfully (eg. @ginsu, @przemek) would have yet another error from this diff landing as-is

Add IF EXISTS to make queries idempotent

This revision is now accepted and ready to land.Nov 3 2022, 11:56 AM
This revision was landed with ongoing or failed builds.Nov 3 2022, 11:57 AM
This revision was automatically updated to reflect the committed changes.