Settings

Theme

A stray commit buried multiple levels deep cost me months

droppedasbaby.com

33 points by offbyone42 · 37 comments

Reader

7 threads
Groxx

    # transaction has been commented out
    # with transaction():
    db_models = DBAccess.fetch_records(ids)
    db_models[0].yo_mama_fat = True
    # request ends, data poofs into the ether
tbh if this doesn't fail either immediately (no open transaction == error on modification attempt) or at GC time (if there's some kind of deferred logic), then I'd say this is an extremely bad framework and it does hold a major part of the blame. "You can mutate database-connected objects and sometimes they save back to the DB, sometimes they do not" is not reasonable behavior.

(obviously these frameworks exist. quite a few of them. quantity does not in any way imply sanity.)

> 2. DO NOT pass DB models in and out of the DB layer.

Yea, the more I've used "thin" ORMs that give you plain objects, the more I've grown convinced they're the best choice basically all the time. Trying to be magical is cute, but it's guaranteed to be made of spicy unobtanium, and at some point it'll blow up in your face in an extremely convoluted way due to a simple cause that you'll notice is absolutely everywhere and you're just stuck being paranoid forever. There's no need to live like that.

  • grebc

    Dapper in .NET is f*cking fantastic. I've not found a better library/pattern for SQL data access.

    • Groxx

      At a glance that looks like exactly what I like, yeah. Object mappers are 100% worth the effort and are often faster than hand translating the objects (because they'll cache and do things that are annoying or error-prone to do by hand) (code generation is nice too, Android has a couple that do that), and plain inline SQL strings with vars are FAR easier to learn and reason about and can take advantage of any database and any feature, including custom ones.

  • ricardobeat

    I think the code doesn't fail because it never existed. AI slop.

Retr0id

> DO NOT blame the framework

Why not? Why should I be allowed to db.commit() midway through a transaction?

  • jiggawatts

    Database engines I've worked with generally support nested transactions, so you can open a transaction, open another transaction, commit it, then roll back the outer transaction... and it's like nothing ever happened.

    This ability to compose transactions is their main benefit over other kinds of concurrency control!

    • manphone

      Many databases do not actually offer this or pretend to offer this, but it is really just one thing.

      • unsnap_biceps

        Are you referring to save points as the "pretend to offer this"? If so, why wouldn't they work? If not, what are you referring to?

        • jiggawatts

          Literally:

              BEGIN TRAN
              BEGIN TRAN
              COMMIT TRAN
              ROLLBACK TRAN
          • unsnap_biceps

            Yes, but what part about that is pretend? Is there a db that doesn't do the right thing and roll back the outer transaction? If so, can you name it?

      • Groxx

        For those that don't, it's almost always modeled in the connection-library as "hold it open until commits == opens, or roll back if any roll back". Or it's often easy to build this yourself, around the library.

        Which works entirely fine in most cases. You're at greater risk of phantom reads and general "stuff that can occur while you hold open a transaction", but if you're not handling that correctly then you're not handling that correctly. It's only a matter of volume, not existence.

        ... with a clear exception for cases where you do need to truly end a transaction, like if you're relying on some other thread to do something on a different transaction that needs to see your changes, or when you risk a deadlock somewhere due to not releasing your lock. Those are both a risky patterns for a lot of reasons though, and worth avoiding at design-time if at all possible.

  • Stitch4223

    With great power comes great responsibility.

    • Retr0id

      Well, I don't want either. Give me a safe API!

    • saghm

      Yes, and the people who designed that API clearly were not worthy of of the responsibility of providing it

      • Stitch4223

        No. Exposing primitives like commit or “begin transaction” isn’t bad or irresponsible design; working with databases is ridiculously hard, which becomes apparent when demand increases.

        Combining that with spaghetti that does transaction magic at random places guarantees the sort of pain that makes cursing the entire human race seem like a pretty mild response.

        Higher-level abstractions may prevent some footguns; e.g., an “atomic” decorator/annotation commits automatically after a successful call. They are somewhat easier to understand but come with their own limitations and caveats.

        • saghm

          > No. Exposing primitives like commit or “begin transaction” isn’t bad or irresponsible design; working with databases is ridiculously hard, which becomes apparent when demand increases.

          The problem isn't being able to commit. The problem is being able to commit and then not notice that you're no longer in the transaction. You could easily have `begin_transaction` return a `Transaction` object, having operations in the transaction happen on the object, and calling `commit` on it makes it throw an error if you try to use it again after. Maybe the reason that working with databases is "ridiculously hard" because the API isn't well-designed...

          • Stitch4223

            Yes, a great API built on a capable database helps in those cases :)

            • saghm

              I don't understand what this has to do with the database being used. There's nothing preventing designing an API for any database with transactions that has the flaw I described, or from using a technique like I described to prevent that flaw.

              You told me "No" when I pointed out that a responsible API developer would not make an API like that. Now you're shifting to make some unrelated point about the database itself. I feel like you're implicitly assuming that the same people who write the database internals need to be the ones writing the code that provides a high-level client library that will be used by applications that need to interact with the database from the outside, and I don't agree with that premise at all. If anything, the fact that database internals are so complicated to get write is an argument in favor of having separate developers with different specialties, and not just slapping together an API without thinking about it because you're too busy working on the internals.

              • Stitch4223

                Ah yes. I read it differently. I agree that this can could be prevented with a decent API and that writing such facility is a challenge on it’s own. It would have helped the developers in the article to write better code as a result.

                • saghm

                  Fair enough; I did initially mention the API specifically as the issue, but it's also probably obvious by now that I have pretty strong opinions about API design as worthy of its own consideration, so it's possible I might have mistakenly assumed it was more clear what I was talking about for people who don't focus on it as much as I do

    • UqWBcuFx6NV4r

      This is like people saying that C is infallible and it’s those stupid lesser-developers that unlike me simply cannot wield its immense power. No. Usability matters. After all these years software development still has an ‘unfounded male confidence / posturing’ problem and it’s just cringeworthy

      • hackthemack

        I agree. No matter how complicated, or unnecessary, or unintuitive a piece of software or technology is... There is always this contingent of people who pop in and say "It is not that hard!" and furthermore tend to express a view of "I am superior because I figured out this obtuse thing, and you must be inferior because you have not figured it out".

        I do not know why that mentality exists in the industry, but I see it all the time... for the past 35 years.

        • saghm

          I've long felt that skill in designing APIs is hugely underrated in software. There's a common perception that what happens under the hood is complicated and requires being very smart, but the crafting a way to interact with a system that prevents issues when possible and at least tries to encourage usage patterns that will not lead to users shooting themselves in the foot is trivial enough to handwave away. All of the bugs and vulnerabilities that could have been completely avoided by a different API design seem to get classified the user's fault, not whoever designed the API.

        • SoftTalker

          It exists in every industry. Listen to anyone in construction, carpenters, electricians, plumbers, etc.

  • offbyone42OP

    Giving "I cut my finger with a knife, why is the knife so sharp?" energy. JKJK.

    I think the option should be there, but it needs to be used responsibly.

    • Retr0id

      What's the use case? I've never found myself wishing my transaction wasn't actually atomic.

    • p-e-w

      The option to make a transaction not be a transaction? What is it then?

magicalhippo

I found the article very hard to follow. Then I read the domain name and it all made sense...

Guess our pattern is different, not really felt the pain point the author is talking about. Or it's the language/framework, not sure whatever the example code is written in, but setting properties on entity records would never update the database in the ones I've used.

When we did things more manually we'd make sure that methods like `create_main_records` would start a transaction if not in one, and only commit if it started the transaction. This way we could nest without worry. Our database supported nested transactions but using this pattern we didn't feel the need.

  • nine_k

    Tired: pass live DB model objects around, mutate them to accumulate random changes, call commit() five levels deep, experience the chaos as described by the author.

    Wired: fetch DB records and construct domain objects, pass them around, do mutation on the outermost level within an explicit transaction. (Typical best practice.)

    Inspired: "functional core, imperative shell".

  • andreareina

    This is the Active Record pattern, I used to hear about it all the time. Don't know if the zeitgeist has changed or if I'm just around different parts of the internet these days.

hparadiz

Feel like I've solved this non problem like 20 times.

If your framework is holding your records in a collection and you keep track of phantoms that have been edited but not saved as they are edited then all this becomes a non issue. That way you do Collection->SaveTransaction or Collection->Save() and the collection does all the db commit, roll back nonsense.

Anyway the examples I don't even know if I could call that an ORM. Use a better one.

eqvinox

> # calls helpers that eventually call commit()

Show, don't tell. The article should've included an example of these helpers.

All in all, this feels poorly written, but also poorly thought out. The DB layer can and should throw an exception if you manually begin/commit inside a context manager. Please do blame it if it doesn't.

quamserena

This is trivially preventable via affine types

khaledh

If someone can trivially introduce bad state (or worse, data loss), then the API is clearly broken.

Keyboard Shortcuts

j
Next item
k
Previous item
o / Enter
Open selected item
?
Show this help
Esc
Close modal / clear selection