fixes schema.json and helm docs bugs (#429)

* add debug logging to custom type gneerator

* handle 4.3.2 boolean json schema

* fix omitChildren nil pointer child bug

* stop json schema from leaking between chart gens in same process

* code cleanup

* add changelog

* fix codegen issue

* support merging json schemas together

* unfocus test

changelog/v0.28.2/428-helm-codegen-fixes.yaml

@@ -0,0 +1,4 @@
+changelog:
+  - type: FIX
+    description: Fixes some bugs generating schema.json and helm docs
+    issueLink: https://github.com/solo-io/skv2/issues/428

codegen/doc/doc_suite_test.go

@@ -0,0 +1,13 @@
+package doc_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestDoc(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Doc Suite")
+}

codegen/doc/helm_values.go

@@ -163,7 +163,7 @@ func docReflect(addValue addValue, path []string, desc string, typ reflect.Type,
 					path += "[]"
 				} else if kind == reflect.Ptr {
 					// get the underlying type of pointer types
-					kind = fieldVal.Elem().Kind()
+					kind = typ.Field(i).Type.Elem().Kind()
 				}
 				// add a HelmValue for the struct field whose children are ignored
 				addValue(HelmValue{Key: path, Type: kind.String(), DefaultValue: valToString(fieldVal), Description: desc})

codegen/doc/helm_values_test.go

@@ -0,0 +1,54 @@
+package doc_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/solo-io/skv2/codegen/doc"
+)
+
+var _ = Describe("GenerateHelmValuesDoc", func() {
+	It("handles case where map value struct has a nil pointer field that omits children", func() {
+		type ChildType2 struct {
+		}
+		type ChildType struct {
+			Field1 *ChildType2 `json:"field1" desc:"my field" omitChildren:"true"`
+		}
+
+		type TestType struct {
+			Sidecars map[string]ChildType `json:"sidecars"`
+		}
+
+		result := doc.GenerateHelmValuesDoc(
+			TestType{},
+			"test",
+			"my test",
+		)
+		expected := doc.HelmValues{
+			{
+				Key:          "test",
+				Type:         "struct",
+				DefaultValue: " ",
+				Description:  "my test",
+			},
+			{
+				Key:          "test.sidecars",
+				Type:         "map[string, struct]",
+				DefaultValue: "null",
+				Description:  "",
+			},
+			{
+				Key:          "test.sidecars.<MAP_KEY>",
+				Type:         "struct",
+				DefaultValue: " ",
+				Description:  "",
+			},
+			{
+				Key:          "test.sidecars.<MAP_KEY>.Field1",
+				Type:         "struct",
+				DefaultValue: " ",
+				Description:  "my field",
+			},
+		}
+		Expect(result).To(Equal(expected))
+	})
+})

codegen/render/chart_renderer.go

@@ -8,31 +8,35 @@ type ChartRenderer struct {
 	templateRenderer
 }
 
-var defaultChartInputs = inputTemplates{
-	"chart/operator-deployment.yamltmpl": {
-		Path: "templates/deployment.yaml",
-	},
-	"chart/operator-rbac.yamltmpl": {
-		Path: "templates/rbac.yaml",
-	},
-	"chart/_helpers.tpl": {
-		Path: "templates/_helpers.tpl",
-	},
-	"chart/values.yamltmpl": {
-		Path: "values.yaml",
-	},
-	"chart/Chart.yamltmpl": {
-		Path: "Chart.yaml",
-	},
+func defaultChartInputs() inputTemplates {
+	return inputTemplates{
+		"chart/operator-deployment.yamltmpl": {
+			Path: "templates/deployment.yaml",
+		},
+		"chart/operator-rbac.yamltmpl": {
+			Path: "templates/rbac.yaml",
+		},
+		"chart/_helpers.tpl": {
+			Path: "templates/_helpers.tpl",
+		},
+		"chart/values.yamltmpl": {
+			Path: "values.yaml",
+		},
+		"chart/Chart.yamltmpl": {
+			Path: "Chart.yaml",
+		},
+	}
 }
 
-var chartInputsNoOperators = inputTemplates{
-	"chart/values.yamltmpl": {
-		Path: "values.yaml",
-	},
-	"chart/Chart.yamltmpl": {
-		Path: "Chart.yaml",
-	},
+func chartInputsNoOperators() inputTemplates {
+	return inputTemplates{
+		"chart/values.yamltmpl": {
+			Path: "values.yaml",
+		},
+		"chart/Chart.yamltmpl": {
+			Path: "Chart.yaml",
+		},
+	}
 }
 
 func RenderChart(chart model.Chart) ([]OutFile, error) {
@@ -50,9 +54,9 @@ func RenderChart(chart model.Chart) ([]OutFile, error) {
 }
 
 func (r ChartRenderer) Render(chart model.Chart) ([]OutFile, error) {
-	templatesToRender := defaultChartInputs
+	templatesToRender := defaultChartInputs()
 	if len(chart.Operators) == 0 {
-		templatesToRender = chartInputsNoOperators
+		templatesToRender = chartInputsNoOperators()
 	}
 
 	if chart.JsonSchema != nil {

codegen/render/funcs.go

@@ -643,14 +643,14 @@ func toJSONSchema(values values.UserHelmValues) string {
 
 	// add json schema properties from the chart's custom values
 	if values.CustomValues != nil {
-		mergeJsonSchemaProperties(schema, reflector.Reflect(values.CustomValues))
+		mergeJsonSchema(schema, reflector.Reflect(values.CustomValues))
 	}
 
 	// add json schema for the operators
 	if values.Operators != nil {
 		for _, o := range values.Operators {
 			opSchema := jsonSchemaForOperator(reflector, &o)
-			mergeJsonSchemaProperties(schema, opSchema)
+			mergeJsonSchema(schema, opSchema)
 		}
 	}
 
@@ -723,14 +723,33 @@ func createCustomTypeMapper(values values.UserHelmValues) customTypeMapper {
 			// or null if it doesn't handle the type
 
 			// serialize default schema into a map
-			var defaultAsMap map[string]interface{}
 			buf, err := schema.MarshalJSON()
 			if err != nil {
 				panic(err)
 			}
-			err = json.Unmarshal(buf, &defaultAsMap)
-			if err != nil {
-				panic(err)
+
+			var defaultAsMap map[string]interface{}
+
+			// if it's a boolean schema (per section 4.3.2 of the spec) convert it
+			// to a map representation
+			var section432Schema bool
+			err = json.Unmarshal(buf, &section432Schema)
+			if err == nil {
+				if section432Schema {
+					// "true" == Always passes validation, as if the empty schema {}
+					defaultAsMap = map[string]interface{}{}
+				} else {
+					// "false" == Always fails validation, as if the schema { "not": {} }
+					defaultAsMap = map[string]interface{}{
+						"not": map[string]interface{}{},
+					}
+				}
+			} else {
+				// if here - schema is not a bool so unmarshal the schema as map
+				err = json.Unmarshal(buf, &defaultAsMap)
+				if err != nil {
+					panic(err)
+				}
 			}
 
 			// if the type is handled, deserialize it into json schema and return that
@@ -896,20 +915,26 @@ func makePointerFieldsNullable(
 	}
 }
 
-// mergeJsonSchemaProperties Adds the properties from the source json schema
+// mergeJsonSchema Adds the properties from the source json schema
 // to the target
-func mergeJsonSchemaProperties(
+func mergeJsonSchema(
 	target *jsonschema.Schema,
 	src *jsonschema.Schema,
 ) {
 	if target.Properties == nil {
 		target.Properties = orderedmap.New()
 	}
 
+	if target.Required == nil {
+		target.Required = []string{}
+	}
+
 	for _, prop := range src.Properties.Keys() {
 		val, _ := src.Properties.Get(prop)
 		target.Properties.Set(prop, val)
 	}
+
+	target.Required = append(target.Required, src.Required...)
 }
 
 // jsonSchemaForOperator creates the json schema for the operator's values.
@@ -926,7 +951,7 @@ func jsonSchemaForOperator(
 
 	// if the opeartor defines custom values, add the properties to the schema
 	if o.CustomValues != nil {
-		mergeJsonSchemaProperties(schema, reflector.Reflect(o.CustomValues))
+		mergeJsonSchema(schema, reflector.Reflect(o.CustomValues))
 	}
 
 	// nest the operator values schema in the correct place

codegen/render/funcs_test.go

@@ -672,6 +672,29 @@ var _ = Describe("toJSONSchema", func() {
 		Expect(resultContainer.Properties.Field1.AnyOf[1].Const).To(BeFalse())
 	})
 
+	It("merges the required fields", func() {
+		type Type1 struct {
+			Field1 string `jsonschema:"required"`
+		}
+		result := render.ToJSONSchema(values.UserHelmValues{
+			CustomValues: &Type1{},
+		})
+		expected := prepareExpected(`
+		{
+			"$schema": "https://json-schema.org/draft/2020-12/schema",
+			"properties": {
+				"Field1": {
+					"type": "string"
+				}
+			},
+			"required": [
+				"Field1"
+			]
+		}`)
+		Expect(result).To(Equal(expected))
+
+	})
+
 	Context("custom type mappings", func() {
 		type MyJsonSchemaType struct {
 			Type  string `json:"type"`
@@ -762,5 +785,43 @@ var _ = Describe("toJSONSchema", func() {
 				}`)
 			Expect(result).To(Equal(expected))
 		})
+
+		It("can custom map an interface type", func() {
+			type ChildType interface {
+				SomeFunc() bool
+			}
+
+			type TestType struct {
+				Child ChildType `json:"myChild" desc:"big fish"`
+			}
+			result := render.ToJSONSchema(values.UserHelmValues{
+				CustomValues: &TestType{},
+				JsonSchema: values.UserJsonSchema{
+					CustomTypeMapper: func(
+						t reflect.Type,
+						schema map[string]interface{},
+					) interface{} {
+						interfaceType := reflect.TypeOf((*ChildType)(nil)).Elem()
+
+						if t == interfaceType {
+							return &MyJsonSchemaType{
+								Type: "number",
+							}
+						}
+						return nil
+					},
+				},
+			})
+			expected := prepareExpected(`
+			{
+				"$schema": "https://json-schema.org/draft/2020-12/schema",
+				"properties": {
+					"myChild": {
+						"type": "number"
+					}
+				}
+			}`)
+			Expect(result).To(Equal(expected))
+		})
 	})
 })

codegen/test/chart/values.schema.json

@@ -1,6 +1,9 @@
 {
   "$schema": "https://json-schema.org/draft/2020-12/schema",
   "properties": {
+    "customField1": {
+      "type": "string"
+    },
     "painter": {
       "properties": {
         "image": {
@@ -8808,6 +8811,9 @@
         },
         "enabled": {
           "type": "boolean"
+        },
+        "customField2_renamed": {
+          "type": "number"
         }
       },
       "type": "object"