Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix conversion of literal null JSON values #104969

Merged
merged 2 commits into from Sep 14, 2021
Merged

Fix conversion of literal null JSON values #104969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba1ca0d
Propagate conversion errors
liggitt Sep 13, 2021
89ae351
Fix null JSON round tripping
liggitt Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/apis/extensions/v1beta1/conversion.go
View file
Open in desktop
Expand Up @@ -189,7 +189,7 @@ func Convert_networking_IngressBackend_To_v1beta1_IngressBackend(in *networking.

func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *extensionsv1beta1.IngressSpec, out *networking.IngressSpec, s conversion.Scope) error {
if err := autoConvert_v1beta1_IngressSpec_To_networking_IngressSpec(in, out, s); err != nil {
return nil
return err
}
if in.Backend != nil {
out.DefaultBackend = &networking.IngressBackend{}
Expand All @@ -202,7 +202,7 @@ func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *extensionsv1beta1

func Convert_networking_IngressSpec_To_v1beta1_IngressSpec(in *networking.IngressSpec, out *extensionsv1beta1.IngressSpec, s conversion.Scope) error {
if err := autoConvert_networking_IngressSpec_To_v1beta1_IngressSpec(in, out, s); err != nil {
return nil
return err
}
if in.DefaultBackend != nil {
out.Backend = &extensionsv1beta1.IngressBackend{}
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/networking/v1beta1/conversion.go
View file
Open in desktop
Expand Up @@ -52,7 +52,7 @@ func Convert_networking_IngressBackend_To_v1beta1_IngressBackend(in *networking.
}
func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *v1beta1.IngressSpec, out *networking.IngressSpec, s conversion.Scope) error {
if err := autoConvert_v1beta1_IngressSpec_To_networking_IngressSpec(in, out, s); err != nil {
return nil
return err
}
if in.Backend != nil {
out.DefaultBackend = &networking.IngressBackend{}
Expand All @@ -65,7 +65,7 @@ func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *v1beta1.IngressSp

func Convert_networking_IngressSpec_To_v1beta1_IngressSpec(in *networking.IngressSpec, out *v1beta1.IngressSpec, s conversion.Scope) error {
if err := autoConvert_networking_IngressSpec_To_v1beta1_IngressSpec(in, out, s); err != nil {
return nil
return err
}
if in.DefaultBackend != nil {
out.Backend = &v1beta1.IngressBackend{}
Expand Down
19 changes: 15 additions & 4 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1

import (
"bytes"

"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/conversion"
Expand All @@ -36,20 +38,29 @@ func Convert_apiextensions_JSONSchemaProps_To_v1_JSONSchemaProps(in *apiextensio
return nil
}

var nullLiteral = []byte(`null`)

func Convert_apiextensions_JSON_To_v1_JSON(in *apiextensions.JSON, out *JSON, s conversion.Scope) error {
raw, err := json.Marshal(*in)
if err != nil {
return err
}
out.Raw = raw
if len(raw) == 0 || bytes.Equal(raw, nullLiteral) {
// match JSON#UnmarshalJSON treatment of literal nulls
out.Raw = nil
} else {
out.Raw = raw
}
return nil
}

func Convert_v1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error {
if in != nil {
var i interface{}
if err := json.Unmarshal(in.Raw, &i); err != nil {
return err
if len(in.Raw) > 0 && !bytes.Equal(in.Raw, nullLiteral) {
if err := json.Unmarshal(in.Raw, &i); err != nil {
return err
}
}
*out = i
} else {
Expand Down Expand Up @@ -103,7 +114,7 @@ func Convert_apiextensions_CustomResourceDefinitionSpec_To_v1_CustomResourceDefi

func Convert_v1_CustomResourceDefinitionSpec_To_apiextensions_CustomResourceDefinitionSpec(in *CustomResourceDefinitionSpec, out *apiextensions.CustomResourceDefinitionSpec, s conversion.Scope) error {
if err := autoConvert_v1_CustomResourceDefinitionSpec_To_apiextensions_CustomResourceDefinitionSpec(in, out, s); err != nil {
return nil
return err
}

if len(out.Versions) == 0 {
Expand Down
55 changes: 55 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion_test.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1

import (
"encoding/json"
"reflect"
"strings"
"testing"
Expand Down Expand Up @@ -605,3 +606,57 @@ func TestJSONConversion(t *testing.T) {
}
}
}

func TestJSONRoundTrip(t *testing.T) {
testcases := []struct {
name string
in string
out string
}{
{
name: "nulls",
in: `{"default":null,"enum":null,"example":null}`,
out: `{}`,
},
{
name: "null values",
in: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
out: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to reject it if people are trying to make null an enum option. That cannot work, it will break assumptions that null means you are trying to clear the field. I can't tell if that is what is going on here or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a nullable value you want to constrain to a specific set of values or null must be able to include null as an enum value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A field being set to null is not a condition we can express, e.g. it can't round-trip. Null is an enum value the way that bald is a hair color. I would not expect to have to list null as a possible value in order to use field: null to delete the field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can/does round-trip in custom resources...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I realize what is going on here now, I agree raw JSON should work like this, given that we have it at all, which I'm not super happy about...

},
}

scheme := runtime.NewScheme()
// add internal and external types
if err := apiextensions.AddToScheme(scheme); err != nil {
t.Fatal(err)
}
if err := AddToScheme(scheme); err != nil {
t.Fatal(err)
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
external := &JSONSchemaProps{}
if err := json.Unmarshal([]byte(tc.in), external); err != nil {
t.Fatal(err)
}

internal := &apiextensions.JSONSchemaProps{}
if err := scheme.Convert(external, internal, nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}
roundtripped := &JSONSchemaProps{}
if err := scheme.Convert(internal, roundtripped, nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}

out, err := json.Marshal(roundtripped)
if err != nil {
t.Fatal(err)
}
if string(out) != string(tc.out) {
t.Fatalf("expected\n%s\ngot\n%s", string(tc.out), string(out))
}
})
}
}
3 changes: 2 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1

import (
"bytes"
"errors"

"k8s.io/apimachinery/pkg/util/json"
Expand Down Expand Up @@ -128,7 +129,7 @@ func (s JSON) MarshalJSON() ([]byte, error) {
}

func (s *JSON) UnmarshalJSON(data []byte) error {
if len(data) > 0 && string(data) != "null" {
if len(data) > 0 && !bytes.Equal(data, nullLiteral) {
s.Raw = data
}
return nil
Expand Down
17 changes: 14 additions & 3 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1beta1

import (
"bytes"

"k8s.io/apimachinery/pkg/conversion"
"k8s.io/apimachinery/pkg/util/json"

Expand All @@ -36,20 +38,29 @@ func Convert_apiextensions_JSONSchemaProps_To_v1beta1_JSONSchemaProps(in *apiext
return nil
}

var nullLiteral = []byte(`null`)

func Convert_apiextensions_JSON_To_v1beta1_JSON(in *apiextensions.JSON, out *JSON, s conversion.Scope) error {
raw, err := json.Marshal(*in)
if err != nil {
return err
}
out.Raw = raw
if len(raw) == 0 || bytes.Equal(raw, nullLiteral) {
// match JSON#UnmarshalJSON treatment of literal nulls
out.Raw = nil
} else {
out.Raw = raw
}
return nil
}

func Convert_v1beta1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error {
if in != nil {
var i interface{}
if err := json.Unmarshal(in.Raw, &i); err != nil {
return err
if len(in.Raw) > 0 && !bytes.Equal(in.Raw, nullLiteral) {
if err := json.Unmarshal(in.Raw, &i); err != nil {
return err
}
}
*out = i
} else {
Expand Down
55 changes: 55 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion_test.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta1

import (
"encoding/json"
"reflect"
"testing"

Expand Down Expand Up @@ -111,3 +112,57 @@ func TestJSONConversion(t *testing.T) {
}
}
}

func TestJSONRoundTrip(t *testing.T) {
testcases := []struct {
name string
in string
out string
}{
{
name: "nulls",
in: `{"default":null,"enum":null,"example":null}`,
out: `{}`,
},
{
name: "null values",
in: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
out: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
},
}

scheme := runtime.NewScheme()
// add internal and external types
if err := apiextensions.AddToScheme(scheme); err != nil {
t.Fatal(err)
}
if err := AddToScheme(scheme); err != nil {
t.Fatal(err)
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
external := &JSONSchemaProps{}
if err := json.Unmarshal([]byte(tc.in), external); err != nil {
t.Fatal(err)
}

internal := &apiextensions.JSONSchemaProps{}
if err := scheme.Convert(external, internal, nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}
roundtripped := &JSONSchemaProps{}
if err := scheme.Convert(internal, roundtripped, nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}

out, err := json.Marshal(roundtripped)
if err != nil {
t.Fatal(err)
}
if string(out) != string(tc.out) {
t.Fatalf("expected\n%s\ngot\n%s", string(tc.out), string(out))
}
})
}
}
3 changes: 2 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta1

import (
"bytes"
"errors"

"k8s.io/apimachinery/pkg/util/json"
Expand Down Expand Up @@ -128,7 +129,7 @@ func (s JSON) MarshalJSON() ([]byte, error) {
}

func (s *JSON) UnmarshalJSON(data []byte) error {
if len(data) > 0 && string(data) != "null" {
if len(data) > 0 && !bytes.Equal(data, nullLiteral) {
s.Raw = data
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go
View file
Open in desktop
Expand Up @@ -165,7 +165,7 @@ func Convert_Map_string_To_runtime_Object_To_Slice_v1_NamedExtension(in *map[str
newExtension := (*in)[key]
oldExtension := runtime.RawExtension{}
if err := runtime.Convert_runtime_Object_To_runtime_RawExtension(&newExtension, &oldExtension, s); err != nil {
return nil
return err
}
namedExtension := NamedExtension{key, oldExtension}
*out = append(*out, namedExtension)
Expand Down