[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
From: |
Daniel P . Berrangé |
Subject: |
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go |
Date: |
Wed, 6 Jul 2022 10:48:06 +0100 |
User-agent: |
Mutt/2.2.6 (2022-06-05) |
On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > > > All this string manipulation looks sketchy. Is there some reason that
> > > > I'm not seeing preventing you for doing something like the untested
> > > > code below?
> > > >
> > > > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > > > if s.HyperV != nil {
> > > > type union struct {
> > > > Discriminator string `json:"type"`
> > > > HyperV GuestPanicInformationHyperV `json:"hyper-v"`
> > > > }
> > > > tmp := union {
> > > > Discriminator: "hyper-v",
> > > > HyperV: s.HyperV,
> > > > }
> > > > return json.Marshal(tmp)
> > > > } else if s.S390 != nil {
> > > > type union struct {
> > > > Discriminator string `json:"type"`
> > > > S390 GuestPanicInformationHyperV `json:"s390"`
> > > > }
> > > > tmp := union {
> > > > Discriminator: "s390",
> > > > S390: s.S390,
> > > > }
> > > > return json.Marshal(tmp)
> > > > }
> > > > return nil, errors.New("...")
> > > > }
> > >
> > > Using these dummy structs is the way I've approached the
> > > discriminated union issue in the libvirt Golang XML bindings
> > > and it works well. It is the bit I like the least, but it was
> > > the lesser of many evils, and on the plus side in the QEMU case
> > > it'll be auto-generated code.
> >
> > It appears to be the standard way to approach the problem in Go. It
> > sort of comes naturally given how the APIs for marshal/unmarshal have
> > been defined.
> >
> > > > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > > > type Alias GuestPanicInformation
> > > > > peek := struct {
> > > > > Alias
> > > > > Driver string `json:"type"`
> > > > > }{}
> > > > >
> > > > > if err := json.Unmarshal(data, &peek); err != nil {
> > > > > return err
> > > > > }
> > > > > *s = GuestPanicInformation(peek.Alias)
> > > > >
> > > > > switch peek.Driver {
> > > > >
> > > > > case "hyper-v":
> > > > > s.HyperV = new(GuestPanicInformationHyperV)
> > > > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > > > s.HyperV = nil
> > > > > return err
> > > > > }
> > > > > case "s390":
> > > > > s.S390 = new(GuestPanicInformationS390)
> > > > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > > > s.S390 = nil
> > > > > return err
> > > > > }
> > > > > }
> > > > > // Unrecognizer drivers are silently ignored.
> > > > > return nil
> > > >
> > > > This looks pretty reasonable, but since you're only using "peek" to
> > > > look at the discriminator you should be able to leave out the Alias
> > > > type entirely and perform the initial Unmarshal operation while
> > > > ignoring all other fields.
> > >
> > > Once you've defined the dummy structs for the Marshall case
> > > though, you might as well use them for Unmarshall too, so you're
> > > not parsing the JSON twice.
> >
> > You're right, that is undesirable. What about something like this?
> >
> > type GuestPanicInformation struct {
> > HyperV *GuestPanicInformationHyperV
> > S390 *GuestPanicInformationS390
> > }
> >
> > type jsonGuestPanicInformation struct {
> > Discriminator string `json:"type"`
> > HyperV *GuestPanicInformationHyperV `json:"hyper-v"`
> > S390 *GuestPanicInformationS390 `json:"s390"`
> > }
>
> It can possibly be even simpler with just embedding the real
> struct
>
> type jsonGuestPanicInformation struct {
> Discriminator string
> GuestPanicInformation
> }
>
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > if (s.HyperV != nil && s.S390 != nil) ||
> > (s.HyperV == nil && s.S390 == nil) {
> > // client hasn't filled in the struct properly
> > return nil, errors.New("...")
> > }
> >
> > tmp := jsonGuestPanicInformation{}
> >
> > if s.HyperV != nil {
> > tmp.Discriminator = "hyper-v"
> > tmp.HyperV = s.HyperV
> > } else if s.S390 != nil {
> > tmp.Discriminator = "s390"
> > tmp.S390 = s.S390
> > }
> >
> > return json.Marshal(tmp)
> > }
>
> And...
>
> var discriminator string
> if s.HyperV != nil {
> discriminator = "hyper-v"
> } else if s.S390 != nil {
> discriminator = "s390"
> }
>
> tmp := jsonGuestPanicInformation{ discriminator, s}
> return json.Marshal(tmp)
>
> >
> > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > tmp := jsonGuestPanicInformation{}
> >
> > err := json.Unmarshal(data, &tmp)
> > if err != nil {
> > return err
> > }
> >
> > switch tmp.Discriminator {
> > case "hyper-v":
> > if tmp.HyperV == nil {
> > return errors.New("...")
> > }
> > s.HyperV = tmp.HyperV
> > case "s390":
> > if tmp.S390 == nil {
> > return errors.New("...")
> > }
> > s.S390 = tmp.S390
> > }
>
> I'm not actually sure this works, because the first json.Unmarshal
> call won't know which branch to try unmarhsalling. So it might be
> unavoidable to parse twice. With the XML parser this wouldn't be
> a problem as it has separated the parse phase and then fills the
> struct after.
Right afer sending, I remember how this is supposed to be done. It
involves use of 'json.RawMessage' eg examples at:
https://pkg.go.dev/encoding/json#example-RawMessage-Unmarshal
So it would look like:
type GuestPanicInformation struct {
HyperV *GuestPanicInformationHyperV
S390 *GuestPanicInformationS390
}
type jsonGuestPanicInformation struct {
Discriminator string `json:"type"`
Payload *json.RawMessage
}
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
var p *json.RawMesage
var err error
if s.HyperV != nil {
d = "hyper-v"
p, err = json.Marshal(s.HyperV)
} else if s.S390 != nil {
d = "s390"
p, err = json.Marshal(s.S390)
} else {
err = fmt.Errorf("No payload defined")
}
if err != nil {
return []byte{}, err
}
return json.Marshal(jsonGuestPanicInformation{d, p}), nil
}
func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
tmp := jsonGuestPanicInformation{}
err := json.Unmarshal(data, &tmp)
if err != nil {
return err
}
switch tmp.Discriminator {
case "hyper-v":
s.HyperV := GuestPanicInformationHyperV{}
err := json.Unmarshal(tmp.Payload, s.HyperV)
if err != nil {
return err
}
case "s390":
s.S390 := GuestPanicInformationS390{}
err := json.Unmarshal(tmp.Payload, s.S390)
if err != nil {
return err
}
}
return fmt.Errorf("Unknown type '%s'", tmp.Discriminator)
}
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|