Repository: vmware/govmomi
Pull request #3128 (2023-05-16 16:29:33)

fix: support `binary` and `dateTime` in JSON
This change adds support for `binary` and `dateTime` in JSON.

For `binary` golang `[]byte` JSON serialization matches the vCenter format. Thus adding the name ot the maps was enough.

For `dateTime` the format of `time.Time` `Marshaler` and `Unmarshaler` matches vCenter format. The discriminator code required support for custom marshaling logic. In addition the name had to be mapped.

## Description

Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. List any dependencies that are required
for this change.

Closes: #3127

## Type of change

Please mark options that are relevant:

- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

- [X] Unit tests

## Checklist:

- [X] My code follows the `CONTRIBUTION`
  [guidelines](https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md) of
  this project
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [X] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged

akutz: Wait on https://github.com/vmware/govmomi/pull/3136 before merging this

Patch:
diff --git a/vim25/json/discriminator.go b/vim25/json/discriminator.go
index 656e0079..ce315dd5 100644
--- a/vim25/json/discriminator.go
+++ b/vim25/json/discriminator.go
@@ -184,14 +184,17 @@ func (d *decodeState) discriminatorGetValue() (reflect.Value, error) {
 			// Assign the type instance to the outer variable, t.
 			t = ti
 
-			switch t.Kind() {
-			case reflect.Map, reflect.Struct:
-				// If the type is a map or a struct then it is not necessary to
-				// continue walking over the current JSON object since it will be
-				// completely rescanned to decode its value into the discovered
-				// type.
+			// Primitive types and types with Unmarshaler are wrapped in a
+			// structure with type and value fields. Structures and Maps not
+			// implementing Unmarshaler use discriminator embedded within their
+			// content.
+			if useNestedDiscriminator(t) {
+				// If the type is a map or a struct not implementing Unmarshaler
+				// then it is not necessary to continue walking over the current
+				// JSON object since it will be completely re-scanned to decode
+				// its value into the discovered type.
 				dd.opcode = scanEndObject
-			default:
+			} else {
 				// Otherwise if the value offset has been discovered then it is
 				// safe to stop walking over the current JSON object as well.
 				if valueOff > -1 {
@@ -247,17 +250,15 @@ func (d *decodeState) discriminatorGetValue() (reflect.Value, error) {
 	// Reset the decode state to prepare for decoding the data.
 	dd.scan.reset()
 
-	switch t.Kind() {
-	case reflect.Map, reflect.Struct:
+	if useNestedDiscriminator(t) {
 		// Set the offset to zero since the entire object will be decoded
 		// into v.
 		dd.off = 0
-	default:
+	} else {
 		// Set the offset to what it was before the discriminator value was
-		// read so only the discriminator value is decoded into v.
+		// read so only the value field is decoded into v.
 		dd.off = valueOff
 	}
-
 	// This will initialize the correct scan step and op code.
 	dd.scanWhile(scanSkipSpace)
 
@@ -325,6 +326,24 @@ func (o encOpts) isDiscriminatorSet() bool {
 
 func discriminatorInterfaceEncode(e *encodeState, v reflect.Value, opts encOpts) {
 	v = v.Elem()
+
+	if v.Type().Implements(marshalerType) {
+		discriminatorValue := opts.discriminatorValueFn(v.Type())
+		if discriminatorValue == "" {
+			marshalerEncoder(e, v, opts)
+		}
+		e.WriteString(`{"`)
+		e.WriteString(opts.discriminatorTypeFieldName)
+		e.WriteString(`":"`)
+		e.WriteString(discriminatorValue)
+		e.WriteString(`","`)
+		e.WriteString(opts.discriminatorValueFieldName)
+		e.WriteString(`":`)
+		marshalerEncoder(e, v, opts)
+		e.WriteByte('}')
+		return
+	}
+
 	switch v.Kind() {
 	case reflect.Chan, reflect.Func, reflect.Invalid:
 		e.error(&UnsupportedValueError{v, fmt.Sprintf("invalid kind: %s", v.Kind())})
@@ -390,6 +409,20 @@ func discriminatorStructEncode(e *encodeState, v reflect.Value, opts encOpts) by
 	return ','
 }
 
+var unmarshalerType = reflect.TypeOf((*Unmarshaler)(nil)).Elem()
+
+// Discriminator is nested in map and struct unless they implement Unmarshaler.
+func useNestedDiscriminator(t reflect.Type) bool {
+	if t.Implements(unmarshalerType) || reflect.PtrTo(t).Implements(unmarshalerType) {
+		return false
+	}
+	kind := t.Kind()
+	if kind == reflect.Struct || kind == reflect.Map {
+		return true
+	}
+	return false
+}
+
 var discriminatorTypeRegistry = map[string]reflect.Type{
 	"uint":         reflect.TypeOf(uint(0)),
 	"uint8":        reflect.TypeOf(uint8(0)),
diff --git a/vim25/types/json.go b/vim25/types/json.go
index 35dd175c..ccb4b3fe 100644
--- a/vim25/types/json.go
+++ b/vim25/types/json.go
@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"io"
 	"reflect"
+	"time"
 
 	"github.com/vmware/govmomi/vim25/json"
 )
@@ -30,14 +31,16 @@ const (
 )
 
 var discriminatorTypeRegistry = map[string]reflect.Type{
-	"boolean": reflect.TypeOf(true),
-	"byte":    reflect.TypeOf(uint8(0)),
-	"short":   reflect.TypeOf(int16(0)),
-	"int":     reflect.TypeOf(int32(0)),
-	"long":    reflect.TypeOf(int64(0)),
-	"float":   reflect.TypeOf(float32(0)),
-	"double":  reflect.TypeOf(float64(0)),
-	"string":  reflect.TypeOf(""),
+	"boolean":  reflect.TypeOf(true),
+	"byte":     reflect.TypeOf(uint8(0)),
+	"short":    reflect.TypeOf(int16(0)),
+	"int":      reflect.TypeOf(int32(0)),
+	"long":     reflect.TypeOf(int64(0)),
+	"float":    reflect.TypeOf(float32(0)),
+	"double":   reflect.TypeOf(float64(0)),
+	"string":   reflect.TypeOf(""),
+	"binary":   reflect.TypeOf([]byte{}),
+	"dateTime": reflect.TypeOf(time.Now()),
 }
 
 // NewJSONDecoder creates JSON decoder configured for VMOMI.
@@ -69,6 +72,8 @@ var discriminatorNamesRegistry = map[reflect.Type]string{
 	reflect.TypeOf(float32(0)): "float",
 	reflect.TypeOf(float64(0)): "double",
 	reflect.TypeOf(""):         "string",
+	reflect.TypeOf([]byte{}):   "binary",
+	reflect.TypeOf(time.Now()): "dateTime",
 }
 
 // NewJSONEncoder creates JSON encoder configured for VMOMI.


Test patch:
diff --git a/vim25/json/discriminator_test.go b/vim25/json/discriminator_test.go
index 33d51ce0..ae782881 100644
--- a/vim25/json/discriminator_test.go
+++ b/vim25/json/discriminator_test.go
@@ -6,7 +6,9 @@ package json_test
 
 import (
 	"bytes"
+	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -59,6 +61,76 @@ type DS8 struct {
 	F1 DS3 `json:"f1"`
 }
 
+type DS9 struct {
+	F1 int64
+}
+
+func (d DS9) MarshalJSON() ([]byte, error) {
+	var b bytes.Buffer
+	b.WriteString(strconv.FormatInt(d.F1, 10))
+	return b.Bytes(), nil
+}
+func (d *DS9) UnmarshalJSON(b []byte) error {
+	s := string(b)
+	if s == "null" {
+		d.F1 = 0
+		return nil
+	}
+	if len(s) < 1 {
+		return fmt.Errorf("Cannot parse empty as int64")
+	}
+	v, e := strconv.ParseInt(s, 10, 64)
+	if e != nil {
+		return fmt.Errorf("Cannot parse as int64: %v; %w", s, e)
+	}
+	d.F1 = v
+	return nil
+}
+
+// Struct implementing UnmarshalJSON with value receiver.
+type DS10 struct {
+	DS9 *DS9
+}
+
+func (d DS10) UnmarshalJSON(b []byte) error {
+	if d.DS9 == nil {
+		return nil
+	}
+	return d.DS9.UnmarshalJSON(b)
+}
+func (d DS10) MarshalJSON() ([]byte, error) {
+	if d.DS9 == nil {
+		return []byte("null"), nil
+	}
+	return d.DS9.MarshalJSON()
+}
+
+type HexInt64 int64
+
+func (i HexInt64) MarshalJSON() ([]byte, error) {
+	var b bytes.Buffer
+	b.WriteString(fmt.Sprintf(`"%X"`, i))
+	return b.Bytes(), nil
+}
+
+func (i *HexInt64) UnmarshalJSON(b []byte) error {
+	s := string(b)
+	if s == "null" {
+		*i = 0
+		return nil
+	}
+	last := len(s) - 1
+	if last < 1 || s[0] != '"' || s[last] != '"' {
+		return fmt.Errorf("Cannot parse as hex int64: %v", s)
+	}
+	v, e := strconv.ParseInt(s[1:last], 16, 64)
+	if e != nil {
+		return fmt.Errorf("Cannot parse as hex int64: %v; %w", s, e)
+	}
+	*i = HexInt64(v)
+	return nil
+}
+
 func customNameWithFilter(t reflect.Type) string {
 	res := json.DefaultDiscriminatorFunc(t)
 	if res == "DS3" {
@@ -121,6 +193,14 @@ var discriminatorTests = []struct {
 	{obj: "hello", str: `{"_t":"string","_v":"hello"}`, mode: json.DiscriminatorEncodeTypeNameRootValue},
 	{obj: true, str: `{"_t":"bool","_v":true}`, mode: json.DiscriminatorEncodeTypeNameRootValue},
 
+	{obj: HexInt64(42), str: `{"_t":"HexInt64","_v":"2A"}`, mode: json.DiscriminatorEncodeTypeNameRootValue},
+	{obj: DS9{F1: 42}, str: `{"_t":"DS9","_v":42}`, mode: json.DiscriminatorEncodeTypeNameRootValue},
+	{obj: DS6{F1: DS9{F1: 42}}, str: `{"f1":{"_t":"DS9","_v":42}}`},
+	{obj: DS9{F1: 42}, str: `42`},
+
+	{obj: DS10{DS9: &DS9{F1: 42}}, str: `42`},
+	{obj: DS6{F1: DS10{DS9: &DS9{F1: 42}}}, str: `{"f1":{"_t":"DS10","_v":42}}`, expObj: DS6{F1: DS10{DS9: nil}}},
+
 	// primitive values stored in interface with 0 methods
 	{obj: DS1{F1: uint(1)}, str: `{"f1":{"_t":"uint","_v":1}}`},
 	{obj: DS1{F1: uint8(1)}, str: `{"f1":{"_t":"uint8","_v":1}}`},
@@ -341,6 +421,10 @@ func discriminatorToTypeFn(discriminator string) (reflect.Type, bool) {
 		return reflect.TypeOf(DS7{}), true
 	case "DS8":
 		return reflect.TypeOf(DS8{}), true
+	case "DS9":
+		return reflect.TypeOf(DS9{}), true
+	case "DS10":
+		return reflect.TypeOf(DS10{}), true
 	case "uintNoop":
 		return reflect.TypeOf(uintNoop(0)), true
 	case "uint8Noop":
@@ -377,6 +461,8 @@ func discriminatorToTypeFn(discriminator string) (reflect.Type, bool) {
 		return reflect.TypeOf(sliceIntNoop{}), true
 	case "arrayOfTwoIntsNoop":
 		return reflect.TypeOf(arrayOfTwoIntsNoop{}), true
+	case "HexInt64":
+		return reflect.TypeOf(HexInt64(0)), true
 	default:
 		return nil, false
 	}
@@ -483,6 +569,15 @@ func testDiscriminatorDecode(t *testing.T) {
 					var o DS8
 					err = dec.Decode(&o)
 					obj = o
+				case "DS9":
+					var o DS9
+					err = dec.Decode(&o)
+					obj = o
+				case "DS10":
+					var o DS10
+					o.DS9 = &DS9{}
+					err = dec.Decode(&o)
+					obj = o
 				default:
 					err = dec.Decode(&obj)
 				}
@@ -715,63 +810,8 @@ func addrOfArrayOfTwoIntsNoop(v arrayOfTwoIntsNoop) *arrayOfTwoIntsNoop {
 	return &v
 }
 
-func assertEqual(t *testing.T, a, b interface{}) {
-	if a == nil && b == nil {
-		return
-	}
-	if a == nil {
-		t.Fatalf("a is nil, b is %T, %+v", b, b)
-		return
-	}
-	if b == nil {
-		t.Fatalf("b is nil, a is %T, %+v", a, a)
-		return
-	}
-
-	// t.Logf("a=%[1]T %+[1]v, b=%[2]T %+[2]v", a, b)
-
-	var (
-		ok bool
-		va reflect.Value
-		vb reflect.Value
-	)
-
-	if va, ok = a.(reflect.Value); ok {
-		a = va.Interface()
-	} else {
-		va = reflect.ValueOf(a)
-	}
-	if vb, ok = b.(reflect.Value); ok {
-		b = vb.Interface()
-	} else {
-		vb = reflect.ValueOf(b)
-	}
-
-	if vat, vab := va.Type(), vb.Type(); vat != vab {
-		t.Fatalf("type of a (%s) != type of b (%s)", vat, vab)
-		return
-	}
-
-	switch va.Kind() {
-	case reflect.Ptr, reflect.Interface:
-		assertEqual(t, va.Elem(), vb.Elem())
-	case reflect.Array, reflect.Slice:
-		for i := 0; i < va.Len(); i++ {
-			ai, bi := va.Index(i), vb.Index(i)
-			assertEqual(t, ai, bi)
-		}
-	case reflect.Map:
-		for _, k := range va.MapKeys() {
-			assertEqual(t, va.MapIndex(k), vb.MapIndex(k))
-		}
-	case reflect.Struct:
-		for i := 0; i < va.NumField(); i++ {
-			f1, f2 := va.Field(i), vb.Field(i)
-			assertEqual(t, f1, f2)
-		}
-	default:
-		if a != b {
-			t.Fatalf("a != b: a=%+v, b=%+v", a, b)
-		}
+func assertEqual(t *testing.T, a, e interface{}) {
+	if !reflect.DeepEqual(a, e) {
+		t.Fatalf("Actual and expected values differ.\nactual: '%#v'\nexpected: '%#v'\n", a, e)
 	}
 }
diff --git a/vim25/types/json_test.go b/vim25/types/json_test.go
index 92520d29..aeeffb4e 100644
--- a/vim25/types/json_test.go
+++ b/vim25/types/json_test.go
@@ -110,6 +110,12 @@ func TestSerialization(t *testing.T) {
 }
 
 func TestOptionValueSerialization(t *testing.T) {
+	tv, e := time.Parse(time.RFC3339Nano, "2022-12-12T11:48:35.473645Z")
+	if e != nil {
+		t.Log("Cannot parse test timestamp. This is coding error.")
+		t.Fail()
+		return
+	}
 	options := []struct {
 		name    string
 		wire    string
@@ -166,6 +172,82 @@ func TestOptionValueSerialization(t *testing.T) {
 				"value": {"_typeName": "string","_value": "test"}}`,
 			binding: OptionValue{Key: "option1", Value: "test"},
 		},
+		{
+			name: "dateTime", // time.Time
+			wire: `{"_typeName": "OptionValue","key": "option1",
+				"value": {"_typeName": "dateTime","_value": "2022-12-12T11:48:35.473645Z"}}`,
+			binding: OptionValue{Key: "option1", Value: tv},
+		},
+		{
+			name: "binary", // []byte
+			wire: `{"_typeName": "OptionValue","key": "option1",
+				"value": {"_typeName": "binary","_value": "SGVsbG8="}}`,
+			binding: OptionValue{Key: "option1", Value: []byte("Hello")},
+		},
+		// during serialization we have no way to guess that a string is to be
+		// converted to uri. Using net.URL solves this. It is a breaking change.
+		// See https://github.com/vmware/govmomi/pull/3123
+		// {
+		// 	name: "anyURI", // string
+		// 	wire: `{"_typeName": "OptionValue","key": "option1",
+		// 		"value": {"_typeName": "anyURI","_value": "http://hello"}}`,
+		// 	binding: OptionValue{Key: "option1", Value: "test"},
+		// },
+		{
+			name: "enum",
+			wire: `{"_typeName": "OptionValue","key": "option1",
+				"value": {"_typeName": "CustomizationNetBIOSMode","_value": "enableNetBIOS"}}`,
+			binding: OptionValue{Key: "option1", Value: CustomizationNetBIOSModeEnableNetBIOS},
+		},
+		// There is no ArrayOfCustomizationNetBIOSMode type emitted i.e. no enum
+		// array types are emitted in govmomi.
+		// We can process these in the serialization logic i.e. discover or
+		// prepend the "ArrayOf" prefix
+		// {
+		// 	name: "array of enum",
+		// 	wire: `{
+		//		"_typeName": "OptionValue",
+		//		"key": "option1",
+		// 		"value": {"_typeName": "ArrayOfCustomizationNetBIOSMode",
+		//                "_value": ["enableNetBIOS"]}}`,
+		// 	binding: OptionValue{Key: "option1",
+		//		Value: []CustomizationNetBIOSMode{
+		//			CustomizationNetBIOSModeEnableNetBIOS
+		//		}},
+		// },
+
+		// array of struct is weird. Do we want to unmarshal this as
+		// []ClusterHostRecommendation directly? Why do we want to use
+		// ArrayOfClusterHostRecommendation wrapper?
+		// if SOAP does it then I guess back compat is a big reason.
+		{
+			name: "array of struct",
+			wire: `{"_typeName": "OptionValue","key": "option1",
+				"value": {"_typeName": "ArrayOfClusterHostRecommendation","_value": [
+					{
+						"_typeName":"ClusterHostRecommendation",
+						"host": {
+							"_typeName": "ManagedObjectReference",
+							"type": "HostSystem",
+							"value": "host-42"
+						},
+						"rating":42
+					}]}}`,
+			binding: OptionValue{
+				Key: "option1",
+				Value: ArrayOfClusterHostRecommendation{
+					ClusterHostRecommendation: []ClusterHostRecommendation{
+						{
+							Host: ManagedObjectReference{
+								Type:  "HostSystem",
+								Value: "host-42",
+							},
+							Rating: 42,
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, opt := range options {