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: unexported embedded fields are marshalled, but cannot be unmarshalled #18009

Closed
jba opened this issue Nov 21, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@jba
Copy link
Contributor

commented Nov 21, 2016

encoding/json will marshal field foo in

type S struct{ foo }
type foo int

But since foo is unexported, unmarshalling panics.

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

1.7.3

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

linux amd64

What did you do?

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

What did you expect to see?

"{}"

The only field of S is unexported, so it shouldn't marshal.

What did you see instead?

{"foo":1}

and panic on unmarshal.

@dsnet

This comment has been minimized.

Copy link
Member

commented Nov 22, 2016

This is a regression from Go 1.5 (i.e., it started to panic in Go 1.6).

@dsnet dsnet added this to the Go1.9 milestone Nov 22, 2016

@dsnet dsnet added the NeedsFix label Nov 22, 2016

@jba

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2016

To be clear, the bug is in marshalling the unexported field, right?

A related thing to think about: what to do with an unexported anonymous struct field with a tag. It certainly shouldn't be serialized, as happens now. But should it be ignored (because it's a mistake to tag an unexported field), or should the struct be visited for exported fields, as would happen if there weren't a tag?

@thoeni

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

I'm trying to look at tests in place and this one (https://github.com/golang/go/blob/master/src/encoding/json/decode_test.go#L852-L892) would clash with what I'm trying to do for the last StructField which is not exported but has something exported inside:

top := &Top{
		Level0: 1,
		Embed0: Embed0{
			Level1b: 2,
			Level1c: 3,
		},
		Embed0a: &Embed0a{
			Level1a: 5,
			Level1b: 6,
		},
		Embed0b: &Embed0b{
			Level1a: 8,
			Level1b: 9,
			Level1c: 10,
			Level1d: 11,
			Level1e: 12,
		},
		Loop: Loop{
			Loop1: 13,
			Loop2: 14,
		},
		Embed0p: Embed0p{
			Point: image.Point{X: 15, Y: 16},
		},
		Embed0q: Embed0q{
			Point: Point{Z: 17},
		},
		embed: embed{
			Q: 18,
		},
	}

The test says it expects this:
want := "{"Level0":1,"Level1b":2,"Level1c":3,"Level1a":5,"LEVEL1B":6,"e":{"Level1a":8,"Level1b":9,"Level1c":10,"Level1d":11,"x":12},"Loop1":13,"Loop2":14,"X":15,"Y":16,"Z":17,"Q":18}"

Is it expected from an unexported embedded StructField to show its content when exported?

I guess the answer is here: https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L111-L113

@jba

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

Right. According to Go rules, an exported field inside an embedded struct (whether that embedded struct is exported or not) is visible. In other words, this works:

t := &Top{}
t.Q = 18

So "Q" should indeed be a key in the marshaled object.

@thoeni

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

I'm trying to do my first contribution, therefore bear with me.
I've written this test, and I have an implementation that makes it pass (and doesn't break anything existing): does it model correctly what we want to achieve?

If I get to a field which happens to be not exported and not a struct, then it shouldn't be marshalled, but if the unexported field is actually a struct, then its members might be marshalled if exported...

I've edited this after re-reading the documentation, in my test I've used odd numbers for exported ints and even numbers for unexported ones, so I expect to see in the marshalled result only odd numbers no matter which level of the hierarchy they belonged to.

type MyStructContainingUnexportedStruct struct {
	unexportedStructType1
	unexportedIntType
}

type unexportedStructType1 struct {
	ExportedIntType1
	unexportedIntType
	unexportedStructType2
}

type unexportedStructType2 struct {
	ExportedIntType2
	unexportedIntType
}

type unexportedIntType int
type ExportedIntType1 int
type ExportedIntType2 int

func TestUnexportedAnonymousStructWithExportedType(t *testing.T) {
	s2 := unexportedStructType2{3, 4}
	s1 := unexportedStructType1{1, 2, s2}
	a := MyStructContainingUnexportedStruct{s1, 6}
	const want = `{"ExportedIntType1":1,"ExportedIntType2":3}`

	b, err := Marshal(a)
	if err != nil {
		t.Fatalf("Marshal: %v", err)
	}
	if got := string(b); got != want {
		t.Errorf("got %q, want %q", got, want)
	}
}
@jba

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

@thoeni That looks like the correct behavior to me.

@thoeni

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

@jba I'll read the instructions on how to contribute and I'll push my proposal for this fix then :-)
Thanks for clarifying it for me!

@gopherbot

This comment has been minimized.

Copy link

commented Dec 1, 2016

CL https://golang.org/cl/33773 mentions this issue.

@gopherbot gopherbot closed this in 296b353 Jun 14, 2017

@golang golang locked and limited conversation to collaborators Jun 14, 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.