Just write a test for it
61 comments
·March 26, 2025kobzol
(author of the post)
Just to clarify a bit, the test ofc isn't a fully general solution to solving issues with database migrations (I hinted what that might be in the blog post), although it's still useful to provide a nice error message even if a more general solution was implemented.
That was not at all the goal of the post. I just wanted to appreciate how easy it was to achieve this specific task in Rust. In any other systems programming language that I used (even most other languages, except maybe for Python), I would never even imagine something like this being feasible, and so easy to do. That's it :)
switch007
Related
https://github.com/sbdchd/squawk
"linter for PostgreSQL, focused on migrations"
It covers this class of problem I think
NoahZuniga
After reading the article I was left with the question why didn't he just use an SQL linter?
kobzol
That didn't even occur to me, tbh :) But it doesn't have to be SQL linting, I just wanted to appreciate the mindset of not being lazy/afraid to write an unorthodox test.
nikolayasdf123
just look at this test for this one-liner DDL. the test is way more complex than thing it tests in first place. such confusing and hard to write and read tests is the reason people avoid writing tests in the first place. make tests great again!
(great = simple, short, easy to write, read, maintain. at very least no more complex than the thing it testing!)
wesselbindt
Totally with you on the merits of simplicity, writability, readability. But I'm getting strong "rest of the owl" vibes. How would you have prevented this defect instead, in a way that you find simple?
lelanthran
> How would you have prevented this defect instead, in a way that you find simple?
I currently prevent this sort of thing using a populated database. This database has actual real data (user info overwritten with random data).
Using a pre-populated database catches many more errors than this article's approach does.
Using a pre-populated database that has actual data generated by the users over a year or so catches even more errors.
This approach is fragile and misses the actual error, fixing only the symptom. The actual error is "there's a hole in my tests big enough to fly a passenger jet through".
The correct approach is to take a dump of the production database, scrub PII from it, and then perform your migration.
karparov
Pre-populate the db.
wesselbindt
This section:
> Apart from parsing the SQL query, I also considered an alternative testing approach that I might implement in the future: go through each migration one by one, and insert some dummy data into the database before applying it, to make sure that we test each migration being applied on a non-empty database. The data would either have to be generated automatically based on the current database schema, or we could commit some example DB dataset together with each migration, to make sure that we have some representative data sample available.
Suggests that this may also be a fairly complicated direction. Although it's not entirely clear to me why he can't just put one record in the db before any migrations, and then pull it all through. Plus it has the added drawback of removing your coverage for the (not unimportant) zero case.
lbreakjai
In my previous job, we implemented something similar to Neon branching. Each MR would start by cloning the "public" schema into a schema scoped to the merge request, and only then run the migrations and the integration tests.
There's a range of errors you just won't catch until you run your code against the real thing. Especially if you write SQL directly, which feels like a lost art even amongst experienced developers.
dreghgh
This is the wrong approach. He should be putting data in the database schema before trying to run migrations on it.
Doing that is simple and can potentially catch all sorts of bugs, including the bugs you didn't think of yet. His solution is complicated but only catches one very specific type of bug.
dmit
The author mentions that approach at the end of the post:
> Apart from parsing the SQL query, I also considered an alternative testing approach that I might implement in the future: go through each migration one by one, and insert some dummy data into the database before applying it, to make sure that we test each migration being applied on a non-empty database. The data would either have to be generated automatically based on the current database schema, or we could commit some example DB dataset together with each migration, to make sure that we have some representative data sample available.
flqn
That would certainly catch nearly all migration issues, but it doesn't provide a helpful error message like a test for a specific mistake like this does.
Ideally both approaches would be used, with the general case being used to detect and inform more targeted tests.
vlovich123
The author correctly notes the challenge of correctly populating some pre-migration data at each step.
dreghgh
You should solve the problem you need to solve, not the problem you have a cool solution for.
The problem to solve is "given my database in an arbitrary but valid state, applying a migration should succeed and leave the database in a equally valid state". Not "how do I stop people adding NOT NULL columns into tables".
vlovich123
The author had an off-the-shelf crate they could trivially add to their project & prevent instances of this specific failure in 5 minutes even though solving the general case is possible but probably significantly harder & probably still contains various corner cases that are hard to completely eliminate.
I'd characterize it less as the cool solution and more as the quick & dirty solution that gets you a lot of value for little effort.
dxdm
You're just over-generalizing the problem into something you will not be able to solve by proving that a migration works for a _specific_ arbitrary state.
ptx
Does it need to be done at each step, though? Couldn't the test data be added at just one point (where the schema is known) and just let it run through all the subsequent migrations to see if it goes boom?
IanCal
Ideally imo you'd have a property based test that does the following:
Perform some arbitrary list of valid actions. (This alone is valuable)
Run the new migrations.
Perform some arbitrary list of valid actions.
Assert no crashes/errors.
I've found this great for testing APIs - just "perform some list of user actions and make sure things don't explode" can catch a lot.
vlovich123
Not if you could have data that could only have been inserted at some stage to behind with and then a subsequent migration only breaks that case (eg create table in step 10 and step 11 breaks that new table - if you only had data that you inserted in step 1, it would still pass all migrations. In other words, you need to have sample data that exercises the entire DDL that is added or you risk missing something.
formerly_proven
Normally you have per-migration tests for non-trivial schema migrations at minimum...
nurettin
Writing a populate script for every migration sounds like a huge overkill.
hardwaresofton
Sounds like this might make for an excellent general SQL lint tool. I thought sqlint[0] was this, but it’s more for syntax than semantics, it looks like.
kristiandupont
I wrote a linter for schemas (Postgres only): https://github.com/kristiandupont/schemalint/.
I've found it to be a pretty useful way of establishing patterns for data modelling in a team.
kleiba
This is a whole ̶l̶o̶n̶g̶ blog post about someone who was surprised that he could use a crate to parse SQL queries in Rust. A bit of an underwhelming read to me personally.
simonw
If someone figured out how to do a neat thing, learned some tricks along the way and wrote about it just in case it was useful to someone else - that person is doing useful work. More people should do that.
_dain_
There's a difference between intellectually "knowing" that you can parse SQL, and actually going out and trying it and finding that it's practical to do in a test suite. I found the article valuable.
kleiba
> I found the article valuable.
And that's great. Note that I specifically said that "for me personally", I found that article less valuable. But I'm certainly not claiming that everyone else should feel the same.
As a matter of fact, I'm sure a lot of people find my comment to be of little value.
kubb
It’s actually a great reminder that a good framing and sales pitch matter a whole lot.
The author isn’t just recounting their story, they’re selling it as a lesson in a generic approach that will make you a better engineer.
Whether they can deliver on that promise doesn’t matter. It’s the feeling that’s clickable.
karparov
Tbf, it's not their fault it made it to the HN front page.
Are we going to criticise every little innocent blog post just because somebody liked it, submitted it to HN and it got enough upvotes?
kubb
I don’t see a problem with making polite observations about aspects of the content that makes it to the top.
nkrisc
If this isn’t a place to discuss the submissions, what is it?
swombat
Not even that long, but I agree on the "underwhelming"...
"Oh I found some niche issue that bothered me and wrote some code to fix it."
-> HN Front Page
bravetraveler
Virtue trifecta; Rust, testing, and "just"
disgruntledphd2
It's a great title of which we all need reminding.
maccard
If it’s that simple where are your front page blog posts for fixing niche issues?
bilalq
Serverless databases with branching support like Neon make this kind of thing trivial to do. You can just have a test that branches off your prod DB, runs the migrations, and then deletes the branch. This is lightweight enough to easily run on every pull request change.
No mocking or anything. This tests that your migrations are safe to run against real data.
conradludgate
I'm obviously biased by being an employee, but this is where Neon's branching[0] functionality can come in useful. We hope to expand on it one day and build more first-party migration tooling but you can already get a good enough system with the features we have.
Neon Branches are zero-copy snapshots of the database, with all the same data, on an isolated postgres instance. You can run migrations on that data without risking any modifications to data or performance in production. You can set up scripts to run it in CI[1].
This isn't perfect. If performance matters, some migrations might hide table locks which can cause major slowdowns. I'm not sure how you might detect this currently, I had some discussions recently about whether we can add "explain analyze" to DDL queries in postgres.
[0]: https://neon.tech/docs/introduction/branching [1]: https://neon.tech/docs/guides/branching-github-actions
davidgomes
> If performance matters, some migrations might hide table locks which can cause major slowdowns.
Do you mean that the migration might work on a side branch, but then it might not work on the main branch, because there's no other activity running on the side branch?
earnestinger
It bugs me that article never qualifies the sql in parser.
Postgre sql parser, mysql sql parser, …
(Solid advise though)
vlovich123
Not sure I follow. The parser can parse almost any dialect you throw at it.
null_deref
Enjoyed the article. Am I out of touch or have the article linked to a PR of the rust-lang eco system that didn’t go through a CR, is this really the standard for such a large language standard library?
simonw
You mean this one? https://github.com/rust-lang/bors/pull/251
If you look at https://github.com/rust-lang/bors it's not a standard library package, it's a tool to support the Rust development process. And the person who opened and landed that PR is the lead developer of that project.
Skipping a code review from someone else feels OK to me for that.
null_deref
Thanks for the clarification, definitely makes sense
shepmaster
Agree with simonw’s sibling comment. To add to it, I’m the primary maintainer of the Rust playground and basically self-review every single commit.
The rust-lang/rust repository has higher scrutiny (in part driven by tools like bors, the subject of the article).
null_deref
I am very sorry for casting doubt, the sibling comment makes a lot of sense.
> This wasn’t caught by the existing test suite (even though it runs almost 200 end-to-end tests), because it always starts from an empty database, applies all migrations and only then runs the test code.
Isn't that where the test coverage has a hole?
I somehow expected the blog post to extend testing for this. A pre-populated database which is then migrated. That seems to catch a wider class of issues than parsing sql and shielding against just checking for non-null without default.