Schema Applications can use MapEntry properties

[ikitommi]

Spike on whether and how MapEntry properties should used in schema applications:

• forms
• transformation
• default transformer
• error messages (postponed)
• value generation
• json schema transformation
• swagger2 transformation

Related issues: #80, #86, #116, #120, #182
Related PRs: #119, #197

forms

• should not change (e.g. not merging data to children)

(m/form [:map [:x {:encode/math inc} int?]])
; => [:map [:x {:encode/math inc} int?]]

transformation

• should allow extra property-based transformations
• should have a target key for :encoders & :decoders?

(m/decode 
  [:map [:x {:encode/math inc} [int? {:encode/math inc}]]]
  {:x 1}
  (mt/transformer {:name :math}))
; => {:x 3}

error messages

• should use the parent props if defined
• despite the orginal cause (Feature request or bug: adding properties directly to map keys. #86), not resolving this with this PR. Handling errors is a special case, will resolve that later

(-> [:map [:x {:error/message "over 18"} [:and int? [:> 18]]]
    (m/explain {:x 12})
    (me/humanize))
; {:x ["over 18"]}

• also for composite schemas

(-> [:and {:error/message "over 18"} int? [:> 18]]
    (m/explain 12)
    (me/humanize))
; ["over 18"]

value generation

• should use the parent props if defined

(mg/generate [:map [:foo {:gen/elements [1 2]} int?]])
; => {:foo 1}

json-schema

• should use merged properties (parent wins)

(json-schema/transform [:map [:x {:json-schema {:type "numberz"}} int?]])
;{:type "object",
; :properties {:x {:type "numberz"},
; :required [:x]}

(json-schema/transform [:map [:x {:json-schema/type "numberz"} [:schema {:json-schema/format "int32"} [int? {:json-schema/example 1}]]]])
;{:type "object",
; :properties {:x {:type "numberz", :format "int32", :example 1},
; :required [:x]}

swagger

• like with json-schema

[ikitommi]

Tested if there is a quick fix for this using m/parent-properties and generic parent handling. Not looking good.

Idea was, that for contained schemas (:and, :or, :map/entry etc) one can look up the parent properties. This would resolve all the cases to some degree but the code and whole contract seems hacky.

For example, one should check :map/entry and :and & :or differently in JSON Schema. Generic solution would be good target but too greedy:

(defn- -json-schema-visitor [schema children _in options]
  (let [p (m/properties schema)
        pp (m/parent-properties schema)]
    (or (unlift pp :json-schema)
        (unlift p :json-schema)
        (merge (select p)
               (select pp)
               (if (satisfies? JsonSchema schema)
                 (-accept schema children options)
                 (accept (m/type schema) schema children options))
               (unlift-keys p :json-schema)
               (unlift-keys pp :json-schema)))))

=>

(json-schema/transform
  [:and {:json-schema/title "and"}
   int?
   boolean?])
;{:allOf [{:type "integer", :format "int64", :title "and"} 
;         {:type "boolean", :title "and"}]
; :title "and"}

Just adding the parent handling to :map/entry (not :and and :or) would not resolve the error cases and would be a new mechanism just for :map/entry.

Pushing the special code into JSON Schema :map would also be bad, expecially as :swagger composes on top of :json-schema:

(defmethod accept :map [_ _ children _]
  (let [required (->> children (filter (comp not :optional second)) (mapv first))]
    {:type "object"
     :properties (apply array-map (mapcat (fn [[k p s]]
                                            [k (or (unlift p :json-schema)
                                                   (merge (select p)
                                                          (unlift-keys p :json-schema)
                                                          s))]) children))
     :required required}))

and copy-paste (previously not neede code!) to swagger:

(defmethod accept :map [_ _ children _]
  (let [required (->> children (filter (comp not :optional second)) (mapv first))]
    {:type "object"
     :properties (apply array-map (mapcat (fn [[k p s]]
                                            [k (or (json-schema/unlift p :swagger)
                                                   (json-schema/unlift p :json-schema)
                                                   (merge
                                                     (json-schema/select p)
                                                     (json-schema/unlift-keys p :json-schema)
                                                     (json-schema/unlift-keys p :swagger)
                                                     s))]) children))
     :required required}))

Based on the few hours spent on this, it looks like we should have first class (internal) MapEntry Schema that is returned in walking. So, this:

(m/to-map-syntax
  [:map
   [:x int?]
   [:y {:optional true} int?]])

would return instead of:

0) current:

• special 3-tuple syntax:

{:type :map
 :children [[:x nil {:type int?}] 
            [:y {:optional true} {:type int?}]]}

1) hybrid syntax:

• pushing properties to a real type:

{:type :map
 :children [[:x {:type :malli.core/entry
                 :children [{:type int?}]}]
            [:y {:type :malli.core/entry
                 :properties {:optional true}
                 :children [{:type int?}]}]]}

2) post-hybrid

• as weird as 1:

{:type :map
 :children [{:type :malli.core/entry
             :children [:x {:type int?}]}
            {:type :malli.core/entry
             :properties {:optional true}
             :children [:y {:type int?}]}]}

3) full type

• encode key as special property, just maps.

{:type :map
 :children [{:type :malli.core/entry
             :properties {:malli.core/key :x}
             :children [{:type int?}]}
            {:type :malli.core/entry
             :properties {:malli.core/key :y
                          :optional true}
             :children [{:type int?}]}]}

Hmm.

[ikitommi]

Heureka! Conditional walking.

[ikitommi]

All work but the error messages.

[ikitommi]

challenge with errors is that errors are reported currently on the failing schema, for composite schemas like :and, :or and ::m/entry, the error messages should be looked from parent schemas. From how far? just from one level up? Should be collected top-down (like everything else)?

top-down would mean:

(-> [:map 
     [:x {:error/message "from entry, over 18"} 
      [:and {:error/message "from and, over 18"} 
       int? [:> 18]]]
    (m/explain {:x 12})
    (me/humanize))
; {:x ["from entry, over 18"]}

should it be always looked up?

(-> [:map 
     [:x {:error/message "from entry, over 18"} 
      [:and {:error/message "from and, over 18"} 
       int? [:> {:error/message "from :>, over 18"} 18]]]
    (m/explain {:x 12})
    (me/humanize))
; {:x ["from entry, over 18"]}

... currently taken from the schema itself, if present. unlike everything else.

[ikitommi]

Reletively happy with this now:

• real, transformable and optionally walkable -val-schema (::m/val)
• m/children returns 3-tuple (key, properties, schma) for MapSchemas
• m/map-entries is removed, m/entries returns a sequence of MapEntry of (key, val-schema)

(def schema
  [:map
   [:x boolean?]
   [:y {:optional true} int?]
   [:z {:optional false} string?]])

(m/children schema)
;[[:x nil boolean?]
; [:y {:optional true} int?]
; [:z {:optional false} string?]]

(m/entries schema)
;[[:x [:malli.core/val boolean?]]
; [:y [:malli.core/val {:optional true} int?]]
; [:z [:malli.core/val {:optional false} string?]]

(every? map-entry? (m/entries schema))
; true

(map key (m/entries schema))
;(:x :y :z)

still, does not throw:

(m/entries int?)
; nil

This would break everyone using m/map-entries. Should we add it back, using the old contract, on top of the new apis?

(defn map-entries [s]
  (if (m/entries s) (m/children s)))

EDIT: IMO no, better to break this to keep things clean.

[rschmukler]

@ikitommi Thanks for all your thought on this issue. Got a chance to play with the latest code and it's working great.

Re. your question, I would break m/map-entries while we can. The difference between m/entries and m/children is already somewhat confusing (at first) - to me, keeping old API baggage from alpha doesn't seem worth it.

I think as we near release it will be worth adding doc-strings to both functions that explain when to use which one. My understanding (and please, correct me if I am wrong) is that most transformers would likely use m/children since it handles unwrapping the internals of MapEntry schema for you, while walking and schema transformations are better served by having direct access to the MapEntry AST node.

Looked over the code as well - looks good!

[ikitommi]

Thanks for the review! Will add the docstrings.

[ikitommi]

some docs in 3443411