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.

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

fix(bundle/definition): add custom validator for contentEncoding #118

Merged
merged 2 commits into from
Sep 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions bundle/definition/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strings"

"github.com/pkg/errors"
"github.com/qri-io/jsonschema"
)

type Definitions map[string]*Schema
Expand Down Expand Up @@ -96,7 +95,7 @@ func (s *Schema) UnmarshalJSON(data []byte) error {
// Before we unmarshal into the cnab-go bundle/definition/Schema type, unmarshal into
// the library struct so we can handle any validation errors in the schema. If there
// are any errors, return those.
js := new(jsonschema.RootSchema)
js := NewRootSchema()
if err := js.UnmarshalJSON(data); err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions bundle/definition/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/json"

"github.com/pkg/errors"
"github.com/qri-io/jsonschema"
)

// ValidationError error represents a validation error
Expand All @@ -24,7 +23,7 @@ func (s *Schema) Validate(data interface{}) ([]ValidationError, error) {
if err != nil {
return nil, errors.Wrap(err, "unable to load schema")
}
def := new(jsonschema.RootSchema)
def := NewRootSchema()
err = json.Unmarshal([]byte(b), def)
if err != nil {
return nil, errors.Wrap(err, "unable to build schema")
Expand Down
69 changes: 69 additions & 0 deletions bundle/definition/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,75 @@ func TestObjectValidationValid(t *testing.T) {
assert.NoError(t, err)
}

func TestObjectValidationValid_CustomValidator_ContentEncoding_base64(t *testing.T) {
s := `{
"type": "object",
"properties" : {
"file" : {
"type": "string",
"contentEncoding": "base64"
}
},
"required" : ["file"]
}`
definition := new(Schema)
err := json.Unmarshal([]byte(s), definition)
require.NoError(t, err, "should have been able to marshal definition")
assert.Equal(t, "object", definition.Type, "type should have been an object")
props := definition.Properties
assert.NotNil(t, props, "should have found properties")
assert.Equal(t, 1, len(props), "should have had a single property")
propSchema, ok := props["file"]
assert.True(t, ok, "should have found file property")
assert.Equal(t, "string", propSchema.Type, "file type should have been a string")
assert.Equal(t, "base64", propSchema.ContentEncoding, "file contentEncoding should have been base64")

val := struct {
File string `json:"file"`
}{
File: "SGVsbG8gV29ybGQhCg==",
Copy link
Contributor

Choose a reason for hiding this comment

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

If I change this to an invalid base64 value such as "SGVsbG8gV29ybGQhCg===", the test still passes. Shouldn't it fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call; indeed, as mentioned in the code comments, the current validator is a no-op (simply allows for a contentEncoding property), but we can and perhaps should actually perform some basic validations specific to this property. Preference on doing so in this PR or as a follow-up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'd prefer it in this PR to check feasibility before merging this PR. Hope it's not too much work - maybe a separate commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the values we need to support are defined by https://json-schema.org/latest/json-schema-validation.html#rfc.section.8.3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, validation added for base64. Wanted to keep scope limited so didn't venture off into adding other content encoding validations quite yet. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Why not raise issue(s) for the other encodings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Deal! #132

}
valErrors, err := definition.Validate(val)
assert.NoError(t, err)
assert.Len(t, valErrors, 0, "expected no validation errors")

invalidVal := struct {
File string `json:"file"`
}{
File: "SGVsbG8gV29ybGQhCg===",
}
valErrors, err = definition.Validate(invalidVal)
assert.NoError(t, err)
assert.Len(t, valErrors, 1, "expected 1 validation error")
assert.Equal(t, "invalid base64 value: SGVsbG8gV29ybGQhCg===", valErrors[0].Error)
}

func TestObjectValidationValid_CustomValidator_ContentEncoding_InvalidEncoding(t *testing.T) {
s := `{
"type": "object",
"properties" : {
"file" : {
"type": "string",
"contentEncoding": "base65"
}
},
"required" : ["file"]
}`
definition := new(Schema)
err := json.Unmarshal([]byte(s), definition)
require.NoError(t, err, "should have been able to marshal definition")

val := struct {
File string `json:"file"`
}{
File: "SGVsbG8gV29ybGQhCg==",
}
valErrors, err := definition.Validate(val)
assert.NoError(t, err)
assert.Len(t, valErrors, 1, "expected 1 validation error")
assert.Equal(t, "unsupported or invalid contentEncoding type of base65", valErrors[0].Error)
}

func TestObjectValidationInValidMinimum(t *testing.T) {
s := `{
"type": "object",
Expand Down
45 changes: 45 additions & 0 deletions bundle/definition/validators.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package definition

import (
"encoding/base64"
"fmt"

"github.com/qri-io/jsonschema"
)

// ContentEncoding represents a "custom" Schema property
type ContentEncoding string
Copy link
Contributor

Choose a reason for hiding this comment

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

As NewContentEncoding is returning a jsonschema.Validator, do we really need to expose that type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair question; I followed the library's examples to set this up and I'd lean towards keeping it for code clarity/comprehension, but can change if majority deems it unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have strong opinion on it, I just have the habit of reducing the exposed parts to their minimum 👍


// NewContentEncoding allocates a new ContentEncoding validator
func NewContentEncoding() jsonschema.Validator {
return new(ContentEncoding)
}

// Validate implements the Validator interface for ContentEncoding
// which, as of writing, isn't included by default in the jsonschema library we consume
func (c ContentEncoding) Validate(propPath string, data interface{}, errs *[]jsonschema.ValError) {
if obj, ok := data.(string); ok {
switch c {
case "base64":
_, err := base64.StdEncoding.DecodeString(obj)
if err != nil {
jsonschema.AddError(errs, propPath, data, fmt.Sprintf("invalid %s value: %s", c, obj))
}
// Add validation support for other encodings as needed
// See https://json-schema.org/latest/json-schema-validation.html#rfc.section.8.3
default:
jsonschema.AddError(errs, propPath, data, fmt.Sprintf("unsupported or invalid contentEncoding type of %s", c))
}
}
}

// NewRootSchema returns a jsonschema.RootSchema with any needed custom
// jsonschema.Validators pre-registered
func NewRootSchema() *jsonschema.RootSchema {
// Register custom validators here
// Note: as of writing, jsonschema doesn't have a stock validator for instances of type `contentEncoding`
// There may be others missing in the library that exist in http://json-schema.org/draft-07/schema#
// and thus, we'd need to create/register them here (if not included upstream)
jsonschema.RegisterValidator("contentEncoding", NewContentEncoding)
return new(jsonschema.RootSchema)
}