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

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

Open
abcdw opened this issue Dec 4, 2022 · 6 comments
Open

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

abcdw opened this issue Dec 4, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@abcdw
Copy link
Contributor

abcdw commented Dec 4, 2022

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 ikitommi added the bug Something isn't working label Jan 10, 2023
@ikitommi
Copy link
Member

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

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 opqdonut self-assigned this Jan 17, 2023
@opqdonut
Copy link
Member

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

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

@opqdonut opqdonut moved this to 📋 Backlog in Metosin Open Source Backlog Jan 24, 2023
@opqdonut opqdonut moved this from 🏗 Doing to 🐰 Next in Metosin Open Source Backlog Feb 7, 2023
@frenchy64
Copy link
Collaborator

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

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?

@opqdonut opqdonut removed their assignment Nov 27, 2024
@opqdonut opqdonut moved this from 🐰 Next to 👍 Backlog in Metosin Open Source Backlog Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 👍 Backlog
Development

No branches or pull requests

4 participants