Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

>       // if we hit none of the cases above, that means the
>       // server has produced a variant we don't know about
> 
>       return nil
>   }
> 
> This avoid parsing the JSON twice as well as having to define
> multiple dummy structs, which keeps the code shorter and more
> readable.
> 
> I've also thrown in some additional error checking for good measure,
> ensuring that we abort when the input is completely nonsensical from
> a semantical standpoint.

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 :|