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

Add Sequence Schemas to JSON Schema generation #795

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NoahTheDuke
Copy link
Contributor

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.

@NoahTheDuke NoahTheDuke force-pushed the nb/add-sequence-schemas-to-json branch from e11d828 to 702fb6a Compare December 6, 2022 21:45
@ikitommi
Copy link
Member

ikitommi commented Dec 7, 2022

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

@NoahTheDuke
Copy link
Contributor Author

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
Copy link
Member

ikitommi commented Dec 9, 2022

I don't think there is. Some options:

1) resolve locally

  • return sequence schema json schemas maps a tag "these can be inlined", e.g. (with-meta cat-json-schema {::regex-schema true}) and inline those in the with sequence schemas json schema functions
  • GOOD: simple
  • BAD: not generic
  • ???: is this really simple or just a hack?

2) just mark them as "arrays of anything"

  • GOOD: least amount of work
  • BAD: not describing the real thing

3) schema simplifier

  • this has been drafted few times already: given any schema, return a simplified schema. e.g. [:and :keyword :keyword keyword?] is just :keyword. Could be used here too. If done via a protocol, could be just implemented first for sequence schemas
  • GOOD: the ultimate solution
  • BAD: lot of work
  • !!!: should not increase JS-bundle size, e.g. not used by default, optional thing that can be DCE'd

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

@ikitommi
Copy link
Member

ikitommi commented Dec 9, 2022

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

meander has good tools for rewriting code.

@NoahTheDuke
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

@ikitommi ikitommi added the enhancement New feature or request label Jan 10, 2023
@NoahTheDuke
Copy link
Contributor Author

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
Copy link
Member

ikitommi commented Feb 3, 2023

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
Copy link
Member

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

@ikitommi
Copy link
Member

ping @NoahTheDuke

@NoahTheDuke
Copy link
Contributor Author

Yeah, I'll do that tonight/tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 🇰‍🇼 Waiting
Development

Successfully merging this pull request may close these issues.

:? vs :sequential in JSON Schema transformers
2 participants