From 878bb6deaed425d5d5c47b7330f47209b2ca71ca Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Tue, 15 Aug 2023 16:28:04 +0200 Subject: [PATCH] Return better error messages for invalid JSON schema types in templates (#661) ## Changes Adds a function to validate json schema types added by the author. The default json unmarshaller does not validate that the parsed type matches the enum defined in `jsonschema.Type` Includes some other improvements to provide better error messages. This PR was prompted by usability difficulties reported by @mingyu89 during mlops stack migration. ## Tests Unit tests --- libs/jsonschema/schema.go | 37 ++++++++++++++++++++++++++++ libs/jsonschema/schema_test.go | 44 ++++++++++++++++++++++++++++++++++ libs/template/config.go | 15 ++++++++---- libs/template/config_test.go | 31 ++++++++++++++++++++++++ libs/template/utils.go | 8 +++++-- libs/template/utils_test.go | 6 +++++ 6 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 libs/jsonschema/schema_test.go diff --git a/libs/jsonschema/schema.go b/libs/jsonschema/schema.go index 49e31bb7..c0d1736c 100644 --- a/libs/jsonschema/schema.go +++ b/libs/jsonschema/schema.go @@ -1,5 +1,11 @@ package jsonschema +import ( + "encoding/json" + "fmt" + "os" +) + // defines schema for a json object type Schema struct { // Type of the object @@ -47,3 +53,34 @@ const ( ArrayType Type = "array" IntegerType Type = "integer" ) + +func (schema *Schema) validate() error { + for _, v := range schema.Properties { + switch v.Type { + case NumberType, BooleanType, StringType, IntegerType: + continue + case "int", "int32", "int64": + return fmt.Errorf("type %s is not a recognized json schema type. Please use \"integer\" instead", v.Type) + case "float", "float32", "float64": + return fmt.Errorf("type %s is not a recognized json schema type. Please use \"number\" instead", v.Type) + case "bool": + return fmt.Errorf("type %s is not a recognized json schema type. Please use \"boolean\" instead", v.Type) + default: + return fmt.Errorf("type %s is not a recognized json schema type", v.Type) + } + } + return nil +} + +func Load(path string) (*Schema, error) { + b, err := os.ReadFile(path) + if err != nil { + return nil, err + } + schema := &Schema{} + err = json.Unmarshal(b, schema) + if err != nil { + return nil, err + } + return schema, schema.validate() +} diff --git a/libs/jsonschema/schema_test.go b/libs/jsonschema/schema_test.go new file mode 100644 index 00000000..76112492 --- /dev/null +++ b/libs/jsonschema/schema_test.go @@ -0,0 +1,44 @@ +package jsonschema + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestJsonSchemaValidate(t *testing.T) { + var err error + toSchema := func(s string) *Schema { + return &Schema{ + Properties: map[string]*Schema{ + "foo": { + Type: Type(s), + }, + }, + } + } + + err = toSchema("string").validate() + assert.NoError(t, err) + + err = toSchema("boolean").validate() + assert.NoError(t, err) + + err = toSchema("number").validate() + assert.NoError(t, err) + + err = toSchema("integer").validate() + assert.NoError(t, err) + + err = toSchema("int").validate() + assert.EqualError(t, err, "type int is not a recognized json schema type. Please use \"integer\" instead") + + err = toSchema("float").validate() + assert.EqualError(t, err, "type float is not a recognized json schema type. Please use \"number\" instead") + + err = toSchema("bool").validate() + assert.EqualError(t, err, "type bool is not a recognized json schema type. Please use \"boolean\" instead") + + err = toSchema("foobar").validate() + assert.EqualError(t, err, "type foobar is not a recognized json schema type") +} diff --git a/libs/template/config.go b/libs/template/config.go index ee5fcbef..173244b0 100644 --- a/libs/template/config.go +++ b/libs/template/config.go @@ -18,13 +18,11 @@ type config struct { func newConfig(ctx context.Context, schemaPath string) (*config, error) { // Read config schema - schemaBytes, err := os.ReadFile(schemaPath) + schema, err := jsonschema.Load(schemaPath) if err != nil { return nil, err } - schema := &jsonschema.Schema{} - err = json.Unmarshal(schemaBytes, schema) - if err != nil { + if err := validateSchema(schema); err != nil { return nil, err } @@ -36,6 +34,15 @@ func newConfig(ctx context.Context, schemaPath string) (*config, error) { }, nil } +func validateSchema(schema *jsonschema.Schema) error { + for _, v := range schema.Properties { + if v.Type == jsonschema.ArrayType || v.Type == jsonschema.ObjectType { + return fmt.Errorf("property type %s is not supported by bundle templates", v.Type) + } + } + return nil +} + // Reads json file at path and assigns values from the file func (c *config) assignValuesFromFile(path string) error { // Read the config file diff --git a/libs/template/config_test.go b/libs/template/config_test.go index 7b8341ec..33524246 100644 --- a/libs/template/config_test.go +++ b/libs/template/config_test.go @@ -161,3 +161,34 @@ func TestTemplateConfigValidateTypeForInvalidType(t *testing.T) { err = c.validate() assert.EqualError(t, err, `incorrect type for int_val. expected type integer, but value is "this-should-be-an-int"`) } + +func TestTemplateValidateSchema(t *testing.T) { + var err error + toSchema := func(s string) *jsonschema.Schema { + return &jsonschema.Schema{ + Properties: map[string]*jsonschema.Schema{ + "foo": { + Type: jsonschema.Type(s), + }, + }, + } + } + + err = validateSchema(toSchema("string")) + assert.NoError(t, err) + + err = validateSchema(toSchema("boolean")) + assert.NoError(t, err) + + err = validateSchema(toSchema("number")) + assert.NoError(t, err) + + err = validateSchema(toSchema("integer")) + assert.NoError(t, err) + + err = validateSchema(toSchema("object")) + assert.EqualError(t, err, "property type object is not supported by bundle templates") + + err = validateSchema(toSchema("array")) + assert.EqualError(t, err, "property type array is not supported by bundle templates") +} diff --git a/libs/template/utils.go b/libs/template/utils.go index bf11ed86..ade6a573 100644 --- a/libs/template/utils.go +++ b/libs/template/utils.go @@ -66,8 +66,10 @@ func toString(v any, T jsonschema.Type) (string, error) { return "", err } return strconv.FormatInt(intVal, 10), nil - default: + case jsonschema.ArrayType, jsonschema.ObjectType: return "", fmt.Errorf("cannot format object of type %s as a string. Value of object: %#v", T, v) + default: + return "", fmt.Errorf("unknown json schema type: %q", T) } } @@ -87,8 +89,10 @@ func fromString(s string, T jsonschema.Type) (any, error) { v, err = strconv.ParseFloat(s, 32) case jsonschema.IntegerType: v, err = strconv.ParseInt(s, 10, 64) - default: + case jsonschema.ArrayType, jsonschema.ObjectType: return "", fmt.Errorf("cannot parse string as object of type %s. Value of string: %q", T, s) + default: + return "", fmt.Errorf("unknown json schema type: %q", T) } // Return more readable error incase of a syntax error diff --git a/libs/template/utils_test.go b/libs/template/utils_test.go index 5fe70243..1e038aac 100644 --- a/libs/template/utils_test.go +++ b/libs/template/utils_test.go @@ -80,6 +80,9 @@ func TestTemplateToString(t *testing.T) { _, err = toString("abc", jsonschema.IntegerType) assert.EqualError(t, err, "cannot convert \"abc\" to an integer") + + _, err = toString("abc", "foobar") + assert.EqualError(t, err, "unknown json schema type: \"foobar\"") } func TestTemplateFromString(t *testing.T) { @@ -112,4 +115,7 @@ func TestTemplateFromString(t *testing.T) { _, err = fromString("1.0", jsonschema.IntegerType) assert.EqualError(t, err, "could not parse \"1.0\" as a integer: strconv.ParseInt: parsing \"1.0\": invalid syntax") + + _, err = fromString("1.0", "foobar") + assert.EqualError(t, err, "unknown json schema type: \"foobar\"") }