6 ms·
Just write a test for it
- hardwaresofton 2y agoSounds 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. [0]: https://github.com/purcell/sqlint https://github.com/purcell/sqlint
- kristiandupont 2y agoI wrote a linter for schemas (Postgres only): [link redacted] I've found it to be a pretty useful way of establishing patterns for data modelling in a team.
- bsaul 2y agoSidenote : i found this code quite readable. I'm usually reading Rust code here on HN that's full of weird lifetime annotations or Dyn or super long generic types. As a non-rust dev this freaked me out. How common is this kind of code in practice ?
- vrnvu 2y agoSpecifying lifetimes and types is mostly used when writing libraries. Writing Rust "as a user" is pretty readable and comfortable.
- bschwindHN 2y agoIf you're running something like this and it isn't performance sensitive, you don't really need to deal with lifetimes at all. Lifetimes mostly come into play if you want to work with zero-copy parsing, or structs that borrow some things and act like a sub-view into the data you're working with. I'm sure there are plenty of other use cases for lifetimes, but they don't come up very often when writing standard "application" code.
- kobzol 2y agoIf you're writing applications or tests, it's mostly the simpler kind of code. If you're writing reusable (or perf. critical code), you will start seeing generics and lifetimes much more often.
- switch007 2y agoRelated https://github.com/sbdchd/squawk https://github.com/sbdchd/squawk "linter for PostgreSQL, focused on migrations" It covers this class of problem I think
- NoahZuniga 2y agoAfter reading the article I was left with the question why didn't he just use an SQL linter?
- kobzol 2y agoThat 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.
- kleiba 2y agoThis 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.
- swombat 2y agoNot 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 2y agoVirtue trifecta; Rust, testing, and "just"
- disgruntledphd2 2y agoIt's a great title of which we all need reminding.
- maccard 2y agoIf it’s that simple where are your front page blog posts for fixing niche issues?
- kubb 2y agoIt’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 2y agoTbf, 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?
- earnestinger 2y agoIt bugs me that article never qualifies the sql in parser. Postgre sql parser, mysql sql parser, … (Solid advise though)
- vlovich123 2y agoNot sure I follow. The parser can parse almost any dialect you throw at it. https://github.com/apache/datafusion-sqlparser-rs https://github.com/apache/datafusion-sqlparser-rs
- dreghgh 2y agoThis 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.
- vlovich123 2y agoThe author correctly notes the challenge of correctly populating some pre-migration data at each step.
- ptx 2y agoDoes 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 2y agoIdeally 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.
- jononor 2y agoI have also found this kind of approach very valuable. With defensive code, where the code itself does a bit of the kind of correctness checking that one might conventionally put into tests, it works even better. Pre/post- conditions in functions for example.
- vlovich123 2y agoNot 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.
- nikolayasdf123 2y agojust 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 2y agoTotally 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?
- karparov 2y agoPre-populate the db.
- wesselbindt 2y agoThis 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.
- kobzol 2y agoIf I only insert data into the DB once, I could miss important states. Like, I could add non-NULL data to a NOT NULL column, then make it NULL, and then make it NOT NULL again. If I don't insert NULL into the column in-between the last two migrations, I won't trigger the issue.
- karparov 2y ago> 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.
- bilalq 2y agoServerless 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.
- bobnamob 2y agoAnd now you've pulled in a full sql parser as a dependency (admittedly a dev/build time dependency, but a dependency nonetheless) in a project that has no business parsing sql. In this day and age of increasingly rampant supply chain attacks & dependency vulnerabilities, I'd definitely be second guessing the approach of "just write a test for it" if that test involved blowing up your attack/vuln surface
- kobzol 2y agoI don't really see an attack surface for a dev dependency.
- conradludgate 2y agoYour development machine, potentially with API keys and access tokens in `$HOME`, is the attack surface
- conradludgate 2y agoI'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 https://neon.tech/docs/introduction/branching [1]: https://neon.tech/docs/guides/branching-github-actions https://neon.tech/docs/guides/branching-github-actions
- davidgomes 2y ago> 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?
- null_deref 2y agoEnjoyed 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 2y agoYou mean this one? https://github.com/rust-lang/bors/pull/251 https://github.com/rust-lang/bors/pull/251 If you look at https://github.com/rust-lang/bors 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 2y agoThanks for the clarification, definitely makes sense
- shepmaster 2y agoAgree 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 2y agoI am very sorry for casting doubt, the sibling comment makes a lot of sense.
- lbreakjai 2y agoIn 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.
- kobzol 2y ago(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 :)