fix(bundle/definition): add custom validator for contentEncoding #118
Conversation
vdice
force-pushed
the
fix/custom-validators
branch
from
1a7a762
to
f51437c
Compare
| val := struct { | ||
| File string `json:"file"` | ||
| }{ | ||
| File: "SGVsbG8gV29ybGQhCg==", |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Fair enough. Why not raise issue(s) for the other encodings?
| ) | ||
|
|
||
| // ContentEncoding represents a "custom" Schema property | ||
| type ContentEncoding string |
There was a problem hiding this comment.
As NewContentEncoding is returning a jsonschema.Validator, do we really need to expose that type?
There was a problem hiding this comment.
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.
I don't have strong opinion on it, I just have the habit of reducing the exposed parts to their minimum 👍
vdice
force-pushed
the
fix/custom-validators
branch
2 times, most recently
from
3768a4e
to
b962d3d
Compare
vdice
force-pushed
the
fix/custom-validators
branch
from
b962d3d
to
47a8d43
Compare
This represents a fix to ensure definitions with a non-empty
contentEncodingproperty pass validation by adding a custom validator for continued use of the jsonschema library cnab-go currently uses. It's specific tocontentEncodingbut illustrates a potential path forward for further "custom" validators (either for properties in the default JSON Schema but not stock in the jsonschema library or for truly custom properties/validators down the line.)See #117 for background on general issue and getporter/porter#579 for feature that uses the changes here to pass validation.