23 ms·

A routine gem update ended up creating $73k worth of subscriptions

• dudeinjapan 5y ago
  I have raised a ticket for the Mongoid team to read the article and HN comments here: https://jira.mongodb.org/browse/MONGOID-5218 https://jira.mongodb.org/browse/MONGOID-5218
• lenkite 5y ago
  The biggest problem is not the lack of a Unit test - which likely wouldn't have caught this problem anyways - but the fact they are using a "joke database". Mongodb is good for your high-school app or prototype demos not for production SaaS that debits credit cards.
• fiddlerwoaroof 5y ago
  This sort of behavior change in a dependency would make me blacklist the dependency.
  • x3n0ph3n3 5y ago
    I shun everything MongoDB because of how immature its engineering culture is.
    • gqewogpdqa 5y ago
      Can you explain?
      • deleted 5y ago
        [deleted]
      • Beltiras 5y ago
        I can't explain for the other user but I'll tack on my own little horror story. I once got the task of porting something written for early nodejs that used MongoDB as a backend to Django. The app had been developed by a contractor and had many iterations of the data layer. I had to somehow make sense of how to make that all fit in a relational database. The changes made were not documented at all and I didn't have time to go digging through the git repo for some history. It was not a particularly fun task. Maybe it's a culture thing. I have this oldschool thing about data meeting invariant criteria and I dislike not using a RDBMS.
        • jacquesm 5y ago
          RDBMS are the solution to many of the problems that these 'modern' systems have, but it isn't cool. Someone joked that the wave of the future will be SQL, and that those start-ups that use it will believe that they have superpowers over the ones that don't. Transactional integrity, complex queries, constraints, what's not to like?
          • jjice 5y ago
            It's funny how true that is. Every day I write SQL I'm grateful that I'm on a relational database and not on some hot new nosql system that is both over and underkill for the work I do.
            • ineedasername 5y ago
              For a while learning my way around a nosql database was on my to-do for professional development list. I won't claim there aren't valid use cases, but once I got around to spending a few hours poking around I quickly realized it would not make my life any easier.
          • GreenWatermelon 5y ago
            This video should speak to you https://youtu.be/b2F-DItXtZs https://youtu.be/b2F-DItXtZs
            • Beltiras 5y ago
              I could find a single sentence from the noob that I could agree with. Redis is superior to memcached. Not due to being faster or scalable, I've just had less operational problems with it than memcached. It's on my list of supersoftwares that are first choices to solve a problem in it's domain (nginx, uwsgi, postgresql, redis).
        • matwood 5y ago
          > I have this oldschool thing about data meeting invariant criteria and I dislike not using a RDBMS. Same. As another 'old school' person, I've been around long enough to see that data outlives whatever the current application is of the day. This means that storing data in something that is standard, has good tooling, and can have some guarantees ends up critical.
      • radicalbyte 5y ago
        MongoDb are infamous for lying about their capabilities. Just search hackernews and you'll read all of the horror stories.
      • x3n0ph3n3 5y ago
        What others have said + I have not forgotten about "webscale."
      • btown 5y ago
        Setting aside the fact that the document storage model is rarely a good fit for most applications (and the primary reason I had once been a fan of MongoDB, Meteor, has gone the way of the dodo), the Jepsen distributed system audits of MongoDB's behavior under load were damning for much of the past decade. They've improved in recent years as shown below, but this remains a company that allowed these types of bugs to be considered acceptable, and it's still easy to use MongoDB in an unsafe way. 2013: https://aphyr.com/posts/284-jepsen-mongodb https://aphyr.com/posts/284-jepsen-mongodb > To recap: MongoDB is neither AP nor CP. The defaults can cause significant loss of acknowledged writes. The strongest consistency offered has bugs which cause false acknowledgements, and even if they’re fixed, doesn’t prevent false failures. [N.B. I thought this was just an edge case until it happened to me in production. Luckily, I had separate storage of ground truth, but I very easily could have been in real trouble.] 2015: https://aphyr.com/posts/322-jepsen-mongodb-stale-reads https://aphyr.com/posts/322-jepsen-mongodb-stale-reads > In this post, we’ll see that Mongo’s consistency model is broken by design: not only can “strictly consistent” reads see stale versions of documents, but they can also return garbage data from writes that never should have occurred. The former is (as far as I know) a new result which runs contrary to all of Mongo’s consistency documentation. The latter has been a documented issue in Mongo for some time. We’ll also touch on a result from the previous Jepsen post: almost all write concern levels allow data loss. 2017: https://jepsen.io/analyses/mongodb-3-4-0-rc3 https://jepsen.io/analyses/mongodb-3-4-0-rc3 > MongoDB has devoted significant resources to improved safety in the past two years, and much of that ground-work is paying off in 3.2 and 3.4. Dirty reads, which we covered in the last Jepsen post, can now be avoided by using the WiredTiger storage engine and selecting majority read concern. Because dirty reads can be written back to the database in read-modify-update cycles, potentially causing the loss of committed writes, users of ODMs and other data mappers should take particular care to use majority reads unless making careful use of findAndModify.
    • GreenWatermelon 5y ago
      But, is <everything else> web scale?
  • cinntaile 5y ago
    They fixed the issue, reversed everything, wrote a detailed explanation of what happened and apologized to their users. I don't think you can expect much better than this to be honest?
    • fiddlerwoaroof 5y ago
      I’m not blaming them: I’m saying that if I was in this position, I’d ban the Mongo library used.
      • cinntaile 5y ago
        Oh ok, I thought you meant if you were using serdapi as a customer.
    • x3n0ph3n3 5y ago
      I believe the dependency OP is referring to is mongoid. I think serpapi did all the right things here.
• warpech 5y ago
  Mongoid docs[1] seem to be pretty cool about this change: "As of Mongoid 7.1, logical operators (and, or, nor and not) have been changed to have the the same semantics as those of ActiveRecord. To obtain the semantics of or as it behaved in Mongoid 7.0 and earlier, use any_of which is described below." Is it just me or is this one of the most terrible breaking changes in a popular, official library ever? [1] https://docs.mongodb.com/mongoid/current/tutorials/mongoid-queries/ https://docs.mongodb.com/mongoid/current/tutorials/mongoid-q...
  • warpech 5y ago
    The SerpAPI blog author seems cool about it, too. After such a problem, I would roll back and never ever update this dependency again.
    • munificent 5y ago
      > seems cool about it, too. How people blog and how they feel aren't always the same thing. Professionals tend to be a lot more tactful in public communication.
      • hyperhopper 5y ago
        I would be fuming if I were an engineer depending on that, but obviously in a public communication like that, where you are explaining to clients why you charged them erroneously, just pointing a finger and acting mad makes you look like you don't have control of your own software.
        • Aeolun 5y ago
          A little bit of passive aggressive comments would have been appropriate I think.
    • kortex 5y ago
      Yep, that gets a pin to "==x.y.z # pinned to prevent BREAKING CHANGE, do not update without reading <link>" and also some defensive regression tests on their api lest someone update by mistake.
• ricardobeat 5y ago
  That sounds like a major, incredibly dangerous update to the DB driver. Their 7.1, 7.2, 7.3 versions seem to all have breaking changes [1]. Yet they are in obvious violation of SemVer expectations, which they declare to follow [2]: > Mongoid follows versioning guidelines as outlined by the Semantic Versioning Specification, so you can expect only backwards incompatible changes in major versions [sic] [1] https://docs.mongodb.com/mongoid/current/tutorials/mongoid-upgrade/#logical-operations https://docs.mongodb.com/mongoid/current/tutorials/mongoid-u... [2] https://mongoid.github.io/old/en/mongoid/docs/upgrading.html https://mongoid.github.io/old/en/mongoid/docs/upgrading.html
  • BlackFingolfin 5y ago
    To be fair, the your second link points to the documentation of an ancient version; the new docs don't seem to mention semver (I still don't think what they are doing is reasonable, but ...)
    • ricardobeat 5y ago
      That link is reachable from their home page right now, under “Upgrading”. EDIT: just noticed the docs I found via Google are marked as “old” in the URL: https://mongoid.github.io/old/en/mongoid/ https://mongoid.github.io/old/en/mongoid/ Are you suggesting they dropped semver in between these releases?
      • rsynnott 5y ago
        > Are you suggesting they dropped semver in between these releases? If this is the case, I kind of hope, for maximum irony, that they dropped it as part of a minor version bump.
        • neya 5y ago
          Haha, this was the best thing I read today :)))
  • dragonwriter 5y ago
• deleted 5y ago
  [deleted]
• recursivedoubts 5y ago
  this has to be a major mistake on the part of the library developers it is completely unacceptable to change semantics like this in a post 1.0 minor version
  • lmeyerov 5y ago
    Breaking changes are things, no project is ever done -- ye olde django is still doing big moves like adding async colors, and that's great! But... CHANGELOG.md with BREAKING CHANGE sections and a major version bump, maybe a DEPRECATED lint and runtime warn() ahead of time, and bam, no surprises for professional teams. For more than that, service contracts are things :)
    • asdfasgasdgasdg 5y ago
      I don't think you can ever make a breaking change like the one described. There's just no way to audit the correctness of all your users after that change. You need to leave the old thing with the old behavior and only add the new behavior to a new thing.
      • booleanbetrayal 5y ago
        Rails changed the meaning of NOT and nobody batted an eye - https://til.hashrocket.com/posts/3zyftipjiu-rails-will-change-the-not-behavior https://til.hashrocket.com/posts/3zyftipjiu-rails-will-chang... These sorts of changes do in fact happen fairly frequently and developers need to be aware that they can't blindly accept upstream dependency changes.
        • asdfasgasdgasdg 5y ago
          That also is a really awful change and probably broke someone in production. Really there is no reason for the not function to take more than one argument. But if you have decided to overload the meaning to NAND, you'd better not later change it to NOR!
        • kortex 5y ago
• tbrock 5y ago
  nit: Mongoid is not the MongoDB ruby driver. Mongoid is an Object::Document mapper for Ruby (which makes use of that driver). Nevertheless this should have warranted a mongoid major version bump because it's backward breaking.
• mushufasa 5y ago
  Of note -- their company services seem to be that you pay them to return you Google search results via API. Isn't that against the Google search Terms of Service??? If google wanted there to be a paid search api, I'm pretty sure they would just provide one. Also, I'm pretty sure I've seen these types of startups before, and then they vanish quickly thereafter....
  • josefx 5y ago
    What do you even call it when the biggest web scraper of the world prohibits web scraping in its terms of service?
    • victor9000 5y ago
      Hypocrisy that is borderline unethical.
  • tssge 5y ago
    How are you sure they ever agreed to Google Terms of Service?
  • easrng 5y ago
    > If google wanted there to be a paid search api, I'm pretty sure they would just provide one. They do[0]. It's not as complete though. [0]: https://developers.google.com/custom-search/v1/introduction https://developers.google.com/custom-search/v1/introduction
    • JimDabell 5y ago
      That’s not really the same thing. It’s intended to search a manually-specified site or collection of sites, not the whole web. It’s basically a Google Site Search API, not a Google Search API.
      • easrng 5y ago
        You can use it for web search though.
• deleted 5y ago
  [deleted]
• noneeeed 5y ago
  We use Mongoid. I treat every upgrade, no matter how small as its own piece of work involving going through the changelogs with a fine-tooth comb. This particular issue got flagged up when we last looked at upgrading. Currently Mongoid upgrades are stalled until we have the time to figure out the impact of some of their more egregious changes. I have a generally low trust approach to all dependency upgrades, irrespective of how minor they are, I've been bitten by enough "minor" changes. But I treat Mongoid as an active adversary who is setting out to break things with every update. It's the complete opposite approach to that of the Rails core team and those that work on ActiveRecord (as the nearest equivalent to Mongoid). Changes to Rails core are always well flagged with really good advance warning of upcoming breaking changes, frequently with deprecation warnings several versions in advance. There just seems to be a core culture of thought and care towards their users.
  • jacquesm 5y ago
    This is part of the TCO of any major software project, kudos for getting it right, that's a very mature process you have there.
  • nefitty 5y ago
    I feel spoiled having most of my experience in js, react and node. They like, try really hard not to totally break shit.
    • fouc 5y ago
      You would feel spoiled if you were a ruby developer too. This type of library API breaking change on a minor version update basically never happens. And if it can happen in ruby land, it can happen in JS land too.
      • weaksauce 5y ago
        i mean the whole lpad debacle was pretty egregious and that's directly in the javascript land
      • deleted 5y ago
• jacquesm 5y ago
  If every breaking change would lead to more income we'd be doing so well. What on Earth were they thinking, to change the default behavior for something that is in production use for so long. This was monumentally stupid, even if it is a windfall for the OP, it could have easily gone the other way. And props for figuring out what it was, likely there are some other people scratching their heads about this.
  • klyrs 5y ago
    > After refunding everyone, we manually double checked the billing state of each account one by one and sent emails to apologize to each customer one by one; all 475 of them.
    • jacquesm 5y ago
      Yes, now imagine the other way around. How are you going to tell your users that you screwed up and will need to invoice them either again, or much closer to their next charge. Or maybe much later still if you don't spot the error right away. Note that my comment isn't about the OP, but about the root cause of the breaking change: the driver.
      • klyrs 5y ago
        Well, you described it as a "windfall for the OP" which really isn't accurate. Accidentally charging customers is a nightmare, and payment processors (rightfully) see large numbers of chargebacks as a red flag which can result in frozen funds / locked account. I'm not sure that the gem authors are benefitting from this either; it's open-source and they've probably lost users as a direct result of this article.
        • jacquesm 5y ago
          See the title of the article. That they ended up returning it is good but either the amount was meant to have some kind of significance and before they returned it they received it. I'm well aware of the effects of various kinds of risks to merchants.
• denotational 5y ago
  > Our app for some reasons was creating new subscriptions from old accounts that was canceled or disabled a long time ago. If I cancel or disable my account for a service I don't expect them to be able to charge me money in the first place! Are they keeping card authorisations (or direct debit mandates, or whatever other mechanism) for customers that don't even have an account with them any more? That sounds like an... ...interesting way to do business, but perhaps I'm misinterpreting this.
  • avianlyric 5y ago
    They don’t need to keep anything. The card details will be stored with Stripe, if they don’t actively tell Stripe to delete those details then they’ll be able to create a new subscription and charge the card. For the vast majority of cases all you need to charge a card is the 16 digit number on the front and an expiry date. Pretty much everything else is optional (CVV, Name, Address etc), but opens you up to stupid levels of liability in the disputes process if you didn’t include it in the original payment request.
    • sbierwagen 5y ago
      Card issuer risk model heuristics will also apply different amounts of scrutiny based on transaction size and type. You could do a two dollar transaction just with card number, I wouldn't expect a twenty thousand dollar transaction to go through without the other fields.
      • ineedasername 5y ago
        Yes-- at my local suoermarket a small purchase will just go through, while a larger one (~$25 is the threshold) requires a signature.
        • sofixa 5y ago
          A signature? I'm sorry, are you from the past? Joking aside, I don't see how a signature is of any use provides your card is supposed to have yours on the back, so anyone having your card has your signature too. PINs have been a thing since I've had a card (2011), and I don't see why anyone still relies on signatures for card authentication. The way things work in developed countries is the following - payments under 30,50,100 euros (depending on the country) are contactless where you just tap your card on top and it's done, and for more you have to insert the card and type your PIN. Or you can just use your phone for any amount by tapping it.
• rubyfan 5y ago
  This reminds me of that bit from Family Guy Peter: since when did they change the meaning of for to from? https://m.youtube.com/watch?v=B12joBZ9-6s https://m.youtube.com/watch?v=B12joBZ9-6s
• yholio 5y ago
  Here's how true professionals would handle this: "We recently became aware of some erroneous subscription renewals made by our platform and traced the root cause to a major bug in downstream database technology affecting a very small number of accounts. Nonetheless, we working hard with our database provider to resolve the issue. In the meantime, if you are affected and believe you might have an unsolicited subscription, please contact our billing department by fax at 212-345...."
  • jrootabega 5y ago
    "Heh heh, by the time those suckers realize we didn't give them the last 4 digits of the phone number, we'll be on a plane to Belize!"
  • aaronbrethorst 5y ago
    I thought you were being serious until I got to this bit: "please contact our billing department by fax"
  • handoflixue 5y ago
    Aside from being more modern than to suggest a fax, that seems to be exactly what they did?
    • partido3619463 5y ago
      No they refunded everyone immediately.
      • fouc 5y ago
        The sheer unprofessionalism is astonishing. They're gonna put the whole business community to shame.
  • kingcharles 5y ago
    Actually had this recently when helping out a friend who was targeted by a shady debt collection company. The only way their lawyer would speak to us is through fax, in order to try to act as a barrier to anyone actually trying to dispute their bullshit.
• seanwilson 5y ago
  > SerpAPI: Scrape Google and other search engines from our fast, easy, and complete API. Does anyone know how this works behind the scenes at scale? How do they get around Google trying to stop their scraping attempts? Just lots of proxies and some way around captcha challenges?
  • gruez 5y ago
    probably a combination of realistic-seeming desktop browsers (eg. headless chrome with stealth patches) and residential IP providers (eg. luminati)
    • kingcharles 5y ago
      Yep. Has to be using a large net of residential addresses in each country to not get banned by Google.
    • forgot_old_user 5y ago
      Their website says >In addition, each API request runs in a full browser, and we'll even solve all CAPTCHAs. Mimicking completely what a human will do. Wow how would they do that?
      • bytebln 5y ago
        Not sure, but maybe they use a CAPTCHA-Solving-API like https://anti-captcha.com https://anti-captcha.com
      • seanwilson 5y ago
        Is this not against Google's terms of service? Could they not make legal threats? And ban the company from using all Google products like Gmail and ads? I know there's companies doing similar things and I'm not saying they should get in trouble, but it feels so risky basing a business around it, unless I'm missing something. Lots of companies seem to do similar scraping to get SEO data for example that Google probably has an interest in preventing.
• raman162 5y ago
  This horror story is more than enough to keep me away form ever touching mongoid. Bookmarking this for future reference.
• draw_down 5y ago
• raman162 5y ago
  Serpapi definitely did the right thing here. Shame on the maintainers for introducing such a breaking change in a minor version update. I guess the takeaways are: - review change logs for any gems that are updated - have extensive test cases for anything that charges customers Hindsight is always 20/20 of course.
  • booleanbetrayal 5y ago
    The should have read over changelogs. What production system just accepts changes blindly without internalizing them?
    • unionpivo 5y ago
      most of them in the real world. And I mean like 98% or more. I have worked in IT for over 20 years, and you actually have to fight clients managers to get time to do it. I work in healthcare. I wish I had problem like that, instead, I just today had to fix something that was running on unpatched ubuntu 14. I know that there are servers running unpatched log4j where people are in "talks" who will pay for "upgrade" etc.
    • booleanbetrayal 5y ago
• karmicthreat 5y ago
  As things get more complicated (and they already are) it would be nice if packages included code analysis tools. Ones that will just search your code, find usage of particular breaking feature change and not go quiet until you acknowledge the change. Might obviate the update and pray nature of SemVer. You should still have comprehensive test though.
• booleanbetrayal 5y ago
  So ultimately this is the result of someone not actually reading through the changelog notes from over a year+ of iteration? I guess you can be annoyed with a third-party's abuse of SemVer, but practically everybody abuses those conventions and any developer who is versed in dependency management should have known better than to not read over changelogs. Likewise, any reviewer should have caught this.
  • cirrus3 5y ago
    It is... but it is also a result of a library changing the meaning of "or". This is next-level breaking change that is beyond what I'd even call a "change".
    • booleanbetrayal 5y ago
      It was fully documented and ActiveRecord has done similar things in the past. See also: https://til.hashrocket.com/posts/3zyftipjiu-rails-will-change-the-not-behavior https://til.hashrocket.com/posts/3zyftipjiu-rails-will-chang...
      • kevin_thibedeau 5y ago
        ... on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying "Beware of the Leopard".
• iamtheworstdev 5y ago
  Do. Not. Break. Userspace.
• cirrus3 5y ago
  > In Mongoid (The MongoDB driver for Ruby) 7.0.8 , or() meant filter documents that contain any of the argument conditions. > In Mongoid 7.3.3 , or() now means filter documents that contain any of the argument conditions OR any of previous method conditions It actually sounds like they "fixed" it to work the way you'd expect... but changing an API like this one in this way is extremely dangerous. I wouldn't call what you have a code smell, you coded it correctly according to the Mongoid API at the time. Mongoid is at fault here. An API that changes the behavior of a function are core as "or()" in this way is pure insanity - even if they were trying to make it do what it really should have in the first place, or if it were a major version, and they documented it well. Note to self - avoid Mongoid.
• cirrus3 5y ago
  Breaking charges are thing sometimes a thing... but do not change "or()"! You may as well just have swapped the behavior of "+" and "-", this is crazy.
• xyst 5y ago
  That's an expensive mistake. Isn't the business charged some small fee for every refund? It's not as bad as a chargeback, but I know it can't be free.
  • mchusma 5y ago
    Stripe charges the same for reminds as regular charges, so approximately 3%.
    • nightpool 5y ago
      Actually, refunds are free, but Stripe keeps the original fee for payment processing and covers it out of that. So you need to pay the transaction fee for the original purchase to make the customer whole. See https://support.stripe.com/questions/understanding-fees-for-refunded-payments https://support.stripe.com/questions/understanding-fees-for-.... In this case, since we know from the article that there are 474 charges, and a total of $73k processed, 474 * $0.30 + $73k * 0.029 comes out to $2,267 that the company owes Stripe
      • hartator 5y ago
        I think we paid around $2.7k in fees because of foreign cards. We also lost an additional $30k that month as we've refunded fully everyone including active customers that was bound to renew for real but another day.
• userbinator 5y ago
  I wish more developers would abide by the old saying "don't fix it if it ain't broke." It's one thing to update because you know a newer version has fixed a bug you're experiencing, but if everything is already working as you'd expect, IMHO you're just asking for trouble. There's a reason a lot of the infrastructure systems that many people don't even know about --- until something breaks --- hasn't changed in literally decades. (Of course, "everything is working" might not be true, in which case you could be trading one bug for some others. But the sad state of "modern" software development is a rant I won't go into here...)
  • loktarogar 5y ago
    yes that's true, but the bigger the set of changes between your application and the current version of dependencies, the more likely you'll have a serious issue if you are eventually forced to upgrade the best change is no change, the second best change is a small change
  • nmstoker 5y ago
    I agree on one hand, but on the other if you take this approach, eventually the number of changes will be enormous and the cause of any issue that crops up may be that much harder to pin point. If you keep somewhat up to date, it's smaller challenges each time. I suppose if they'd done the update into a test environment like a regular release then it's far more likely these issues would've come out there and it wouldn't have been so stressful.
  • booleanbetrayal 5y ago
    You are asking for security issues if you take this approach. Better to increment on several small iterations of dependencies, making changes accordingly, such that you will be able to take a CVE-fixing patch without having to refactor your entire app all at once.
    • kortex 5y ago
      Introducing changes to the semantics of query operators in minor releases sounds like a great way to introduce security issues. How many CVEs over the years are because of someone screwing up their operator precedence in permissions checks?
• andrew_ 5y ago
  The mongoid repository (https://github.com/mongodb/mongoid https://github.com/mongodb/mongoid) doesn't have issues turned on and instead points people to JIRA. That's one way to avoid users reporting issues like this - I'd see JIRA and "nope" right out.
  • ineedasername 5y ago
    Is JIRA that bad even for just logging an issue? Or do you nope out of the dependency?
    • plorkyeran 5y ago
      At the minimum it's another account you have to register. The more Jira-specific problem is that making drive-by issue reporting frictionless is an extremely low priority for Jira (or for the people who choose to use Jira), and there's often going to be a bunch of confusing steps required. It also just signals that they aren't really that interested in hearing from users. People who crave feedback and bug reports go to where the users are rather than making the users come to them. Even if they use Jira internally, they'll do things like monitor stack overflow and provide support on Github.
    • house9-2 5y ago
      It really is "that bad". JIRA is not made for humans.
  • zionic 5y ago
    My favorite example is Zendesk. They’re a customer support company with all their GitHub issues turned off, forcing you to use their support system. Of course, if you email them telling them they broke their library (for the 3rd time this year) and forget to email them on your account holder email they’ll respond that they only take requests from the designated email, and say goodbye. Even when you use the right email you’re still playing telephone between some poor support person and a dev. Absolute trash-tier company, I have a backlog item to completely scrap their SDK from my apps I just haven’t gotten around to it.
• bijoo 5y ago
  nice writeup - adding a test should also help increase confidence here
• rubyist5eva 5y ago
  Semantic Versioning is bullshit. Even "backward compatible features or bug-only fixes" can completely break your application. With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody. -- Hyrum's Law
  • kortex 5y ago
    That's not really the point. There's a gulf of difference between "we patched a CVE and now spacebar-heating is broken" and "we knowingly and deliberately changed the semantics of a query operator in a minor/patch release in a 7.x library in wide production with 3,900 stars on gitub." Semver doesn't mean "guarantee no breaking changes in minor" - that's impossible - and people love to point this out for whatever reason. It totally misses the point. I think most engineers have a decent intuition most of the time about whether a change is breaking or not.
    • rubyist5eva 5y ago
      The amount of time I had to tell engineers across multiple companies that one cannot just blindly bundle update all of the minor/patch versions of our rubygems without properly testing everything begs to differ. "It's semver bro" No.
• compiler-guy 5y ago
  This change gives me new respect for Denis Ritchie's decision not to mess with a wart in operator precedence in C. Even though we still live with this wart 30 years later. It was exactly this type of failure he sought to prevent. He refused to fix it because, "After all, we had several hundred kilobytes of source code, and maybe 3 installations...." https://www.lysator.liu.se/c/dmr-on-or.html https://www.lysator.liu.se/c/dmr-on-or.html
  • potamic 5y ago
    Add Torvalds to the list too - SHUT THE FUCK UP. WE DO NOT BREAK USERSPACE! https://lkml.org/lkml/2012/12/23/75 https://lkml.org/lkml/2012/12/23/75 This style is indeed controversial but honestly, these kind of situations seem apt for such a chewing out. Backwards compatibility seems to be one of those things that people regularly compromise despite it repeatedly hitting back. For a database driver of one of the most popular databases in the world, it should not be taken casually. I wish more tools adopted the first rule of kernel maintenance - "If a change results in user programs breaking, it's a bug in the library. Never EVER blame user programs." (pp)
    • cesaref 5y ago
      Absolutely. Breaking changes, major version update, bug fixes, minor version update, and think really hard about the pain the major version update will cause. It seems so easy... The problem comes when applications depend on a bug. The library maintainers don't necessarily know about this, but it's handy to think further ahead and understand what the effect of an update is. That's why one line fixes take time - thinking through the implications. Perl 5 vs 6, Python 2 vs 3 are good examples of breaking major changes to languages, and the various fallout that happens, both good and bad. It's amazing how far c++ has come without such a major bifurcation
  • CamperBob2 5y ago
    It's interesting that we have procedural languages that enforce strict whitespace conventions, ostensibly to improve readability and eliminate common sources of errors, but none that require the use of parentheses in complex expressions. When someone I work with writes something like that example: if (a==b & c==d) ... I tend to (want to) lose it. I have decades of continuous C/C++ experience and I have no earthly idea what the relative precedence of & and == is. Don't know, don't care. Use parentheses whenever it looks even remotely like they'll help clarify the expression. They're free. (Floating-point precision shenanigans aside.)
• mchusma 5y ago
  Stripe also bears some responsibility here, as they don't support production testing, so it's impossible to have a test suite checking for charge related behaviors in production. If you use stripe, please contact them and request this long overdue feature. (I do not think that is the primary issue, but it does not help)
  • mickotron 5y ago
    Why not test with a stub? Issue would have been picked up in testing even without Stripe integration.
    • caspar 5y ago
      Exactly, https://github.com/thoughtbot/fake_stripe https://github.com/thoughtbot/fake_stripe works well enough for this sort of thing.
    • rajin444 5y ago
      Yeah, the gem library changes are pretty brutal but this 100% should have been caught by a test.
  • nightpool 5y ago
    For more context, what exactly do you mean by "production testing" and how do you think it would have fixed this issue? I spent 9 months developing a moderately complex Stripe integration, with thorough automated tests, and I never ran into any Stripe issues that I really would have considered a showstopper in terms of testing. Stripe's test environment setup was super super helpful throughout. But this breaking change is on another level of subtle, and it impacts their entire application at a very basic level, but in a way that would be exceedingly hard to detect in almost every case. Frankly, I don't think any reasonable level of testing would have been thorough enough to catch such a subtle issue.
    • brian_cloutier 5y ago
      I'm not sure it's that subtle. In the screenshot it looks like a random user was charged each time the method was called. A test for "run the query, notice the user was charged" would have failed. Maybe they got very unlucky and the test randomly picked the correct user, or maybe they ran the test against a database which contained just a single user, but that last part is within their control. > I don't think any reasonable level of testing would have been thorough enough to catch such a subtle issue. If there was a staging environment which tried to match production as closely as possible, and end to end tests of this feature were run, then this bug would have been caught. That doesn't seem like an unreasonable level of care for something as sensitive as billing.
• mickotron 5y ago
  And what kind of testing was done prior to Prod deployment?
• jbverschoor 5y ago
  Besides the issue, I always create a method like get/find find_one, find_unique, find_unique, or find_only, in cases where I expect exactly one result. I’m a query or collection. I’ll have a separate for when I expect one or zero results. Unfortunately it’s crazy difficult to even get such a small addition committed upstream, but that’s a different story
  • nightpool 5y ago
    I think you may have slightly misunderstood the issue—the problem isn't that the method was returning too many accounts, the problem was that the method was returning an account unrelated to the one whose subscription had expired. Therefore, subsequent calls by the same expired user would renew for more and more random accounts—but only one random account per call.
    • jbverschoor 5y ago
      Yeah I didn't look into it.. But reading further, it still would've prevented this bug. If you expect 1 or 0 results, just make sure it does. The query here returned where id = xxx or renew_locked_at >= xxx or renew_locked_at is null. That will return more than 1 result. If find_one actually did what the name says it does, find 1. Not 0, not 2, not 475. This issue would've never existed. Both mongoid and activerecord (don't remember if this was the case with hibernate / jpa) will not throw an exception if there are more than one records. They will check for zero results, although it would just result in an NPE if they didn't.. Besides this, I'd have made it more explicit by doing something like: where(xxx, or(yyy, yyy)). This is still an implicit 'and', but chaining always seems confusing to me, so I never use it like that. Mongoid shouldn't have done such a change, even with a major upgrade. At least not in this way. IMO, both the author and mongoid set themselves up for issues. I like to throw around exceptions and asserts. I like to fail fast. Strictness is easy to handle for a programmer. Only external human input should/could be handled less strict. Unfortunately, many disagree.. It's different with other types of software (client side apps), but for applications that have no state, or a state that can easily be recovered (webapps), it seems plain stupid not to fail fast whenever something's wrong
• whalesalad 5y ago
  Whenever I am writing complex/transactional routines that can have grave consequences such as this (billing logic) I try to do a few things: 1. Build it out as a standalone operation/class/module so it has more ceremony around it, while also being limited in scope and easy to audit. 2. Continue to utilize the DB/ORM's filtering/querying to grab data as was done in this case 3. Additionally, when it comes down to performing the big io/side-effect, ensure additional checks are in place that confirm the data I am working on matches the query that was used to ask for the data. So in each customer loop I might double check that this customer is indeed needing an upgrade/etc. It can be slower, but it is worthwhile.
• LAC-Tech 5y ago
  Breaking change on minor version changes. Ouch. Not a MongoDB expert, but you don't really need an 'ORM'[0] do you? I thought it spoke JSON natively like couch. If I was in this project I might have argued strongly for just doing that. Others might have argued back telling me that we can't possibly send JSON to a thing that expects JSON that's too low level, let's rely on this library by some guy instead. Sorry, flashbacks. Look libraries that do stuff for us are great. But let's make sure they're worth the cost of admission. [0] yes I know it's not really an ORM because it's not a relational store but you know what I mean
  • hartator 5y ago
    I think Mongoid is an official MongoDB lib. But yeah, we should have used the raw Ruby MongoDB client at least for this. This way the code would have been more explicit.
    • LAC-Tech 5y ago
      Right didn't mean to have a go at you specifically. It's very common in software to introduce dependencies at the drop of a hat. I really hope we can move away from it.
• welder 5y ago
  This would never have happened had they used a real database instead of Mongo.
  • dudeinjapan 5y ago
    This comment isn't helpful; this was entirely an ORM-layer API change and had nothing to do with the underlying DB. The exact same bug could have happened in a SQL ORM.
• kurtbuilds 5y ago
  Three easy lessons: 1. Don't use MongoDB. 2. Don't use high level ORMs. Stay (reasonably) close to SQL. And yes, it should be SQL. Almost certainly Postgres. 3. Especially don't use Mongoid.
  • kevingadd 5y ago
    If Postgres feels too heavy, don't worry! Just use SQLite, and swap in Postgres later once you need to.
    • cpursley 5y ago
      I guess this is a Rails thing where people treat the db as a just dumb store.
      • capableweb 5y ago
        I think I've used Rails seriously only once or twice in my professional life but yeah, mostly treat data-stores as just dumb stores in most projects, unless it has some special requirements.
      • guiriduro 5y ago
        Not just Rails, any webdev group without the oversight of a DBA/Architect, and a penchant for simplistic ORM (mis)use plus uncorrected opinions around 'webscale' state persistence.
      • Nextgrid 5y ago
        Do you have any resources you can recommend about using the DB for more than just a dumb datastore? The first thing I'd be concerned about is how do you manage changes? Code is easy to version control, deploy and roll-back. Database state and things such as triggers, stored procedures, etc, less so.
        • cpursley 5y ago
          Here's one example: https://sive.rs/pg https://sive.rs/pg
• antholeole 5y ago
  How did unit tests not catch this?
  • emodendroket 5y ago
    The same answer as always, nobody thought to test this particular interaction.
  • Too 5y ago
    A unit test would likely have mocked the mongoid gem with the old behavior. Integration test on the other hand… Or Mocking one level further down, not sure about ruby but in Python there is a mongomock package which simulates most of the mongo queries in memory, so an ORM on top of raw queries does not need to be mocked. Because it simulates the database rather than just EXCPECT_CALL it’s also invariant to how you chain operations as long as end result is the same.
• ab_testing 5y ago
  I understand that Mongoid introduced a breaking chnages and should have obviously should have made this breaking change as a part of a major release number. However, should'nt the author have tested this in development first and then found out about this as part of his development testing. Any patches that you apply in production should be fully tested before in development, UAT and other instances.
  • nightpool 5y ago
    This is a deeply fundamental library that's used across their entire app. How likely would the author have been to find this subtle of a bug in such a minor part of their application code, if their app was of any appreciable size? Depending on how their testing environment was set up, they might not even have enough database users in their DB during automated testing to notice this bug if it did trigger.
    • mdavidn 5y ago
      A method that bills customers is not a “minor part” of the application. This is where automated integration tests can make the most difference. “It should not call the Stripe API if customer has credits remaining.”
• nightpool 5y ago
  This is my nightmare scenario, and frankly it's why I never update gems regularly unless there's a really compelling reason (a feature I need, or a bugfix, or a security vulnerability) where I can really spend the time to isolate and test the change. Otherwise, it's really just not worth it.
• troysk 5y ago
  What made you choose mongodb in the first place? Looking at your code and service, the data model is more relational. I want to understand the perspective as it never comes up as a solution when I architect most systems. Help me clear my blindspot.
• techie128 5y ago
  I am curious what sort of testing strategy was being employed? Stripe offers pretty good testing / staging environment which is intended to be used for testing. Surely, one would assume renewals / auto renewal functionality should be exercised _before_ code was pushed to prod. Maybe Stripe's staging environment is too much of an overkill. A integration test covering the 'renew_early_protected' method would have flagged this issue as well. Spinning up a Mongo instance and executing queries and asserting their correctness is imperative as I am too paranoid to trust dependency upgrades to maintain strict compatibility. OP, its great that you apologize to your users but your write up does not capture any actions that you would take to avoid recurrence of these issues in the future.
• tdiff 5y ago
  Without trying to defend authors of Mongoid for a breaking change, if you have deployed into prod something you obviously have not tested, you are the only one to blame. To me this post is more about lack of due process in Serpapi itself.
• csours 5y ago
  Code this tightly coupled makes me itchy.
• dudeinjapan 5y ago
  Wow, it's funny. I'm a regular contributor to Mongoid, and just I was thinking what a bad/unnecessary change this "or" thing was. And then I saw this article!
• dasil003 5y ago
  This title reminded me of a bug I experienced many years ago upgrading the Money gem where they introduced the subunit_to_unit ratio (instead of assuming it's 100). This caused our Japanese customers to be charged 100x what they were supposed to.
• jounker 5y ago
  Billing renewal is a major feature. The ultimate problem here is that renewal behavior didn’t have adequate test coverage.
• vmception 5y ago
  That’s an emergency To me this is basically a malicious hack of a dependency under the excuse of conforming it to activerecord I would investigate contributors to this version and the code review and discussion This could be actually malicious as someone could know or suspect that an app is vulnerable to this fundamental change
• kgeist 5y ago
  How would you introduce this breaking change if you were the maintainer? Maybe this: 1. In the first release, introduce a config value which enables the new behavior. 2. In the next release, print a warning or refuse to compile if the config option is not set. 3. In the final release, make the new behavior the default. Plenty of time to adjust your code to the new behavior even if you don't read the changelogs. But it won't work if several releases are skipped during updates.
  • stefano 5y ago
    Fourth option: don't introduce this change at all. It's a debatable stylistic improvement in exchange for a big breaking change that will force your users to go through, update and re-test every single query. Not worth it.
• pictur 5y ago
  package management is a really big security issue nowadays. for example, npm is a really great tool and it's just as insecure. If a package you use is hacked and you haven't made a version lock, anything can happen to your application.
• ramchip 5y ago
  Elixir has a command `mix hex.outdated` that shows every library about to be updated, and links to an auto-generated page with the source diff between the current and latest version for each. It encourages auditing changes before updating. It's a great habit to take to avoid this kind of trouble.
• OmegaPG 5y ago
  This is why you need a staging server and test cases. You only upgrade in the weekends after fully testing. Also, you always need to be extra careful with payment code. We test it multiple times before deploying it. Granted all this wouldn't had helped OP, still you need to test everything before upgrading and deploying.
• sorokod 5y ago
  "We had no idea why but we decided to react proactively as fast as we can..." Proactive would be tests failing before going live. The action described is totally reactive.
• dorianmariefr 5y ago
  Using mongodb Renewing automatically subscriptions Stripe access somehow from gem Hmmmmm
• bmilleare 5y ago
  While the breaking change in a Mongoid minor release is particularly egregious, let's face it - the real failure here is lack of sufficient tests. I would be very surprised if this was the only place they used or() in their app so it should have made their test suite light up like a Christmas tree.
• cpatuzzo 5y ago
  It's one thing to apologise, but there was no talk in the article about measures they're taking to prevent this happening again. I sincerely hope they're scrambling internally to put some safeguards in place. Yes, it's bad a minor version bump changed behaviour, but now they know this can happen, it's irresponsible not to mitigate this risk, e.g. by scrubbing the card details for expired subscriptions from Stripe.
• mattacular 5y ago
  I trust semver about as far as I could throw it. It's obviously prone to human error and more. Yes, the library maintainers have done something irresponsible, but the onus is ultimately on you as the consumer to test updates. At least give the change logs a good thorough read and make sure nothing that sounds dangerous jumps out at you. Reading the semver string difference is not good enough and this breaking change was actually documented. I would almost expect there to be problems jumping up 3 minor versions even.
• yua_mikami 5y ago
  It's such an aptly named database. They probably thought they were being funny when they came up with the name MongoDB but it's just sad when people have constant issues with missing data, breaking changes, security misconfigurations because of stupid defaults etc., the list is endless with MongoDB.
• deleted 5y ago
  [deleted]
• tigerBL00D 5y ago
  Using mongodb for anything but pet projects is like drunk driving. It's fast and fun, but when you eventually crash and burn you get no sympathy. And people do get hurt. I am joking of course :D
• tyingq 5y ago
  Might be worth adding a safeguard on the Stripe side? It looks like you can add arbitrary metadata to customer objects, subscription objects, etc. So that you could keep the customer/subscription marked as "canceled". Then there's something to check before charging them.
• iamleppert 5y ago
  Don’t deploy untested code to production. The fact that it’s a dependency doesn’t excuse a proper code review. It is ironic to me that software teams have code review processes and seem to trust each other less than random people they have never met on the internet. Every new dependency, you need to read the source. Read it completely. Every new change in every dependency needs to be reviewed as well. I’m a big fan of forking all dependencies and using only those forks in my code. Then, do upgrades via standard rebase or pull requests on that fork. If you have dependencies that are too large to do this with your time, you probably should avoid them in the first place.
• phendrenad2 5y ago
  The code is fine, you're publicly chastising yourself over it for no reason. Mongoid had bad API semantics, and you did what you needed to do to make your code work. Then, you got rug pulled by a minor version update when they said "oopsie, we think these semantics make more sense!". There might be an argument to make that your test suite could have caught this, however.
• ilyazub 5y ago
  SerpApi dev here. I've requested, reviewed, tested, and deployed this upgrade of gems. I want to clarify some things. This code was running on staging for three weeks before the deployment to production. There were four application errors on staging that were related to the problem with subscription renewals. I and the author of the pull request have made three mistakes: - haven't carefully read code in all methods from the backtrace of four app errors on a staging environment - upgraded several gems at once - didn't review the changelog of mongoid gem As multiple people commented here, integration tests for renewals should've been caught that bug. We hadn't a test case when all user's subscriptions are checked after the renewal of a specific test user. So the lesson here is to not yolo minor upgrades of dependencies and avoid mistakes above.