On Fri, Aug 19, 2022 at 09:20:52AM +0200, Victor Toso 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: > > > > > 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 > > > > } > > > > Similar to what I said in previous email (same thread) to Andrea, > > this would not work because the end result does not match with > > QAPI spec, where HyperV or S390 fields should be at the same > > level as 'type'.
Yeah, you're absolutely correct, I was misreading the schema and suggested an implementation that couldn't possibly work. > > If we embed either HyperV or S390, then it should work, like: > > > > tmp := struct { > > GuestPanicInformationHyperV > > Discriminator string "type" > > }{} > > For the same reason, I could not use json.RawMessage, sadly. > > As Andrea pointed out before, json.RawMessage is just an alias > for []byte. We need a field for that (so, it can't be embed) and > the contents of the JSON need to match that field's name. Right. It would work if unions looked like { "type": "...", "data": { ... }} with the "data" part having different fields based on the value of "type", but not for the flat JSON dictionaries that are used for QAPI unions. > 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. > 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") } You should have enough information to produce such a check when generating the function, right? 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. > 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 } 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. -- Andrea Bolognani / Red Hat / Virtualization