Details

Reviewers

ginsu
tomek
• derek

Commits

rCOMM6e570829c37c: [sql/keyserver] added multipleStatements to fix fatal error

Summary

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

Test Plan

Ran yarn dev in keyserver and:

Confirmed migration 7 ran by checking the logs for the following:

[NODEM] (node:90015) DB version: 6
[NODEM] (node:90015) migration 7 succeeded.

Confirmed that the MySQL tables looks as expected by inspecting with the mysql CLI utility

Tested the above for three distinct scenarios:

Somebody has a broken setup due to the issues introduced in the previous diff (ran broken migration 6)
Somebody has never run any migration 6 and still has the columns
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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• derek created this revision.Nov 2 2022, 11:18 AM

Herald added a subscriber: • abosh. · View Herald TranscriptNov 2 2022, 11:18 AM

• derek mentioned this in D5519: Revert "[sql/keyserver] removed/edited columns affected by SIWE".Nov 2 2022, 11:19 AM

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...

Harbormaster completed remote builds in B13137: Diff 18031.Nov 2 2022, 11:32 AM

• derek requested review of this revision.Nov 2 2022, 11:32 AM

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

Cover testing the scenario where somebody has a broken setup due to the issues introduced in your previous diff (ran broken migration 6)
Cover testing the scenario where somebody has never run any migration 6 and doesn't have these columns
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)

• derek edited the test plan for this revision. (Show Details)Nov 2 2022, 12:12 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

updated migration number

Harbormaster completed remote builds in B13140: Diff 18034.Nov 2 2022, 1:13 PM

Harbormaster completed remote builds in B13141: Diff 18035.

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

• derek added inline comments.Nov 3 2022, 9:16 AM

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

ashoat added inline comments.Nov 3 2022, 11:38 AM

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 commandeered this revision.Nov 3 2022, 11:39 AM

ashoat edited reviewers, added: • derek; removed: ashoat.

This revision is now accepted and ready to land.Nov 3 2022, 11:39 AM

ashoat edited the test plan for this revision. (Show Details)Nov 3 2022, 11:41 AM

Fix indentation

ashoat edited the test plan for this revision. (Show Details)Nov 3 2022, 11:47 AM

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

Harbormaster completed remote builds in B13158: Diff 18059.Nov 3 2022, 11:55 AM

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

Closed by commit rCOMM6e570829c37c: [sql/keyserver] added multipleStatements to fix fatal error (authored by • derek, committed by ashoat). · Explain Why

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMM6e570829c37c: [sql/keyserver] added multipleStatements to fix fatal error.

Harbormaster completed remote builds in B13159: Diff 18060.Nov 3 2022, 12:09 PM

ashoat mentioned this in D5566: [sql] added ethereum_address to users table.Nov 9 2022, 6:02 AM