Comment by onli | Hacker News Reader

Comment by onli | Hacker News Reader

Parent Full thread

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.

> 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)

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.

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

Alright, thanks.

Reply on news.ycombinator.com