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

[Andrea Bolognani]

On Wed, Jul 06, 2022 at 10:48:06AM +0100, Daniel P. Berrangé 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:
> > >   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.

It does, because the struct it's filling with data
(jsonGuestPanicInformation) covers all possible cases. We then pick
only the part we care about and transfer it to the user-provided
return location.

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

I guess this version would work too, but I think it might still
perform more computation than the one I suggested?

json.RawMessage is a type alias for []byte, so I think the first call
to json.Unmarshal will have to go through the message to figure out
its structure and the contents of the discriminator field, then
tweak the original input so that only the part that hasn't been
consumed yet is returned. The second call to json.Marshal will then
parse that part, which means that the "inner" chunk of JSON ends up
being processed twice. In the approach I suggested, you go over the
entire JSON in one go.

Things might even out when you take into account allocating and
copying data around though. I'm not particularly interested in
splitting hair when it comes to the most efficient approach at this
point in time :) Knowing that we're not going through the entire JSON
twice is good enough.

-- 
Andrea Bolognani / Red Hat / Virtualization