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

:merge with registries does not round-trip #1088

Open
frenchy64 opened this issue Aug 9, 2024 · 2 comments
Open

:merge with registries does not round-trip #1088

frenchy64 opened this issue Aug 9, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@frenchy64
Copy link
Collaborator

If schemas with registries are merged with :merge, the dereferenced schema often cannot be round-tripped.

Notice the following schema is missing a ::z binding so is no longer well-formed.

(-> (m/deref
      [:merge
       [:map {:registry {::y int?}}
        [:y ::y]]
       [:map {:registry {::z boolean?}}
        [:z ::z]]]
      options)
    m/form)
=>
[:map {:registry {::y boolean?}}
 [:y ::y]
 [:z ::z]]

We might be able to fix this by merging the registries, but something else is needed if the registries both define the same names.

For example, here the schema for :y is being captured by the wrong scope, and becomes [:y boolean?] instead of [:y int?].

(-> (m/deref
      [:merge
       [:map {:registry {::y int?}}
        [:y ::y]]
       [:map {:registry {::y boolean?}}
        [:z ::y]]]
      options)
    m/form)
=>
[:map {:registry {::y boolean?}}
 [:y ::y]
 [:z ::y]]
@frenchy64
Copy link
Collaborator Author

frenchy64 commented Aug 13, 2024

This example doesn't work at all, and complains that ::y is not in scope.

[:merge
 [:map
  {::y boolean?}
  [:y ::y]]
 [:map]]

@ikitommi
Copy link
Member

ikitommi commented Jan 1, 2025

The latter example works right, got fixed at some point?

(-> (m/schema
     [:merge
      [:map {:registry {::z boolean?}}
       [:y ::z]]
      [:map {:registry {::z int?}}
       [:x ::z]]])
    (malli.edn/write-string)
    (malli.edn/read-string))
;[:merge
; [:map {:registry #:use{:z boolean?}} [:y :user/z]]
; [:map {:registry #:user{:z int?}} [:x :user/z]]]

and:

(-> (m/deref
     [:merge
      [:map {:registry {::z boolean?}}
       [:y ::z]]
      [:map {:registry {::z int?}}
       [:x ::z]]])
    (malli.edn/write-string)
    (malli.edn/read-string))
;[:map {:registry #:user{:z int?}} 
; [:y :user/z] 
; [:x :user/z]]

but this doesn't:

(-> (m/deref
     [:merge
      [:map {:registry {::z boolean?
                        ::y boolean?}}
       [:y ::z]
       [:z ::y]]
      [:map {:registry {::z int?}}
       [:x ::z]]])
    (malli.edn/write-string)
    (malli.edn/read-string))
; =throws=> :missing ::y

so, yes, the registries should be merged propertly.

my 1 cent:

  1. we could have 2 registries, global and local -> schemas can assume that the global is always available and only carry the accumulated local registry with them. this should be pushed to schema forms so one can create schemas out of the forms

@ikitommi ikitommi added the bug Something isn't working label Jan 1, 2025
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: No status
Development

No branches or pull requests

2 participants