Merge pull request #77772 from sttts/sttts-structural-schema-nullable-go-openapi

apiextensions: remove nullable roundtrip hacks after go-openapi gained support

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert_test.go

@@ -75,11 +75,6 @@ func TestStructuralRoundtripOrError(t *testing.T) {
 			continue
 		}
 		f.Fuzz(x.Field(n).Addr().Interface())
-		if origSchema.Nullable {
-			// non-empty type for nullable. nullable:true with empty type does not roundtrip because
-			// go-openapi does not allow to encode that (we use type slices otherwise).
-			origSchema.Type = "string"
-		}
 
 		// it roundtrips or NewStructural errors out. We should never drop anything
 		orig, err := NewStructural(origSchema)
@@ -93,9 +88,8 @@ func TestStructuralRoundtripOrError(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		str := nullTypeRE.ReplaceAllString(string(bs), `"type":"$1","nullable":true`) // unfold nullable type:[<type>,"null"] -> type:<type>,nullable:true
 		v1beta1Schema := &apiextensionsv1beta1.JSONSchemaProps{}
-		err = json.Unmarshal([]byte(str), v1beta1Schema)
+		err = json.Unmarshal([]byte(bs), v1beta1Schema)
 		if err != nil {
 			t.Fatal(err)
 		}

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi.go

@@ -20,13 +20,7 @@ import (
 	"github.com/go-openapi/spec"
 )
 
-// ToGoOpenAPI converts a structural schema to go-openapi schema. It is faithful and roundtrippable
-// with the exception of `nullable:true` for empty type (`type:""`).
-//
-// WARNING: Do not use the returned schema to perform CRD validation until this restriction is solved.
-//
-// Nullable:true is mapped to `type:[<structural-type>,"null"]`
-// if the structural type is non-empty, and nullable is dropped if the structural type is empty.
+// ToGoOpenAPI converts a structural schema to go-openapi schema. It is faithful and roundtrippable.
 func (s *Structural) ToGoOpenAPI() *spec.Schema {
 	if s == nil {
 		return nil
@@ -57,14 +51,8 @@ func (g *Generic) toGoOpenAPI(ret *spec.Schema) {
 
 	if len(g.Type) != 0 {
 		ret.Type = spec.StringOrArray{g.Type}
-		if g.Nullable {
-			// go-openapi does not support nullable, but multiple type values.
-			// Only when type is already non-empty, adding null to the types is correct though.
-			// If you add null as only type, you enforce null, in contrast to nullable being
-			// ineffective if no type is provided in a schema.
-			ret.Type = append(ret.Type, "null")
-		}
 	}
+	ret.Nullable = g.Nullable
 	if g.AdditionalProperties != nil {
 		ret.AdditionalProperties = &spec.SchemaOrBool{
 			Allows: g.AdditionalProperties.Bool,

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go

@@ -49,25 +49,13 @@ func TestStructuralRoundtrip(t *testing.T) {
 			case 2:
 				s.Object = ""
 			case 3:
-				s.Object = []string{}
+				s.Object = []interface{}{}
 			case 4:
 				s.Object = map[string]interface{}{}
 			case 5:
 				s.Object = nil
 			}
 		},
-		func(g *Generic, c fuzz.Continue) {
-			c.FuzzNoCustom(g)
-
-			// TODO: make nullable in case of empty type survive go-openapi JSON -> API schema roundtrip
-			// go-openapi does not support nullable. Adding it to a type slice produces OpenAPI v3
-			// incompatible JSON which we cannot unmarshal (without string-replace magic to transform
-			// null types back into nullable). If type is empty, nullable:true is not preserved
-			// at all.
-			if len(g.Type) == 0 {
-				g.Nullable = false
-			}
-		},
 	)
 	f.MaxDepth(3)
 	f.NilChance(0.5)
@@ -93,9 +81,8 @@ func TestStructuralRoundtrip(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		str := nullTypeRE.ReplaceAllString(string(bs), `"type":"$1","nullable":true`) // unfold nullable type:[<type>,"null"] -> type:<type>,nullable:true
 		v1beta1Schema := &apiextensionsv1beta1.JSONSchemaProps{}
-		err = json.Unmarshal([]byte(str), v1beta1Schema)
+		err = json.Unmarshal(bs, v1beta1Schema)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -110,7 +97,7 @@ func TestStructuralRoundtrip(t *testing.T) {
 		}
 
 		if !reflect.DeepEqual(orig, s) {
-			t.Fatalf("original and result differ: %v", diff.ObjectDiff(orig, s))
+			t.Fatalf("original and result differ: %v", diff.ObjectGoPrintDiff(orig, s))
 		}
 	}
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go

@@ -29,7 +29,7 @@ func NewSchemaValidator(customResourceValidation *apiextensions.CustomResourceVa
 	// Convert CRD schema to openapi schema
 	openapiSchema := &spec.Schema{}
 	if customResourceValidation != nil {
-		// WARNING: do not replace this with Structural.ToGoOpenAPI until it supports nullable.
+		// TODO: replace with NewStructural(...).ToGoOpenAPI
 		if err := ConvertJSONSchemaProps(customResourceValidation.OpenAPIV3Schema, openapiSchema); err != nil {
 			return nil, nil, err
 		}
@@ -76,9 +76,7 @@ func ConvertJSONSchemaPropsWithPostProcess(in *apiextensions.JSONSchemaProps, ou
 		out.VendorExtensible.AddExtension("x-kubernetes-int-or-string", true)
 		out.Type = spec.StringOrArray{"integer", "string"}
 	}
-	if out.Type != nil && in.Nullable {
-		out.Type = append(out.Type, "null")
-	}
+	out.Nullable = in.Nullable
 	out.Format = in.Format
 	out.Title = in.Title
 	out.Maximum = in.Maximum

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go

@@ -72,7 +72,6 @@ func TestRoundTrip(t *testing.T) {
 		if err := json.Unmarshal(openAPIJSON, &j); err != nil {
 			t.Fatal(err)
 		}
-		j = convertNullTypeToNullable(j)
 		j = stripIntOrStringType(j)
 		openAPIJSON, err = json.Marshal(j)
 		if err != nil {
@@ -97,49 +96,6 @@ func TestRoundTrip(t *testing.T) {
 	}
 }
 
-func convertNullTypeToNullable(x interface{}) interface{} {
-	switch x := x.(type) {
-	case map[string]interface{}:
-		if t, found := x["type"]; found {
-			switch t := t.(type) {
-			case []interface{}:
-				for i, typ := range t {
-					if s, ok := typ.(string); !ok || s != "null" {
-						continue
-					}
-					t = append(t[:i], t[i+1:]...)
-					switch len(t) {
-					case 0:
-						delete(x, "type")
-					case 1:
-						x["type"] = t[0]
-					default:
-						x["type"] = t
-					}
-					x["nullable"] = true
-					break
-				}
-			case string:
-				if t == "null" {
-					delete(x, "type")
-					x["nullable"] = true
-				}
-			}
-		}
-		for k := range x {
-			x[k] = convertNullTypeToNullable(x[k])
-		}
-		return x
-	case []interface{}:
-		for i := range x {
-			x[i] = convertNullTypeToNullable(x[i])
-		}
-		return x
-	default:
-		return x
-	}
-}
-
 func stripIntOrStringType(x interface{}) interface{} {
 	switch x := x.(type) {
 	case map[string]interface{}: