[HN Gopher] A routine gem update ended up creating $73k worth of...
___________________________________________________________________
A routine gem update ended up creating $73k worth of subscriptions
Author : hartator
Score : 498 points
Date : 2022-01-07 22:26 UTC (1 days ago)
(HTM) web link (serpapi.com)
(TXT) w3m dump (serpapi.com)
| kespindler wrote:
| 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 wrote:
| If Postgres feels too heavy, don't worry! Just use SQLite, and
| swap in Postgres later once you need to.
| cpursley wrote:
| I guess this is a Rails thing where people treat the db as a
| just dumb store.
| capableweb wrote:
| 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 wrote:
| 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 wrote:
| 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.
| forty wrote:
| I agree with the 3 lesson, but here I find the main problem is
| the lack of test. And weirdly the author doesn't seem to have
| noticed it.
| radicality wrote:
| Could you comment on why you recommend Postgres (vs say mysql)?
| I know mysql well but haven't had much exposure to Postgres.
| sam_lowry_ wrote:
| Postgres implements the SQL standards however awful they are.
|
| MySQL is a bunch of different engines that share a meaningful
| subset of SQL.
|
| Somehow, Postgres got more popular lately, probably because
| it makes migrating from Oracle easier.
| hu3 wrote:
| just a nitpick: MySQL comes with the same default engine
| (InnoDB) for at least 10 years now. It is recommended by
| docs and it is what everyone uses unless they go out of
| their way to install some very specialized storage engine.
|
| In my years of consulting I have yet to see someone not
| using just InnoDB in MySQL.
| f311a wrote:
| Yeah, MySQL is well known for weird behavior when it comes
| to SQL queries that don't follow SQL standards.
| vishnugupta wrote:
| > 2. Don't use high level ORMs.
|
| Seconded. I prefer high level abstractions in general. However
| when it comes to ORMs; I wouldn't go anywhere near them.
| Admittedly there's a sweet spot where you could hand-pick ORM
| libraries such that you hand-code the queries and they only map
| query results to your objects. But to achieve that you need to
| wade through documentations and some undocumented APIs. So
| developers usually go all-in on ORMs. A big mistake.
| 999900000999 wrote:
| Even though I hate SQL with a passion, I agree with this 100%.
|
| Mongo is a nightmare when things get more complex, document
| based DBs don't work well. It's hard to say why, but strange
| things tend to happen
|
| Ether use something like Firebase which manages everything for
| you, I use Firebase extensively for almost all of my side
| projects.
|
| It's basically magic. I'm so hooked I ended up using Firebase
| even though I needed to talk to AWS apis as well. Not fun...
|
| Edit: I would NOT use Firebase for a corporate project, there's
| a very strange feeling of not really being in control
| cpursley wrote:
| Supabase is a good option these days.
| nottorp wrote:
| > 2. Don't use high level ORMs. Stay (reasonably) close to SQL.
| And yes, it should be SQL.
|
| In my experience using any ORM will bite you sooner or later
| and you'll end up bypassing it and writing SQL manually. Last
| case we had were a couple queries that went from double digit
| seconds to instant as we rewrote them natively.
| Nextgrid wrote:
| Both have their place. I agree that for complex queries,
| nothing beats raw SQL. However, for simple, straightforward
| queries fetching data from a single table without JOINs/etc,
| the ORM is generally easier.
| zestyping wrote:
| And another:
|
| 4. Don't design an API that mixes fluent style (methods are
| like infix operators) with conventional style (methods are like
| prefix operators).
|
| Fluent methods should never take multiple operands. It's a
| terrible, horrible, no-good, very bad idea.
| flubflub wrote:
| I think the default should be named prefix operators. Then
| you can have infix operators that are aliases.
|
| This is a problem in Haskell with infix operators being
| directly defined I would argue.
|
| I disagree that infix and prefix should not be mixed.
| Although there is a special hell for people who use + with
| non-commutative operators.
| driverdan wrote:
| TL;DR: Everything has its place, stop implying you should never
| use use something.
|
| > Don't use MongoDB.
|
| Don't use MongoDB for something that should be in a relational
| DB. Use it as the document DB it is. Use the right tool for the
| right job. Mongo and other document DBs have their place.
|
| > Don't use high level ORMs. Stay (reasonably) close to SQL.
| And yes, it should be SQL. Almost certainly Postgres.
|
| Hard disagree. I've used many of the ORMs in different popular
| frameworks. They work well, save time, and are especially good
| for simple queries. They have their place, just as SQL does.
|
| > Especially don't use Mongoid.
|
| Unfortunately it's the go-to Ruby library for mongo. This could
| be an argument for not using mongo with Ruby. It could be an
| argument for creating an alternative. But simply saying don't
| use it isn't practical for many people.
| troysk wrote:
| 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.
| cirrus3 wrote:
| > 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.
| lenkite wrote:
| 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.
| mushufasa wrote:
| 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 wrote:
| What do you even call it when the biggest web scraper of the
| world prohibits web scraping in its terms of service?
| victor9000 wrote:
| Hypocrisy that is borderline unethical.
| tssge wrote:
| How are you sure they ever agreed to Google Terms of Service?
| easrng wrote:
| > 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
| JimDabell wrote:
| 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.
| techie128 wrote:
| 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.
| dorianmariefr wrote:
| Using mongodb
|
| Renewing automatically subscriptions
|
| Stripe access somehow from gem
|
| Hmmmmm
| LAC-Tech wrote:
| 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 wrote:
| 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 wrote:
| 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.
| [deleted]
| compiler-guy wrote:
| 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
| CamperBob2 wrote:
| 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.)
| potamic wrote:
| Add Torvalds to the list too - SHUT THE FUCK UP. WE DO NOT
| BREAK USERSPACE!
|
| 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 wrote:
| 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
| raman162 wrote:
| This horror story is more than enough to keep me away form ever
| touching mongoid.
|
| Bookmarking this for future reference.
| bmilleare wrote:
| 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.
| denotational wrote:
| > 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.
| FanaHOVA wrote:
| The Ruby 1.9 hash syntax was the first red flag honestly... :P
| loktarogar wrote:
| "old" hash syntax is still necessary for things where the key
| isn't a symbol - which was the case for 1 of the hashes
| avianlyric wrote:
| 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.
| MBCook wrote:
| Agreed. I'd go as far as to say they were irresponsible to
| not tell Stripe to delete/deauthorize the tokens.
|
| In fact, why does any customer in their database have ANY
| connection to Stripe after they've canceled? I haven't used
| stripe but I'm guessing there's some sort of customer
| ID/token. If they've canceled why are you retaining that?
|
| So that's two things. Even if the code encountered a bug like
| they did here, it shouldn't of been possible to actually
| trigger a new subscription.
| jahewson wrote:
| Stripe offers a billing product that serves as a source of
| truth for the entire subscription lifecycle. Companies are
| legally required to maintain financial accounts and
| nowadays accountants can even do your reporting using the
| data directly from stripe.
|
| What's happening here is that there's a user who has a card
| on file and a cancelled subscription, and an erroneous new
| subscription is being created under that user.
|
| Whether or not it makes sense to remove the card from the
| user account depends on whether or not that user could
| meaningfully have multiple subscriptions or expect to renew
| in the medium-term future.
| sbierwagen wrote:
| 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 wrote:
| Yes-- at my local suoermarket a small purchase will just go
| through, while a larger one (~$25 is the threshold)
| requires a signature.
| sofixa wrote:
| 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.
| SnowflakeOnIce wrote:
| > A signature? I'm sorry, are you from the past?
|
| No, the US :)
|
| Signatures are still common enough when paying with a
| card in the US, for whatever reasons.
| cpursley wrote:
| It's not a joke, US payment systems are hot garbage (as
| are bank apps). Much if the rest of the world has been
| doing contactless payment for 5 years already.
| Ozzie_osman wrote:
| It's unclear whether by canceled they mean the account was
| canceled or the subscription was canceled.
|
| As a user, if I delete my account, I'd definitely expect my
| payment details gone. However, if I just disable my
| subscription, I'd expect they keep my payment details on file
| in case I reactivate.
| eknkc wrote:
| If I delete an account yes but if I simply unsubscribe from a
| paid service I actually expect them to store any billing info
| in case I want to resubscribe. Should be a single click
| operation.
| mchusma wrote:
| 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)
| nightpool wrote:
| 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 wrote:
| 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 wrote:
| Why not test with a stub? Issue would have been picked up in
| testing even without Stripe integration.
| rajin444 wrote:
| Yeah, the gem library changes are pretty brutal but this 100%
| should have been caught by a test.
| emptysea wrote:
| Why doesn't test mode work for testing?
| cirrus3 wrote:
| 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.
| bijoo wrote:
| nice writeup - adding a test should also help increase confidence
| here
| mattacular wrote:
| 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.
| csours wrote:
| Code this tightly coupled makes me itchy.
| warpech wrote:
| 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-q...
| Aeolun wrote:
| They made a new function for the 'old' behavior... that makes
| it even worse. Just make a new one for the new behavior and
| everyone is happy.
| [deleted]
| vasco wrote:
| I agree though I comend the bravery of those upgrading database
| driver libraries without so much as a glance to the release
| notes before releasing to production.
| zomglings wrote:
| Seriously, why the fuck not just bump to 8.0.0 or whatever?
| mad182 wrote:
| Wow. This is really awful, would be bad idea even for a major
| version. Shipping it in minor version is insane. It's the worst
| kind of breaking change, not just resulting in error or
| exception, but can easily lead to loss or damage of data and
| unexpected behavior without anyone immediately noticing.
| warpech wrote:
| The SerpAPI blog author seems cool about it, too.
|
| After such a problem, I would roll back and never ever update
| this dependency again.
| fshbbdssbbgdd wrote:
| Maybe I'm just projecting, but to me the author seems deeply
| bothered and holding it together to write the most effective
| takedown of the gem's developers.
| munificent wrote:
| _> 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 wrote:
| 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 wrote:
| A little bit of passive aggressive comments would have
| been appropriate I think.
| kortex wrote:
| 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.
| jimbobimbo wrote:
| Change of semantics in a minor version!? And no way to retain
| the semantics by flipping a flag or something!? SMH
| contravariant wrote:
| Yeah we just changed the meaning of 'or', no biggie.
|
| That said I _would_ normally read 'User.where({id:
| id}).or({condition1},{condition2})' as 'User where id=id or
| condition1 or condition2' and not 'User where id=id and
| (condition1 or condition2)'. Though I could probably get used
| to either, after all you've also got languages like Lisp where
| 'or' isn't an infix operator either.
|
| And doing something with _any_ one of the results when you only
| expect one result to exist is just bad practice.
| jve wrote:
| Coming from .NET/LINQ/SQL, I would have assumed the latter.
| But then again, if I'd ever write ruby, I should have
| consulted the docs.
|
| But this kind of behavior change at best should have
| introduced different API.
|
| Crazy to think that someone remotely can alter your query
| ANDs to ORs. That may very well destroy your database data
| and a whole lot of pain to rollback.
| Sebb767 wrote:
| > That said I would normally read '[...]' as '[...]'
|
| The problem is, once you deploy to production you have
| (hopefully) tested this case and rely on the actual
| implementation.
| stefan_ wrote:
| The changelog for 7.1 (https://github.com/mongodb/mongoid/blob/
| master/docs/release-...) even explicitly call this out as a
| breaking change:
|
| _Breaking change: In Mongoid 7.1, when condition methods are
| invoked on a Criteria object, they always add new conditions to
| the existing conditions in the Criteria object. Previously new
| conditions could have replaced existing conditions in some
| circumstances._
|
| Ironically they also have a breaking change in 7.1.1, so they
| just don't give a fuck at all.
| mattigames wrote:
| I mean, they care enough to mention it in the changelog, if
| they cared even less they wouldn't mention it there (not
| condoning any of their behavior, just pointing that out)
| warpech wrote:
| Perhaps before 7.1 Mongoid had a problem that the logical
| operators acted differently to ActiveRecord. But come on, this
| adjustment could have been implemented in another namespace or
| something...
| cedricd wrote:
| It's clearly a breaking change how a core feature of the
| library behaves. This is extremely unprofessional on the part
| of the maintainers. I'd completely lose trust in the gem.
|
| They knew they were making a breaking change, documented it,
| and didn't increment a major version number. That breaks the
| entire point of semver.
|
| Also, this is generating SQL ffs. Like how more nasty of a
| breaking change could you make in terms of potential impact to
| live apps that upgrade? The experience in the post is a perfect
| example of how badly this can go wrong.
| andrewxdiamond wrote:
| All dependency changes are changes. And changes should be
| tested before being deployed.
|
| If you update your dependencies and ship it based on version
| numbers alone, you can't blame the maintainers
| [deleted]
| thrwn_frthr_awy wrote:
| > All dependency changes are changes. And changes should be
| tested before being deployed.
|
| OP did not say changes should not be tested.
|
| > If you update your dependencies and ship it based on
| version numbers alone, you can't blame the maintainers
|
| OP did not if you update your dependencies and ship it
| based on version numbers alone you can blame the
| maintainers.
| Retric wrote:
| Shipping something with zero testing is _always_ crazy.
|
| Even a minor bug fix in a library can expose a critical
| bug in your own code.
| kortex wrote:
| There's a huge difference between "zero testing" and "we
| didn't have this particular test case covered because the
| state space is massive". Lets say you even have "100%
| code coverage", do you really have a unique case for each
| possible condition in that query? Often times chaining
| operators like TFA give "deceptive" coverage results
| because they mark that whole code path as covered,
| without verifying the state space of the operands is
| covered fully. You can sometimes "cheat" coverage e.g. by
| using ternary operator assignment in place of if/else
| (often by mistake).
| [deleted]
| Retric wrote:
| > update your dependencies and ship it based on version
| numbers alone
|
| The above is zero testing. Update your dependencies and
| ship it based on version numbers and integration tests is
| different. In that case as you suggested test coverage
| may easily have missed something, but there's moving fast
| and there's moving blindly and the second is just
| wasteful.
| brian_cloutier wrote:
| In the general case you are right, but in this specific
| case... this test case should have been covered. Even a
| rudimentary integration test would have caught this bug.
|
| It sounds like the user story is:
|
| - A user runs a query
|
| - The user does not have any queries left in their plan
|
| - They are billed.
|
| - When the user runs the query again they are not billed
| a second time.
|
| I'm struggling to imagine how the test would have failed
| to catch this. Maybe it was unit tested but with a
| database containing just one user? Maybe they got very
| unlucky and the correct user was randomly chosen?
| kortex wrote:
| Yeah, they might have just had unit tests with mocked db,
| or there wasn't enough combinatorial variety. It's
| insufficient to test the lack of double billing - you
| have to specifically induce a race condition, assert the
| race occurred, and that the interlock prevented double
| spend. It's tricky. Personally I'd try to engineer around
| race condition entirely, like some scheme with tokens and
| pagination.
|
| I agree, this sort of thing seems like it should be
| extra-well covered, but, y'know, move fast and double
| charge folks.
| lucumo wrote:
| You're demonstrating the opposite point perfectly. That
| test scenario would NOT have catched the issue.
|
| The problem wasn't that requesters that should've gotten
| billed didn't. The problem wasn't even that requesters
| that shouldn't have gotten billed did.[1] The problem was
| that clients OTHER than the requester got billed.
|
| You can, of course, write test cases that check your
| entire database for unintended state changes, but I
| struggle to find that a reasonable amount of effort.
| Especially since you'd have to do that for all code
| paths. That will very quickly cost a large multiple of
| the 73k this bug caused.
|
| The adequate monitoring and quick response they did here
| is probably a very good trade-off. Like it or not,
| production is ALWAYS your last test. Issues are less
| costly if you realise that, than if you don't.
|
| [1] Though for this specific bug, that scenario would've
| failed too, and might've triggered an extra look at the
| code.
| kcartlidge wrote:
| Purely in terms of the comment you're replying to, it
| doesn't matter whether or not the consuming app should have
| had or done more testing. It also doesn't matter whether or
| not they should have checked the documentation or change
| logs for the dependencies.
|
| The isolated point the commenter was making is that a
| dangerous breaking change was introduced without the
| versioning reflecting that fact.
|
| Both these things can be true, and any failure on the part
| of the startup does not remove the failure of the package
| versioning. These things are not contradictory so yes, you
| _can_ blame the maintainers ( _also_ ) as the guilt or
| otherwise of the startup does not alter the original
| versioning fail.
| z3t4 wrote:
| Why update at all if the current version pass all tests
| though?
|
| So in order to update a dependency you must first write a
| test that fail on current version and is green on updated
| version.
| cpatuzzo wrote:
| 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.
| antholeole wrote:
| How did unit tests not catch this?
| emodendroket wrote:
| The same answer as always, nobody thought to test this
| particular interaction.
| Too wrote:
| 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.
| raman162 wrote:
| 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 wrote:
| The should have read over changelogs. What production system
| just accepts changes blindly without internalizing them?
| unionpivo wrote:
| 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 wrote:
| dudeinjapan wrote:
| 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!
| ricardobeat wrote:
| 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-u...
|
| [2] https://mongoid.github.io/old/en/mongoid/docs/upgrading.html
| BlackFingolfin wrote:
| 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 wrote:
| 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/
|
| Are you suggesting they dropped semver in between these
| releases?
| rsynnott wrote:
| > 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 wrote:
| Haha, this was the best thing I read today :)))
| dragonwriter wrote:
| > Mongoid follows versioning guidelines as outlined by the
| Semantic Versioning Specification, so you can expect only
| backwards incompatible changes in major versions
|
| I will note that this reverses the direction of implications.
|
| In SemVer, you should expect breaking changes only in major
| versions. (All version changes with breaking changes should be
| major, but nonbreaking changes can occur in major or minor
| versions.)
|
| This is _not_ the same as expecting only breaking changes in
| major versions. (Which would mean all changes in major versions
| should be breaking, nonbreaking changes cannot occur in major
| versions, and breaking changes might still occur in minor
| versions.) .
| munificent wrote:
| I'm guessing the sentence they wrote is just a consequence of
| English not being the author's primary language. I frequently
| see ESL speakers get adjective and adverb positions wrong in
| ways that unintentially change the meaning.
|
| Even skilled English speakers make mistakes here because
| English is both very permissive about word order, but also
| tends to give different shades of meaning to each other.
| "Only" is a pernicious one. When I got my last book
| copyedited, fixing the location of "only" was one of the most
| common changes.
| jzwinck wrote:
| There is a real parallel between this mistake and the or()
| mistake. As a native English speaker I glossed over both of
| them at the first reading.
| thaumasiotes wrote:
| > When I got my last book copyedited, fixing the location
| of "only" was one of the most common changes.
|
| I remember a puzzle which presented a (fairly long)
| sentence and asked "provide a word that can be correctly
| inserted at any point in this sentence".
|
| The answer was "only". (Of course the meaning would change
| according to where the "only" was placed, but still...
| you'd have a hard time inserting "experience" at every
| position of a sentence.)
| JimDabell wrote:
| In case anybody is wondering, the sentence normally given
| as an example of this is "She told him that she loved
| him". You can put the word "only" anywhere in that
| sentence and each position makes the sentence mean
| something different.
| thaumasiotes wrote:
| As a technicality, while it's possible for "she only told
| him that she loved him" and "she told him only that she
| loved him" to mean different things, it's also possible
| for them to mean the same thing.
|
| (And in fact, the primary meaning of the first sentence
| is the one identical to the meaning of the second - you
| _can_ read it "she only told him that she loved him [and
| she didn't do anything else]", but by default you'll read
| it "she only told him that she loved him [and she didn't
| tell him anything else]".)
|
| The reason for the ambiguity is that the verb is the head
| of the sentence, and so an "only" placed to scope over
| the verb has several different options for where the
| scope "really is".
| philwelch wrote:
| It's kind of like the saying, "all that glitters is not
| gold". Gold doesn't glitter? That makes no sense.
|
| English syntax is not mathematical logic. The song, "I
| Can't Get No Satisfaction" is not a song about someone who
| is forced to receive satisfaction.
| mananaysiempre wrote:
| > English syntax is not mathematical logic
|
| Open any book or paper on semantics (the linguistics
| kind, not the philosophy or the early-20th-century
| pseudoscience kind) and you might be surprised :) It's
| just that the logic is not that straightforward to get
| out, and of course there's semantics (the literal
| meaning, where "Can you pass the salt?" is a question
| about ability) and then there's _pragmatics_ (the thing
| one is actually trying to communicate, where "Can you
| pass the salt?" is a request with certain degrees of
| respect and formality attached to it).
|
| > "I Can't Get No Satisfaction"
|
| This is probably not a very good example as it's just
| (AFAIU) not strictly "standard English" in that it
| exhibits negative concord absent from the standard
| grammar (for dramatic effect, although "We Don't Need No
| Education" is probably a better case). That is to say,
| this is a phrase in a language (which is very similar to
| standard English but not quite the same) where the
| grammar requires dummy negations on complements of
| negative verbs--a purely syntactic thing that doesn't
| even reach the layer of semantics / logic.
|
| But then many other varieties of English do that (I
| remember reading somewhere that the dialect that gave
| rise to the current standard is actually somewhat unusual
| among other dialects in that respect), so does my native
| Russian (and other Slavic languages, and some but not all
| Romance ones), and Japanese actually requires you to
| negate the adjective when using the adverb meaning
| "hardly, not very"[1] while at the same time using some
| double negations as polite _affirmations_ [2].
|
| [1] https://japanese.stackexchange.com/q/18006
|
| [2] https://www.nhk.or.jp/lesson/english/teacher/36.html
| dagmx wrote:
| Somewhat related to your points of music...
|
| There's an episode of "This is Pop!" on Netflix that
| explores why the Swedes write so many pop hits.
|
| One of the arguments presented is that they speak English
| well as a second/third language so they're less focused
| on the lyrics making sense and being grammatically
| correct, and are free to make lyrics that sound like they
| work but are non-sensical on reflection. It's just the
| right amount of separation from the language.
|
| Ever since I saw that, I've been listening closer to a
| lot of the top pop songs over the last decade, and it's
| fascinating how English is so breakable while sounding
| right to our mind, but falls apart under scrutiny.
| (Obviously this is not unique to the swedish writers,
| even people who only speak English do it as well)
| wincy wrote:
| The song "I want it that way" comes to mind, the song
| topped many charts including in the US, and the lyrics
| are very strange upon reflection. What does "I never
| wanna hear you say. I want it that way." mean exactly?
| Apparently the Swedish songwriter barely spoke any
| English at all at the time.
| dagmx wrote:
| That's actually one of the songs that's mentioned, as
| well as Oops I Did It Again by Britney and Hit Me Baby
| One More Time
|
| Even more recently, Shake it Off by Taylor Swift.
|
| Somewhat along the same lines, I was watching the Get
| Back documentary on the Beatles and the number of songs
| they write by effectively scatting and then filling in
| the words that were their big hits is amazing.
|
| Again they're writing to fit the tune, then in filling
| the words based on a general theme. It's very different
| than the songs they write where you can tell someone's
| sat down and written the words first.
| rightbyte wrote:
| >> I never wanna hear you say, "I want it that way" <<
|
| Probably?
| cobaltoxide wrote:
| I guess Shakespeare did write it that way, but I've
| always heard the saying as "Not all that glitters is
| gold." Perhaps he took some poetic license at the expense
| of logical correctness.
| dragonwriter wrote:
| > Shakespeare [...] Perhaps he took some poetic license
|
| That was kind of his thing, when he wasn't just inventing
| words to fit rhyme/meter/mood.
|
| He's not who you would go to if you wanted unambiguous
| documentation of the guarantees you were providing
| downstream users, that wasn't really his thing.
| derefr wrote:
| A lot of the confusion in English can be relieved,
| sometimes, by flipping clauses around. "All that glitters
| / is not / gold" = "gold / is not / all that glitters."
| ay wrote:
| It's curious that a Russian version is much more precise:
|
| "Not everything that glitters is gold". (The truly 1:1
| translation is "not everything is gold that glitters" but
| that word order felt very wrong and I believe the change
| doesn't change the meaning, does it ?)
|
| How does it sound for a native speaker ?
| mannykannot wrote:
| I am a native speaker of English, yet my knowledge of its
| grammar is almost entirely informal. Having said that, it
| seems to me that in the 1:1 translation, the object of
| 'is' is 'gold that glitters', rather than just 'gold',
| and its subject is 'everything' rather than 'everything
| that glitters', and that these are semantic differences,
| as the intended meaning is specifically about things that
| glitter. I hope someone more knowledgable can cite the
| relevant rules here, whether for or against this reading.
|
| To avoid ambiguity (whether actual or merely perceived by
| me), I might say "Just because something glitters, it
| need not be gold", but that is not much of an aphorism!
| egeozcan wrote:
| Reminds me the "any cars that have been lapped by the
| leader will be required to pass the cars on the lead lap
| and the safety car" clause in the Formula 1 racing
| regulations (article 48.12), which got a lot of attention
| after the last race in the season.
|
| Red Bull Racing argued that in that sentence "any" doesn't
| mean "all". As a non-native speaker, this is just mind
| games for me.
|
| More reading:
| https://english.stackexchange.com/questions/580131/does-
| any-...
|
| If you really have a lot of free time, interest in
| regulations, F1 AND the English language, this deep dive
| can also be interesting: https://www.reddit.com/r/F1Technic
| al/comments/rkv633/unpacki...
| BucketsMcG wrote:
| I'm a native English speaker with a degree in
| linguistics, but I didn't appreciate how hard "some/any"
| are for native speakers until a Polish colleague asked me
| to explain it, and I pretty much failed. It's something
| you learn how to do intuitively, but when you try to
| formulate a consistent set of rules explaining why you
| choose one over the other, it turns out to be pretty
| difficult.
| SOLAR_FIELDS wrote:
| A decent amount of English is full of inconsistencies
| like this. Every language has its warts, of course, but
| like you I never really realized how difficult English is
| as a language until asked to explain certain behaviors to
| non-native speakers.
| deviantintegral wrote:
| They may actually mean what they say. It's not uncommon for
| projects to release the last minor version at the same time
| as the next major. For example, Drupal 9.0.0 was functionally
| equivalent to Drupal 8.9.0, except 9.0 had all deprecations
| dropped. New 9.0+ only features didn't show up until 9.1.
| kortex wrote:
| Yeah, that is probably the smart way to do it. Frontload
| all of your necessary new features in 8.9/9.0-dev (so you
| can complete the deprecations), roll over the major and
| remove only the deprecated code paths without introducing
| any new ones.
| lordgroff wrote:
| Yeah, semver is great, except nobody follows it. The amount of
| times I have updated minor or even patch releases of packages
| with things breaking--I have pretty much determined everything
| in a project stays the same unless there's some pressing need.
|
| The worst part is that breakage is often far from obvious, and
| spending good time wondering why everything breaks when it
| shouldn't is really really infuriating and makes me feel like
| the entire world of software is hopelessly broken.
| tshaddox wrote:
| When you say "breaking" in that comment are you talking about
| backwards compatible API changes, or straight up bugs? It
| kinda sounds like you're talking about the latter, which of
| course isn't within the scope of semver.
| yebyen wrote:
| That's actually something that is in the author's purview
| to decide.
|
| (Edit: whoops, I misread you. I thought you were saying
| APIs are not in scope for SemVer. Now I think I realized
| you meant, you can have bugs that break things and that's
| not related to semver.)
|
| Is the semver contract with respect to the end user
| activities or the API? For Flux and Helm for example, the
| APIs are supported with semver code (and the interfaces use
| Kubernetes API versioning)
|
| In Helm, the CLI and user activities are explicitly not
| part of the semver contract as I understand it, only the
| API (at least with respect to experimental features like
| OCI charts for now?)
| sofixa wrote:
| > I have pretty much determined everything in a project stays
| the same unless there's some pressing need
|
| Unless you have a way to stay up to date with the security of
| each and every dependency you have, that seems dangerous.
| There are static security scanners out there which can parse
| your dependency tree and alert you, but often they have a
| high noise ratio ( e.g. npm audit), so that only helps so
| much.
| chousuke wrote:
| The "easy" way to do this is to write your application
| using whatever is included in a stable Linux distribution.
|
| You get free, trivially applied security updates and a much
| lower chance of breakage. Optionally you can pay for
| support to have them fix problems in dependencies.
|
| Granted, It'll be difficult with languages like Javascript
| where many popular dependencies aren't likely to be
| packaged.
| lordgroff wrote:
| Application is airgapped, so it's not as dangerous as it
| sounds.
| lamontcg wrote:
| A big part of the problem with SemVer is that "breaking
| changes" is subjective and there's always the xkcd spacebar
| heating problem.
|
| The flip side of most breaking changes that are released is
| some software developer who either wasn't experienced enough
| to imagine that the change would break someone or else
| they're dealing with a very hard problem that they're trying
| to solve and the break change was collateral damage in some
| edge condition they didn't consider. Many people's breaking
| changes are someone else's bugfix, and often users fluidly
| wind up on either side of that condition on different bugs --
| screaming about bugs that haven't been fixed for years and
| then screaming about other bugfixes that broke them.
| lordgroff wrote:
| Here's a recent example from sqlalchemy. I'm using a type
| annotation.
|
| Update to new minor version. BAM, annotation no longer
| works, the classes have moved around and I'm staring at an
| ugly exception. To be clear: these were not private
| modules, methods, what have you. So we're not talking about
| the space bar issue here, it wasn't some undocumented or
| buggy behavior, things just stopped working as the API got
| shuffled around. At that point this was literally the third
| semver breakage in the same month, and yeah, this is
| unusual, but dependencies have stayed fixed since then,
| it's a headache and it makes me feel like everything is
| just built on a pile of shifting sand.
| lamontcg wrote:
| That's probably because its open source and the Patreon
| contributions don't compare well to a FAANG staff SDE
| salary. SemVer is highly constraining and its a PITA to
| work with when you're getting a 6-figure salary.
|
| TANSTAAFL.
| Gigachad wrote:
| Tbh when you work with ruby to have to assume everything could
| break with any update and just unit test the shit out of
| everything. Of course it's easy to miss something, but unit
| tests could have caught this issue.
|
| It's still just really bad work from the gem developers though.
| Ideally you shouldn't ever drastically change the behavior of a
| method. Just introduce it again with a new name and remove the
| old one. Yeah it might not be as nice, but it avoids triggering
| $73,000 worth of incorrect transactions.
| emodendroket wrote:
| Ultimately this is why I don't like to work in Ruby. You can
| just never trust any line of code between the gem updates,
| the unhelpful signatures, the overreliance on hashes
| everywhere, and a million different levels of mix-ins and
| indirection. Yeah, it's expressive, but how much time are you
| really saving once you consider all these maintenance
| headaches?
| stouset wrote:
| This exact problem could have happened in _any_ language.
| Literally zero of it has to do with Ruby or the ecosystem.
| jchw wrote:
| Certainly it has something to do with the ecosystem, if
| the most popular MongoDB driver in the ecosystem is
| making breaking changes in minor versions.
|
| That said, strong types can be a godsend for catching
| accidental breaks, even if it wouldn't _necessarily_ have
| helped here. A strongly typed language that has a more
| disciplined ecosystem is less likely to run into these
| kinds of issues.
|
| It's one of the tradeoffs you pick when deciding between
| languages. I decided the tradeoffs didn't work well for
| me and left for compiled languages, for now.
| julik wrote:
| This is exactly the case where nothing about types,
| language or opinions about the Ruby community has to do
| with the problem at hand.
|
| This would not be caught by compiling, strong types or
| anything else - this is a change in behavior which has
| been arbitrarily done by the maintainers of the
| dependency.
|
| > Certainly it has something to do with the ecosystem, if
| the most popular MongoDB driver in the ecosystem is
| making breaking changes in minor versions.
|
| How should this be handled? Should a "good" ecosystem vet
| every single change in every single library that gets
| updated? Or should there be no libraries and only stdlib
| instead (wait, I think I know a compiled language which
| did just that for quite a few years, let me think which
| one that was again...)
| jchw wrote:
| You can feel that it has nothing to do with the ecosystem
| or its norms, but it happened in the Ruby ecosystem right
| in a rather important foundational library, and
| anecdotally it does feel like it's a problem that recurs
| over time. That doesn't mean there's a good solution. You
| can't just upend the defacto attitude of an ecosystem at
| will.
|
| > How should this be handled? Should a "good" ecosystem
| vet every single change in every single library that gets
| updated? Or should there be no libraries and only stdlib
| instead (wait, I think I know a compiled language which
| did just that for quite a few years, let me think which
| one that was again...)
|
| This isn't really about Ruby vs all compiled languages, I
| mean, Brainfuck is a compiled language and that doesn't
| mean I was endorsing Brainfuck to replace Ruby. However I
| would certainly endorse Go to replace Ruby. Rust too,
| although using Rust to do web development stuff feels
| like using a nuclear warhead to open a door.
|
| Anyway, the reason why types is relevant to this
| discussion is because types are contracts that are
| statically enforced and the break that occurred happened
| due to a contract change. This change illustrates that
| even if you do have types it doesn't guarantee you won't
| have breaking contract changes. However, eliminating
| entire classes of accidental or intentional contract
| breaks is an obvious win/win. If it was as complex and
| obtuse as C++, nobody would bother. But C++ isn't the
| only game in town anymore for fast compiled strongly
| typed languages.
| stouset wrote:
| > Certainly it has something to do with the ecosystem, if
| the most popular MongoDB driver in the ecosystem is
| making breaking changes in minor versions.
|
| Am I to believe that you hold every other language to
| this same standard? A single maintainer of a reasonably
| popular project makes one boneheaded decision in a
| release, and that's enough reason to denounce the entire
| ecosystem?
|
| > That said, strong types can be a godsend for catching
| accidental breaks...
|
| Ruby is a strongly typed language.
|
| You probably mean _static_ types, but even that wouldn 't
| have helped here. This was a behavioral change and did
| not affect any of the (implicit, in Ruby) type signatures
| of the function. It would not have affected any of the
| explicit type signatures of the function in other
| languages. I say this as an enormous proponent of Rust
| and static typing.
|
| You are tilting at windmills.
| emodendroket wrote:
| But it's not really about a single maintainer and
| project, is it? The thing is that this sort of problem is
| somewhat common in this environment, which is why people
| have adopted "rules" they consider common sense about
| which hoops you're supposed to jump through before minor
| version updates.
| astrowilson wrote:
| It definitely has something to do with Ruby and Rails.
| Ruby and Rails introduces API changes on minor versions
| all the time, sometimes even without prior warnings, so
| people shouldn't expect related gems to be any different.
| [deleted]
| ramchip wrote:
| In theory yes, but in practice there's a big difference
| between ecosystems. I maintain apps in Ruby and Elixir,
| and as far as I can remember, all the Elixir libraries I
| use respect semver. Whereas with Ruby it's much more
| mixed.
| gedy wrote:
| No, this is particularly bad in Ruby. Many languages,
| esp. compiled do not have the same issues.
| chucke wrote:
| Java and log4j would like a word with you.
| jorams wrote:
| The problem with log4j is that they _didn 't_ introduce a
| breaking change, allowing vulnerable functionality the
| maintainers already considered problematic to stick
| around.
| dagmx wrote:
| What language would avoid the problem?
| Gigachad wrote:
| That's often the case but for this particular issue, it
| could have happened in any language because it's still
| returning the exact same type. Its an array of the same
| record type in both instances.
| emodendroket wrote:
| Ultimately this is more social than technical.
| metafunctor wrote:
| You're right that the language isn't to blame as such.
|
| But if a change like this doesn't cause some kind of
| uproar in the Ruby community, then the community has a
| problem.
| astrowilson wrote:
| It won't cause a uproar because Ruby/Rails changes APIs
| on minor versions all the time. Breaking changes are
| pushed at a yearly rate and everyone is just expected to
| cope with them.
| metafunctor wrote:
| That's what I've gathered. That still seems stupid to me.
| Why does the Ruby/Rails community do that?
| ljm wrote:
| We're just trading anecdotes at this point but I can't say
| I've encountered the same problems over the past decade of
| working with it. That's generally because of limiting
| dependencies, but also because it's a good idea to peek at
| the changelog or release notes when bumping the minor or
| major version.
|
| TFA doesn't mention or acknowledge this aspect of the
| issue, but it did surprise me that they considered a
| version bump from 7.0.8 to 7.3.3 to be innocent looking.
|
| In fact, these 'minor' releases actually appear to be quite
| significant and the issue in question in TFA was explicitly
| documented. [0][1]
|
| I hate to say it but this was just a case of sloppy work on
| the startup's part. It took me less than 5 minutes to find
| that info.
|
| [0] https://docs.mongodb.com/mongoid/current/tutorials/mong
| oid-u... [1]
| https://github.com/mongodb/mongoid/releases/tag/v7.1.0
| newaccount74 wrote:
| It took you 5 minutes to find the info because you knew
| what you were looking for.
|
| Expecting programmers to audit all of their code before a
| minor update is ridiculous.
| ljm wrote:
| 'Auditing all your code' isn't the same as looking up the
| changes for a certain version on its github release page
| or a changelog page (if it has one).
|
| It's not the responsibility of OSS maintainer to ensure
| your software still functions after an upgrade if you're
| not even willing to spend a modicum of effort to hold up
| your own end of that bargain. This isn't a big ask, it's
| why there are changelogs and release notes in the first
| place; they make it so you don't have to audit the code
| every time the version changes.
|
| If you're not willing to do that and want unattended
| upgrades for all dependencies, then this is where the 'no
| warranty' aspect of that OSS license comes in.
| Gigachad wrote:
| >It's not the responsibility of OSS maintainer
|
| The OSS maintainer has no responsibilities at all so you
| are right. But if anyone was to blame, it's certainly the
| library. It's outrageous to radically change a function
| while keeping the name the same. Because of exactly this
| issue. If they renamed it you would get an exception
| which is much nicer.
| blindmute wrote:
| I'd say that updating any version of anything, without
| reading the changelog, is irresponsible. Why are you
| updating a gem if you have no idea what has changed?
| emodendroket wrote:
| In other programming environments people do this without
| issue routinely.
| ljm wrote:
| I used to do this, especially with patch releases. Just
| bump the version and hope the build passes. My mindset
| has gradually shifted over time though.
|
| These days, it's more a case of updating one thing at a
| time, and doing the research up front to see what I'm
| gaining from it or if I might as well stay on the current
| version. No point updating for the sake of it.
|
| That's 5-10 minutes of up-front, preventative effort that
| might otherwise become hours of reactive firefighting,
| and as much time spent on damage control, if it got into
| production unchecked.
| losvedir wrote:
| Seriously? It's eminently reasonable to expect you to
| know the code you're deploying. Perhaps you're a JS
| developer. I agree it's incredibly difficult to keep up
| with the churn there, but in my Elixir deps, the updates
| tend to be less frequent and more reviewable.
|
| Some deps you can trust the owner and just carefully
| review the change log. Even that would have caught this
| issue, though I'm not sure I'd count this gem as
| trustworthy.
| newaccount74 wrote:
| I'm not a JS developer, I'm just someone who has shipped
| enough software to know that the fraction of people
| reading change logs is vanishingly small.
|
| Maybe you are a very careful developer, but the vast
| majority out there is not.
|
| Shipping an udate that will corrupt data if you don't
| read the changelog is very very dangerous.
|
| I see why they did it. Having a method with the same name
| as in Active Record but with different behavior is also
| dangerous.
|
| But they really could have handled this better.
| ricardobeat wrote:
| The NPM mindset.
| joelbluminator wrote:
| This is quite rare and also types would have done nothing
| in this case.
| gingerlime wrote:
| > Of course it's easy to miss something, but unit tests could
| have caught this issue.
|
| Nitpick, but I don't think unit tests would have caught this.
| Integration or systems tests might have caught this however,
| but those can be much harder to create and maintain in
| practice, in this particular case.
| sorokod wrote:
| Well, you get what you pay for. If integration tests are
| viewed as too expensive, then an occasional penalty is just
| the price of doing business.
| Railsify wrote:
| Semantic Versioning does not mean you can update without
| reading the changelog. Blame here remains with the developer
| who updated without reading the changelog.
| kinduff wrote:
| Learned this the hard way. No matter the change or version, I
| always try to double check both CHANGELOG and the commits.
| montblanc wrote:
| I think tests should have caught that one but I agree.
| henry700 wrote:
| Not if you follow the One True Way of Unit Testing (tm)
| where the Mongoid lib would have been mocked away, and
| the test would pass :)
|
| Integration tests might've done the job here.
| montblanc wrote:
| You dont mock the db layer, i dont do that at least, hell
| no. Let the db be hit, check that the records it returns
| make sense. Thats how I roll at least.
| [deleted]
| kortex wrote:
| Do you or your team review every item of every changelog of
| every dependency in your stack (recursively for their
| dependencies) for every minor update, and/or 100% coverage
| for all of your assumptions of how they are used?
|
| Sure it was a mistake by OP in TFA but it's an honest mistake
| any of us could make. And it's a really Bad Move by the
| Mongoid devs. It'd be a bad move in 1.2->1.3, but by 7.3? I'd
| just be like "whelp, this is how this function behaves now in
| perpetuity, I guess just document that it has different
| semantics than AR" even if an 8.0 was released. This is the
| kind of change which causes _really_ subtle bugs. It 's
| rarely worth it. Just introduce .chain_or() or something.
| Railsify wrote:
| Yes, I generally scan the changelog and look at the diff on
| github from the previous version. Not doing so is penny
| wise and pound foolish, the risk to brand and the risk of
| downtime is too great to install a crypto miner, or a
| vulnerable log4j version, etc.
| ljm wrote:
| I think you're trying to make the case sound a bit more
| absurd than it really is, in terms of recursively scanning
| dependencies and checking for 100% coverage for all
| assumptions.
|
| It's really just as simple as going to the release notes
| and seeing if there's a mention of breaking changes or
| deprecations. If no such thing is mentioned then you're
| fine, otherwise you go and see if that change affects you
| at all and just take a little bit more time to test the
| upgrade. This has been standard practice at plenty of
| places that I've worked.
|
| It also doesn't mean that it makes the change a
| particularly good one, but I don't think I can make a bunch
| of OSS maintainers responsible for my own failure to review
| an upgrade and test it before rolling it out. In those
| terms, there is some culpability both with the maintainers
| and with the library users.
| Railsify wrote:
| This is well stated. It's about taking your fair share of
| responsibility, as engineering we bend code to our will,
| this includes reading the code of opensource packages.
| karmicthreat wrote:
| 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.
| jounker wrote:
| Billing renewal is a major feature. The ultimate problem here is
| that renewal behavior didn't have adequate test coverage.
| noneeeed wrote:
| 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.
| jrochkind1 wrote:
| That sure doesn't seem to be an endorsement of mongoid.
| noneeeed wrote:
| It isn't.
|
| We have used Mongo for good reasons, it's been appropriate
| for our workload, and we have used Mongoid since the
| inception of the product long before I joined the company.
| Replacing Mongoid at this stage would be a massive piece of
| work.
|
| I find Mongoid's documentation is vague and lacks important
| detail, and the API itself violates the principal of least
| surprise frequently enough to be a problem.
|
| It really is the ugly stepchild of ActiveRecord, in whose
| image it was created, and which by comparison has been a
| pleasure to both use and to manage.
| dsjoerg wrote:
| It wasn't meant to be. But in a way it is -- mongoid is so
| useful, that it's worth putting up with some of the flaws in
| its release/upgrade management. So, it's temperamental. Like
| owning a Jaguar.
| nefitty wrote:
| I feel spoiled having most of my experience in js, react and
| node. They like, try really hard not to totally break shit.
| fouc wrote:
| 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.
| astrowilson wrote:
| > This type of library API breaking change on a minor
| version update basically never happens.
|
| Literally happens all the time with Rails. To the point
| they decided to call their versioning schema "shifted
| semver" to afford themselves API changes on minor versions:
| https://guides.rubyonrails.org/maintenance_policy.html
|
| Good luck if you are using Rails and ecosystem and you
| expect any sort of sensible versioning.
| chucke wrote:
| Rails never was semver. Their yearly major-minor grand
| release always, always breaks API all around. It's
| inevitable. Any rails-only gem will as a result have a
| strong incentive not to follow several.
| byroot wrote:
| Rails committer here.
|
| First I get that people are used to SemVer, but it is
| unreasonable to assume all projects follow it. And yes in
| semver terms, you can simply assume that Rails X.Y is a
| major release.
|
| Then, any breaking change in Rails must first emit
| deprecation warnings, so unless you are jumping one
| version, this kind of scenario shouldn't happen with
| Rails itself.
|
| Also note that Ruby (MRI) itself more or less behave the
| same regarding versioning, deprecations and breaking
| changes. First emit warnings, then break.
| flubflub wrote:
| I don't use Ruby.
|
| Are packages that don't follow semver allowed to be
| released?
|
| Haskell asks for PVP for example.
| byroot wrote:
| Yes, you'd need an automated way to catch the API
| breakage, which would be particularly tricky.
|
| And even with very strict typing, the breaking change
| showcased in the article wouldn't have been caught, the
| API stayed the same, it just behave differently. Not
| every backward incompatible change is as simple as a
| function signature change.
| flubflub wrote:
| When I tried to submit a package to Hackage, I was asked
| if I was aware of the packaging guidelines. The person
| who I emailed to register also inspected my package and
| told me that my dependencies were wrong and not
| compliant.
|
| I really don't think this is a scalable approach and was
| really surprised that someone took the time to check my
| dependencies personally. My experience may just be rare.
|
| I meant more on the human side than automated. Haskell
| doesn't have that much to do with formal verification in
| usual use. I'm not sure what the best policy for a good
| yet vibrant package ecosystem is.
| astrowilson wrote:
| That's precisely my point. With Rails and Ruby breaking
| APIs on every new minor release, the only realistic
| expectation is that every minor release of anything may
| have breaking changes, because that's the example being
| set. And it just makes the experience of updating any
| Rails project even worse than it already is.
|
| The language and framework, by your own statement, are
| pushing breaking changes at a yearly rate. That's a
| horrible developer experience.
| byroot wrote:
| No the expectation is that users of projects stop
| assuming SemVer is followed by every projects.
|
| > pushing breaking changes at a yearly rate. That's a
| horrible developer experience.
|
| That's your opinion. I much prefer these bite sized
| yearly changes to much bigger changes over longer
| periods. e.g. Ruby with this strategy never had the big
| divide Python 3 had with its much larger change.
| astrowilson wrote:
| You may not believe this, but there are other ways of
| versioning software beyond breaking things every year and
| doing whatever Python 3 did.
| byroot wrote:
| > You may not believe this
|
| You may not need to be confrontational...
|
| > there are other ways of versioning software
|
| I'm sure there is. But some features and other
| improvements sometimes require to deprecate and remove
| some older features.
|
| Each project will chose its own tradeoff between bringing
| the improvement faster vs keeping compatibility longer.
| weaksauce wrote:
| i mean the whole lpad debacle was pretty egregious and
| that's directly in the javascript land
| [deleted]
| rgoulter wrote:
| Well, except for leftpad.
| jacquesm wrote:
| 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.
| vmception wrote:
| 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
| [deleted]
| mickotron wrote:
| And what kind of testing was done prior to Prod deployment?
| rubyfan wrote:
| 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
| dasil003 wrote:
| 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.
| dudeinjapan wrote:
| I have raised a ticket for the Mongoid team to read the article
| and HN comments here:
| https://jira.mongodb.org/browse/MONGOID-5218
| ab_testing wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| rubyist5eva wrote:
| 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 wrote:
| 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 wrote:
| 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.
| OmegaPG wrote:
| 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.
| yua_mikami wrote:
| 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]
| recursivedoubts wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| Rails changed the meaning of NOT and nobody batted an eye -
| 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.
| kortex wrote:
| That is asinine. Why not change it in 5.x->6.0 if they
| knew it was a problem. Why even use version numbers at
| that point? Just use DateVer or 0ver at that point.
| dragonwriter wrote:
| > Why not change it in 5.x->6.0 if they knew it was a
| problem
|
| Because Rails explicitly has a versioning policy where
| minor versions are equivalent to SemVer major versions
| (but with deprecation notice in a previous minor version)
| and where major versions are _also_ SemVer major with
| subjective significance distinctions; they call it
| "shifted SemVer".
|
| https://guides.rubyonrails.org/maintenance_policy.html
|
| This is somewhat obnoxious, but not as bad as saying you
| use real SemVer and then brazenly breaking things in
| minor releases.
| kortex wrote:
| Sure, but like, you don't _have_ to break code on .Y
| changes.
|
| Changing semantics of a query operator is something worth
| saving for a "big major" update, if you change it all.
|
| Some breaking changes are really obvious and easy to
| catch. Others can introduce pernicious bugs which slip
| through tests. The change Mongoid is solidly the latter.
| _And_ yeah, they say they use _actual Semver_.
| dragonwriter wrote:
| > Changing semantics of a query operator is something
| worth saving for a "big major" update, if you change it
|
| Arguably, it was so important that they did a big major
| update _just to deprecate it_.
| asdfasgasdgasdg wrote:
| 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!
| lmeyerov wrote:
| It's the users who need to audit, the social contract of
| OSS is just not to be sneaky/sloppy about it (when posing
| as a serious project).
|
| Semvar is beautiful bc it lets you be explicit. Likewise,
| end users can judge "wow major version 27 in as many
| months, maybe not so good for us." We had a gov customer
| today upfront about needing slow updates, and same deal --
| maybe our SaaS and OSS libs are too fast moving, so they
| are probably better off with our enterprise distros.
| asdfasgasdgasdg wrote:
| I am a toil/backwards incompatibility hawk. I don't like
| to impose them on my customers. It's extremely rare that
| the balance of equities favors making a change like this,
| where a function that filters data and will necessarily
| impact correctness is _changed_ , and not in subtle edge
| cases but in its primary behavior. In fact, I've never
| seen a case in my career where it ended up being worth
| it.
|
| I'm a big fan of Linus' mantra: we don't break userspace.
| lmeyerov wrote:
| I agree - we probably err too long on avoiding breaking
| legacy code: we are about to do our first deprecation in
| years as the old protocols are getting too unconscionably
| insecure + costly to maintain relative to our new ones.
|
| But that's company code we get paid to be stable on. Very
| diff story for OSS we at most contribute to, we don't
| assume that's part of the social contract.
| asdfasgasdgasdg wrote:
| Linux is also OSS. I don't think this is an OSS/paid
| dichotomy. This is a good software/bad software
| dichotomy. Also isn't this library maintained by mongo db
| anyway? So the "we are doing this for free" excuse does
| not apply.
| lmeyerov wrote:
| Agreed in part, I'd indeed want careful motivation and
| change management of such a change from my language or DB
| vendor -- JS/Python takes years for tinier semantic
| changes :)
|
| The average OSS project is just ~1 fulltime maintainer,
| maybe 2, with tiny drive-by contributors: there are great
| studies measuring this. Even most popular and long-lived
| OSS projects are more like that than Linux. The
| exceptions are inspiring, but most (my bet, I don't
| recall stats here) seem to be open core largely driven by
| 1 company. Something foundational with many contributors
| under open governance like Linux is better to discuss as
| an abnormality ("why can't the typical project grow to
| this size, maturity, tooling, and stability?").
|
| So when talking about modern OSS, the typical case of
| looking through a pip or npm tree is NOT big shops and
| instead something closer to a network of volunteer
| passion project. From such individuals, I'm thankful for
| semvar, CHANGELOG, CI, and a few niceties like that. From
| there, the history of BREAKING would signal the project
| moves too fast for us or with too big swings, or looks
| more acceptable.
| ineedasername wrote:
| Where I work, the biggest enterprise system we have (ERP)
| has has a support cycle of ToS upgrades quarterly, with
| only urgent security patches in-between. And detailed
| advisories ahead of time on which (depending on modules
| and features in use) must be updated, may be updated, or
| can be ignored.
|
| It makes maintenance a highly predictable endeavor:
|
| Update T+0 == dev instance
|
| Update T+1 week == test instance
|
| Update T+2 weeks == prod deployment.
|
| UAT inserted as needed when there are user-facing
| changes.
| recursivedoubts wrote:
| Sure. That's what a major version is for.
| yholio wrote:
| 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...."
| handoflixue wrote:
| Aside from being more modern than to suggest a fax, that seems
| to be exactly what they did?
| partido3619463 wrote:
| No they refunded everyone immediately.
| fouc wrote:
| The sheer unprofessionalism is astonishing. They're gonna
| put the whole business community to shame.
| jrootabega wrote:
| "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 wrote:
| I thought you were being serious until I got to this bit:
| "please contact our billing department by fax"
| kingcharles wrote:
| 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.
| emodendroket wrote:
| When I moved out of Massachusetts I had to send a few faxes
| to state agencies! Well, it's not too painful with Hellofax I
| guess.
| kingcharles wrote:
| Yeah, thank god for free online fax gateways.
| cr3ative wrote:
| And then nobody is aware of the huge breaking change in a
| popular open source library. This isn't "professional", this is
| your legal team making things worse.
| jacquesm wrote:
| 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 wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| welder wrote:
| This would never have happened had they used a real database
| instead of Mongo.
| dudeinjapan wrote:
| 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.
| booleanbetrayal wrote:
| 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 wrote:
| 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 wrote:
| It was fully documented and ActiveRecord has done similar
| things in the past. See also:
| https://til.hashrocket.com/posts/3zyftipjiu-rails-will-
| chang...
| kevin_thibedeau wrote:
| ... 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".
| tigerBL00D wrote:
| 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
| xyst wrote:
| 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 wrote:
| Stripe charges the same for reminds as regular charges, so
| approximately 3%.
| nightpool wrote:
| 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-....
|
| 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 wrote:
| 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.
| tbrock wrote:
| 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.
| userbinator wrote:
| 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...)
| matwood wrote:
| > I wish more developers would abide by the old saying "don't
| fix it if it ain't broke."
|
| This can work for internal software (see people still running
| old Windows and IE for some random internal app), but breaks
| down for software with public access. The problem is when a bug
| is found, and one will be found, the farther behind the current
| release you are, the harder it is likely to fix.
|
| I prefer to update my dependencies every so often so that I'm
| never too far behind if I'm suddenly forced to update for some
| critical security issue.
| jrochkind1 wrote:
| The problem is the technical debt you acquire this way.
|
| Eventually you're going to need to upgrade X for a security
| fix, but the new version of X isn't compatible with the version
| of Y you are using, and upgrading that would break Z, and Z
| would break your app here and here. Now you're spending a
| couple weeks trying to do the upgrade to get a security fix
| that was 0 day two weeks ago, when you should have been writing
| things that actually advanced your app instead.
|
| I keep my dependencies up to date, and consider it technical
| debt when they are not, and would never want to go back.
| loktarogar wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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?
| fouc wrote:
| Unfortunately if you don't update the packages your app depends
| on, you don't gain access to the newer conveniences &
| improvements (security, performance, etc) that have been
| introduced.
| jbverschoor wrote:
| 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 wrote:
| 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 wrote:
| 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
| house9-2 wrote:
| Rails 7 has finally added this to active record: sole and
| find_sole_by
|
| Both raise an exception unless a single record is found.
|
| Of course this would not make a difference when working
| with mongoid... I also like the fail fast approach.
|
| https://blog.saeloun.com/2021/03/16/rails-adds-sole-and-
| find...
| jbverschoor wrote:
| Finally! I don't like the name though. but I'll live with
| it. Well, at least they added it to Enumerable, so you
| could use it with mongoid.
| kgeist wrote:
| 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 wrote:
| 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.
| andrew_ wrote:
| The mongoid repository (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.
| zionic wrote:
| 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.
| ineedasername wrote:
| Is JIRA that bad even for just logging an issue? Or do you nope
| out of the dependency?
| house9-2 wrote:
| It really is "that bad".
|
| JIRA is not made for humans.
| plorkyeran wrote:
| 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.
| tyingq wrote:
| 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.
| seanwilson wrote:
| > 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 wrote:
| probably a combination of realistic-seeming desktop browsers
| (eg. headless chrome with stealth patches) and residential IP
| providers (eg. luminati)
| kingcharles wrote:
| Yep. Has to be using a large net of residential addresses in
| each country to not get banned by Google.
| forgot_old_user wrote:
| 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 wrote:
| Not sure, but maybe they use a CAPTCHA-Solving-API like
| https://anti-captcha.com
| seanwilson wrote:
| 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.
| sorokod wrote:
| "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.
| iamtheworstdev wrote:
| Do. Not. Break. Userspace.
| fiddlerwoaroof wrote:
| This sort of behavior change in a dependency would make me
| blacklist the dependency.
| cinntaile wrote:
| 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 wrote:
| I'm not blaming them: I'm saying that if I was in this
| position, I'd ban the Mongo library used.
| cinntaile wrote:
| Oh ok, I thought you meant if you were using serdapi as a
| customer.
| x3n0ph3n3 wrote:
| I believe the dependency OP is referring to is mongoid. I
| think serpapi did all the right things here.
| x3n0ph3n3 wrote:
| I shun everything MongoDB because of how immature its
| engineering culture is.
| gqewogpdqa wrote:
| Can you explain?
| btown wrote:
| 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
|
| > 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
|
| > 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
|
| > 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.
| x3n0ph3n3 wrote:
| What others have said + I have not forgotten about
| "webscale."
| [deleted]
| Beltiras wrote:
| 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 wrote:
| 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?
| GreenWatermelon wrote:
| This video should speak to you
|
| https://youtu.be/b2F-DItXtZs
| Beltiras wrote:
| 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).
| jjice wrote:
| 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 wrote:
| 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.
| matwood wrote:
| > 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 wrote:
| MongoDb are infamous for lying about their capabilities.
| Just search hackernews and you'll read all of the horror
| stories.
| GreenWatermelon wrote:
| But, is <everything else> web scale?
| pictur wrote:
| 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.
| draw_down wrote:
| tdiff wrote:
| 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.
| ramchip wrote:
| 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.
| whalesalad wrote:
| 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.
___________________________________________________________________
(page generated 2022-01-08 23:03 UTC)