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

feat: :map ::m/rename-keys transformer metadata #132

Closed

Conversation

rschmukler
Copy link
Contributor

Introduces the ability for map transformers to provide information about key-renames they perform by including the ::m/rename-keys key-map in metadata attached to either a run-time value (ie. the data returned by the map transformer) or on the returned
interceptor functions themselves.

This allows map transformers to not break subsequent calls to child
value transformers.

Initially I tried to solve this generically with a :leave interceptor
in the map transformer, but this broke down with nested maps since
:after is currently executed as a prewalk.

It may be useful to also change the behavior of :leave to behave more
like a postwalk, but that requires more thought and is certainly a much
larger PR.

Introduces the ability for map transformers to provide (at compile time
or run-time) information about key-ranames they perform by including the
`::m/rename-keys` key-map in metadata attached to either a run-time
value (ie. the data returned by the map transformer) or on the returned
interceptor functions themselves.

This allows map transformers to not break subsequent calls to child
value transformers.

Initially I tried to solve this generically with a `:leave` interceptor
in the map transformer, but this broke down with nested maps since
`:after` is currently executed as a prewalk.

It may be useful to also change the behavior of `:leave` to behave more
like a postwalk, but that requires more thought and is certainly a much
larger PR.
@ikitommi
Copy link
Member

ikitommi commented Dec 10, 2019

I see the problem, but not happy with the meta-data. Would a normal pre-walk work here?

(m/decode
  [:and
   {:decode/before (constantly {:leave (partial
                                         walk/postwalk
                                         #(cond-> % (map? %) (set/rename-keys {:foo :oof :bar :rab})))})}
   [:map
    [:foo [:map
           [:foo int?]
           [:bar int?]]]
    [:bar int?]]]
  {:foo {:foo 2 :bar 1} :bar 3}
  (transformer
    {:name :before
     :decoders {'int? (constantly inc)}}))
; => {:oof {:oof 3, :rab 2}, :rab 4}

@ikitommi
Copy link
Member

or from outside:

(->> (m/decode
       [:map
        [:foo [:map
               [:foo int?]
               [:bar int?]]]
        [:bar int?]]
       {:foo {:foo 2 :bar 1} :bar 3}
       (transformer
         {:name :before
          :decoders {'int? (constantly inc)}}))
     (walk/postwalk #(cond-> % (map? %) (set/rename-keys {:foo :oof :bar :rab}))))
; => {:oof {:oof 3, :rab 2}, :rab 4}

@ikitommi
Copy link
Member

... or some way to backtrack changes keys. hmm. might be too complex.

@rschmukler
Copy link
Contributor Author

Backtracking key changes is easy if we have a key mapping function (such as map-of) but not when the transformer can arbitrarily change things.

A normal post-walk does work if you can modify the schema entry's decoder, but not if you want to put the functionality into a transformer. As far as I can tell, there is no way to perform the change in the test (as a :map transformer) without this type of functionality.

@ikitommi
Copy link
Member

ikitommi commented Dec 10, 2019

what about:

(->> (m/decode
       [:map
        [:foo [:map
               [:foo int?]
               [:bar int?]]]
        [:bar int?]]
       {:foo {:foo 2 :bar 1} :bar 3}
       (transformer
         {:decoders {'int? (constantly inc)
                     :map (constantly {:leave (partial
                                                walk/postwalk
                                                #(cond-> % (map? %) (set/rename-keys {:foo :oof :bar :rab})))})}})))
; => {:oof {:oof 3, :rab 2}, :rab 4}

@ikitommi
Copy link
Member

... actually, should be encode as decode should transform the values into domain:

(->> (m/encode
       [:map
        [:foo [:map
               [:foo int?]
               [:bar int?]]]
        [:bar int?]]
       {:foo {:foo 2 :bar 1} :bar 3}
       (transformer
         {:encoders {'int? (constantly inc)
                     :map (constantly {:leave (partial
                                                walk/postwalk
                                                #(cond-> % (map? %) (set/rename-keys {:foo :oof :bar :rab})))})}})))

@rschmukler
Copy link
Contributor Author

rschmukler commented Dec 10, 2019

The above solution makes it so that the root-most map transformer is responsible for all renames. To make things a bit less abstract, let me explain my intended use case.

I'd like to encode namespaced maps as unnamespaced entries, using no prefix if the namespace is the most common namespace in the schema, and otherwise prefixing with the namespace. Eg:

(def schema
  [:map 
     [:person/name string?]
     [:person/age pos-int?]
     [:person/location
       [:map [:location/city string?]
             [:location/state string?
              :location/coordinates
              [:map [:coordinate/lon float?]
                    [:coordinate/lat float?]]
     [:oauth/id int?]
     [:oauth/username string?]])

{:person/name "Bob Jones"
 :person/age 42
 :person/location {:location/city "Brooklyn" :location/state "NY"
                   :location/coordinates {:coordinate/lon 1.51
                                          :coordinate/lat 1.21}}
 :oauth/id 5
 :oauth/username "bobbyj"}

;; should become

{:name "Bob Jones"
 :age 42
 :location {:city "Brooklyn" :state "NY" :coordinates {:lon 1.51 :lat 1.21}
 :oauth-id 5
 :oauth-username "bobbyj"}

With key renames this can look like:

(defn generate-encode-key-rename-map
  "Function to generate a map of key renames for a provided
  schema with the given options."
  [schema opts]
  (let [allowed-keys (:keys (m/-parse-keys (m/children schema) opts))
        root-ns      (or (:json/root-namespace opts)
                         (->> (frequencies (map namespace allowed-keys))
                              (sort-by val)
                              (last)
                              key))
        rename-map   (into {}
                           (map
                            (fn [k]
                              [k (let [ns   (namespace k)
                                       name (name k)]
                                   (if (= root-ns ns)
                                     name
                                     (str (str/kebab ns) "-" name)))]))
                           allowed-keys)]
    rename-map))


(defn encode-namespaced-keys
  "Returns an interceptor that will encode keys as strings as well as filtering
  the map to only allowed keys."
  [schema opts]
  (let [rename-map   (generate-encode-key-rename-map schema opts)
        allowed-keys (keys rename-map)]
    (with-meta
      (fn [x]
        (-> x
            (select-keys allowed-keys)
            (set/rename-keys rename-map)))
      {::m/rename-keys rename-map})))

(def my-transformer
  {:name :customs 
    {:encoders {:map encode-namespaced-keys}})

Doing something like the above becomes extremely difficult with the current implementation.

@ikitommi
Copy link
Member

I think #140 will help here.

Related to the (m/-parse-keys (m/children schema) opts), that's horrible - the same code exists in the json-schema & generator namespaces. Maybe we could have a new Protocol that the :map schema implements and which would have the map-releated helpers? e.g. public api instead of impl details...

rschmukler added a commit to rschmukler/malli that referenced this pull request Dec 12, 2019
This commit fixes two issues:

1) It no longer invokes `transformer` related methods twice while
creating parent transformers. This fixes errors with stateful
transformers that depend on sharing data between enter and leave.

2) It fixes the order in which interceptors call their children for the
`map-of`, `sequence`, `tuple` and `map` interceptors - causing the child
transformers to be invoked first on the `leave`.

Closes metosin#140, metosin#132
rschmukler added a commit to rschmukler/malli that referenced this pull request Dec 13, 2019
This commit fixes two issues:

1) It no longer invokes `transformer` related methods twice while
creating parent transformers. This fixes errors with stateful
transformers that depend on sharing data between enter and leave.

2) It fixes the order in which interceptors call their children for the
`map-of`, `sequence`, `tuple` and `map` interceptors - causing the child
transformers to be invoked first on the `leave`.

Closes metosin#140, metosin#132
ikitommi pushed a commit that referenced this pull request Dec 13, 2019
This commit fixes two issues:

1) It no longer invokes `transformer` related methods twice while
creating parent transformers. This fixes errors with stateful
transformers that depend on sharing data between enter and leave.

2) It fixes the order in which interceptors call their children for the
`map-of`, `sequence`, `tuple` and `map` interceptors - causing the child
transformers to be invoked first on the `leave`.

Closes #140, #132
@rschmukler
Copy link
Contributor Author

Fixed with proper interceptor order. Put it in the :leave of a :map transformer!

@rschmukler rschmukler closed this Dec 13, 2019
@rschmukler rschmukler deleted the rschmukler/track-key-changes branch December 13, 2019 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants