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/gob: encoder should ignore embedded structs with no exported fields #5819

Open
gopherbot opened this issue Jun 30, 2013 · 22 comments

Comments

@gopherbot
Copy link

commented Jun 30, 2013

by alan.strohm:

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.
1. http://play.golang.org/p/ayZMy_HFKE

What is the expected output?

"worked"

I don't see why hidden1 (an unexported field in an embedded struct) should be handled
differently than hidden2 (a top level unexported field). Making the embedded struct type
itself unexported ("inner" vs "Inner") works but seems unecessary.

What do you see instead?

gob: type main.Inner has no exported fields
@robpike

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2013

Comment 1:

This is a tough one. The reason to leave things alone is that it's a very common user
error to attempt to marshal a struct with no exported fields, and the error message
helps diagnose the case with a clear error.
However, diagnosing it in this more complicated case might be possible.

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@orian

This comment has been minimized.

Copy link

commented Sep 12, 2013

Comment 2:

For consideration, we have:
var req http.Request
It has req.URI.User field of type *url.Userinfo. And url.Userinfo doesn't have exported
fields.
This blocks possibility of dumping http.Request to file using gob.
I also wonder why does it happen when req.URI.User==nil.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 3:

Labels changed: added go1.3maybe.

@dsymonds

This comment has been minimized.

Copy link
Member

commented Dec 2, 2013

Comment 4:

Labels changed: added packagechange.

Owner changed to @robpike.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 5:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 6:

Labels changed: added repo-main.

@glycerine

This comment has been minimized.

Copy link

commented Dec 17, 2013

Comment 7:

This is a real issue when you have structs that contain  an os.File field, for example.
Go has annotations right? Seems like a good use for them here: annotate the field as
being ignorable by gob without error.
@mever

This comment has been minimized.

Copy link

commented Dec 16, 2014

@robpike is there any indication when this is going to be implemented?

@robmccoll

This comment has been minimized.

Copy link

commented Feb 10, 2016

Would it be possible to make the encoder configurable with a handful of flags? Have an interface that structs with unexported fields can meet to serialize / deserialize themselves? Running into problems with a library's configuration struct that includes an x509.CertPool not having any exported fields...

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

First, surely it's wiser to change the x509 library (which can be done as a strict add-on by providing a GobEncoder interface) than to change the transport layer.

Second, if you have configurations in the transport layer you introduce the likelihood that both sides of the transport may need to be configured the same, and that introduces a whole new set of problems.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

@robmccoll

This comment has been minimized.

Copy link

commented Feb 15, 2016

tl;dr my expectation is that gob should be able to cleanly serialize the total visible serializable state of any instance of a Go type.

I was trying to serialize and deserialize cluster configurations within the gocql library: https://godoc.org/github.com/gocql/gocql#ClusterConfig

(Also, embarrassed to say I missed the encoding.Binary{Marshaler,Unmarshaler} interfaces)

Unfortunately I didn't write gocql, so for now I'm using my own struct and copying values over. Either way I find the behavior of gob in this circumstance to be somewhat inconsistent with the rest of the library.

Given that gob does nice things around skipping function pointers, channels, unexported fields, etc. cleanly and making a best effort around type matching when the tags and field names don't match, throwing an error if a struct with no exported fields is visible anywhere in your object hierarchy is surprising. The spirit of gob otherwise seems to be very much do the sane thing with best effort around all exposed state and while I would understand the condition of no exported fields anywhere in the hierarchy being an error, it would be convenient to be able to override the behavior with respect to non-root nodes in the object tree so to speak. Not asking for a change in default behavior, but perhaps increased configurability. Also a change would have no effect on the binary format or the unmarshal / deserialize operation.

Happy to propose a change and put up a PR if you are amenable to it?

@kyhavlov

This comment has been minimized.

Copy link

commented Jul 31, 2016

👍 this can be annoying if you're trying to serialize a type from a dependency and there's a struct with no exported fields embedded in something. In that case the answer shouldn't have to be "change the downstream library just to deal with this".

@eklitzke

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2016

There's a pretty common pattern in the code I'm working on where a sync.RWMutex is embedded into structs that need mutual exclusion, e.g.:

type foo struct {
  sync.RWMutex

  Foo string
  Bar int
}

When trying to encode structs like this I get an error with gob: type sync.RWMutex has no exported fields. It's simple enough to make the mutex into a named field, but handling this case automatically would be really nice.

@ianberdin

This comment has been minimized.

Copy link

commented Oct 29, 2016

Have another way to solve locks for struct fields and convert them to gob?

@bryanjeal

This comment has been minimized.

Copy link

commented Oct 20, 2017

@happierall I know this is quite an old issue, but here is how we solved it:

If we have the following structure foo:

type foo struct {
  sync.RWMutex

  Bar string
  Baz int
}

We create an Encode struct:

type fooEncode struct {
  Bar *string
  Baz *int
}

We then do this to encode:

fooInstance.RLock()
defer fooInstance.RUnlock()

encodable := fooEncode {
  Bar: &fooInstance.Bar,
  Baz: &fooInstance.Baz,
}

err := gobEncoder.Encode(encodable)
...

This is a lot of work for just embedding sync.RWMutex, but we embed more than just sync.RWMutex (it was worth it for us).

@sb10

This comment has been minimized.

Copy link

commented Feb 27, 2018

Any word on this? My use case is having structs that embed log15.Logger. For the one struct that I encode with gob I've had to work-around this by storing it on unexported field, giving the ugly mystruct.logger.Debug("foo"), instead of mystruct.Debug("foo") used everywhere else in my code.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 11, 2018

Change https://golang.org/cl/117875 mentions this issue: encoding/gob: fix handling of embedded structs without exported fields

@iWdGo

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

Unexported fields were correctly handled except structs without exported field for which an error message is returned.

The check returning the error in the Encode is removed in the submitted fix as documentation is not excluding the case. The Decode part is also fixed for idem-potency. Tests have been amended.

@odiferousmint

This comment has been minimized.

Copy link

commented Mar 7, 2019

What is the rationale for the "limitation"? The fields of my structs are intentionally unexported because I do not want any other parts of the program to have direct access to it, but inside this file I would like to encode and decode this struct because I have visibility and it is OK or should be OK to do that.

What should I use instead then? Would encoding/binary work?

Nevermind, I got "invalid type".

@bryanjeal

This comment has been minimized.

Copy link

commented Mar 7, 2019

@odiferousmint

This comment has been minimized.

Copy link

commented Mar 7, 2019

The Encoder you are using isn't part of your code. It is another library - one that might be in the standard library, but still separate from your package. Your package could either create a temporary struct to Encode/Decode or you can write your own Encoder/Decoder within your package.

On Thu, 7 Mar 2019 at 10:30, odiferousmint @.***> wrote: Why is this even a thing, what is the rationale for it? The fields of my structs are intentionally unexported because I do not want any other parts of the program to have direct access to it, but inside this file I would like to encode and decode this struct because I have visibility and it is OK or should be OK to do that. What should I use instead then? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5819 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AJS5jKsaeNQ49LcLAYhPAW1_tn4LzWT5ks5vUUzDgaJpZM4DI-zO .

I see. Thanks. :)

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