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
signature.asc
Description: PGP signature