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

Liberator support #185

Closed
simax opened this issue Dec 17, 2015 · 19 comments
Closed

Liberator support #185

simax opened this issue Dec 17, 2015 · 19 comments

Comments

@simax
Copy link

simax commented Dec 17, 2015

HI,

In issue #182 you mentioned that there would be better support for Liberator, albeit as a side effect of the fix for the aforementioned issue.

I'm experiencing the same issue described in issue #175.

I've already built an API using Liberator and was hoping I could just put compojure-api routes in front of it and then get the Swagger documentation, validation and all the other awsomeness that you guys have created for free but it's not quite working out that way. So I'm excited by the comment you made stating that better Liberator support was coming by the end of the year.

Is that still the case? Will we be able to use the 2 libraries together soon?

@ikitommi
Copy link
Member

Yes.

We know people who are already using Liberator + Compojure-api together (disabling partially/fully the coercion on c-api side and using the :swagger just for docs), but it bit ugly. With the upcoming, protocol-based Swagger, the Liberator (or any other handler-lib) can produce their endpoint docs which get merged into the routing libs docs. Allows coercion to happen at right time in the (Liberator) decision tree. I think. Chatted with @ordnungswidrig some time ago, needs something on the Liberator side too.

So, definetely on the roadmap.

Let's use this issue (&Slack) for this.

@simax
Copy link
Author

simax commented Dec 18, 2015

Excellent, really looking forward to it. I couldn't find a Slack channel specifically for compojure-api. Is it worth making one are do you plan just to use the general Clojure channel?

@Deraen
Copy link
Member

Deraen commented Dec 18, 2015

The channel is #ring-swagger

@ikitommi
Copy link
Member

ikitommi commented Jan 5, 2016

Btw, I think you can already disable the Liberator auto-serialization with it's ring-response. This allows one to use c-api response coercion & formats.

(defresource x
  :handle-ok (fn [ctx] (ring-response {:kikka "kukka"})))

@ikitommi
Copy link
Member

The protocols are mostly done. Will ping the Liberator-guys after 1.0.0 gets finalized.

@simax
Copy link
Author

simax commented Jan 20, 2016

Great news guys. Looking forward using swagger in my project.

On Mon, 18 Jan 2016 at 21:35 Tommi Reiman [email protected] wrote:

The protocols are mostly done. Will ping the Liberator-guys after 1.0.0
gets finalized.


Reply to this email directly or view it on GitHub
#185 (comment)
.

@preoctopus
Copy link

Interested if there's any updates on this.

@ikitommi
Copy link
Member

1.0.0 is out, so let's do this. I think the options are:

A) Just generate docs

we could write a helper to describe the full swagger-spec for a resource and provide it as a wrapper: consuming ring-swagger options and the Liberator-handler.

(endpoint
  {:get {:parameters {:query {:x Long, :y Long}}
         :summary "get it is"
         :responses {200 {:schema User
                          :description "Found it!"}
                     404 {:description "Ohnoes."}}}
   :post {:parameters {:body User}}
   ...}
  liberator-resource-here)

B) also do coercion based on the ring-swagger description

Like the a, but input-coercion would be done before hitting the endpoint and response-coercion when coming back.

C) make Liberator resource extend the compojure.api.routing/Routing protocol

Would need changes in the Liberator side, the resource would be responsible for the coercion and provide the swagger-stuff for compojure-api.

what do you think?

@ikitommi
Copy link
Member

Did a small spike of the A. Here goes:

  • lein new compojure-api simple
  • modify the handler.clj to:
(ns simple.handler
  (:require [compojure.api.sweet :refer :all]
            [ring.util.http-response :refer :all]
            [compojure.api.routes :as routes]
            [schema.core :as s]))

(defn endpoint [info handler]
  (let [methods #{:get :head :patch :delete :options :post :put}
        root-info (reduce dissoc info methods)
        child-info (select-keys info methods)
        childs (map (fn [[method info]] (routes/map->Route {:path "/", :method method, :info info})) child-info)]
    (routes/map->Route {:info root-info, :childs childs, :handler handler})))

(s/defschema Pizza
  {:name s/Str
   (s/optional-key :description) s/Str
   :size (s/enum :L :M :S)
   :origin {:country (s/enum :FI :PO)
            :city s/Str}})

(def app
  (api
    {:swagger
     {:ui "/"
      :spec "/swagger.json"
      :data {:info {:title "Simple"
                    :description "Compojure Api example"}
             :tags [{:name "rest", :description "some rest"}]}}}

    (context "/rest" []
      :tags ["rest"]

      (endpoint
        {:description "shared description, can be overridden"
         :get {:parameters {:query {:x Long, :y Long}}
               :description "overridden description"
               :summary "get endpoint"
               :responses {200 {:schema Pizza
                                :description "pizza"}
                           404 {:description "Ohnoes."}}}
         :post {:parameters {:body Pizza}
                :responses {200 {:schema Pizza
                                 :description "pizza"}}}
         :head {:summary "head"}
         :patch {:summary "patch"}
         :delete {:summary "delete"}
         :options {:summary "options"}
         :put {:summary "put"}}
         (fn [request]
            (ok {:liberate "me!"}))))))

.. providing the following Swagger-ui (just the docs, no coercion):

rest

@preoctopus
Copy link

This is far from flawless, but B is doable thanks to :as-resource in Liberator.

(def dumbdb (atom {:a "Andrew"}))

(lib/defresource libep [apidata]
  :media-type-available? (constantly true)
  :method-allowed? (constantly true)
  :as-response (fn [data ctx] (lib.util/combine {:body data} ctx))
  :post! (fn [ctx]
           (when-let [skey (:specifickey apidata)]
             (swap! dumbdb merge {(keyword skey) "something"}))
           nil)
  :exists? (fn [ctx]
             (contains? @dumbdb (:specifickey apidata)))
  :handle-ok (fn [ctx] {:a (get ctx :a "Nope")})
  :handle-created (fn [ctx] {:created? true}))

(api/defroutes routes
  (api/api
    swagger
    (api/context "/v1" []
                        ;; 8<---- snip 
                        (api/context "/libtest" []
                                     (api/GET "/two" []
                                              :query-params [q :- s/Str]
                                              :return {:a s/Str}
                                              :responses {404 {:schema s/Str :description "Value not found"}}
                                              libep)
                                     (api/POST "/three" []
                                               :responses {201 {:schema {:created? s/Bool}}}
                                               :body [b {:specifickey s/Str
                                                         :boop s/Int}]
                                               (libep b))))

Lots of this is crap from just playing with it but:

:as-response (fn [data ctx] (lib.util/combine {:body data} ctx))

being the line that disables Liberators serialization and returns the raw object for compojure-api to handle.

@ikitommi
Copy link
Member

Cool. I could wire the coercion into the my example, based on the ring-swagger data model. Just to have Møre options on which would be the best way for this. Both (macro-driven & data-driven) have pros and cons.

@ikitommi
Copy link
Member

ok. rewriting the endpoint into:

(defn endpoint [info handler]
  (let [methods #{:get :head :patch :delete :options :post :put}
        parameter-mapping {:query [:query-params :string true]
                           :body [:body-params :body false]
                           :formData [:form-params :string true]
                           :header [:header-params :string true]
                           :path [:path-params :string true]}
        root-info (reduce dissoc info methods)
        child-info (select-keys info methods)
        childs (map (fn [[method info]] (routes/create "/" method info nil nil)) child-info)
        coerce-request (fn [request kws]
                         (reduce-kv
                           (fn [request k [v type open?]]
                             (if-let [schema (get-in info (concat kws [:parameters k]))]
                               (let [schema (if open? (assoc schema s/Keyword s/Any) schema)]
                                 (assoc request v (meta/coerce! schema v type request)))
                               request))
                           request
                           parameter-mapping))
        coerced-handler (fn [request]
                          (handler (-> request
                                       (coerce-request [])
                                       (coerce-request [(:request-method request)]))))]
    (routes/create nil nil root-info childs coerced-handler)))

will also do request coercion just like compojure-api, I'll add response coercion later in.

So, would this be a good approach (data-driven coercion & swagger in front of Liberator)? Should we use ring-terms (:query-params) or swagger-terms (query) here?

I can anyway push this (when cleaned up & tested propertly) to compojure-api, but should the integration be tighter (option C), so that Liberator would define the rules and c-api would just read the swagger-docs?

ikitommi added a commit that referenced this issue Feb 28, 2016
codes for #185.
Renamed `operation` into `resource` and embedded `:handler`
to the info map. Subject to change.
@ikitommi
Copy link
Member

There is now a liberator-support branch now for this. Pushed the endpoint codes into compojure.api.resource. Does now both request- & response coercion but not any destucturing as it's only data.

It now takes only the info map as parameter, the ring-handler function should be placed under key :handler. I think single map is better, as the whole map can be defined in the Liberator side as it's only data and the end result can be mapped to compojure-api side with the wrapper-function.

;; Liberator-side
(def swaggered-liberator-resource
   {:get {:parameters {:query {:x Long, :y Long}}
          :description "overridden description"
          :summary "get endpoint"
          :responses {200 {:schema {:kikka s/Str, :total s/Int}
                           :description "kikka with total"}}}
    :handler (liberator/resource
               :handle-ok (fn [ctx]
                             (liberator/ring-response
                               {:kikka "kukka"
                                :total (+ (get-in ctx [:request :query-params :x])
                                          (get-in ctx [:request :query-params :y]))})))})

;; Compojure-api side
(context "/pizza" []
  (resource swaggered-liberator-resource))

No idea about naming of the new function, it could be handler, endpoint, endpoints or resource. In Swagger, it is called PathItemObject, but that's lame.

@ikitommi
Copy link
Member

The new doc string of resource below, which is also in compojure.api.sweet now (with tests). Both the compojure-api & Liberator call this resource, but I think it's ok and actually might be a good idea to reuse names as they mean same things.

Pushed 1.0.2-SNAPSHOT with this to play with.

Feedback welcome!

  "Creates a nested compojure-api Route from an enchanced ring-swagger operations map.
  Applies both request- and response-coercion based on those definitions.

  Enchancements:

  1) :parameters use ring request keys (query-params, path-params, ...) instead of
  swagger-params (query, path, ...). This keeps things simple as ring keys are used in
  the handler when destructuring the request.

  2) special key `:handler` either under operations or at top-level. Value should be a
  ring-handler function, responsible for the actual request processing. Handler lookup
  order is the followin: operations-level, top-level, exception.

  3) at top-level, one can add any ring-swagger operation definitions, which will be
  shared for all operations. Top-level request-coercion will be applied before operation
  level coercion and top-level response-coercion will be applied after operation level
  coercion. All other definitions will be accumulated into operation info using normal
  compojure-api rules.

  Example:

  (resource
    {:parameters {:query-params {:x Long}}
     :responses {500 {:schema {:reason s/Str}}}
     :get {:parameters {:query-params {:y Long}}
           :responses {200 {:schema {:total Long}}}
           :handler (fn [request]
                      (ok {:total (+ (-> request :query-params :x)
                                     (-> request :query-params :y))}))}
     :post {}
     :handler (constantly
                (internal-server-error {:reason \"not implemented\"}))})"

@heimojuh
Copy link

heimojuh commented Mar 2, 2016

a silly question if I may, I just can't wrap my head around this currently. All works kinda nicely, but I have a bit of a problem in writing liberator resource that would fit the needs of compojure api, see the gist:

https://gist.github.com/heimojuh/93e9faf60dc03da5521c

Might be that I get something wrong, but that non-working (which results to an error that looks like something get's serialized a few times) result comes when the liberator resource is written like the one in your prior example. Maybe.

When liberator resource is written like

(defresource getfoo
  :handle-ok (fn [req]
               (ring-response  {:body {:kukkuu "yay"}}))

  :available-media-types ["application/json"])

all is fine. That said, I'm really not an expert in liberator, usually I believe we have ring-middlewares that allow us to just return plain what-ever from liberator without wrapping it to any particular response, you probably know what I mean better than me ::)

So did you get my question? Which would be: what could cause this error in explained situation? :)

;;{
;;  "errors": "(not (map? \"{\\\"kukkuu\\\":\\\"yay\\\"}\"))"
;;}

@heimojuh
Copy link

heimojuh commented Mar 2, 2016

In addition (not sure if of any use) this is actually the same situation what's in the example above:

(defresource getfoo
  :handle-ok (fn [req]
               (ring-response  {:kukkuu "foo"}))

And this results to this error on swagger ui:

{
  "errors": "(not (map? nil))"
}

slightly peculiar.

(though if I read liberator specs right, this should indeed result in :body on response object to be nil).

@ikitommi
Copy link
Member

ikitommi commented Mar 2, 2016

we should split this into two:

  1. how compojure-api can support Liberator, to enable schema coercion and produce swagger docs.

  2. how Liberator should be used when used with compojure-api.

For the first: I'm quite happy with the resource-abstraction, but would like to get real-life feedback before freezing this. For example coercion: currently enforced at both top-level & operation-level. Is this good? Optiona are:

a) always enforce both, separately (current)
b) top-level schemas can be fully overridden by operation-level schemas
c) merge schemas and enforce only once
d) allow user to define how to do this per case, with a good default-handling.

  1. this would benefit from examples. I'm not fluent at Liberator, so help needed. As @preoctopus noted, the :as-response might be the best way to passthrough responses. Not sure how it retains the original status codes. ring-response requires full response, had a typo in my example.

There are other things overlapping (listing allowed-methods, produces, . .), not a blocker, but in the long run, would be nice to have more boilerplate-free integration. One option for this would be to create a new namespace with an new resource function (compojure.api.liberator/liberator? but without actual dependency to Liberator) where one could define overlapping things once and it would return a fully configured c.a.r/resource backed with a Liberator resource. Both use maps, so should be doable. If this is good idea, feel free to thow in gists etc.

Btw, there is a Liberator slack channel, I think people there could help with this. Ping @preoctopus & @heimojuh.

@ikitommi
Copy link
Member

ikitommi commented Mar 7, 2016

Did small adjustments:

  • request-coercion is done only once, with deep-merged parameters
  • response-coercion is done only once, with merged responses
  "Creates a nested compojure-api Route from an enchanced ring-swagger operations map.
  Applies both request- and response-coercion based on those definitions.

  Enchancements:

  1) :parameters use ring request keys (query-params, path-params, ...) instead of
  swagger-params (query, path, ...). This keeps things simple as ring keys are used in
  the handler when destructuring the request.

  2) at resource root, one can add any ring-swagger operation definitions, which will be
  available for all operations, using the following rules:

    2.1) :parameters are deep-merged into operation :parameters
    2.2) :responses are merged into operation :responses (operation can fully override them)
    2.3) all others (:produces, :consumes, :summary,...) are deep-merged by compojure-api

  3) special key `:handler` either under operations or at top-level. Value should be a
  ring-handler function, responsible for the actual request processing. Handler lookup
  order is the following: operations-level, top-level, exception.

  4) request-coercion is applied once, using deep-merged parameters for a given
  operation or resource-level if only resource-level handler is defined.

  5) response-coercion is applied once, using merged responses for a given
  operation or resource-level if only resource-level handler is defined.

  Example:

  (resource
    {:parameters {:query-params {:x Long}}
     :responses {500 {:schema {:reason s/Str}}}
     :get {:parameters {:query-params {:y Long}}
           :responses {200 {:schema {:total Long}}}
           :handler (fn [request]
                      (ok {:total (+ (-> request :query-params :x)
                                     (-> request :query-params :y))}))}
     :post {}
     :handler (constantly
                (internal-server-error {:reason \"not implemented\"}))})"

@ikitommi
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants