Merge pull request #612 from mozilla/drop-foreign-key-constraints

fix(db): Drop foreign key constraints.

lib/db/mysql/index.js

@@ -163,7 +163,15 @@ const QUERY_CLIENT_UPDATE = 'UPDATE clients SET ' +
   'trusted=COALESCE(?, trusted), allowedScopes=COALESCE(?, allowedScopes), ' +
   'canGrant=COALESCE(?, canGrant) ' +
   'WHERE id=?';
-const QUERY_CLIENT_DELETE = 'DELETE FROM clients WHERE id=?';
+// This query deletes everythin related to the client, and is thus quite expensive!
+// Don't worry, it's not exposed to any production-facing routes.
+const QUERY_CLIENT_DELETE = 'DELETE clients, codes, tokens, refreshTokens, clientDevelopers ' +
+  'FROM clients ' +
+  'LEFT JOIN codes ON clients.id = codes.clientId ' +
+  'LEFT JOIN tokens ON clients.id = tokens.clientId ' +
+  'LEFT JOIN refreshTokens ON clients.id = refreshTokens.clientId ' +
+  'LEFT JOIN clientDevelopers ON clients.id = clientDevelopers.clientId ' +
+  'WHERE clients.id=?';
 const QUERY_CODE_INSERT =
   'INSERT INTO codes (clientId, userId, email, scope, authAt, amr, aal, offline, code, codeChallengeMethod, codeChallenge, keysJwe, profileChangedAt) ' +
   'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)';
@@ -188,7 +196,10 @@ const QUERY_REFRESH_TOKEN_DELETE_USER =
   'DELETE FROM refreshTokens WHERE userId=?';
 const QUERY_CODE_DELETE_USER = 'DELETE FROM codes WHERE userId=?';
 const QUERY_DEVELOPER = 'SELECT * FROM developers WHERE email=?';
-const QUERY_DEVELOPER_DELETE = 'DELETE FROM developers WHERE email=?';
+const QUERY_DEVELOPER_DELETE = 'DELETE developers, clientDevelopers ' +
+  'FROM developers ' +
+  'LEFT JOIN clientDevelopers ON developers.developerId = clientDevelopers.developerID ' +
+  'WHERE developers.email=?';
 const QUERY_PURGE_EXPIRED_TOKENS = 'DELETE FROM tokens WHERE clientId NOT IN (?) AND expiresAt < NOW() LIMIT ?;';
 const QUERY_EXPIRED_TOKENS =
   'SELECT expiresAt, token, clientId FROM tokens WHERE expiresAt >= ? AND expiresAt <= NOW() ORDER BY expiresAt ASC LIMIT ?';

lib/db/mysql/patch.js

@@ -6,4 +6,4 @@
 // Update this if you add a new patch, and don't forget to update
 // the documentation for the current schema in ../schema.sql.
 
-module.exports.level = 22;
+module.exports.level = 23;

lib/db/mysql/patches/patch-022-023.sql

@@ -0,0 +1,55 @@
+-- Drop foreign key constraints.  They make DB migrations harder
+-- and aren't really providing us much value in practice.
+
+-- The `clientDevelopers` table needs indexes on `developerId` a `clientId`
+-- for fast lookup.  Prior to this patch, we were taking advantage of the
+-- index that is automatically created to enforce foreign key constraints,
+-- which the MySQL docs at [1] describe as:
+--
+--   """
+--   In the referencing table, there must be an index where the foreign key
+--   columns are listed as the first columns in the same order. Such an index
+--   is created on the referencing table automatically if it does not exist.
+--   This index might be silently dropped later, if you create another index
+--   that can be used to enforce the foreign key constraint.
+--   """
+--   [1] https://dev.mysql.com/doc/refman/5.7/en/create-table-foreign-keys.html
+--
+-- The "might" in there leaves some doubt about the exact circumstances under
+-- which we can depend on this index continuing to exist, so this migration
+-- explicitly creates the indexes we need.  It's a two step process:
+--
+--  1) Explicitly create the indexes we need.  This "might" cause the ones
+--     that were created automatically for the FK constraint to be dropped.
+--
+--  2) Drop the FK constraints, which might leave behind the auto-created
+--     indexes if they weren't dropped in (1) above.
+--
+-- In my testing, the auto-created indexes are indeed dropped in favour
+-- of the explicit ones.  If they aren't, then at least we wind up with
+-- duplicate indexes which can be cleaned up manually, which is much better
+-- than winding up with no indexes at all.
+-- 
+
+ALTER TABLE clientDevelopers ADD INDEX idx_clientDevelopers_developerId(developerId),
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE clientDevelopers ADD INDEX idx_clientDevelopers_clientId(clientId),
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE clientDevelopers DROP FOREIGN KEY clientDevelopers_ibfk_1,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE clientDevelopers DROP FOREIGN KEY clientDevelopers_ibfk_2,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE refreshTokens DROP FOREIGN KEY refreshTokens_ibfk_1,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE codes DROP FOREIGN KEY codes_ibfk_1,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE tokens DROP FOREIGN KEY tokens_ibfk_1,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+UPDATE dbMetadata SET value = '23' WHERE name = 'schema-patch-level';

lib/db/mysql/patches/patch-023-022.sql

@@ -0,0 +1,23 @@
+
+-- ALTER TABLE clientDevelopers DROP INDEX idx_clientDevelopers_developerId,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE clientDevelopers DROP INDEX idx_clientDevelopers_clientId(clientId),
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE clientDevelopers ADD FOREIGN KEY (developerId) REFERENCES developers(developerId) ON DELETE CASCADE,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE clientDevelopers ADD FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE refreshTokens ADD FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE codes ADD FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE tokens ADD FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- UPDATE dbMetadata SET value = '22' WHERE name = 'schema-patch-level';

lib/db/mysql/schema.sql

@@ -24,7 +24,6 @@ CREATE TABLE IF NOT EXISTS codes (
   code BINARY(32) PRIMARY KEY,
   clientId BINARY(8) NOT NULL,
   INDEX codes_client_id(clientId),
-  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
   userId BINARY(16) NOT NULL,
   INDEX codes_user_id(userId),
   email VARCHAR(256) NOT NULL,
@@ -39,7 +38,6 @@ CREATE TABLE IF NOT EXISTS tokens (
   token BINARY(32) PRIMARY KEY,
   clientId BINARY(8) NOT NULL,
   INDEX tokens_client_id(clientId),
-  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
   userId BINARY(16) NOT NULL,
   INDEX tokens_user_id(userId),
   email VARCHAR(256) NOT NULL,
@@ -53,25 +51,24 @@ CREATE TABLE IF NOT EXISTS tokens (
 
 CREATE TABLE IF NOT EXISTS developers (
   developerId BINARY(16) NOT NULL,
-  FOREIGN KEY (developerId) REFERENCES developers(developerId) ON DELETE CASCADE,
-  clientId BINARY(8) NOT NULL,
-  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
-  createdAt TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
+  email VARCHAR(255) NOT NULL,
+  createdAt TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
+  UNIQUE(email)
 ) ENGINE=InnoDB CHARACTER SET utf8 COLLATE utf8_unicode_ci;
 
 CREATE TABLE IF NOT EXISTS clientDevelopers (
   rowId BINARY(8) PRIMARY KEY,
   developerId BINARY(16) NOT NULL,
   clientId BINARY(8) NOT NULL,
-  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
-  createdAt TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
+  createdAt TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
+  INDEX idx_clientDevelopers_developerId(developerId),
+  INDEX idx_clientDevelopers_clientId(clientId)
 ) ENGINE=InnoDB CHARACTER SET utf8 COLLATE utf8_unicode_ci;
 
 CREATE TABLE IF NOT EXISTS refreshTokens (
   token BINARY(32) PRIMARY KEY,
   clientId BINARY(8) NOT NULL,
   INDEX tokens_client_id(clientId),
-  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
   userId BINARY(16) NOT NULL,
   INDEX tokens_user_id(userId),
   email VARCHAR(256) NOT NULL,