6 ms·

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 s

Mongoid docs[1] seem to be pretty cool about this change: "As of Mongoid 7.1, logical operators (and, or, nor and not) have been changed to have the the same semantics as those of ActiveRecord. To obtain the semantics of or as it behaved in Mongoid 7.0 and earlier, use any_of which is described below." Is it just me or is this one of the most terrible breaking changes in a popular, official library ever? [1] https://docs.mongodb.com/mongoid/current/tutorials/mongoid-queries/ https://docs.mongodb.com/mongoid/current/tutorials/mongoid-q...

warpech 5y ago
The SerpAPI blog author seems cool about it, too. After such a problem, I would roll back and never ever update this dependency again.
- munificent 5y ago
  > seems cool about it, too. How people blog and how they feel aren't always the same thing. Professionals tend to be a lot more tactful in public communication.
  - hyperhopper 5y ago
    I would be fuming if I were an engineer depending on that, but obviously in a public communication like that, where you are explaining to clients why you charged them erroneously, just pointing a finger and acting mad makes you look like you don't have control of your own software.
    - Aeolun 5y ago
      A little bit of passive aggressive comments would have been appropriate I think.
- kortex 5y ago
  Yep, that gets a pin to "==x.y.z # pinned to prevent BREAKING CHANGE, do not update without reading <link>" and also some defensive regression tests on their api lest someone update by mistake.
- fshbbdssbbgdd 5y ago
  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.
warpech 5y ago
Perhaps before 7.1 Mongoid had a problem that the logical operators acted differently to ActiveRecord. But come on, this adjustment could have been implemented in another namespace or something...
cedricd 5y ago
It's clearly a breaking change how a core feature of the library behaves. This is extremely unprofessional on the part of the maintainers. I'd completely lose trust in the gem. They knew they were making a breaking change, documented it, and didn't increment a major version number. That breaks the entire point of semver. Also, this is generating SQL ffs. Like how more nasty of a breaking change could you make in terms of potential impact to live apps that upgrade? The experience in the post is a perfect example of how badly this can go wrong.
- andrewxdiamond 5y ago
  All dependency changes are changes. And changes should be tested before being deployed. If you update your dependencies and ship it based on version numbers alone, you can’t blame the maintainers
  - thrwn_frthr_awy 5y ago
    > All dependency changes are changes. And changes should be tested before being deployed. OP did not say changes should not be tested. > If you update your dependencies and ship it based on version numbers alone, you can’t blame the maintainers OP did not if you update your dependencies and ship it based on version numbers alone you can blame the maintainers.
    - Retric 5y ago
      Shipping something with zero testing is always crazy. Even a minor bug fix in a library can expose a critical bug in your own code.
      - kortex 5y ago
        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).
stefan_ 5y ago
The changelog for 7.1 (https://github.com/mongodb/mongoid/blob/master/docs/release-notes/mongoid-7.1.txt https://github.com/mongodb/mongoid/blob/master/docs/release-...) even explicitly call this out as a breaking change: Breaking change: In Mongoid 7.1, when condition methods are invoked on a Criteria object, they always add new conditions to the existing conditions in the Criteria object. Previously new conditions could have replaced existing conditions in some circumstances. Ironically they also have a breaking change in 7.1.1, so they just don't give a fuck at all.
- mattigames 5y ago
  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)
jimbobimbo 5y ago
Change of semantics in a minor version!? And no way to retain the semantics by flipping a flag or something!? SMH
vasco 5y ago
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.
contravariant 5y ago
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.
- Sebb767 5y ago
  > That said I would normally read '[...]' as '[...]' The problem is, once you deploy to production you have (hopefully) tested this case and rely on the actual implementation.
- jve 5y ago
  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.
zomglings 5y ago
Seriously, why the fuck not just bump to 8.0.0 or whatever?
Aeolun 5y ago
They made a new function for the ‘old’ behavior… that makes it even worse. Just make a new one for the new behavior and everyone is happy.
mad182 5y ago
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.
deleted 5y ago
[deleted]

warpech 5y ago

The SerpAPI blog author seems cool about it, too. After such a problem, I would roll back and never ever update this dependency again.

munificent 5y ago
> seems cool about it, too. How people blog and how they feel aren't always the same thing. Professionals tend to be a lot more tactful in public communication.
- hyperhopper 5y ago
  I would be fuming if I were an engineer depending on that, but obviously in a public communication like that, where you are explaining to clients why you charged them erroneously, just pointing a finger and acting mad makes you look like you don't have control of your own software.
  - Aeolun 5y ago
    A little bit of passive aggressive comments would have been appropriate I think.
kortex 5y ago
Yep, that gets a pin to "==x.y.z # pinned to prevent BREAKING CHANGE, do not update without reading <link>" and also some defensive regression tests on their api lest someone update by mistake.
fshbbdssbbgdd 5y ago
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.

warpech 5y ago

Perhaps before 7.1 Mongoid had a problem that the logical operators acted differently to ActiveRecord. But come on, this adjustment could have been implemented in another namespace or something...

cedricd 5y ago

It's clearly a breaking change how a core feature of the library behaves. This is extremely unprofessional on the part of the maintainers. I'd completely lose trust in the gem. They knew they were making a breaking change, documented it, and didn't increment a major version number. That breaks the entire point of semver. Also, this is generating SQL ffs. Like how more nasty of a breaking change could you make in terms of potential impact to live apps that upgrade? The experience in the post is a perfect example of how badly this can go wrong.

andrewxdiamond 5y ago
All dependency changes are changes. And changes should be tested before being deployed. If you update your dependencies and ship it based on version numbers alone, you can’t blame the maintainers
- thrwn_frthr_awy 5y ago
  > All dependency changes are changes. And changes should be tested before being deployed. OP did not say changes should not be tested. > If you update your dependencies and ship it based on version numbers alone, you can’t blame the maintainers OP did not if you update your dependencies and ship it based on version numbers alone you can blame the maintainers.
  - Retric 5y ago
    Shipping something with zero testing is always crazy. Even a minor bug fix in a library can expose a critical bug in your own code.
    - kortex 5y ago
      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).

stefan_ 5y ago

The changelog for 7.1 (https://github.com/mongodb/mongoid/blob/master/docs/release-notes/mongoid-7.1.txt https://github.com/mongodb/mongoid/blob/master/docs/release-...) even explicitly call this out as a breaking change: Breaking change: In Mongoid 7.1, when condition methods are invoked on a Criteria object, they always add new conditions to the existing conditions in the Criteria object. Previously new conditions could have replaced existing conditions in some circumstances. Ironically they also have a breaking change in 7.1.1, so they just don't give a fuck at all.

mattigames 5y ago
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)

jimbobimbo 5y ago

Change of semantics in a minor version!? And no way to retain the semantics by flipping a flag or something!? SMH

vasco 5y ago

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.

contravariant 5y ago

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.

Sebb767 5y ago
> That said I would normally read '[...]' as '[...]' The problem is, once you deploy to production you have (hopefully) tested this case and rely on the actual implementation.
jve 5y ago
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.

zomglings 5y ago

Seriously, why the fuck not just bump to 8.0.0 or whatever?

Aeolun 5y ago

They made a new function for the ‘old’ behavior… that makes it even worse. Just make a new one for the new behavior and everyone is happy.

mad182 5y ago

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.

deleted 5y ago

[deleted]