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

proposal: encoding/json: allow merging unmarshalled map value into existing map value #21857

Closed
bgaifullin opened this issue Sep 13, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@bgaifullin
Copy link
Contributor

commented Sep 13, 2017

What version of Go are you using (go version)?

go version go1.8 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/b.gaifullin/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qn/p8wx9pvd3dd90xmd6w_kx50c0000gn/T/go-build536782081=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

https://play.golang.org/p/Cv7cGN6hdv

What did you expect to see?

I expect that structure will not be re-created during decoding and result will be:
"a": &main.T{S0:"a", S1:"c"}

What did you see instead?

I see that json.Decode overrides values in map by new items.
the result is:
"a": &main.T{S0:"", S1:"c"}

@dsnet dsnet changed the title [Proposal] re-use existing map value in encoding/json proposal: encoding/json: allow unmarshaling into existing map value Sep 13, 2017

@gopherbot gopherbot added this to the Proposal milestone Sep 13, 2017

@gopherbot gopherbot added the Proposal label Sep 13, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

I thought I'd seen this before, but I can't find another issue in this tracker.

Have you tried alternatives, such as merging the maps afterward?

I also assume that if we're to make the decoder incremental, it should be consistent across all types - struct fields and slices too, for example.

@bgaifullin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

I have tried to merge map afterwards, but I encountered the following issues:

  • How to detect that field was specified in input data. (pointers can be used for this, but it is convenient)
  • map[string]T which predefined items, where T is an interface cannot be used as target for Decode.

enconding/json do not override struct fields and the structure can be used instead of map.

@rsc rsc changed the title proposal: encoding/json: allow unmarshaling into existing map value proposal: encoding/json: allow merging unmarshalled map value into existing map value Oct 16, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2017

Because it wasn't clear to me, this is not about reusing a map during Unmarshal, which Unmarshal already does, but instead about some way to merge a specific values contained in the map already under a given key with the new data in the JSON value with the same key. In general this is not a well-formed operation, so it would have to be opt-in. It seems too specialized to be worth the complexity it would incur in package json. (Package json is already quite complex.)

If you really need this, my suggestion would be to write a custom merger or to fork the json package and modify your own copy.

@rsc rsc closed this Oct 16, 2017

@bgaifullin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

@rsc I do not agree. When I unmarshal structure, the fields already reused.
when I unmarshal map, the items always re-created. IMHO, the behaviour should identical.
and this is not about merge, it is about re-using existing map values instead of creating a new one, which also implemented for struct fields.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2017

In both the struct and the map case, a specific element is being updated. In your example, you are updating the map element for the key "a". You are asking for something further: you want to say that if the map element for the key "a" already exists, you want to update only specific fields. As Russ said, you want a merge operation.

@bgaifullin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

in case of struct, elements which are not specified in json keeps they original values.
in case of using map, the whole map will be constructed from scratch.

https://play.golang.org

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2017

@bgaifullin, I see your point, and if we were designing json from scratch, we'd probably sit down and think about whether the struct semantics should be changed or the map semantics should be changed or it's OK for them to be subtly different in this way. But we're not designing json from scratch. We have many existing uses that must be taken into account, and I am sure that we cannot change the default behavior here without breaking some existing uses. That leaves us with adding an option that we must maintain, or encouraging people who need this merge semantics to build their own. For now at least we are choosing the latter. If evidence accumulates that merging into existing map values is needed by many users then we will certainly revisit this.

@bgaifullin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

@rsc I got it. thank you.

@golang golang locked and limited conversation to collaborators Oct 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.