Multi schema not working with transformers

[jaen]

I would have expected the following code (taken from the readme and modified to add a string-to-keyword key transformer) to work - the transformer works correctly with a :map schema – but it doesn't:

(m/decode
  [:multi {:dispatch :type
           :decode/string '#(update % :type keyword)}
   [:sized [:map [:type [:= :sized]] [:size int?]]]
   [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]]
  {"type" "human"
   :name "Tiina"
   :size "98"
   :address {:country "finland"
             :street "this is an extra key"}}
  (mt/transformer
    ;; this is new
    (mt/key-transformer {:decode (comp keyword str/kebab)
                         :encode str/camel})
    mt/strip-extra-keys-transformer
    mt/string-transformer))
;; expected:
;; => {:type :human, :name "Tiina", :address {:country "finland"}}
;; actual:
;; => {"type" "human", :name "Tiina", :size "98", :address {:country "finland", :street "this is an extra key"}, :type nil}

Am I doing something wrong or is this a bug?

[ikitommi]

The :multi has a :string/decode property which should be run before anything else so that the :type will match a branch in :multi. You can move the mt/string-transformer first.

or you could add a {:name :before} transformer as first and change the :multi property to :decode/before, e.g.:

(m/decode
  [:multi {:dispatch :type
           :decode/before '#(update % :type keyword)}
   [:sized [:map [:type [:= :sized]] [:size int?]]]
   [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]]
  {"type" "human"
   :name "Tiina"
   :size "98"
   :address {:country "finland"
             :street "this is an extra key"}}
  (mt/transformer
    {:name :before}
    (mt/key-transformer {:decode (comp keyword str/kebab)
                         :encode str/camel})
    mt/strip-extra-keys-transformer
    mt/string-transformer))

Not on computer, so didn't check if that works.

[jaen]

It didn't seem to have worked (explain says it's an :m/invalid-dispatch-value). And maybe the example itself is doing a bad job of explaining my issue (sorry for that), so I'll try to describe where I think the issue is.

I'm trying to use malli to coerce API responses from JSON to Clojure data structures. Idiomatic Clojure maps use keywords as keys, hence the key transformer doing (comp keyword str/kebab) on the keys.

This all works well enough with simple :maps – my transformer runs first and coercion happens correctly, an int? coerces "98" to 98 and so on, even if the key is a string in the input. However some API responses have shape conditional on the response type, so I need to use the :multi schema. This however has a problem – the :multi schema seems to decide on the branch before the string-to-keyword transformer is applied to the keys, so the dispatch function :type has no value to choose from, because it's still the string "type".

The issue as I see it, is that the :multi schema tries to decide which schema to apply before any other transformer runs (so it doesn't see :type, only "type") which seems somewhat unituitive, if other transformers can influence it's choice. I can see how this can't be the default, because for example mt/strip-extra-keys-transformer makes sense only after we know which schema :multi chooses, but there should be a way to specify some interceptors need to be run before others. I assume it's what the {:name :before} is meant for in your example, but I don't see it documented in the readme and as said above, it doesn't seem to help in this case.

Dispatching on a string is something that works:

(m/decode
  [:multi {:dispatch #(get % "type")
           :decode/string '#(update % "type" keyword)}
   [:sized [:map [:type [:= :sized]] [:size int?]]]
   [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]]
  {"type" "human"
   "name" "Tiina"
   "size" "98"
   "address" {"country" "finland"
              "street" "this is an extra key"}}
  (mt/transformer
    (mt/key-transformer {:decode (comp keyword str/kebab)
                         :encode str/camel})
    mt/strip-extra-keys-transformer
    mt/string-transformer))

but this means the schema needs to be aware of what the type of input keys is – I think it's better that schema would always deal with keyword keys and the transformer would apply any key coercion (if needed) to maintain the separation of concerns.

Hopefully that is a clearer explanation what my issue and you can see what needs to be addressed here, either implementation- or documentation-wise. Thanks for the help!

[ikitommi]

Ok, had time to investigate: mt/key-transformer is mapped to work on :map schemas - so it doesn't do anything for :multi. Because of this, the key is "type", not :type. Options:

1. change the mt/key-transformer impl so that it works for all schemas, not just :map
2. like 1, but make this configurable, as there might be cases where it is not a wanted, e.g. convert keys in :map-of
3. just make an example of this

the example being:

(require '[malli.core :as m])
(require '[malli.transform :as mt])
(require '[clojure.string :as str])

(def ->keyword (comp keyword str/lower-case m/-keyword->string))

(m/decode
  [:multi {:dispatch :type
           :decode/string #(update % :type ->keyword)}
   [:sized [:map [:type [:= :sized]] [:size int?]]]
   [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]]
  {"Type" "human"
   :Name "Tiina"
   :sizE "98"
   :Address {:country "finland"
             :street "this is an extra key"}}
  (mt/transformer
    ;; step1: transform ALL map keys, regardless of related schema
    {:default-decoder (mt/-transform-map-keys ->keyword)}
    ;; step2: string->edn, including the property-override in :multi
    mt/string-transformer
    ;; step3: effects just :map s, but we are in the right branch of :multi, so it works
    mt/strip-extra-keys-transformer))
;{:type :human
; :name "Tiina"
; :address {:country :finland}}

[ikitommi]

The current code (both decoding & encoding just for :maps):

malli/src/malli/transform.cljc

Lines 374 to 375 in bb3e6df

	{:decoders {:map (transform decode :enter)}
	:encoders {:map (transform encode :leave)}})))

[jaen]

Thanks for the example, it works as advertised!

While I think it's certainly useful to showcase how this can be solved with the :default-decoder option, I feel it's a bit of an unexpected gotcha that this doesn't work for :multis of :maps. If your transformer converts the keys of your :maps and you have a :multi to choose between schemas, you would naively expect them to just compose and work if the type for :dispatch after key conversion matches (as it does here) – that it does not is surprising.

As such, I think changing implementation to make the behaviour more uniform and less surprising would be good (maybe with some kind of an escape hatch, like you suggest) – I'll leave the issue open for you to decide what action you prefer.

[ikitommi]

options

1: use :and

(m/decode
  [:and
   [:map]
   [:multi {:dispatch :type
            :decode/string #(update % :type ->keyword)}
    [:sized [:map [:type [:= :sized]] [:size int?]]]
    [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]]]
  {"Type" "human"
   :Name "Tiina"
   :sizE "98"
   :Address {:country "finland"
             :street "this is an extra key"}}
  (mt/transformer
    (mt/key-transformer {:decode ->keyword})
    mt/string-transformer
    mt/strip-extra-keys-transformer))
;{:type :human
; :name "Tiina"
; :address {:country :finland}}

• good: works already
• bad: looking from outside, it's an :and, not :multi anymore

2: add targets to mt/key-transformer

(mt/key-transformer {:decode ->keyword, :types #{:map :multi}})

• bad: it's global
• good: simple impl

3: (derived) type property

now that we have m/type-properties, we actually could pull the derived :multi type from the childs and if all are the same, we could use that. In your example, all types are :map so the multi could have a type-property :type :map.

(m/type-properties
  [:multi {:dispatch :type
             :decode/string #(update % :type ->keyword)}
     [:sized [:map [:type [:= :sized]] [:size int?]]]
     [:human [:map [:type [:= :human]] [:name string?] [:address [:map [:country keyword?]]]]]])
; => {:type :map}

• good: resolves Define a :type for all schemas #39 too, could be set manually via normal schema props too
• bad: more complex, really needed? all schema applications need check type info from multiple places. might not work in all places? e.g. with JSON Schema, it should be oneOf, not object

[jaen]

Sorry for missing the reply, just want to say I appreciate you resolving this! I would've preferred the default to be all the types, but I understand it's for backward-compatibility and that #264 will solve it anyway.

Thanks again!