metosin / malli

High-performance data-driven data specification library for Clojure/Script.
Eclipse Public License 2.0
1.5k stars 211 forks source link

Add Sequence Schemas to JSON Schema generation #795

Open NoahTheDuke opened 1 year ago

NoahTheDuke commented 1 year ago

Closes #793

Tried to stick with how things work in the existing code. Looks like Malli's generated JSON Schema isn't up to date with the most recent JSON Schema specs, so I'm not entirely sure I did this right.

ikitommi commented 1 year ago

Does this work correctly with inlined sequence scehmas? e.g. [:cat [:cat :string] [:+ [:+ :string]]] presents a flat list of strings.

NoahTheDuke commented 1 year ago

Oh that's a good point. I didn't know that Malli inlined those schemas. Is there a way to apply that inlining upfront?

ikitommi commented 1 year ago

I don't think there is. Some options:

1) resolve locally

2) just mark them as "arrays of anything"

3) schema simplifier

@frenchy64 had one (partial) solution for this, with the first version of the recursive regex.

ikitommi commented 1 year ago

here's one old test to this: https://github.com/miikka/boolean-simplifier

meander has good tools for rewriting code.

NoahTheDuke commented 1 year ago

After thinking about it for a little while, I feel like 1) is the way to go. 2) is basically what Malli has now which is unintuitive ("why doesn't json schema transformation work?").

3) doesn't work because malli.json-schema/-transform uses m/walk which is a post-walk. This means we'd have to walk the schema 2+ times (first to do any merging/inlining, then second to transform resulting schema).

1) can be done in a single pass in malli.json-schema/accept, where the sequence schemas could add metadata and check their children for the metadata and merge as necessary.

What do you think?

NoahTheDuke commented 1 year ago

I realize now that I basically said the same thing as you regarding 1). lol Sorry about that. I don't think it's hacky. I don't know that there's a good abstraction that doesn't require a full rewrite of the existing json schema architecture.

ikitommi commented 1 year ago

If you think you can make it work with 1, let's go with that.

NoahTheDuke commented 1 year ago

I spent some time on this, and I don't know that the "resolve locally" solution is possible. I find myself building an ad-hoc version of "schema simplifier", which I don't have the skill or time to develop the skill to properly implement.

Given that, how would you feel about some sort of "NOT CURRENTLY WORKING" warning when converting a sequence schema to json schema? They currently just return {}, which I think is reasonable but surprising without any warning.

ikitommi commented 1 year ago

Thanks for giving it a shot. I think a reasonable default would be a vector of anything. At least it's of right type.

When we have a generic simplifier, we can make this better.

ikitommi commented 1 year ago

could you change these to return "vector of anything"?

ikitommi commented 1 year ago

ping @NoahTheDuke

NoahTheDuke commented 1 year ago

Yeah, I'll do that tonight/tomorrow.