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

[Andrea Bolognani]

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

  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.

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.

-- 
Andrea Bolognani / Red Hat / Virtualization