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

[Andrea Bolognani]

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("...")
  }

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

The comments made elsewhere about potentially being more strict and
rejecting invalid JSON also apply here.

-- 
Andrea Bolognani / Red Hat / Virtualization