Make validating a bundle thread-safe #237
Conversation
| // 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) { | ||
| func (c ContentEncoding) Validate(propPath string, data interface{}, errs *[]jsonschema.KeyError) {} |
There was a problem hiding this comment.
Is this the standard approach then with the latest jsonschema lib version? Have Validate be a no-op and place all logic in ValidateKeyword?
There was a problem hiding this comment.
Probably not! Sorry about that, I followed what they did in the tests to define a custom keyword and I didn't dig further. Looks like there may be more that can be done.
There was a problem hiding this comment.
Here was their example for making a custom keyword:
https://github.com/qri-io/jsonschema/blob/master/README.md#custom-keywords
They just implemented ValidateKeyword and left the other functions empty.
I looked at a few implementations of string keywords and most also leave those functions empty.
An example of a keyword that does implement Register and resolve is anyOf which allows you to nest a subschema inside the keyword. That doesn't apply here though.
I don't think that the ContentEncoding keyword needs to implement them but if you see something that we should be doing in those functions, let me know. The doc is a bit scarce so I'm piecing it together from usage.
There was a problem hiding this comment.
Ah, thank you for the references. Indeed, it looks like what you've done for ContentEncoding should be the right approach. 👍
|
I assume there are not other JSON schema libraries that are thread-safe? Do you have any preference to maintaining the fork in your account, or in this organization? |
|
The other library is https://github.com/xeipuuv/gojsonschema. They don't say that they are threadsafe, there are still open issues with people asking about that or reporting errors (without reproduction). I like the idea of trying it out and seeing if it would work for us. Let me give that a try and see if it blows up or not. |
carolynvs
force-pushed
the
jsonschema-lock
branch
from
9cf69c8
to
f200d1e
Compare
|
Update: Both libraries need more patches to address threadsafety and also neither is making progress towards accepting any patches. Another cnab-go user, @dataoleg, has run into problems with this bug. Since the "better" fix than this patch isn't forthcoming, I would like to see us merge this PR here so that at least cnab-go users are no longer affected. |
carolynvs
force-pushed
the
jsonschema-lock
branch
from
f200d1e
to
104c6a2
Compare
* Use fork of jsconschema library with a fix for threadsafe calls to validate. https://github.com/carolynvs/jsonschema/tree/local-keyword-registry Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
force-pushed
the
jsonschema-lock
branch
from
104c6a2
to
742da2c
Compare
Background:
The jsonschema library we use is not thread-safe: qri-io/jsonschema#80. This prevents callers from validating and working with bundles concurrently.
Current Fix:
I made a fork of the latest version jsonschema and tweaked it so that it doesn't use package variables for everything, and uses a lock when it does access the global schema. I'm not sure if that is a solution that they will merge, still waiting on a reply. We are still using unreleased commits on docker repos so at least there is precedent. 😀
The other changes you see in the PR are due to changes in the latest version of jsonschema.
I tried to fix this without changing jsonschema and wasn't successful. However I am not sure how we feel about using a fork of jsonschema when the patch may never get merged, or the upstream bug fixed.
We really needed this in Porter, so either way I'm maintaining the fork... 🤷♀️