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

Use CBOR tags to disambiguate which struct to fill an interface field with #8

Open
rayhaanj opened this issue May 29, 2018 · 4 comments
Assignees

Comments

@rayhaanj
Copy link
Collaborator

If a struct like the following is passed to the Unmarshaler, then it is unclear as to how the unmarshaler would know which concrete type to fill into the struct field:

type ConvolutedIndirectable interface {
  ConvolutedIndirection() int
}

type Indirector struct {
  I int
}

func (i *Indirector) ConvolutedIndirection() int {
  return i.I
}

type One struct {
  A ConvolutedIndirectable
}

func main() {
  reader := ...
  var o One
  if err := reader.Unmarshal(&o); err != nil {
    ...
  }
}

When the struct is marshaled the information about what struct was actually in the interface is lost, only an un-type-annotated map is transferred via CBOR, so it does not seem possible to work out what type to create to put in this field when unmarshaling.

@rayhaanj rayhaanj self-assigned this May 29, 2018
rayhaanj added a commit that referenced this issue May 29, 2018
Added support for slices containing pointers to struct.

Started cleaning up support for different int sizes in structs. These seem to
require some manual coding and shrinking to smaller types. More implementation
and exhaustive testing is needed in this regard.

We should also clean up all the type assertions to check the type and return an
error if there is a mismatch instead of panicing because it would be bad for a
server process to panic when a client sends malformed CBOR.

For the purposes of using this library for RAINS, issue #8 will need to be fixed
which seems most suitable by not having interface types in the data struct, as
this is how the previous cap'n proto implementation was working. Personally I
feel that if there is only one implementation of that interface there is not
much benefit for it being an interface, it is much easier to just have a struct
which the needed methods are implemented on. If replacement is a concern then
it could be placed in a different package which can be swapped out easily.
@britram
Copy link
Owner

britram commented May 30, 2018

tl;dr This is what CBOR tags are for.

In the case that there is not enough semantic information in the CBOR object to determine what Go type an object needs to be (where this semantic information can come from the containing object, as in the RAINS message case, where the message section type is defined by the content of the section), that object should be preceded with a tag.

Missing from Read() is an option to interpret a CBORTag as a type prefix. This would require the addition of tag registry (maybe in typeinf.go) that mapped tags to constructors of structs that implement CBORUnmarshaler. Read() would then, before returning a tag, check its tag registry to determine if it had a registered unmarshaler, and if so, pass the stream after the tag to said unmarshaler.

We could use this to handle timestamps automatically, as well as to allow Read() to return RAINSMessage objects based on the RAINS tag, see the RAINS spec and https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml.

(Will leave it up to @rayhaanj as whether you want this as a separate issue, or just want to mutate this issue to be about tag reading. I can also drop by CAB before the group meering to discuss if it'd be helpful)

@rayhaanj
Copy link
Collaborator Author

I delved too deep into the reflection that I forgot about CBOR tags. I'll work on using tags to disambiguate the structs that go into interfaces, that seems like a nice solution (and rename this issue to that).

@rayhaanj rayhaanj changed the title Unmarshal with interface type in struct? Use CBOR tags to disambiguate which struct to fill an interface field with May 30, 2018
@rayhaanj
Copy link
Collaborator Author

rayhaanj commented Jun 6, 2018

Been going back and forth on this for a while, but it seems that it is unnecessary to have the registered structs themselves implement CBORUnmarshaler since if we already know which constructor to use then we might as well use the automatic struct decoding on that struct.

The problem is knowing which struct is being read in, and this can simply be done within borat itself, however will require the map parser to be changed to return a map which contains potentially tagged items. Then the struct reader can interpret the tag and choose the appropriate structure to unmarshal into. If the map is just being read as a normal map then the optionally tagged items can be converted into untagged items and returned to the caller.

So I'll clean up and follow this plan now.

@geoah
Copy link

geoah commented Nov 6, 2018

I do apologise if this is the wrong place to ask but would this also allow borat to tag structs? And if so is this something that is being worked on?

Thank you :)

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

No branches or pull requests

3 participants