-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package definition | ||
|
||
import ( | ||
"github.com/qri-io/jsonschema" | ||
) | ||
|
||
// ContentEncoding represents a "custom" Schema property | ||
type ContentEncoding string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
// Currently, this is a no-op and is only used to register with the jsonschema library | ||
// that 'contentEncoding' is a valid property (as of writing, it isn't one of the defaults) | ||
func (c ContentEncoding) Validate(propPath string, data interface{}, errs *[]jsonschema.ValError) {} | ||
|
||
// 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 intances of type `contentEncoding` | ||
vdice marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deal! #132