4 ms·

Changing the default behavior for duplicate keys is unnecessary... and therefore should not be done. IMO, it's nuts to purposely introduce random bugs into the

by jmull 1y ago

Changing the default behavior for duplicate keys is unnecessary... and therefore should not be done. IMO, it's nuts to purposely introduce random bugs into the apps of everyone who uses your dependency.

anitil 1y ago
There was a post here recently about how this sort of behaviour on duplicate keys has actually led to security issues (in the context of Go) [0] [0] https://news.ycombinator.com/item?id=44308953 https://news.ycombinator.com/item?id=44308953
- jmull 1y ago
  The issue was triggered by using multiple parsers, with different behaviors. The change of behavior here would prevent the specific problem, but makes the general problem worse. This change creates a new code path that did not exist when all the app code was developed and tested, so there's a decent chance something bad can happen that it's not prepared for.
  - anitil 1y ago
    The thing that I find surprising about this is that it seems so innocuous. Duplicate keys in an object? Meh just take the first or the last or something, no big deal, I'm sure whatever library I'm using will handle it
    - kazinator 1y ago
      RFC 8259 describes these possibilities, accurately quoted in the article: "Many implementations report the last name/value pair only. Other implementations report an error or fail to parse the object, and some implementations report all of the name/value pairs, including duplicates." Keeping the first key/value would be a poor idea since it is a behavior not mentioned in the RFC.
    - jmull 1y ago
      Just to point out, the problem in that issue isn’t really about JSON parsing. It’s about the layer whose job it is to verify/sanitize the input passing the unverified/unsanitized input through to the next layer. The idea that your JSON parser can catch this kind of error in the general case just doesn’t make sense. Changing the default behavior of the parser would catch this specific problem — by coincidence — but will also create new problems. They are as yet unknown, but we will find out (unless someone gains the sense to back off this bad idea before it makes it into production).
onli 1y ago
I agree. This will actually cause me problems. I have a huge selection of mostly handwritten JSON data that powers a benchmark collection. Sometimes, mistakes happen and keys get duplicated. That is not a big problem, if e.g. a graphics card is two times in an array nothing bad can happen. Even if the actual value is wrong because of such a duplication (like a benchmark update gone wrong) that will usually not be a problem, cause there is a lot of other data that will place the component at around the correct position regardless. Now, what will happen is that a ruby update forces me to update the gems, and then the new JSON gem will crash with my data, and then I will have to take time I don't have to fix something that does not need fixing for a project that does not really generate income. Awesome. The right solution here is to specify the unspecified behaviour about the duplicate keys as it is currently for this parser. Then applications randomly running into new bugs is prevented. And then print a warning, but that's done now already. And then maybe offer an opt-in to disallow input data that has duplicated keys. Not to waste the developers time by making the breakage default. And that's why dependencies breaking are unacceptable (if the old behaviour is not completely broken), and relying on invisible deprecation messages is not okay. The sqlite gem did the same thing, completely broke their old API of how parameters could be supplied and did not revert the change even when the chaos that caused was reported, and then took it as a personal insult when I opened a discussion about how that's still a problem. Nice is also the YAML and psych gem, that just this month suddenly could not write a YAML file with YAML::Store.new anymore. I had to workaround that with `file.write(Psych.dump(theyamlhash))`, https://gitlab.com/onli/sustaphones/-/commit/fecae4bb2ee36c868583182c363b419de1753773 https://gitlab.com/onli/sustaphones/-/commit/fecae4bb2ee36c8... is the commit. If I got that right this is about `permitted_classes: [Date]` not being given and not being givable to psych in YAML::Store. Made me pause. Is the ruby community breaking? I'm not aware of gems producing developer hostile situations like that before.
- byroot 1y ago
  If you don't care about duplicated keys, all you got to do is to set the `allow_duplicated_key: true` option. The whole point of the article is to explain why while I have empathy for code owners that will be impacted by various changes sometimes I believe the benefits outweigh the cost. I even linked to an example of a very nasty security issue that would have been prevented if that change had been made sooner. > Is the ruby community breaking? No, the community is trying to fix past mistakes. For instance the YAML change you are cursing about has been the source of numerous security vulnerabilities, so tenderlove had to do something to remove that massive footgun. I totally get that it annoyed you, you can't imagine how much code I had to update to deal with that one. But again, while I totally agree maintainers should have empathy for their users, and avoid needless deprecations, it'd be good if users had a bit of empathy for maintainers that are sometimes stuck in "damned if you do, damned if you don't" situations...

by jmull 1y ago

anitil 1y ago

There was a post here recently about how this sort of behaviour on duplicate keys has actually led to security issues (in the context of Go) [0] [0] https://news.ycombinator.com/item?id=44308953 https://news.ycombinator.com/item?id=44308953

jmull 1y ago
The issue was triggered by using multiple parsers, with different behaviors. The change of behavior here would prevent the specific problem, but makes the general problem worse. This change creates a new code path that did not exist when all the app code was developed and tested, so there's a decent chance something bad can happen that it's not prepared for.
- anitil 1y ago
  The thing that I find surprising about this is that it seems so innocuous. Duplicate keys in an object? Meh just take the first or the last or something, no big deal, I'm sure whatever library I'm using will handle it
  - kazinator 1y ago
    RFC 8259 describes these possibilities, accurately quoted in the article: "Many implementations report the last name/value pair only. Other implementations report an error or fail to parse the object, and some implementations report all of the name/value pairs, including duplicates." Keeping the first key/value would be a poor idea since it is a behavior not mentioned in the RFC.
  - jmull 1y ago
    Just to point out, the problem in that issue isn’t really about JSON parsing. It’s about the layer whose job it is to verify/sanitize the input passing the unverified/unsanitized input through to the next layer. The idea that your JSON parser can catch this kind of error in the general case just doesn’t make sense. Changing the default behavior of the parser would catch this specific problem — by coincidence — but will also create new problems. They are as yet unknown, but we will find out (unless someone gains the sense to back off this bad idea before it makes it into production).

onli 1y ago

I agree. This will actually cause me problems. I have a huge selection of mostly handwritten JSON data that powers a benchmark collection. Sometimes, mistakes happen and keys get duplicated. That is not a big problem, if e.g. a graphics card is two times in an array nothing bad can happen. Even if the actual value is wrong because of such a duplication (like a benchmark update gone wrong) that will usually not be a problem, cause there is a lot of other data that will place the component at around the correct position regardless. Now, what will happen is that a ruby update forces me to update the gems, and then the new JSON gem will crash with my data, and then I will have to take time I don't have to fix something that does not need fixing for a project that does not really generate income. Awesome. The right solution here is to specify the unspecified behaviour about the duplicate keys as it is currently for this parser. Then applications randomly running into new bugs is prevented. And then print a warning, but that's done now already. And then maybe offer an opt-in to disallow input data that has duplicated keys. Not to waste the developers time by making the breakage default. And that's why dependencies breaking are unacceptable (if the old behaviour is not completely broken), and relying on invisible deprecation messages is not okay. The sqlite gem did the same thing, completely broke their old API of how parameters could be supplied and did not revert the change even when the chaos that caused was reported, and then took it as a personal insult when I opened a discussion about how that's still a problem. Nice is also the YAML and psych gem, that just this month suddenly could not write a YAML file with YAML::Store.new anymore. I had to workaround that with `file.write(Psych.dump(theyamlhash))`, https://gitlab.com/onli/sustaphones/-/commit/fecae4bb2ee36c868583182c363b419de1753773 https://gitlab.com/onli/sustaphones/-/commit/fecae4bb2ee36c8... is the commit. If I got that right this is about `permitted_classes: [Date]` not being given and not being givable to psych in YAML::Store. Made me pause. Is the ruby community breaking? I'm not aware of gems producing developer hostile situations like that before.

byroot 1y ago
If you don't care about duplicated keys, all you got to do is to set the `allow_duplicated_key: true` option. The whole point of the article is to explain why while I have empathy for code owners that will be impacted by various changes sometimes I believe the benefits outweigh the cost. I even linked to an example of a very nasty security issue that would have been prevented if that change had been made sooner. > Is the ruby community breaking? No, the community is trying to fix past mistakes. For instance the YAML change you are cursing about has been the source of numerous security vulnerabilities, so tenderlove had to do something to remove that massive footgun. I totally get that it annoyed you, you can't imagine how much code I had to update to deal with that one. But again, while I totally agree maintainers should have empathy for their users, and avoid needless deprecations, it'd be good if users had a bit of empathy for maintainers that are sometimes stuck in "damned if you do, damned if you don't" situations...