add support for or/and key-groups in s/keys

[jimpil]

Tried to do the absolute minimum changes - hence I didn't touch parse-keys which would be the ideal place to do it in...

By the way this whole thing started here

[jimpil]

Hello? Is there no interest in this?

[opqdonut]

I am truly sorry I didn't notice this PR earlier. I'll have a look at it now!

[opqdonut]

Trying to make this mergeable, the first thing I notice is that it breaks pretty much every json-schema test. This is because things like

:required ["name" "parent"]

turn into

:allOf [{:required ["name"]} {:required ["parent"]}]

I feel like with a small adjustment of the logic, it would be possible to still generate :required [x y] in the simple case. Perhaps with something like:

(if (every? :required children)
  {:required (vec (mapcat :required children))}
  children)

This would make it easier to see what this change actually changes.

Additionally, (throw (IllegalArgumentException. "...")) doesn't work in CLJS. Consider this instead:

(throw (ex-info "unsupported key-group expression") {:expression (first x)})

Do you feel like taking this PR forwards after all this time?

[jimpil]

Hi, I 'll try to revisit this over the weekend, taking your feedback into account 👍

btw, what is the semantic or functional difference between the following?

:required ["name" "parent"]

VS

:allOf [{:required ["name"]} {:required ["parent"]}]

[jimpil]

Both comments addressed, and conflict/test resolved 👍

[opqdonut]

As far as I understand the json-schema spec, :required ["name" "parent"] and :allOf [{:required ["name"]} {:required ["parent"]}] mean the same thing. In both cases object must have properties "name" and "parent" to be valid.

However, tools that interpret json-schema (like swagger ui) might have different handling for these. :allOf is potentially much more difficult to handle than just a :required. That's why I think it'll be a bit nicer for users to output just a :required when possible.

I see the CI pipeline is broken in this project at least for CLJS, I'll try fixing it.

One more thing: would you mind adding a new test for this new behaviour?

[opqdonut]

CI is now fixed in #292 . If you push new commits they should get a working CI run.

[jimpil]

I see the CI pipeline is broken in this project at least for CLJS, I'll try fixing it.

ok, I'll push a dummy commit to get the passing tests

One more thing: would you mind adding a new test for this new behaviour?

I was going to do that, but (to my surprise) such a test already exists! See the relevant test file I had to update...

[jimpil]

CI still requires approval, however all tests pass locally (via lein test).

[opqdonut]

Looks good! This is definitely mergeable. Thanks! Consider my suggestion.

[opqdonut]

If you pushed the (every? :required ...) check into parse-required1, it would also combine these two :required, making the output schema simpler.

[jimpil]

I'm afraid it's not enough to just 'push' it into parse-required1 - I have to duplicate it in order to get the outcome you're describing (see last commit). Think of it this way: The one in accept-spec 'clojure.spec.alpha/keys is needed in order to keep the simple (no key groups) case as-is. The one in parse-required1 is needed to simplify a particular use-case after finding a key-group.
If you don't mind that sort of duplication, feel free to merge 👍

[opqdonut]

Oh no, I just realised that while these are the same:

{:allOf [{:required ["a"]} {:required ["b"]}}
{:allOf [{:required ["a" "b"]}]}

These aren't:

{:anyOf [{:required ["a"]} {:required ["b"]}}
{:anyOf [{:required ["a" "b"]}]}

... and I think your last commit (that I asked for) also does the anyOf transformation. So probably best to merge without the check in parse-required1. Sorry about not thinking my suggestion through!

Optimizing the allOf-anyOf tree to be smaller but semantically equivalent is a complex task, and I don't think we really need that in spec->json_schema. At least we should wait until some user bumps into a problem caused by big allOf-anyOf trees.

[jimpil]

... and I think your last commit (that I asked for) also does the anyOf transformation. So probably best to merge without the check in parse-required1. Sorry about not thinking my suggestion through!

You are indeed right - i should have thought that through a bit more too... The good news that we're now an and expression away from fixing the bug (see my last commit a few minutes ago).

Optimizing the allOf-anyOf tree to be smaller but semantically equivalent is a complex task, and I don't think we really need that in spec->json_schema. At least we should wait until some user bumps into a problem caused by big allOf-anyOf trees.

That was pretty much my thought when you first asked for this 'optimisation'...with that said, at this point (and after having fixed the bug I introduced yesterday) I kind of see the point of it all. In any case, if you want me to revert, I will.

[opqdonut]

Thanks, that's great! I intend to merge this tomorrow. I'll add some test cases to make sure I really understand all the repercussions of this, and also have a crack at simplifying the output even more. I have some vague ideas about it, so I don't feel like dumping them on you.

I'll also add some reitit-side test cases so that I can close the original metosin/reitit#619 .

Thanks again for your contribution.

[opqdonut]

released in 0.10.8 🎉