metosin / malli

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

mu/-select-keys doesn't work with local-registry #791

Open abcdw opened 1 year ago

abcdw commented 1 year ago

This one works:

(def registry
  (merge
   schemas
   {:merge (mu/-merge)}))

(m/deref
 [:schema {:registry registry}
  [:merge [:map [:a :int]]
   [:map [:b :string]]]])

This one throws an error:

(def registry
  (merge
   schemas
   {:merge (mu/-merge)
    :select-keys (mu/-select-keys)}))

(m/deref
 [:schema {:registry registry}
  [:merge [:map [:a :int]]
   [:map [:b :string]]]])
:malli.core/child-error {:type :select-keys, :properties nil,
:children nil, :min 2, :max 2}
{:type :malli.core/child-error,
 :message :malli.core/child-error,
 :data
 {:type :select-keys, :properties nil, :children nil, :min 2, :max 2}}
ikitommi commented 1 year ago

Minimal repro:

(m/schema [:schema {:registry {:select-keys (mu/-select-keys)}} :int])

Root cause might be that m/schema walks the local registries and tries to instantiate all IntoSchemas. select-keys blows on invalid child count.

ikitommi commented 1 year ago

even more minimal:

;; :and requires 1+ children
(m/schema [:schema {:registry {::and (m/-and-schema)}} :int])

Not instantiating IntoSchemas should have a positive effect on Schema creation.

opqdonut commented 1 year ago

The problem is that we can't not instantiate the registry. Otherwise we get errors like this:

FAIL in malli.core-test/validation-test (core_test.cljc:739)
schema schemas
Expected:
  [:and
   {:registry {:malli.core-test/a :malli.core-test/b,
               :malli.core-test/b :malli.core-test/c,
               :malli.core-test/c [:schema pos-int?]}}
   [:and :malli.core-test/a :malli.core-test/b :malli.core-test/c]]
Actual:
  [:and
   {:registry {:malli.core-test/a :malli.core-test/b,
               :malli.core-test/b :malli.core-test/c,
               :malli.core-test/c [:schema -pos-int? +#<Fn@7b3c0ecb clojure.core/pos_int_QMARK_>]}}
   [:and :malli.core-test/a :malli.core-test/b :malli.core-test/c]]

The problem here is that the registry is kind of meant for actual schemas, but something like (m/-and-schema) isn't a schema, it's a higher-order-schema. A schema factory.

One hacky option would be to make (m/-and-schema) (and friends) return something that is both an IntoSchema and a Schema with a suitable -form method.

opqdonut commented 1 year ago

One more partial solution would be to make -form fail with a nice error message when it encounters a :registry it can't handle.

frenchy64 commented 3 months ago

The problem here is that the registry is kind of meant for actual schemas, but something like (m/-and-schema) isn't a schema, it's a higher-order-schema. A schema factory.

Yes, I remember @ikitommi noting that IntoSchema's aren't serializable, which is a key requirement of schemas. (Therefore they shouldn't be in local registries).

ikitommi commented 3 months ago

As this popped up, re-collecting my thouhgts on this:

  1. There are already many legal ways to make Schemas not serializable, e.g. with a :gen/gen having a generator as a value => this is ok
  2. Because of 1, forcing :registry to be serialisable doesn't make much sense, user should know what he/she is doing here
  3. Current code which handles the property-registries is bit messy, not easiest to understand and slow because of all the merging and instantiation happening.

Is there a good reason not to support IntoSchemas in the local registry?