3 ms·
> 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. De
by 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 agoIt 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.
- byroot 1y ago> Dont feel obliged to further discuss Just clarifying this one: > I do not understand why the option to do so would not have helped in the hacker issue. Because users aren't omnipotent. When a user need to parse some JSON, they'll reach to `JSON.parse`, which they either already know about or will find from a cursory search. That will have solved their problem so they will not likely look in detail for the dozen or so various options this method takes and won't consider the possibility of duplicated keys nor their potential nefarious impact. Hence why the defaults are so important. > it is important for me to try to argue against churn It's alright, I'm with you on this in general, just not in this particular case.
- onli 1y agoAlright, thanks.