metosin / malli

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

feat: support for (mutual) recursion in malli.json-schema #883

Closed opqdonut closed 1 year ago

opqdonut commented 1 year ago

Mutual recursion support. Fixes #464 #868

A schema walker bug was making life difficult so this includes a fix for it. Fixes #884 . Removed mu/from-map-syntax and mu/to-map-syntax to make the fix easier.

opqdonut commented 1 year ago

This fix has some weird consequences:

user> (def closed (mu/closed-schema [:schema {:registry {"Foo" [:map [:a :int]]}} "Foo"]))
#'user/closed
user> (m/form closed)
[:schema {:registry {"Foo" [:map [:a :int]]}} "Foo"]
user> (-> closed m/deref m/deref m/form)
[:map {:closed true} [:a :int]]

So the instance of "Foo" used at the top level is mutated (and now differs from the one in the registry), but it doesn't show up in the m/form because it's hidden by the id of the :schema.

opqdonut commented 1 year ago

I think I'll have to rethink the closed-schema fix, perhaps on the level of schema-walker or set-children.

opqdonut commented 1 year ago

I managed to implement a workaround for malli.json-schema. I opened #884 for the mu/closed-schema problem. This PR is mergeable even if we don't solve #884.

opqdonut commented 1 year ago

I think I have an actual solution to the walker problem now. I'll test it out & then rebase this PR to get rid of all the intermediate steps.

opqdonut commented 1 year ago

just added a test on the schema-walker level

opqdonut commented 1 year ago

I tried removing the [id] branch from -schema-schema. That caused a number of tests to fail, so we are relying on the [id] case. Those could probably be reworked, but we might break outsider users as well. However, all my new tests passed, so this is a viable fix.

If you prefer, I can go forward with this slightly-nicer but slightly-more-breaking fix.

diff --git a/src/malli/core.cljc b/src/malli/core.cljc
index aa21916..6d53e20 100644
--- a/src/malli/core.cljc
+++ b/src/malli/core.cljc
@@ -1705,7 +1705,7 @@
               (when (-accept walker this path options)
                 (if (or (not id) ((-boolean-fn (::walk-schema-refs options false)) id))
                   (-outer walker this path (-inner-indexed walker path children options) options)
-                  (-outer walker this path [id] options))))
+                  (-outer walker this path children #_[id] options))))
             (-properties [_] properties)
             (-options [_] options)
             (-children [_] children)
opqdonut commented 1 year ago

Discussed this with Tommi. This PR now has the more principled fix, and as a side-effect removes mu/from-map-syntax and mu/to-map-syntax.

ikitommi commented 1 year ago

This is really good, fixes real issue and simplifies the core 🙇, also:

Screenshot 2023-04-12 at 16 09 32