7 ms·

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 t

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).
      - brian_cloutier 5y ago
        In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught this bug. It sounds like the user story is: - A user runs a query - The user does not have any queries left in their plan - They are billed. - When the user runs the query again they are not billed a second time. I'm struggling to imagine how the test would have failed to catch this. Maybe it was unit tested but with a database containing just one user? Maybe they got very unlucky and the correct user was randomly chosen?
        kortex 5y ago
        Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
        lucumo 5y ago
        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.
        5y ago
- deleted 5y ago
  [deleted]
- z3t4 5y ago
  Why update at all if the current version pass all tests though? So in order to update a dependency you must first write a test that fail on current version and is green on updated version.
- kcartlidge 5y ago
  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.

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).
    - brian_cloutier 5y ago
      In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught this bug. It sounds like the user story is: - A user runs a query - The user does not have any queries left in their plan - They are billed. - When the user runs the query again they are not billed a second time. I'm struggling to imagine how the test would have failed to catch this. Maybe it was unit tested but with a database containing just one user? Maybe they got very unlucky and the correct user was randomly chosen?
      - kortex 5y ago
        Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
      - lucumo 5y ago
        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.
        5y ago
deleted 5y ago
[deleted]
z3t4 5y ago
Why update at all if the current version pass all tests though? So in order to update a dependency you must first write a test that fail on current version and is green on updated version.
kcartlidge 5y ago
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.