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