Only run a migration if the previous migration has been run

When deploying train-108 to stage there were two migrations required:

However, I only ended up applying the 74 to 75 migration. This caused the auth server to start up fine because the migration patch level was correct, but the missing tables/stored procedures meant that nothing worked.

Rather than relying on fallible humans, what if we added some sort of assertion to each migration on what the patch level should be before applying changes?

[12:00:54] add an assertion in sql
[12:01:05] which bails if the patch level isn't what it should be

Hey @vladikoff! @jbuck Can I work on this issue. Any help from where to get started would be helpful. Thanks

@hritvi, it looks like the code for patching the db lives in this repo:

https://github.com/mozilla/mysql-patcher

I've not looked at it before but it seems like there is a test that is supposed to do the right thing inside checkAllPatchesAvailable:

// check that this patch exists
if ( !this.patches[currentPatchLevel] || !this.patches[currentPatchLevel][nextPatchLevel] ) {
  process.nextTick(function() {
    callback(new Error('Patch from level ' + currentPatchLevel + ' to ' + nextPatchLevel + ' does not exist'))
  })
  return
}

It might be worth setting up a test migration in your local copy of fxa-auth-db-mysql, which is numbered so that there is a gap where the preceding migration should be and then testing to see how the above code behaves.

You can create an empty migration by running node scripts/migration from the command line in your local fxa-auth-db-mysql directory. Then change the numbering on the two patch files that get created in lib/db/schema and also inside lib/db/patch.js, so that there is a missing migration (it looks like the gap should be 78 for current master).

Then either set your debugger to break on that check in the mysql-patcher code (line 279 of node_modules/mysql-patcher/index.js), or just stick a couple of console.log statements immediately before and inside of it. You're interested in whether the condition is being entered or not. If it is being entered, we need to find out why that isn't causing the patcher to fail. If it's not being entered, we need to find out what this.patches looks like and what we'd need to change to make the if work.

Make sense?

You can create an empty migration by running node scripts/migration from the command line in your local fxa-auth-db-mysql directory.

Wait, no you can't. It's a shell script, sorry, so use scripts/migration.sh if you're on Linux or Mac. If you're on Windows you might need to create the migration by hand, or I also hear there is some kind of bash shell thing available on Windows too, but I don't know anything about it.

Oh and last thing I forgot to mention, in case it's not clear, you can run the db patcher from the command line in fxa-auth-db-mysql, like this:

node bin/db_patcher

...and if you want to re-run the same migration multiple times while you're testing, the easiest thing to do is just drop your database:

mysql -u root -p -e 'DROP DATABASE fxa'

The next time you run bin/db_patcher, it will take care of creating your db as well as applying all of the migrations.

@philbooth when I run migrations on stage/prod, I need to use the raw SQL - would it be possible to make this check happen in the raw SQL instead? so I guess this issue would be more about figuring a way to raise an error when running an SQL statement, that we can apply towards future sql migrations

Ah, gotcha!

Will peruse the MySQL manual for ideas tomorrow. Or if anyone else has suggestions then please weigh in with them!

@hritvi, if you're still interested in working on this issue, it turns out that it's much easier now. You can ignore most of what I said before!

We need to change scripts/migration.sh so that it adds an extra check in the forward migration file, before the UPDATE dbMetadata command. So all the work is inside the fxa-auth-db-mysql repo.

Line 37 of scripts/migration.sh is where the UPDATE command is appended to the forward migration. Immediately before that, you need to add a few lines that SELECT the current value for schema-patch-level into a variable and then do a SIGNAL SQLSTATE if the value doesn't equal $PREV_LEVEL. You can see an example of that in some of the existing migations, for instance in lib/db/schema/patch-074-075.sql:

fxa-auth-db-mysql/lib/db/schema/patch-074-075.sql

Lines 25 to 29 in 9bdb47b

    
           -- Signal error if no user found 
        
           SELECT COUNT(*) INTO @accountCount FROM `accounts` WHERE `uid` = `uidArg`; 
        
           IF @accountCount = 0 THEN 
        
             SIGNAL SQLSTATE '45000' SET MYSQL_ERRNO = 1643, MESSAGE_TEXT = 'Can not generate recovery codes for unknown user.'; 
        
           END IF;

Picking an appropriate value for MYSQL_ERRNO is not an exact science, there's a bajllion of them listed in the manual. I think 1643 might be a reasonable choice here.

You can test out your changes to migration.sh by just running it and seeing what file it writes to disk. Keep tweaking it until you're happy with the result, and then you can test out the resulting migration by running node bin/db_patcher as mentioned in an earlier comment.

Does that make sense? Have I given you enough information?

Yes, it did make sense to me @philbooth @jbuck. Thanks

Some code this is here: #331

	-- Signal error if no user found
	SELECT COUNT(*) INTO @accountCount FROM `accounts` WHERE `uid` = `uidArg`;
	IF @accountCount = 0 THEN
	SIGNAL SQLSTATE '45000' SET MYSQL_ERRNO = 1643, MESSAGE_TEXT = 'Can not generate recovery codes for unknown user.';
	END IF;