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

[Victor Toso]

Hi,

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.

Yep, string manipulation was bad choice. Some sort of anonymous
struct is a better fit. So I'll be changing this ...

> > > > 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"`
>   }

I didn't test this so I could be wrong but, I think this should
not work in case you want to remove the string manipulation.

The marshalling of either HyperV or S390 fields would return a
JSON Object but QAPI spec expects the fields at the same level as
the discriminator's type [0]. So, with this you still need some
string manipulation to remove the extra {}, like I did poorly
without any comment 0:-)

[0] 
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L358

>   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)
>   }
>
>   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
>       }
>       // 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.

If not too verbose, I'd still use anonymous structs whenever
possible. They are created for a given scope, they are basically
self documented in that given context and don't polluted
package's namespace.

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

Thanks!
Victor

signature.asc
Description: PGP signature