From aeef92b3bd3bcbfc5977e824f0d83d42b0a55628 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 3 Apr 2020 13:24:46 -0400 Subject: [PATCH] Preserve int/float distinction when decoding raw values --- .../pkg/apiserver/schema/goopenapi_test.go | 6 +- .../test/integration/defaulting_test.go | 32 +++- .../k8s.io/apimachinery/pkg/util/json/json.go | 25 +++ .../apimachinery/pkg/util/json/json_test.go | 167 ++++++++++++++---- 4 files changed, 192 insertions(+), 38 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go index 8106312a8b2..19c4945d46b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go @@ -38,9 +38,9 @@ func TestStructuralRoundtrip(t *testing.T) { f.RandSource(rand.New(rand.NewSource(seed))) f.Funcs( func(s *JSON, c fuzz.Continue) { - switch c.Intn(6) { + switch c.Intn(7) { case 0: - s.Object = float64(42.0) + s.Object = float64(42.2) case 1: s.Object = map[string]interface{}{"foo": "bar"} case 2: @@ -51,6 +51,8 @@ func TestStructuralRoundtrip(t *testing.T) { s.Object = map[string]interface{}{} case 5: s.Object = nil + case 6: + s.Object = int64(42) } }, ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go index 552bb48bb86..351f4cc453c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go @@ -53,6 +53,10 @@ var defaultingFixture = &apiextensionsv1.CustomResourceDefinition{ Served: true, Subresources: &apiextensionsv1.CustomResourceSubresources{ Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + Scale: &apiextensionsv1.CustomResourceSubresourceScale{ + SpecReplicasPath: ".spec.replicas", + StatusReplicasPath: ".status.replicas", + }, }, }, { @@ -61,6 +65,10 @@ var defaultingFixture = &apiextensionsv1.CustomResourceDefinition{ Served: false, Subresources: &apiextensionsv1.CustomResourceSubresources{ Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + Scale: &apiextensionsv1.CustomResourceSubresourceScale{ + SpecReplicasPath: ".spec.replicas", + StatusReplicasPath: ".status.replicas", + }, }, }, }, @@ -94,6 +102,11 @@ properties: default: "v1beta1" v1beta2: type: string + replicas: + default: 1 + format: int32 + minimum: 0 + type: integer status: type: object properties: @@ -110,6 +123,11 @@ properties: default: "v1beta1" v1beta2: type: string + replicas: + default: 0 + format: int32 + minimum: 0 + type: integer ` const defaultingFooV1beta2Schema = ` @@ -131,6 +149,11 @@ properties: v1beta2: type: string default: "v1beta2" + replicas: + default: 1 + format: int32 + minimum: 0 + type: integer status: type: object properties: @@ -147,6 +170,11 @@ properties: v1beta2: type: string default: "v1beta2" + replicas: + default: 0 + format: int32 + minimum: 0 + type: integer ` const defaultingFooInstance = ` @@ -274,7 +302,7 @@ func testDefaulting(t *testing.T, watchCache bool) { // spec.a and spec.b are defaulted in both versions // spec.v1beta1 is defaulted when reading the incoming request // spec.v1beta2 is defaulted when reading the storage response - mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "v1beta1"}, {"spec", "v1beta2"}}) + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "v1beta1"}, {"spec", "v1beta2"}, {"spec", "replicas"}}) mustNotExist(foo.Object, [][]string{{"status"}}) t.Logf("Updating status and expecting 'a' and 'b' to show up.") @@ -282,7 +310,7 @@ func testDefaulting(t *testing.T, watchCache bool) { if foo, err = fooClient.UpdateStatus(context.TODO(), foo, metav1.UpdateOptions{}); err != nil { t.Fatal(err) } - mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}}) + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}, {"status", "replicas"}}) t.Logf("Add 'c' default to the storage version and wait until GET sees it in both status and spec") addDefault("v1beta2", "c", "C") diff --git a/staging/src/k8s.io/apimachinery/pkg/util/json/json.go b/staging/src/k8s.io/apimachinery/pkg/util/json/json.go index 0e2e3017547..204834883fa 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/json/json.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/json/json.go @@ -66,11 +66,36 @@ func Unmarshal(data []byte, v interface{}) error { // If the decode succeeds, post-process the map to convert json.Number objects to int64 or float64 return convertSliceNumbers(*v, 0) + case *interface{}: + // Build a decoder from the given data + decoder := json.NewDecoder(bytes.NewBuffer(data)) + // Preserve numbers, rather than casting to float64 automatically + decoder.UseNumber() + // Run the decode + if err := decoder.Decode(v); err != nil { + return err + } + // If the decode succeeds, post-process the map to convert json.Number objects to int64 or float64 + return convertInterfaceNumbers(v, 0) + default: return json.Unmarshal(data, v) } } +func convertInterfaceNumbers(v *interface{}, depth int) error { + var err error + switch v2 := (*v).(type) { + case json.Number: + *v, err = convertNumber(v2) + case map[string]interface{}: + err = convertMapNumbers(v2, depth+1) + case []interface{}: + err = convertSliceNumbers(v2, depth+1) + } + return err +} + // convertMapNumbers traverses the map, converting any json.Number values to int64 or float64. // values which are map[string]interface{} or []interface{} are recursively visited func convertMapNumbers(m map[string]interface{}, depth int) error { diff --git a/staging/src/k8s.io/apimachinery/pkg/util/json/json_test.go b/staging/src/k8s.io/apimachinery/pkg/util/json/json_test.go index cd0c18bb2e4..1602feb0036 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/json/json_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/json/json_test.go @@ -19,6 +19,8 @@ limitations under the License. package json import ( + gojson "encoding/json" + "fmt" "math" "reflect" @@ -278,42 +280,139 @@ func TestEvaluateTypes(t *testing.T) { }, } - for _, tc := range testCases { - inputJSON := fmt.Sprintf(`{"data":%s}`, tc.In) - expectedJSON := fmt.Sprintf(`{"data":%s}`, tc.Out) - m := map[string]interface{}{} - err := Unmarshal([]byte(inputJSON), &m) - if tc.Err && err != nil { - // Expected error - continue - } - if err != nil { - t.Errorf("%s: error decoding: %v", tc.In, err) - continue - } - if tc.Err { - t.Errorf("%s: expected error, got none", tc.In) - continue - } - data, ok := m["data"] - if !ok { - t.Errorf("%s: decoded object missing data key: %#v", tc.In, m) - continue - } - if !reflect.DeepEqual(tc.Data, data) { - t.Errorf("%s: expected\n\t%#v (%v), got\n\t%#v (%v)", tc.In, tc.Data, reflect.TypeOf(tc.Data), data, reflect.TypeOf(data)) - continue - } + for i, tc := range testCases { + t.Run(fmt.Sprintf("%d_map", i), func(t *testing.T) { + // decode the input as a map item + inputJSON := fmt.Sprintf(`{"data":%s}`, tc.In) + expectedJSON := fmt.Sprintf(`{"data":%s}`, tc.Out) + m := map[string]interface{}{} + err := Unmarshal([]byte(inputJSON), &m) + if tc.Err && err != nil { + // Expected error + return + } + if err != nil { + t.Fatalf("%s: error decoding: %v", tc.In, err) + } + if tc.Err { + t.Fatalf("%s: expected error, got none", tc.In) + } + data, ok := m["data"] + if !ok { + t.Fatalf("%s: decoded object missing data key: %#v", tc.In, m) + } + if !reflect.DeepEqual(tc.Data, data) { + t.Fatalf("%s: expected\n\t%#v (%v), got\n\t%#v (%v)", tc.In, tc.Data, reflect.TypeOf(tc.Data), data, reflect.TypeOf(data)) + } - outputJSON, err := Marshal(m) - if err != nil { - t.Errorf("%s: error encoding: %v", tc.In, err) - continue - } + outputJSON, err := Marshal(m) + if err != nil { + t.Fatalf("%s: error encoding: %v", tc.In, err) + } - if expectedJSON != string(outputJSON) { - t.Errorf("%s: expected\n\t%s, got\n\t%s", tc.In, expectedJSON, string(outputJSON)) - continue + if expectedJSON != string(outputJSON) { + t.Fatalf("%s: expected\n\t%s, got\n\t%s", tc.In, expectedJSON, string(outputJSON)) + } + }) + + t.Run(fmt.Sprintf("%d_slice", i), func(t *testing.T) { + // decode the input as an array item + inputJSON := fmt.Sprintf(`[0,%s]`, tc.In) + expectedJSON := fmt.Sprintf(`[0,%s]`, tc.Out) + m := []interface{}{} + err := Unmarshal([]byte(inputJSON), &m) + if tc.Err && err != nil { + // Expected error + return + } + if err != nil { + t.Fatalf("%s: error decoding: %v", tc.In, err) + } + if tc.Err { + t.Fatalf("%s: expected error, got none", tc.In) + } + if len(m) != 2 { + t.Fatalf("%s: decoded object wasn't the right length: %#v", tc.In, m) + } + data := m[1] + if !reflect.DeepEqual(tc.Data, data) { + t.Fatalf("%s: expected\n\t%#v (%v), got\n\t%#v (%v)", tc.In, tc.Data, reflect.TypeOf(tc.Data), data, reflect.TypeOf(data)) + } + + outputJSON, err := Marshal(m) + if err != nil { + t.Fatalf("%s: error encoding: %v", tc.In, err) + } + + if expectedJSON != string(outputJSON) { + t.Fatalf("%s: expected\n\t%s, got\n\t%s", tc.In, expectedJSON, string(outputJSON)) + } + }) + + t.Run(fmt.Sprintf("%d_raw", i), func(t *testing.T) { + // decode the input as a standalone object + inputJSON := fmt.Sprintf(`%s`, tc.In) + expectedJSON := fmt.Sprintf(`%s`, tc.Out) + var m interface{} + err := Unmarshal([]byte(inputJSON), &m) + if tc.Err && err != nil { + // Expected error + return + } + if err != nil { + t.Fatalf("%s: error decoding: %v", tc.In, err) + } + if tc.Err { + t.Fatalf("%s: expected error, got none", tc.In) + } + data := m + if !reflect.DeepEqual(tc.Data, data) { + t.Fatalf("%s: expected\n\t%#v (%v), got\n\t%#v (%v)", tc.In, tc.Data, reflect.TypeOf(tc.Data), data, reflect.TypeOf(data)) + } + + outputJSON, err := Marshal(m) + if err != nil { + t.Fatalf("%s: error encoding: %v", tc.In, err) + } + + if expectedJSON != string(outputJSON) { + t.Fatalf("%s: expected\n\t%s, got\n\t%s", tc.In, expectedJSON, string(outputJSON)) + } + }) + } +} + +func TestUnmarshalNil(t *testing.T) { + { + var v *interface{} + err := Unmarshal([]byte(`0`), v) + goerr := gojson.Unmarshal([]byte(`0`), v) + if err == nil || goerr == nil || err.Error() != goerr.Error() { + t.Fatalf("expected error matching stdlib, got %v, %v", err, goerr) + } else { + t.Log(err) + } + } + + { + var v *[]interface{} + err := Unmarshal([]byte(`[]`), v) + goerr := gojson.Unmarshal([]byte(`[]`), v) + if err == nil || goerr == nil || err.Error() != goerr.Error() { + t.Fatalf("expected error matching stdlib, got %v, %v", err, goerr) + } else { + t.Log(err) + } + } + + { + var v *map[string]interface{} + err := Unmarshal([]byte(`{}`), v) + goerr := gojson.Unmarshal([]byte(`{}`), v) + if err == nil || goerr == nil || err.Error() != goerr.Error() { + t.Fatalf("expected error matching stdlib, got %v, %v", err, goerr) + } else { + t.Log(err) } } }