"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...
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.
If you update your dependencies and ship it based on version numbers alone, you can’t blame the maintainers
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.
Even a minor bug fix in a library can expose a critical bug in your own code.
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?
I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
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.
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.
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.
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.
So in order to update a dependency you must first write a test that fail on current version and is green on updated version.
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.
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.
The problem is, once you deploy to production you have (hopefully) tested this case and rely on the actual implementation.
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.
After such a problem, I would roll back and never ever update this dependency again.
How people blog and how they feel aren't always the same thing. Professionals tend to be a lot more tactful in public communication.