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

[Daniel P . Berrangé]

On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> On Fri, Jun 17, 2022 at 02:19:28PM +0200, Victor Toso wrote:
> > qapi:
> >   | { 'union': 'SetPasswordOptions',
> >   |   'base': { 'protocol': 'DisplayProtocol',
> >   |             'password': 'str',
> >   |             '*connected': 'SetPasswordAction' },
> >   |   'discriminator': 'protocol',
> >   |   'data': { 'vnc': 'SetPasswordOptionsVnc' } }
> >
> > go:
> >   | type SetPasswordOptions struct {
> >   |         // Base fields
> >   |         Password  string             `json:"password"`
> >   |         Connected *SetPasswordAction `json:"connected,omitempty"`
> >   |
> >   |         // Variants fields
> >   |         Vnc *SetPasswordOptionsVnc `json:"-"`
> >   | }
> 
> Generated code:
> 
> > type GuestPanicInformation struct {
> >     // Variants fields
> >     HyperV *GuestPanicInformationHyperV `json:"-"`
> >     S390   *GuestPanicInformationS390   `json:"-"`
> > }
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> >     type Alias GuestPanicInformation
> >     alias := Alias(s)
> >     base, err := json.Marshal(alias)
> >     if err != nil {
> >         return nil, err
> >     }
> >
> >     driver := ""
> >     payload := []byte{}
> >     if s.HyperV != nil {
> >         driver = "hyper-v"
> >         payload, err = json.Marshal(s.HyperV)
> >     } else if s.S390 != nil {
> >         driver = "s390"
> >         payload, err = json.Marshal(s.S390)
> >     }
> >
> >     if err != nil {
> >         return nil, err
> >     }
> >
> >     if len(base) == len("{}") {
> >         base = []byte("")
> >     } else {
> >         base = append([]byte{','}, base[1:len(base)-1]...)
> >     }
> >
> >     if len(payload) == 0 || len(payload) == len("{}") {
> >         payload = []byte("")
> >     } else {
> >         payload = append([]byte{','}, payload[1:len(payload)-1]...)
> >     }
> >
> >     result := fmt.Sprintf(`{"type":"%s"%s%s}`, driver, base, payload)
> >     return []byte(result), nil
> 
> 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.

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

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