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

encoding/json: multiple anonymous struct fields shouldn't be ignored #11006

Closed
themihai opened this issue May 30, 2015 · 19 comments

Comments

Projects
None yet
3 participants
@themihai
Copy link

commented May 30, 2015

Currently if there are duplicate anonymous struct fields they are all ignored. I think this is wrong because the encoding process shouldn't loose information unless the output format is not capable to represent it(which is not the case).
For example marshalling AllTypes produces an empty output.
type AllTypes struct {
Type1
Type2
}
type Type1 struct {
Name string
Address string
}
type Type2 struct {
Name string
Address string
}
To overcome this issue an additional step is required to create a boilerplate type specially for the encoding/decoding process. Most likely we end-up with a new type (AllTypes2) as below. This is against the composability of the language and adds additional burden to the developer(look for duplicate fields -> create a new type).

type AllTypes2 struct{
Type1 Type1
Type2 Type2
}

Any fix/convention that preserves the data would be fine to me but I present my proposal below too as starting point. I think it's also backward compatible.
Use the same addressing like the native go types.
If there are duplicate anonymous fields, they should be addressed using the type name as prefix. If the type name is duplicate too both the package name and the type name should be used as prefix. For type AllTypes I would expect the output below:

{
"Type1": {"Name":"example", "Address": "example"},
"Type2": {"Name":"example", "Address": "example"}
}

Using the same addressing as on go types makes the behaviour intuitive and easier to understand too as we have only one familiar rule instead of 3 special rules.
It would make the behaviour more consistent too because the type name is already used as key on certain cases. For example type SomeType is encoded to {"MySlice":["somedata"]}.
type SomeType struct{
MySlice
}
type MySlice []string

@themihai themihai changed the title encoding/json: anonymous struct fields shouldn't be ignored encoding/json: multiple anonymous struct fields shouldn't be ignored May 30, 2015

@minux

This comment has been minimized.

Copy link
Member

commented May 30, 2015

This is the documented behavior of encoding/json, so we
can't change it.

http://golang.org/pkg/encoding/json/#Marshal
Anonymous struct fields are usually marshaled as if their inner exported
fields were fields in the outer struct, subject to the usual Go visibility
rules amended as described in the next paragraph. An anonymous struct field
with a name given in its JSON tag is treated as having that name, rather
than being anonymous. An anonymous struct field of interface type is
treated the same as having that type as its name, rather than being
anonymous.

The Go visibility rules for struct fields are amended for JSON when
deciding which field to marshal or unmarshal. If there are multiple fields
at the same level, and that level is the least nested (and would therefore
be the nesting level selected by the usual Go rules), the following extra
rules apply:

  1. Of those fields, if any are JSON-tagged, only tagged fields are
    considered, even if there are multiple untagged fields that would otherwise
    conflict. 2) If there is exactly one field (tagged or not according to the
    first rule), that is selected. 3) Otherwise there are multiple fields, and
    all are ignored; no error occurs.

Handling of anonymous struct fields is new in Go 1.1. Prior to Go 1.1,
anonymous struct fields were ignored. To force ignoring of an anonymous
struct field in both current and earlier versions, give the field a JSON
tag of "-".

You can add struct tags to the embedded fields to achieve what
you want without another type.

http://play.golang.org/p/K0LwyLe7fJ

@minux minux closed this May 30, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

@minux The issue is about the multiple anonymous fields that are ignored due the rule# 3. The tags have their place but I think that by default the encoder should not ignore these fields. It looks wrong to me to ignore fields/loose data by default. I think they should be encoded and if the developer doesn't like the output it can use tags to alter it(i.e ignore them) just like it does for non-multiple-anonymous fields. Let me know if this makes sense to you.
I'm aware that I can use tags but sometimes I don't own the types(i.e belong to a 3rd party package) or at the time I've created the embedded type I didn't consider the JSON encoding. This also adds to the cognitive load and looks like a workaround thus the reason I proposed to replace the 3 special rules with the go standard visibility and addressing rules.
I think a such change is backward compatible becuse it will only encode fields that are currently ignored but the output remains the same for all the other fields just like the change in Go 1.1

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

I think the proposal is backward compatible. It provides/encodes fields that were otherwise ignored(the multiple anonymous fields) but it doesn't change the encoding for the other fields. It's basically one step ahead after the change in Go 1.1.
// Prior to Go 1.1, anonymous struct fields were ignored. To force ignoring of
// an anonymous struct field in both current and earlier versions, give the field
// a JSON tag of "-".
------After the fix-------
// Prior to Go 1.(5?), multiple anonymous struct fields were ignored. To force ignoring of
// an anonymous struct field in both current and earlier versions, give the field
// a JSON tag of "-".

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

I could try a CL if the issue is accepted. I might be wrong but I don't see any reason to ignore the multiple anonymous fields unless there is a technical issue (i.e. the performance), even though I don't think the correctness of the encoder and the developer time should be sacrificed for a small performance gain.

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

The issue is about the conflicting(aka duplicate) multiple anonymous fields which falls under the special rule # 3. I thought it's enough to specify it at the beginning of the thread.
It's worth to mention that "conflicting" is a "made up" term by the json package but the fields themselves are not truly conflicting because they are addressable and they work in Go. I think it was a bad design decision to put them on a conflicting/ignore list thus the reason why I've created this issue.

Concerning the breaking change I'm wondering how is this different than the change in go 1.1. Was that a breaking change too? It looks same to me. People relying on the fact that the anonymous fields were not marshalled had to to add explicit json:"-". Quote from the package doc below[0].
The fact that a such change made into Go 1.1 makes me think that the proposed fix is not a breaking change either and can be included in a future Go 1.x version.
Tbh I doubt people were relaying on the conflicts because doing that to have fields ignored is quite a bad practice. Nevertheless my main argument for the backward compatibility is that we already have a such change in Go 1.1.

[0]
// Prior to Go 1.1, anonymous struct fields were ignored. To force ignoring of // an anonymous struct field in both current and earlier versions, give the field // a JSON tag of "-".

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

That makes sense but it begs the same question: how is the change I propose a breaking change? It just adds meaning to anonymous (embedded) struct fields that are currently ignored(the "conflicting" ones), isn't it?
It's the same kind of change we had on Go 1.1 and actually continues the work from Go 1.1 making the anonymous fields finally first class citizens by encoding them regardless the apparent "conflicting" status. It aligns the encoding process of the embedded fields to the way they work on internally on Go.

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

I fail to see how marshalling the "conflicting" fields would cause a breaking change.
The behaviour changes only for the fields that are currently ignored. The fact that the conflicting fields are ignored is a limitation/restriction of the package which is surpassed by provision. The developer shouldn't expect guarantees to have these fields ignored regardless their conflicting status unless it uses a json:"-" tag in the struct definition.
The existing documented behaviour doesn't change more than it was changed in 1.1. Basically it marshals more anonymous fields that were otherwise ignored due special rules.
Shortly said if the developer doesn't use the json:"-" it has no guarantee that the field won't be marshalled thus the reason why it's not a breaking change.
A breaking change in the behaviour would only exist if existing marshalled fields would stop being marshalled or their structure would change.

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

@minux If they assume that fields without json:"-" tags are ignored they assume wrong just like they did before Go 1.1 for "simple" anonymous fields. If the secret is that secret the developer should better ensure that it won't be leaked under any circumstance. That includes reading the provision to the anonymous fields and using the proper json:"-" tags.
I think a more common use case is actually the other way around. You have Type1 embedded in type T and at some point you embed another type TypeX which has one or more fields embedded or not that conflict with fields from Type1. This makes the Type1 to not be encoded anymore along with the conflicting fields from TypeX. This can be fixed with the tags but as I outlined there are reasons against this approach.
Concerning the output I think this can be discussed and more thought can be put into if the issue is reopened and perhaps others can contribute to the final form.
Using the provided example I think it can encode into { "A": "a", "B":"b", "Type1": {"Secret": "secret"}, {"Type2": "Secret": "secret"} }.
The output the output looks a bit fragmented/ugly due the compatibility issue but perhaps there are better forms to consider. Nevertheless I still believe that any solution is better than ignoring the field entirely. It looks better when all the members are conflicting (i.e. the example I provided at the beginning of the thread) but I don't think that's an issue for the Go developers as long as the data is unmarshallable using the same type definition.

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

Trying to find an analogy I imagine what would be if the "conflicting" fields would not be exported in a Go program (unless you use tags; the behaviour we currently have on the json package). That would pretty much suck isn't it? I think it's the same case for the encoding process as the conflicting fields are not encoded unless you use tags, thus it makes the development of JSON enabled services more laborious and prone to errors.

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

I can read this[0] on the current go documentation. I think it makes a clear statement for the fields you want to ignore, such sensitive information. Relaying on an implementation detail to avoid data leaking doesn't sounds right to me nor I believe people responsible(i.e. sane) with sensitive data do that. Not to mention that the behaviour is unstable and prone to errors because if one conflicting field is removed the data is leaked from the other one.

Handling of anonymous struct fields is new in Go 1.1. Prior to Go 1.1, anonymous struct fields were ignored. To force ignoring of an anonymous struct field in both current and earlier versions, give the field a JSON tag of "-".

@minux

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@themihai

This comment has been minimized.

Copy link
Author

commented May 31, 2015

Ok then...unless additional functions in Go 1.x (i.e. MarshalExtended) with a different behaviour to handle the conflicting fields could be considered, can label this as an issue for Go 2.0?

@themihai

This comment has been minimized.

Copy link
Author

commented Jun 8, 2015

I've ended-up with a generator which rewrites the structs I'm using for encoding and adds tags automatically inferred from the type name or type + pkg id where so-called "conflicts" exists.
Nevertheless it still feels like a hack and I hope you will reconsider the package behaviour in Go 2.0.

@golang golang locked and limited conversation to collaborators Jun 25, 2016

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.