-
Notifications
You must be signed in to change notification settings - Fork 211
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
Initial implementation of json-schema import #211
base: master
Are you sure you want to change the base?
Conversation
Looking forward to this. Related project: https://github.com/akovantsev/json-schema-to-clojure-spec |
Thanks for the reference, I'll take a look at it. |
This allows a very basic json-schema->malli conversion and will most likely need to be adequated to: - cleanup TODOS; - ensure function names are sane; - ensure all json-schema features are supported (at least for one specific version, i.e. draft-7).
Also, remove duplication.
Also, reorganize functions to simplify processing, reduce duplication and avoid special casing. Lastly, make schema->malli the first entry point since anything can be a schema. It then delegates to types, aggregates or `$ref`s.
Also, adds top-level enums (and consts) and prepares for better range checks in int/number.
59d42d7
to
18332a6
Compare
@ikitommi I'm really sorry for taking so long to release the PR for review. I'm intending to write some tests, but wanted to get a review going not to delay this any further. Once again, sorry for having you waiting on this. Happy new year, btw. |
I've decided to leave some stuff out for now, but I can implement if needed. Again, the idea was to get this rolling. Could be in a separate PR as well. |
8e7a7b3
to
daeef5a
Compare
@ikitommi revisting this now since it might be that I'll end up getting back to the project that inspired this PR. wdyt about this PR? |
any update or review on this work? could i contribute to it too? |
It is unfortunate to have it dangling... I would really like to merge this, but it is unfair of me to push for it since I'm not even using clojure on my daily work anymore for quite some time now... Feel free to take over the patches here, @tangrammer. I wanted to get some feedback from the approach and merge something - even incomplete if the case - at least as a basis. |
I'll try it 😅 , could you provide some data example to test the overall functionality? |
@hkupty no worries just found the way! (require '[clojure.walk :refer (keywordize-keys)])
(def data {"$id" "https://example.com/person.schema.json",
"$schema" "https://json-schema.org/draft/2020-12/schema",
"title" "Person",
"type" "object",
"properties"
{"firstName"
{"type" "string", "description" "The person's first name."},
"lastName"
{"type" "string", "description" "The person's last name."},
"age"
{"description"
"Age in years which must be equal to or greater than zero.",
"type" "integer",
"minimum" 0}}})
(def my-schema (schema->malli (keywordize-keys data)))
(m/schema? my-schema)
(m/form my-schema)
(assert (= true (m/validate my-schema (keywordize-keys {"firstName" "John", "lastName" "Doe", "age" 21}))))
(assert (not= true (m/validate my-schema (keywordize-keys {"firstName" "John", "lastName" 1, "age" 21}))))
|
It's really nice to see that this PR is alive again! We've had discussions with @bsless about bi-directional JSON-schema support on Clojurians Slack #malli channel. Welcome there to discuss @tangrammer |
Great PR! It's missing some essential features though, e.g. the following syntax examples fail:
I think I have fixed both (in my local copy) and I'll try to work further on this during the weekend if there's interest. I'd like to know what is missing from this in order to be merged. Would adding some round-trip unit tests (malli->json-schema->malli) to achieve parity with |
(-keys :const) [:enum (:const js-schema)] | ||
|
||
;; Aggregates | ||
(-keys :oneOf) (into |
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.
You can do this by combining and
, or
, and not
.
(is (equivalent? (xor X Y) (or (and X (not Y)) (and Y (not X))))))
It would be nice if malli just added a xor schema though (which could do the same expansion under the hood).
@ikitommi have you considered adding :xor
?
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.
xor != oneOf
https://en.wikipedia.org/wiki/Exclusive_or
Since it is associative, it may be considered to be an n-ary operator which is true if and only if an odd number of arguments are true. That is, a XOR b XOR ... may be treated as XOR(a,b,...).
I have pushed the results to a branch on my fork. You can check out the ground truth here for a list of conversions that work. Should I create a new PR or...?
FYI unfortunately round trip tests don't work because some syntax on both sides is ambiguous |
Could be a handy use case for generative testing. i.e. Generate data and verify that it validates both against a json schema directly (using a separate library) and against its malli representation. Really slick would be a function from a particular json schema to a generator. The generator could emit both valid and subtly invalid data and you could just check that the two validation paths agree. |
Is this PR still active? |
This ticket is on our "Open source backlog" in "Waiting" status but I'm not absolutely sure what it's waiting for. Anyway I think this is still on our radar but not on top of the backlog. |
This PR is missing some stuff (e.g. it doesn't have any tests and some conversions just don't work). I spent some time on it a couple months ago and while there are still some nuances because the conversions are lossy (e.g. after a round trip you aren't guaranteed to get the initial result), I think it's an improvement. If there's interest I could raise a new PR but it will need feedback before it can be reviewed by metosin and hopefully merged. |
I tried it out on a complex schema with a graph of |
Unless, is there some way to do late binding? |
@spieden Do you have a small repro & expected result? I'll try to work on it during the weekend. |
@PavlosMelissinos Thank you for running with this, I think this would be a very useful feature. I've been trying to do json-schema validation in this tiny example: Sadly, it throws a I'm trying to use the vega-lite schema which is very large. I'm hoping that by converting to a Malli schema, it will produce nice error messages if I miss-spell a field etc. |
@timothypratley I'll try to take a look later today (but I can't promise I will manage to). However, the example you're using is huge and my version of the converter still has some known problems (e.g. with refs). In the meantime, I've opened a new (draft) PR to keep discussions clean; do you think you can try to come up with a smaller repro and add a comment there? |
I'm wondering if this is relatively stable. would like to try it out. |
@zcaudate Sadly I haven't managed to work on this since my last comment. Feel free to try it out but the problems that @timothypratley has pointed out here and in the other PR are still there. Depending on your use cases you might be able to get some value out of it but I wouldn't call it stable... |
This allows a very basic json-schema->malli conversion.
Still missing for this PR