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

Initial implementation of json-schema import #211

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hkupty
Copy link

@hkupty hkupty commented Jun 30, 2020

This allows a very basic json-schema->malli conversion.

Still missing for this PR

  • cleanup TODOS;
  • ensure function names are sane;
  • ensure all json-schema features are supported (at least for one specific version, i.e. draft-7):
    • Enum
    • Const
    • Annotation
    • Comments
    • String
      • Length
      • Regular Expressions
      • Format
    • Numeric Types
      • Number
      • Integer
      • Multiples
      • Range
    • Object
      • Properties
      • Required
      • Size
      • Dependencies
      • Pattern Properties
    • Array
      • Items (list)
      • Items (tuple)
      • Length
      • Uniqueness
    • Boolean
    • Null
    • Combining
      • oneOf
      • allOf
      • anyOf
      • not
    • $ref

@ikitommi
Copy link
Member

ikitommi commented Jul 4, 2020

Looking forward to this. Related project: https://github.com/akovantsev/json-schema-to-clojure-spec

@hkupty
Copy link
Author

hkupty commented Jul 4, 2020

Thanks for the reference, I'll take a look at it.

@ikitommi ikitommi changed the title Initial implementation of json-schema import DRAFT: Initial implementation of json-schema import Nov 15, 2020
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, 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.
@hkupty hkupty marked this pull request as ready for review January 6, 2021 14:01
@hkupty
Copy link
Author

hkupty commented Jan 6, 2021

@ikitommi I'm really sorry for taking so long to release the PR for review.
I've unfortunately shifted priorities at my project (which was the main driver for this PR) and couldn't really invest time into finish. Found some time after holidays to tidy things up and I want to release this as soon as possible.

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.
Best regards,
Henry

@hkupty
Copy link
Author

hkupty commented Jan 6, 2021

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.

@hkupty hkupty changed the title DRAFT: Initial implementation of json-schema import Initial implementation of json-schema import Jan 18, 2021
@ikitommi ikitommi force-pushed the master branch 2 times, most recently from 8e7a7b3 to daeef5a Compare February 28, 2021 18:15
@hkupty
Copy link
Author

hkupty commented Mar 30, 2021

@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?

@aneilbaboo aneilbaboo mentioned this pull request Mar 6, 2022
@tangrammer
Copy link

any update or review on this work? could i contribute to it too?

@hkupty
Copy link
Author

hkupty commented May 10, 2022

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.

@tangrammer
Copy link

I'll try it 😅 , could you provide some data example to test the overall functionality?

@tangrammer
Copy link

@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}))))

tangrammer added a commit to proofzero/rollupid that referenced this pull request May 10, 2022
@vharmain
Copy link
Contributor

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

@PavlosMelissinos
Copy link

PavlosMelissinos commented Mar 3, 2023

Great PR! It's missing some essential features though, e.g. the following syntax examples fail:

{"type": "string", "format": "uuid"} ;; this parses as string, should be `uuid?`

{"type": ["string", "null"]} ;; throws an exception and I'm not seeing it in the TODO list

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 malli.json-schema-test suffice or would you prefer dedicated tests?

(-keys :const) [:enum (:const js-schema)]

;; Aggregates
(-keys :oneOf) (into
Copy link

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 ?

Copy link

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,...).

@PavlosMelissinos
Copy link

PavlosMelissinos commented Mar 7, 2023

I'll try to work further on this during the weekend

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...?

Would adding some round-trip unit tests (malli->json-schema->malli) to achieve parity with malli.json-schema-test suffice

FYI unfortunately round trip tests don't work because some syntax on both sides is ambiguous

@spieden
Copy link

spieden commented May 4, 2023

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.

@bsless
Copy link
Contributor

bsless commented May 4, 2023

Is this PR still active?

@vharmain
Copy link
Contributor

vharmain commented May 4, 2023

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.

@PavlosMelissinos
Copy link

I'm not absolutely sure what it's waiting for

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.

Here's the diff in case anyone wants to play with it.

@spieden
Copy link

spieden commented May 4, 2023

I tried it out on a complex schema with a graph of $refs and it encountered the problem of schemas in :definitions referring to one another. Will need to reduce over a topological sort and pass along a registry map I guess. 🤔

@spieden
Copy link

spieden commented May 5, 2023

Unless, is there some way to do late binding?

@PavlosMelissinos
Copy link

@spieden Do you have a small repro & expected result? I'll try to work on it during the weekend.

@timothypratley
Copy link

@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:
https://github.com/timothypratley/vegamite
I checked out your branch in a parallel directory and used a :local/root dependency to it to get your code.

Sadly, it throws a :malli.core/invalid-schema and it's not clear to me why.
Any suggestions?

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.

@PavlosMelissinos
Copy link

PavlosMelissinos commented Jul 3, 2023

@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?

@zcaudate
Copy link

I'm wondering if this is relatively stable. would like to try it out.

@PavlosMelissinos
Copy link

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🇰‍🇼 Waiting
Development

Successfully merging this pull request may close these issues.

None yet

10 participants