Consider erroring on JSON duplicate keys instead of "last value wins"

[zamicol]

For an implementation with this rule applied, see https://github.com/Cyphrme/Coze/blob/76f019b67712c0dc16cdde9895cff5448342d63f/orderedmap.go#L31

Duplicate fields are a problem in JSON that wasn't addressed by the original spec.

After JSON was widely adopted, Douglas Crockford, JSON's inventor, tried to fix this but it was decided it was too late.

Although Douglas Crockford couldn't change the spec forcing all implementations to error on duplicate, his Java JSON implementation errors on duplicate names. Others use last-value-wins, support duplicate keys, or other non-standard behavior. The JSON RFC states that implementations should not allow duplicate keys, notes the varying behavior of existing implementations, and states that when names are not unique, "the behavior of software that receives such an object is unpredictable." Also note that Javascript objects (ES6) and Go structs already require unique names.

Duplicate fields are a security issue, a source of bugs, and a surprising behavior to users. See the article, "An Exploration of JSON Interoperability Vulnerabilities"

Disallowing duplicates conforms to the small I-JSON RFC. The author of I-JSON, Tim Bray, is also the author of current JSON specification (RFC 8259). See also https://github.com/json5/json5-spec/issues/38.

[iancoleman]

This is a really informative comment, thanks for raising it, much appreciated.

The behavior of orderedmap will follow the standard map functionality in golang, so I can't see this change being implemented unless go changes the behavior of the base map type.

Further reference https://go.dev/ref/spec#Composite_literals

For map literals, all elements must have a key. It is an error to specify multiple elements with the same field name or constant key value. For non-constant map keys, see the section on evaluation order.

An area that I feel is good to explore is using const for keys, which might be desired to throw a compile-time error. In this lib, const keys don't throw that error. eg:

func TestDuplicateKeys(t *testing.T) {
    // can use duplicate keys if not const, works same as map[key] = val
    canDuplicate := New()
    canDuplicate.Set("notConstKey", "initial")
    canDuplicate.Set("notConstKey", "overriden")
    val, _ := canDuplicate.Get("notConstKey")
    // expect duplicate key to be overridden
    if val.(string) == "initial" {
        t.Error("Duplicate key was not overridden")
    }
    // can not use duplicate keys if const - maybe not the best behavior by the lib here?
    cannotDuplicate := New()
    const constKey string = "constKey"
    cannotDuplicate.Set(constKey, "initial")
    // possible compile error on the next line, but it doesn't
    cannotDuplicate.Set(constKey, "duplicated")
}

I'm open to further thoughts on this issue.

[zamicol]

Sorry I should have mentioned this is only for JSON, so the check would be added to UnmarshalJSON. It is not changing the behavior of OrderedMap itself which would still work like Go's map.

See the pull request: https://github.com/iancoleman/orderedmap/pull/35

[iancoleman]

The PR is good and I'd like to merge that if possible, let's keep track of the discussion there. I'll close this and have further discussion in #35.