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

feature: support deserializing registered CBOR tag to an interface type #301

Closed
Gui-Yom opened this issue Nov 29, 2021 · 5 comments · Fixed by #308
Closed

feature: support deserializing registered CBOR tag to an interface type #301

Gui-Yom opened this issue Nov 29, 2021 · 5 comments · Fixed by #308
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Gui-Yom
Copy link

Gui-Yom commented Nov 29, 2021

What version of fxamacker/cbor are you using?

2.3.0

Does this issue reproduce with the latest release?

What OS and CPU architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Guillaume\AppData\Local\go-build
set GOENV=C:\Users\Guillaume\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\Guillaume\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Guillaume\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\apps\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\apps\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.17.3
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=D:\Coding\Go\test-cbor\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\GUILLA~1\AppData\Local\Temp\go-build758706208=/tmp/go-build -gno-record-gc c-switches

What did you do?

This code prints an err at the Unmarshal step.

type B interface {
	Foo()
}

type C struct {
	Field int
}

func (c C) Foo() {

}

type A struct {
	Field B
}

func main() {
	tags := cbor.NewTagSet()
	err := tags.Add(cbor.TagOptions{EncTag: cbor.EncTagRequired, DecTag: cbor.DecTagRequired}, reflect.TypeOf(C{}), 279)
	if err != nil {
		panic(err)
	}
	encMode, _ := cbor.PreferredUnsortedEncOptions().EncModeWithTags(tags)
	decMode, _ := cbor.DecOptions{}.DecModeWithTags(tags)

	data, _ := encMode.Marshal(A{Field: C{Field: 5}})
	var orig A
	err = decMode.Unmarshal(data, &orig)
	if err != nil {
		panic(err)
	}
	fmt.Println(orig)
}

I either do not understand how to use this library or there is a bug somewhere.

What did you expect to see?

Not a panic. Unmarshal should be working.

What did you see instead?

panic: cbor: cannot unmarshal map into Go struct field main.A.Field of type main.B

If I define the interface B like that instead :

type B interface {}

B becomes an alias to interface{} and then Unmarshal works, correctly reading the cbor tag.

@fxamacker
Copy link
Owner

fxamacker commented Nov 30, 2021

Thanks for reporting this. The decoder needs to know the specific Go type that implements the interface to decode to.

One workaround is to make this change:

  • Decoder needs to decode to orig := A{Field: &C{}}, instead of var orig A.

This scenario is interesting because CBOR data contains a tag number which decoder might be able to use to create an object without any other hint. To determine if supporting this (without the workaround) is feasible, I'll need to look into it.

Please let me know if the workaround resolves this issue for you.

https://go.dev/play/p/i6EszRImp2__b

package main

import (
	"fmt"
	"reflect"

	"github.com/fxamacker/cbor/v2"
)

type B interface {
	Foo()
}

type C struct {
	Field int
}

func (c C) Foo() {
}

type A struct {
	Field B
}

func main() {
	tags := cbor.NewTagSet()
	err := tags.Add(cbor.TagOptions{EncTag: cbor.EncTagRequired, DecTag: cbor.DecTagRequired}, reflect.TypeOf(C{}), 279)
	if err != nil {
		panic(err)
	}
	encMode, _ := cbor.PreferredUnsortedEncOptions().EncModeWithTags(tags)
	decMode, _ := cbor.DecOptions{}.DecModeWithTags(tags)

	data, _ := encMode.Marshal(A{Field: C{Field: 5}})

	orig := A{Field: &C{}}
	err = decMode.Unmarshal(data, &orig)
	if err != nil {
		panic(err)
	}
	fmt.Printf("decoded to %+v, Field %+v\n", orig, orig.Field)
}

@fxamacker fxamacker added the enhancement New feature or request label Nov 30, 2021
@fxamacker fxamacker changed the title bug: Deserializing a field with an interface type doesn't work feature: support deserializing registered CBOR tag to an interface type Nov 30, 2021
@fxamacker fxamacker added the undecided Final decision hasn't been made yet label Nov 30, 2021
@Gui-Yom
Copy link
Author

Gui-Yom commented Nov 30, 2021

Well, the problem is that with your workaround, I loose the main point : decoding an unknown struct bound by an interface.
Consider this case :

type Task interface {
	Do()
}

type MoveTask struct {
	// omitted
}

// MoveTask impl Task

type SingTask struct {
	// omitted
}

// SingTask impl Task

type Tasks struct {
	Tasks []Task
}

I can encode but not decode :

	data, _ := encMode.Marshal(Tasks{Tasks: []Task{MoveTask{}, SingTask{}}})
	var tasks Tasks // I can't specify the type more than that
	err = decMode.Unmarshal(data, &tasks)
	if err != nil {
		panic(err)
	}
	fmt.Println(orig)

Usually, libraries that support this add a discriminator (manually or automatically defined) to the serialized objects that identify the type. I tought that it was the intent of CBOR tags since I also specify the Go type, but it apparently isn't.

Also, another surprising bit. If I relieve the interface constraint like that :

type Task struct {
	Tasks []interface{}
}

It is now able to decode each struct in the slice properly (given I define a CBOR tag for each struct).

@fxamacker
Copy link
Owner

Yes, I agree the workaround isn't exactly the same as what you were trying to do.

The difference in behavior you describe is because the decoder takes two different paths depending on the type being decoded to:

  • interface{}
  • all other types

Thanks for going into more detail. I'll try to make time to look into it. It seems a worthwhile improvement. 👍

@fxamacker fxamacker removed the undecided Final decision hasn't been made yet label Dec 6, 2021
@fxamacker fxamacker added this to the v2.4.0 milestone Dec 6, 2021
@fxamacker
Copy link
Owner

@Gui-Yom I'll implement and include this feature in the next release.

@fxamacker
Copy link
Owner

@Gui-Yom I created PR #308 to resolve this issue. Please take a look and let me know if it works for you.

@fxamacker fxamacker self-assigned this Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants