Malli parameters merging broken #422
Assignees
Labels
Comments
|
Thanks for the reproduction example. Seems like a bug; further investigation and PR welcome! |
|
Oh, didn't think how the hiccup-syntax meta-merges. Not good: (meta-merge.core/meta-merge
{:parameters {:path [:map [:a string?]]}}
{:parameters {:path [:map [:b string?]]}})
; => {:parameters {:path [:map [:a string?] :map [:b string?]]}}this is what should happen: (malli.util/merge
[:map [:a 'string?]]
[:map [:b 'string?]])
; => [:map [:a string?] [:b string?]]... might need to add a callback to core-router to handle certain paths with custom logic. This is not just malli, also spec and plumatic are effected: (require '[spec-tools.data-spec :as ds])
(meta-merge.core/meta-merge
{:parameters {:path {(ds/opt :a) string?}}}
{:parameters {:path {:a string?}}})
; => {:parameters {:path {#spec_tools.data_spec.OptionalKey{:k :a} string?, :a string?}}}(require '[schema.core :as s])
(meta-merge.core/meta-merge
{:parameters {:path {(s/optional-key :a) s/Str}}}
{:parameters {:path {:a s/Str}}})
; => {:parameters {:path {#schema.core.OptionalKey{:k :a} java.lang.String, :a java.lang.String}}} |
Deraen
changed the title
|
Changing meta-merge logic would be inconvenient, but maybe it is possible to handle certain paths, like |
Deraen
added a commit
that referenced
this issue
Feb 26, 2021
|
I would add a new protocol |
bwalex
pushed a commit
to bwalex/reitit
that referenced
this issue
Dec 12, 2021
Deraen
added a commit
that referenced
this issue
Dec 25, 2021
|
It seems |
|
I just got bit by the inability to merge nested path parameters. It would be awesome if reitit could automatically handle merging of both these cases ;; With malli syntax
["/foo/:foo"
{:parameters {:path [:map [:foo :int]]}}
["/bar/:bar"
{:parameters {:path [:map [:bar :int]]}}]];; With malli schemas
["/foo/:foo"
{:parameters {:path (m/schema [:map [:foo :int]])}}
["/bar/:bar"
{:parameters {:path (m/schema [:map [:bar :int]])}}]] |
|
Assigning myself to this |
|
@ikitommi Check the latest mentioned PR also (#561). At least that would solve providing API that isn't specific to parameters value, but it leaves it to the user to solve how to apply certain merge logic to one parameter. I think one option we talked about was that the merging rules could be given as a map (similar to exception handlers), the key would be a top-level data key, and the default would be the current implementation. That would only allow customizing rules for top-level data, but that is probably enough for most cases, and users could still implement more complex logic by configuring the default merge handler. |
|
Thanks, will check that. |
|
I think I have a found a fix for this:
Looks like: (require '[reitit.ring :as ring]
'[malli.util :as mu]
'[reitit.core :as r]
'[ctrl.merge :as cm])
(defn ring-path-map [f]
[[[:parameters cm/any] f]
[[ring/http-methods :parameters cm/any] f]
[[:responses cm/any :body] f]
[[ring/http-methods :responses cm/any :body] f]])
(defn malli-merge [x y _] (mu/merge x y))
(-> (ring/router
["/api" {:parameters {:header [:map ["Api" :string]]}}
["/math/:x" {:parameters {:path [:map [:x :int]]
:query [:map [:b :string]]
:header [:map ["Math" :string]]}
:responses {200 {:body [:map [:total :int]]}
500 {:description "fail"}}}
["/plus/:y" {:get {:parameters {:query ^:replace [:map [:a :int]]
:body [:map [:b :int]]
:header [:map ["Plus" :string]]
:path [:map [:y :int]]}
:responses {200 {:body [:map [:total2 :int]]}
500 {:description "fail"}}
:handler (constantly {:status 200, :body "ok"})}}]]]
{:meta-merge #(cm/merge %1 %2 {::cm/path-map (ring-path-map malli-merge)})})
(ring/ring-handler)
(ring/get-router)
(r/compiled-routes)
(last)
(last)
:get
:data)
;{:parameters {:header [:map
; ["Api" :string]
; ["Math" :string]
; ["Plus" :string]],
; :path [:map
; [:x :int]
; [:y :int]],
; :query [:map [:a :int]],
; :body [:map [:b :int]]},
; :responses {200 {:body [:map
; [:total :int]
; [:total2 :int]]}
; 500 {:description "fail"}},
; :handler #object[clojure.core$constantly$fn__5740]}TODO:
|
|
proposing #626 as a fix for this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The text was updated successfully, but these errors were encountered: