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]
> 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.)
.
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.
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.
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.
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.
> 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.)
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.
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".
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)
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.
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.
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.”
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.
> 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.
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].
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 ?)
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!
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.
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.
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.
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?
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.
'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.
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.
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.
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?
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.
Yes, this is just part of life. Most open-source developers go well beyond the technically-correct "I don't owe you anything" stance and actively try to help their users, which includes adhering to SemVer, and trying to do more work so that the user doesn't have to.
However, you can't rely on this. Many open-source projects fill a niche and become popular when it isn't the intention of the author. This causes a disconnect where the users expect the project to be for them, when the project is really for the author. Mongoid seems to be more in the former category (for the user).
Because some libraries are badly managed, you need to audit all of them.
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.
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...)
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.
> 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.
I am using the Wikipedia definition of 'strong' vs 'weak' typing.
> Generally, a strongly typed language has stricter typing rules at compile time, which implies that errors and exceptions are more likely to happen during compilation. Most of these rules affect variable assignment, function return values, procedure arguments and function calling. Dynamically typed languages (where type checking happens at run time) can also be strongly typed. Note that in dynamically typed languages, values have types, not variables.
Just out of curiosity, exactly what do you even think weakly typed could mean by your own definition of it?
No need to address the other points, because we're talking about anecdotes and not scientific data. Yes, I'm using a single data point as an example about a feeling I have to justify my opinion.
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.
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.
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.
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.
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.
The problem with log4j is that they didn't introduce a breaking change, allowing vulnerable functionality the maintainers already considered problematic to stick around.
> 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.
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.
> 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.
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.
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.
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?)
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.
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.
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.
> A big part of the problem with SemVer is that "breaking changes" is subjective
I don't think that's true. I've always thought a breaking change is defined in broad terms as "the public API surface, runtime, and output". Given that, each of these would be a SemVer major:
* remove a publicly-exported function
* public function changes order of arguments
* drop support for an old version of the language
* a function changes behavior significantly (the issue the OP wrote about)
Relying on a specific major version, you should be able to assume none of the above will happen. The following would _not_ be a major version:
* renamed/removed private versions
* add extra, optional arguments to public function
* add support for new version of language
* the xkcd example about CPU usage
In each of these cases, if you rely on non-guaranteed behavior, you should check every version upgrade closely- package maintainers don't make any assurances about those.
I'm not sure how mongoid is developed, but this smells like a bug to me. The `or` behavior started considering a thing it didn't used to, possibly unintentionally. Unfortunately, released mistakes are the one thing SemVer doesn't cover. Testing sufficiently about your own assumptions (in this case, that the user upgraded matches the one found in the query) is the best way to CYA.
That is a nice neat and tidy way of thinking about changes. The messy details are things like "oh the default options result in a massive security hole, they need to change immediately". There's also a messy medium ground where behavior changes and its just a question as to if anyone is dependent upon that behavior. It was wrong when it was implemented, never documented, but has a currently defined behavior that most people don't want -- unless they happened to find it and use it and work around it and they'll be broken by the switch. This is the realm of things like the undefined behaviors of the C standard which are compiler-dependent. Any sufficiently complicated API will have those all over the place. You can argue that APIs should never have those kinds of undefined standards, but in reality everyone does shoddy work and people accept PRs that they shouldn't (and the flip side of trying to be careful is people yelling about open PRs that have been stalled for months/years trying to sort out a sufficiently robust solution).
> It was wrong when it was implemented, never documented, but has a currently defined behavior that most people don't want
That strikes me as exactly the sort of guarantee that's _not_ made by semver. You can rely on that behavior, but then you have to check against every patch version, because that's exactly the sort of thing that could change out from under you. Maintainers shouldn't worry about breaking this case- they never promised to support it in the first place.
I think a lot of those cases should be a major version change, and that's totally fine. We _should_ minimize the number of breaking changes to code people depend on, but sometimes the best fix to the (totally reasonable) situations you outlined above is to make a new major version. Users will have to make changes to upgrade (instead of just bumping the versions), but it won't change out from under them. Some docs about what's changing, why, and how to migrate your code go a long way here.
We do major version bumps on a yearly cycle and people get stuck on 8 year old versions and won't upgrade. Its easy to say that integers are free and limitless, but constant major version upgrades are something that people won't tolerate one way or another either.
And you have to understand that the people making these decisions make dozens of them for every single release, and when they make category mistakes that is how you get breaking changes released early. Every single bug when looked at in isolation looks obvious what should have happened given hindsight bias.
And I just don't think open source software, that isn't corporate backed in one way or another, should ever go 1.0 and it should stay 0.x.y and go ahead and break compat every 3-6 months or so as necessary. The cost of SemVer and trying to get it continuously correct on every single patch is a large tax.
> We do major version bumps on a yearly cycle and people get stuck on 8 year old versions and won't upgrade
We do the same thing and have the exact same experience, unfortunately. Software is a messy business and it's hard to apply strict rules to human process.
I think ultimately SemVer can be used to to accurately communicate if you, a user, are safe to upgrade between versions if applied strictly. Whether or not your users will (or be happy about it) is another ballgame.
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 ...)
Semantic Versioning does not mean you can update without reading the changelog. Blame here remains with the developer who updated without reading the changelog.
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.
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.
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.
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.
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.
Didn't want to aim at you with the reply, just remembered the collective delusion of 100% coverage unit testing zealots and how this would ironically fail to catch this.
You'd be surprised how far people go to shoot themselves in the foot.
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?
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.
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).
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?
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.
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.
> 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.
I think you're overthinking this. I'm not suggesting they should have looked for any unintentional state changes, we both agree that is overkill (until you start doing FP).
> Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code.
Yes, exactly. For this specific bug even the simplest test would have failed, which would have caused someone to take a look at what was happening. You are correct that if the bug had been more complicated, such as causing both the proper user AND an additional random user to be charged, then it's unlikely a reasonable level of testing would have caught it.
> The adequate monitoring and quick response they did here is probably a very good trade-off.
We agree that there is a trade-off here, and if sacrificing some correctness is what it takes to win you a much higher velocity then they probably made the correct trade off; nobody died as a result of this bug.
But... surely you see there are some cheap steps they could have taken which would have caught this bug? Not all bugs, but this specific bug.
- Write integration tests for important behaviors, such as charging users!
- Make sure those integration tests run in an environment which closely simulates production.
> 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.
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.
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.
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)
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.
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.
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.
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.
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.
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.
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...
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.
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.
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.
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.
> 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.
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.
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.
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.
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.
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.
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.
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...."
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)
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
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.)
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.
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.
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.
> 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.
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.
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.
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.
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.
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.
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.
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.
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
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.
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.
> 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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?
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.
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.
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).
> 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.
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.
> 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.]
> 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.
> 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.
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?
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.
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)
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.
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.
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...)
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.
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.
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.
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
> 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.
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.
"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...."
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.
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.
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
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.
> 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.
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!
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 :)
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.
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.
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.
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.
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.
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.
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.
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!
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.
> 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”.
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.
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.
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.
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.
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.
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.
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
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.
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.
> 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?
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.
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.
> 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.
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.
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.
That they ended up returning it is good but either the amount was meant to have some kind of significance and before they returned it they received it.
I'm well aware of the effects of various kinds of risks to merchants.
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.
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.
SerpApi dev here. I've requested, reviewed, tested, and deployed this upgrade of gems.
I want to clarify some things. This code was running on staging for three weeks before the deployment to production. There were four application errors on staging that were related to the problem with subscription renewals.
I and the author of the pull request have made three mistakes:
- haven't carefully read code in all methods from the backtrace of four app errors on a staging environment
- upgraded several gems at once
- didn't review the changelog of mongoid gem
As multiple people commented here, integration tests for renewals should've been caught that bug. We hadn't a test case when all user's subscriptions are checked after the renewal of a specific test user.
So the lesson here is to not yolo minor upgrades of dependencies and avoid mistakes above.
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.
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.
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.
Don’t deploy untested code to production. The fact that it’s a dependency doesn’t excuse a proper code review.
It is ironic to me that software teams have code review processes and seem to trust each other less than random people they have never met on the internet. Every new dependency, you need to read the source. Read it completely.
Every new change in every dependency needs to be reviewed as well. I’m a big fan of forking all dependencies and using only those forks in my code. Then, do upgrades via standard rebase or pull requests on that fork.
If you have dependencies that are too large to do this with your time, you probably should avoid them in the first place.
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.
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.
The code is fine, you're publicly chastising yourself over it for no reason. Mongoid had bad API semantics, and you did what you needed to do to make your code work. Then, you got rug pulled by a minor version update when they said "oopsie, we think these semantics make more sense!".
There might be an argument to make that your test suite could have caught this, however.
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
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.
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
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.
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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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".
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