metosin / malli

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

properties are not preserved by the `:merge` schema #998

Closed vise890 closed 4 months ago

vise890 commented 5 months ago

I would expect (:merge malli.util/schemas) to behave like malli.util/merge:

(ns schemas
  (:require [malli.core :as ma]
            [malli.util :as mu]))

;; this is the behavior of malli.util/merge: properties are preserved (expected)
(ma/properties
 (ma/schema (mu/merge [:map {:p1 1}
                       [:x :int]]
                      [:map
                       [:y :int]])
           ))
;; => {:p1 1}

;; this is the behavior of the malli.util :merge schema: properties are lost in the process
(ma/properties
 (ma/schema [:merge
             [:map {:p1 1}
              [:x :int]]
             [:map
              [:y :int]]]
            {:registry
             (merge (ma/default-schemas)
                    (mu/schemas))}))
;; => nil

This seems to stem from the implementation of into-schema of malli.util/-util-schema. Perhaps it could fall back to the merged schema's properties like so:

(defn -util-schema [{:keys [type min max childs type-properties fn]}]
  ^{:type ::m/into-schema}
  (reify m/IntoSchema
    (-type [_] type)
    (-type-properties [_] type-properties)
    (-properties-schema [_ _])
    (-children-schema [_ _])
    (-into-schema [parent properties children options]
      (m/-check-children! type properties children min max)
      (let [[children forms schema] (fn properties (vec children) options)
            properties (or properties (m/properties schema)) ; 👈 HERE
            form (delay (m/-create-form type properties forms options))

(works, but I'm unsure of the implications)

Tested on malli 0.14.0

Thank you!

ikitommi commented 5 months ago

Makes sense, PR welcome.

vise890 commented 5 months ago

Thank you for the quick reply @ikitommi !

So i put the proposed fix in and ran all the tests, and one unintended consequence is this:

(m/form
 (m/schema
  [:merge
   [:map {:closed true}
    [:x :int]]
   [:map
    [:y :int]]]
  {:registry (merge (m/default-schemas)
                    (mu/schemas))}))
;; => [:merge {:closed true} [:map {:closed true} [:x :int]] [:map [:y :int]]]
;;            ^^^^^^^^^^^^^^

Perhaps it's best to leave the form alone, an just rely on m/deref to get at the properties of the result after all.. I'm happy to close this unless you have another suggestion.

Thank you again, and keep up the amazing work!

ikitommi commented 4 months ago

don't have other suggestions, let's close this. thanks for your work on this!