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

[Victor Toso <victortoso@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5350 bytes --]

Hi,

On Fri, Aug 19, 2022 at 10:25:26AM -0500, Andrea Bolognani wrote:
> > 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.

In general, I too prefer to return early! So I agree with you
that it is nicer. At the same time, I was thinking a bit from the
code generator point of view and the overall expected diffs when
generating new code.  More below.

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

Main problem with this approach is that there is some big unions
like BlockdevOptions, this means that we would have to repeat all
fields by the number fields times (40+ in BlockdevOptions' case).

> You should have enough information to produce such a check when
> generating the function, right?

We do!

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

I started with this approach actually but then I realized that we
can just keep the if checks and instead of counter, do check the
variable bytes []byte. So, do you think that the counter is
actually preferred instead of inner check? I don't mind changing
it.

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

I think this one adds a bit more complexity and I'm not convinced
that the gains are worth.

For example, with types that differ over fields with the same
name? We would need to move that inside the anonymous struct
somehow;

For example,if Luks's KeySecret was KeyScretType we would have to
use KeySecretLuks *KeySecretType. Still, both of them would
likely be inside of "key-secret" json object (so, same key for
two different fields, not really sure how that would work out)

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

It does sound nice to parse it once but if we really want to do
that, we would need to work with Golang's JSON decoder [0]. IMHO,
parsing twice with in-memory data might be okay for the moment
while we are trying to keep things simple and later we could
benchmark against a custom decoder [0] in the future.

[0] https://pkg.go.dev/encoding/json#Decoder

Cheers,
Victor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]