feat(database): add migration hash checking

[erikaraujo]

This PR adds Hash-checking to database migrations - preventing migration files from being tempered with only if the --validate flag is used.

php tempest migrate:rehash              // Rehashes all existing migrations
php tempest migrate:validate            // Validate that no existing migration file have been tempered with

php tempest migrate:fresh               // Calls `validate` before even dropping all tables
php tempest migrate:up                  // Calls `validate` before executing the migrations

php tempest migrate:fresh --no-validate // Skips calling `:validate`
php tempest migrate:up --no-validate    // Skips calling `:validate`

Note

Documentation update PR: tempestphp/tempest-docs#68

[brendt]

So what exactly is the flow here? Let's say I have a migration file that has changed. Then what? We run migrate:up locally and get an error? I'm not sure I fully understand the flow yet.

[erikaraujo]

So what exactly is the flow here? Let's say I have a migration file that has changed. Then what? We run migrate:up locally and get an error? I'm not sure I fully understand the flow yet.

Yup, that's it. Migrations will not run if existing migrations have been tempered with. It's been a while since I've used more restrict migration libraries (been doing Laravel for a while now), but I believe that's how they do it.

We could not have the check run when you do migrate:up and instead have a separate migrate:check-for-tempering command or something that devs could opt to run before migrations on the CI.

Anyways, definitely open to suggestions here.

[innocenzi]

We could not have the check run when you do migrate:up and instead have a separate migrate:check-for-tempering command or something that devs could opt to run before migrations on the CI.

I'd prefer migrate:up to be the command failing, and --force or --allow-changes would be the bypass. Alternatively, we make it opt-in with --no-tempering, but I'd prefer it to be a default behavior (if the implementation works well, that is!)

[brendt]

I'm fine with it being the default mode. Let's not hook into --force though, but keep it as a separate argument. I'm still struggling to understand how this works in practice.

Let's say you make a change to an existing migration during development and run migrate:fresh, no error will be thrown since we start from scratch. Next we deploy and run migrate:up. This time the migration fails because the existing migration's hash has a mismatch. What's the next step then? How do you get out of this "error state"?

Possibly by running with --allow-changes. Does that mean that the old migration hash gets updated to the new one? Is there another way to fix the error state without running with --allow-changes?

To me it feels like, by the time this error is triggered, it's already pretty (too) late: we're deploying to production. That's why I suggested adding a warning of some sorts (maybe an error) that's shown when running migrate:fresh, I think that makes more sense since that way we'll get feedback during development instead of deployment. We can still keep the check in migrate:up as a last-resort, but ideally it should never trigger.

The end goal here is that people don't update existing migrations, but instead make new ones that apply on top of the original one.

I hope I'm making sense 😅

[erikaraujo]

I'm fine with it being the default mode. Let's not hook into --force though, but keep it as a separate argument. I'm still struggling to understand how this works in practice.

Let's say you make a change to an existing migration during development and run migrate:fresh, no error will be thrown since we start from scratch. Next we deploy and run migrate:up. This time the migration fails because the existing migration's hash has a mismatch. What's the next step then? How do you get out of this "error state"?

Possibly by running with --allow-changes. Does that mean that the old migration hash gets updated to the new one? Is there another way to fix the error state without running with --allow-changes?

To me it feels like, by the time this error is triggered, it's already pretty (too) late: we're deploying to production. That's why I suggested adding a warning of some sorts (maybe an error) that's shown when running migrate:fresh, I think that makes more sense since that way we'll get feedback during development instead of deployment. We can still keep the check in migrate:up as a last-resort, but ideally it should never trigger.

The end goal here is that people don't update existing migrations, but instead make new ones that apply on top of the original one.

I hope I'm making sense 😅

I understand your concern. One of the most popular migration libraries (Redgate Flyway) doesn't have a :fresh command per se, but you can do a clean followed by a migrate, which is the same thing in the end. When you do such a thing, there's no warning or anything - it's like you're recreating it all from scratch.

And I don't know how we'd accomplish a warning or something on Tempest's :fresh command either without compromising the performance of it. Like, we'd need to make all checks before the ->dropAll call.

This may not be a huge issue, though. Here's my idea:

• Add a migrate:validate command that just checks the hashes;
• Run this command before the ->dropAll call in the fresh command;
• Run this command before the ->up call in the up command;

Of course, we can skip running that command entirely if --allow-changes (or another name) is passed.

We could even improve the :validate command later to allow for passing custom DB config/env-variables - so that this command can be used in PRs to validate against staging/prod DBs.

[brendt]

I like the :validate approach a lot!

[brendt]

Ping me when ready :)

[coveralls]

Pull Request Test Coverage Report for Build 14116559981

Details

• 75 of 83 (90.36%) changed or added relevant lines in 8 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 79.445%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Tempest/Database/src/Migrations/MigrationException.php	2	4	50.0%
src/Tempest/Database/src/Migrations/MigrationManager.php	37	43	86.05%

Totals
Change from base Build 14096555604:	0.1%
Covered Lines:	10590
Relevant Lines:	13330

---

💛 - Coveralls

[erikaraujo]

Ping me when ready :)

@brendt Ready! :)

[innocenzi]

Super cool PR! Is there a reason why validation is opt-in though? I'd prefer having to specify --no-validate to opt-out

[erikaraujo]

Super cool PR! Is there a reason why validation is opt-in though? I'd prefer having to specify --no-validate to opt-out

@innocenzi I thought we were already avoiding breaking changes (I was wrong 😅 ).
I can definitely change this. Should I go ahead and do it, or is it better to wait for @brendt 's feedback?

What would a good name for the argument be? I'm not a huge fan of --no-validate because I usually prefer having "positive" options. Like --no-validate=false is confusing.

What about --skip-validation? --unsafe? --allow-tampering?
--skip-validation is the clearest, in my opinion.

PS: I hate that github doesn't allow replying to such comments

[innocenzi]

I can definitely change this. Should I go ahead and do it, or is it better to wait for @brendt 's feedback?

I think Brent would agree with me, but you can wait to be sure to avoid having to revert the change

What would a good name for the argument be? I'm not a huge fan of --no-validate because I usually prefer having "positive" options. Like --no-validate=false is confusing.

We have support for negating arguments, so it's fine: you can set the current validate argument to true by default, and passing --no-validate will set it to false. So no issue with confusing booleans

PS: I hate that github doesn't allow replying to such comments

I'm with you! 😅

[erikaraujo]

We have support for negating arguments, so it's fine: you can set the current validate argument to true by default, and passing --no-validate will set it to false. So no issue with confusing booleans

🤯
Neat! I actually have all the changes done locally already - but I'll wait on the feedback to push them.

[brendt]

Looks good, some minor questions, but that's ok.

FYI I'm merging #1076 today, I'll check this PR afterwards to see if anything breaks because of it. If it does, I'll fix it on your branch.

[brendt]

I think Brent would agree with me, but you can wait to be sure to avoid having to revert the change

Yeah it's fine, make it true by default :)

[brendt]

I've merged main, let's see whether anything breaks

Done

[brendt]

Thanks!