Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add SetJSONLastValueWins allowing JSON unmarshal to error on duplicate instead of last-value-wins #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zamicol
Copy link

@zamicol zamicol commented Jul 13, 2023

No description provided.

@iancoleman
Copy link
Owner

Thanks for this, looks good. The reason and benefit for this PR is clear (thanks for the excellent comments in the code).

The main hesitation I have with this is surprising behavior compared to the standard encoding/json package.

One of the goals is for OrderedMap to be as close to a drop-in replacement for map as possible, and this would change that expectation.

b := []byte(`{"x":1,"x":2}`)

// normal map behavior
m := map[string]int{}
err := json.Unmarshal(b, &m) // no error

// ordered map behavior
o := New()
err = json.Unmarshal(b, &o) // no error, but would have error if this PR is merged

I tried to see if json v2 in https://github.com/go-json-experiment/json was likely to be incorporated into golang standard libraries but couldn't find any info about it. I'm not sure if deviating from the standard map is desired at this time. Is there some more info about 'the planned revision' in the comment "Until Go releases the planned revision to the JSON package"? Is this planned to be added to encoding/json in the future?

One alternative option might be changing jsonLastValueWins to jsonv2 and only throwing an error if this is true (ie existing behavior is retained, new behavior is opt-in).

@zamicol
Copy link
Author

zamicol commented Aug 11, 2023

For a drop-in replacement, I believe the easy fix is to flip the default value of jsonLastValueWins to true so the default behavior matches the existing behavior. So Go's default value of false can be used, jsonLastValueWins can be renamed to jsonErrorOnDuplicate.

"Until Go releases the planned revision to the JSON package"?

Yes, the Go team has acknowledge that there are many concerns with the existing implementation of JSON, and that it's deserving of a revision. mvdan has confirmed this, and was more explicit on a personal email. The abstract "JSONv2" will include many changes from the existing implementation but there's no firm plans to get this done any time soon.

Edit:

Looks like Go is planning on DisallowDuplicateFields
golang/go#48298

For other JSON problems, this is a good link: https://github.com/golang/go/issues?q=is%3Aissue+is%3Aopen+encoding%2Fjson+in%3Atitle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants