Repository: vmware/govmomi
Pull request #3093 (2023-04-13 16:02:45)

chore: Err on json.Decode for invalid type/vals
## Description

This patch updates the behavior of the JSON codec package so that if the decode operation cannot convert a value into the destination type, the error is no longer ignored silently, but returned so the caller can handle it.

This patch also adds tests to validate these cases.

Please note this _is_ potentially a breaking change since previously, if `{"_typeName":"string", "_value":true}` was decoded, the result would have been an empty string. Now the result is an error returned from the decode operation that would read `json: cannot unmarshal bool into Go value of type string`.

Closes: `NA`

## Type of change

Please mark options that are relevant:

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] 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?

Several unit tests have been added to validate the test cases.

- [x] Invalid type/name value tests in `discriminator_test.go`
- [x] Invalid ExtraConfig type/name value in `json_test.go`

## 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
- [x] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged


Patch:
diff --git a/vim25/json/discriminator.go b/vim25/json/discriminator.go
index 6dbd0487..9362612c 100644
--- a/vim25/json/discriminator.go
+++ b/vim25/json/discriminator.go
@@ -266,6 +266,19 @@ func (d *decodeState) discriminatorGetValue() (reflect.Value, error) {
 		return reflect.Value{}, err
 	}
 
+	// Check the saved error as well since the decoder.value function does not
+	// always return an error. If the reflected value is still zero, then it is
+	// likely the decoder was unable to decode the value.
+	if err := dd.savedError; err != nil {
+		switch v.Kind() {
+		case reflect.Ptr, reflect.Interface:
+			v = v.Elem()
+		}
+		if v.IsZero() {
+			return reflect.Value{}, err
+		}
+	}
+
 	return v, nil
 }
 


Test patch:
diff --git a/vim25/json/discriminator_test.go b/vim25/json/discriminator_test.go
index cf596420..33d51ce0 100644
--- a/vim25/json/discriminator_test.go
+++ b/vim25/json/discriminator_test.go
@@ -80,6 +80,11 @@ var discriminatorTests = []struct {
 	dd                bool
 	discriminatorFunc json.TypeToDiscriminatorFunc
 }{
+
+	// invalid type/value combinations
+	{obj: true, str: `{"_t":"string","_v":true}`, expStr: `{"_t":"bool","_v":true}`, expDecErr: `json: cannot unmarshal bool into Go value of type string`, mode: json.DiscriminatorEncodeTypeNameRootValue},
+	{obj: DS1{F1: true}, str: `{"f1":{"_t":"string","_v":true}}`, expStr: `{"f1":{"_t":"bool","_v":true}}`, expDecErr: `json: cannot unmarshal bool into Go struct field DS1.f1 of type string`},
+
 	// encode/decode nil/null works as expected
 	{obj: nil, str: `null`},
 	{obj: nil, str: `null`, mode: json.DiscriminatorEncodeTypeNameRootValue},
@@ -207,7 +212,7 @@ var discriminatorTests = []struct {
 	// address of primitive values stored in interface with >0 methods where only the address
 	// of the value satisfies the interface.
 	//
-	// Unmarshaling the JSON into the object causes the deocder to check to see if the JSON objects
+	// Unmarshaling the JSON into the object causes the decoder to check to see if the JSON objects
 	// can be stored in DS7.F1, finding out that they cannot due to all the types implementing
 	// DS7.F1 by-address. Thus the decoder will then check to see if the value can be assigned to
 	// DS7.F1 by-address, which will work.
@@ -301,7 +306,7 @@ var discriminatorTests = []struct {
 	// discriminator type not a string
 	{obj: DS1{}, str: `{"f1":{"_t":0,"_v":1}}`, expStr: `{"f1":null}`, expDecErr: "json: discriminator type at offset 12 is not string"},
 
-	// discriminator not used for non-iterface field
+	// discriminator not used for non-interface field
 	{obj: DS8{F1: DS3{F1: "hello"}}, str: `{"f1":{"f1":"hello"}}`},
 	{obj: DS8{F1: DS3{F1: "hello"}}, str: `{"_t":"DS8","f1":{"f1":"hello"}}`, mode: json.DiscriminatorEncodeTypeNameRootValue},
 	{obj: DS8{F1: DS3{F1: "hello"}}, str: `{"_t":"DS8","f1":{"_t":"DS3","f1":"hello"}}`, mode: json.DiscriminatorEncodeTypeNameRootValue | json.DiscriminatorEncodeTypeNameAllObjects},
@@ -399,7 +404,7 @@ func TestDiscriminator(t *testing.T) {
 
 func testDiscriminatorEncode(t *testing.T) {
 	for _, tc := range discriminatorTests {
-		tc := tc // caputre the loop variable
+		tc := tc // capture the loop variable
 		t.Run("", func(t *testing.T) {
 			ee := tc.expEncErr
 			var w bytes.Buffer
@@ -431,7 +436,7 @@ func testDiscriminatorEncode(t *testing.T) {
 
 func testDiscriminatorDecode(t *testing.T) {
 	for _, tc := range discriminatorTests {
-		tc := tc // caputre the loop variable
+		tc := tc // capture the loop variable
 		t.Run("", func(t *testing.T) {
 			ee := tc.expDecErr
 			dec := json.NewDecoder(strings.NewReader(tc.str))
@@ -723,6 +728,8 @@ func assertEqual(t *testing.T, a, b interface{}) {
 		return
 	}
 
+	// t.Logf("a=%[1]T %+[1]v, b=%[2]T %+[2]v", a, b)
+
 	var (
 		ok bool
 		va reflect.Value
diff --git a/vim25/types/json_test.go b/vim25/types/json_test.go
index d08b45aa..2c710f31 100644
--- a/vim25/types/json_test.go
+++ b/vim25/types/json_test.go
@@ -28,10 +28,11 @@ import (
 )
 
 var serializationTests = []struct {
-	name   string
-	file   string
-	data   interface{}
-	goType reflect.Type
+	name      string
+	file      string
+	data      interface{}
+	goType    reflect.Type
+	expDecErr string
 }{
 	{
 		name:   "vminfo",
@@ -45,6 +46,13 @@ var serializationTests = []struct {
 		data:   &retrieveResultForTests,
 		goType: reflect.TypeOf(RetrieveResult{}),
 	},
+	{
+		name:      "vminfo-invalid-type-name-value",
+		file:      "./testdata/vminfo-invalid-type-name-value.json",
+		data:      &vmInfoObjForTests,
+		goType:    reflect.TypeOf(VirtualMachineConfigInfo{}),
+		expDecErr: `json: cannot unmarshal bool into Go struct field VirtualMachineConfigInfo.extraConfig of type string`,
+	},
 }
 
 func TestSerialization(t *testing.T) {
@@ -58,19 +66,29 @@ func TestSerialization(t *testing.T) {
 
 			dec := NewJSONDecoder(f)
 
+			ee := test.expDecErr
 			data := reflect.New(test.goType).Interface()
 			if err := dec.Decode(data); err != nil {
-				t.Fatal(err)
-			}
-
-			a, e := data, test.data
-
-			if diff := cmp.Diff(a, e); diff != "" {
-				t.Errorf("mismatched %v: %s", test.name, diff)
+				if ee != err.Error() {
+					t.Errorf("expected error mismatch: e=%v, a=%v", ee, err)
+				} else if ee == "" {
+					t.Errorf("unexpected error: %v", err)
+				}
+			} else if ee != "" {
+				t.Errorf("expected error did not occur: %v", ee)
+			} else {
+				a, e := data, test.data
+				if diff := cmp.Diff(a, e); diff != "" {
+					t.Errorf("mismatched %v: %s", test.name, diff)
+				}
 			}
 		})
 
 		t.Run(test.name+" Encode", func(t *testing.T) {
+			if test.expDecErr != "" {
+				t.Skip("skipping due to expected decode error")
+			}
+
 			expJSON, err := os.ReadFile(test.file)
 			if err != nil {
 				t.Fatal(err)
diff --git a/vim25/types/testdata/vminfo-invalid-type-name-value.json b/vim25/types/testdata/vminfo-invalid-type-name-value.json
new file mode 100644
index 00000000..1fc4fefa
--- /dev/null
+++ b/vim25/types/testdata/vminfo-invalid-type-name-value.json
@@ -0,0 +1,22 @@
+{
+    "_typeName": "VirtualMachineConfigInfo",
+    "name": "test",
+    "extraConfig": [
+        {
+            "_typeName": "OptionValue",
+            "key": "nvram",
+            "value": {
+                "_typeName": "string",
+                "_value": true
+            }
+        },
+        {
+            "_typeName": "OptionValue",
+            "key": "svga.present",
+            "value": {
+                "_typeName": "string",
+                "_value": "TRUE"
+            }
+        }
+    ]
+}