4 ms·

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

by 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...

onli 1y ago
I will try to understand your position, and thanks for answering. I appreciated the "avoiding breakage" sentiment included in the post (and commented accordingly below). I'm just really of the opinion that dependencies should not trigger me to do something like set `allow_duplicated_key: true` if it is not absolutely necessary, and I guess I do not see why it is absolutely necessary here. I saw the link to a security vulnerability, but why not let devs set `allow_duplicated_key: false` manually in contexts where it matters? Avoiding churn is more important - this is not a "the api will cause random code to be executed" situation, unlike the create_additions options situation you describe and possibly unlike the YAML situation. There I understand the need (even with YAML, there I'm just sad about the currently broken default code path). Also, we saw with Yaml how there it wasn't viable to do such a change without breakage via the intermittent dependencies (and I was very happy you mentioned that API not being nice) and churn via the direct. The same is very likely to happen here, programs will break because their dependencies to not set the new option when those store JSON.
- byroot 1y ago
  > but why not let devs set `allow_duplicated_key: false` manually in contexts where it matters? Because that wouldn't have prevented the issue I linked to. Default settings are particularly important, because most of the gem's users have no idea that duplicated keys are even a concern. JSON is used a lot to parse untrusted data, as such having strict and safe default is particularly valuable. In your case the JSON documents are trusted, so I understand this change is of negative value for you, but I believe it has positive value overall when I account for all the users of the gem. Additionally, even for the trusted configuration file or similar case, I believe most users would see it as valuable to not let duplicated keys unanswered, because duplicated keys are almost always a mistake and can be hard to track down. e.g. a developer might have some `config.json` with: { "enabled": true, // many more keys, "enabled": false, } And waste time figuring out why `JSON.parse(doc)["enabled"]` is `false` when they can see it's clearly `true` in their config. So again, I understand you are/will be annoyed, because your use case isn't the majority one, but I'm trying to cater to lots of different users. If going over your code to add the option is really too much churn for your low maintenance project, as I mention in the post, for such cases a totally OK solution is to monkey patch and move on: require "json" module JSONAllowDuplicateKey def parse(doc, options = nil) options = { allow_duplicate_key: true }.merge(options || {}) super(doc, options) end end JSON.singleton_class.prepend(JSONAllowDuplicateKey)
  - onli 1y ago
    It is the other way around, I am convinced. In the config scenario you describe the input is also trusted, as will be the bulk of JSON usage. In all those scenarios duplications will happen regularly and developers rely on the JSON parser to work with the input regardless. Which means they all will have to churn the change required by this to get a working program again, one that does not crash with input that passed before. I get that the option to detect and prevent this is good. The warning is seriously useful. But it does not need to be a new default to forbid the old valid inputs. And I do not understand why the option to do so would not have helped in the hacker issue. Thanks for the code snippet. Actually, it is not a lot of code for me to change and likely I do not have an intermediate dependency, I hope. Problem is the dependencies vastly outnumbering my program and thus work like this potentially adding up. Dont feel obliged to further discuss if it is a waste of your time - but it is important for me to try to argue against churn.

by byroot 1y ago

onli 1y ago

I will try to understand your position, and thanks for answering. I appreciated the "avoiding breakage" sentiment included in the post (and commented accordingly below). I'm just really of the opinion that dependencies should not trigger me to do something like set `allow_duplicated_key: true` if it is not absolutely necessary, and I guess I do not see why it is absolutely necessary here. I saw the link to a security vulnerability, but why not let devs set `allow_duplicated_key: false` manually in contexts where it matters? Avoiding churn is more important - this is not a "the api will cause random code to be executed" situation, unlike the create_additions options situation you describe and possibly unlike the YAML situation. There I understand the need (even with YAML, there I'm just sad about the currently broken default code path). Also, we saw with Yaml how there it wasn't viable to do such a change without breakage via the intermittent dependencies (and I was very happy you mentioned that API not being nice) and churn via the direct. The same is very likely to happen here, programs will break because their dependencies to not set the new option when those store JSON.

byroot 1y ago
> but why not let devs set `allow_duplicated_key: false` manually in contexts where it matters? Because that wouldn't have prevented the issue I linked to. Default settings are particularly important, because most of the gem's users have no idea that duplicated keys are even a concern. JSON is used a lot to parse untrusted data, as such having strict and safe default is particularly valuable. In your case the JSON documents are trusted, so I understand this change is of negative value for you, but I believe it has positive value overall when I account for all the users of the gem. Additionally, even for the trusted configuration file or similar case, I believe most users would see it as valuable to not let duplicated keys unanswered, because duplicated keys are almost always a mistake and can be hard to track down. e.g. a developer might have some `config.json` with: { "enabled": true, // many more keys, "enabled": false, } And waste time figuring out why `JSON.parse(doc)["enabled"]` is `false` when they can see it's clearly `true` in their config. So again, I understand you are/will be annoyed, because your use case isn't the majority one, but I'm trying to cater to lots of different users. If going over your code to add the option is really too much churn for your low maintenance project, as I mention in the post, for such cases a totally OK solution is to monkey patch and move on: require "json" module JSONAllowDuplicateKey def parse(doc, options = nil) options = { allow_duplicate_key: true }.merge(options || {}) super(doc, options) end end JSON.singleton_class.prepend(JSONAllowDuplicateKey)
- onli 1y ago
  It is the other way around, I am convinced. In the config scenario you describe the input is also trusted, as will be the bulk of JSON usage. In all those scenarios duplications will happen regularly and developers rely on the JSON parser to work with the input regardless. Which means they all will have to churn the change required by this to get a working program again, one that does not crash with input that passed before. I get that the option to detect and prevent this is good. The warning is seriously useful. But it does not need to be a new default to forbid the old valid inputs. And I do not understand why the option to do so would not have helped in the hacker issue. Thanks for the code snippet. Actually, it is not a lot of code for me to change and likely I do not have an intermediate dependency, I hope. Problem is the dependencies vastly outnumbering my program and thus work like this potentially adding up. Dont feel obliged to further discuss if it is a waste of your time - but it is important for me to try to argue against churn.