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

[Andrea Bolognani]

On Fri, Aug 19, 2022 at 09:20:52AM +0200, Victor Toso wrote:
> > > 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:
> > > > > 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
> > > >    }
> >
> > Similar to what I said in previous email (same thread) to Andrea,
> > this would not work because the end result does not match with
> > QAPI spec, where HyperV or S390 fields should be at the same
> > level as 'type'.

Yeah, you're absolutely correct, I was misreading the schema and
suggested an implementation that couldn't possibly work.

> > If we embed either HyperV or S390, then it should work, like:
> >
> >     tmp := struct {
> >         GuestPanicInformationHyperV
> >         Discriminator string "type"
> >     }{}
>
> For the same reason, I could not use json.RawMessage, sadly.
>
> As Andrea pointed out before, json.RawMessage is just an alias
> for []byte. We need a field for that (so, it can't be embed) and
> the contents of the JSON need to match that field's name.

Right. It would work if unions looked like

  { "type": "...", "data": { ... }}

with the "data" part having different fields based on the value of
"type", but not for the flat JSON dictionaries that are used for QAPI
unions.

> func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
>       var bytes []byte
>       var err error
>       if s.Qcow != nil {
>               tmp := struct {
>                       QCryptoBlockOptionsQCow
>                       Discriminator string `json:"format"`
>               }{
>                       QCryptoBlockOptionsQCow: *s.Qcow,
>                       Discriminator:           "qcow",
>               }
>               if bytes, err = json.Marshal(tmp); err != nil {
>                       return nil, err
>               }
>       }
>       if s.Luks != nil {
>               if len(bytes) != 0 {
>                       return nil, errors.New(`multiple fields set for 
> QCryptoBlockOpenOptions`)
>               }

Why are you checking this here? I would just have a check like

  if s.Qcow != nil && s.Luks != nil {
      return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
  }

at the top of the function, so that you can abort early and cheaply
if the user has provided invalid input instead of having to wait
after one of the fields has already been serialized.

>               tmp := struct {
>                       QCryptoBlockOptionsLUKS
>                       Discriminator string `json:"format"`
>               }{
>                       QCryptoBlockOptionsLUKS: *s.Luks,
>                       Discriminator:           "luks",
>               }
>               if bytes, err = json.Marshal(tmp); err != nil {
>                       return nil, err
>               }
>       }
>       if len(bytes) == 0 {
>               return nil, errors.New(`null not supported for 
> QCryptoBlockOpenOptions`)
>       }

Similarly, this should be checked early. So the complete check could
be turned into

  if (s.Qcow != nil && s.Luks != nil) || (s.Qcow == nil && s.Luks == nil) {
      return nil, errors.New("must set exactly one field")
  }

You should have enough information to produce such a check when
generating the function, right? If making sure all possible
combinations are covered up ends up being too complicated, something
like

  var setFields int
  if s.Field1 != nil {
      setFields += 1
  }
  if s.Field2 != nil {
      setFields += 1
  }
  // ...
  if setFields != 1 {
      return nil, errors.New("must set exactly one field")
  }

would also work.

> func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
>       tmp := struct {
>               Discriminator string `json:"format"`
>       }{}
>       if err := json.Unmarshal(data, &tmp); err != nil {
>               return err
>       }
>       switch tmp.Discriminator {
>       case "qcow":
>               s.Qcow = &QCryptoBlockOptionsQCow{}
>               if err := json.Unmarshal(data, s.Qcow); err != nil {
>                       s.Qcow = nil
>                       return err
>               }
>       case "luks":
>               s.Luks = &QCryptoBlockOptionsLUKS{}
>               if err := json.Unmarshal(data, s.Luks); err != nil {
>                       s.Luks = nil
>                       return err
>               }
>       }
>       return nil
> }

Alternatively, you could define a struct that covers all possible
fields and deserialize everything in one go, then copy the various
parts over to the appropriate struct:

  func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
      tmp := struct {
          Discriminator string  `json:"format"`
          KeySecret     *string `json:"key-secret,omitempty"`
      }{}
      if err := json.Unmarshal(data, &tmp); err != nil {
          return err
      }
      switch tmp.Discriminator {
      case "qcow":
          s.Qcow = &QCryptoBlockOptionsQCow{}
          s.Qcow.KeySecret = tmp.KeySecret
      case "luks":
          s.Luks = &QCryptoBlockOptionsLUKS{}
          s.Luks.KeySecret = tmp.KeySecret
      }
      return nil
  }

Honestly I'm unclear on which option is better. Parsing the JSON
twice, as you're doing in your version, could be problematic when the
document is large; on the other hand, my approach will probably
result in more copies of the same data being created.

-- 
Andrea Bolognani / Red Hat / Virtualization