Clojurians Log v2
code-reviews
I needed some data from our CI system, and so I wrote some functions so I could paste in lines from the API docs (Gitlab if you're curious) and get it done. People work with APIs every day so I thought I'd show the way I approached it and get some feedback on
• idiomaticity
• how to go about writing an asynchronous version of the function I called read
Code in the thread...
small semantic quibble, "edn" is a text format, you are parsing into native clojure data types
also, ex-info is much more useful than Exception, instead of stringifying the data into the message you can include the data as payload on the exception itself
(ins)user=> (throw (ex-info "oops" {:a 42}))
Execution error (ExceptionInfo) at user/eval183 (REPL:1).
oops
(ins)user=> *e
#error {
:cause "oops"
:data {:a 42}
:via
[{:type clojure.lang.ExceptionInfo
:message "oops"
:data {:a 42}
:at [user$eval183 invokeStatic "NO_SOURCE_FILE" 1]}]
:trace
[[user$eval183 invokeStatic "NO_SOURCE_FILE" 1]
[user$eval183 invoke "NO_SOURCE_FILE" 1]
[clojure.lang.Compiler eval "Compiler.java" 7177]
...(ins)user=> (ex-data *e)
{:a 42}also, I'd suggest checking the result of (get known-actions action) - you can throw a much nicer error there ("unknown action...") as opposed to the regex op blowing up on nil down lower
I also find the decision to encode multiple things as one string and then split/parse quite odd - why not just have hash-maps inside known-actions ?
my personal preference is not to keywordize arbitrary data from the network - maps with string keys are OK. But I think I'm in a minority here
> why not just have hash-maps inside known-actions ? I think you're saying, it's weird to have "GET /blah/:thing", right? The only real reason was, I wanted to get as close as I could to, "copy from API docs, paste, invent a name, done" and trying to avoid editing the data
it definitely might be weird
oh - so it's effectively auto-generated, but at runtime
maybe in that case, isolate the code that turns the string into usable data as a named function, and document something like "this takes the string as exposed in the api doc and turns it into the data clojure can use"
instead of making string-parsing an implementation detail of the request function
MMM ok i see what you're saying
from a development perspective i also thought "maybe this METHOD /path/:parameter/more_path is so standard that there's existing work that converts that and I shouldn't think too hard until refactor time"
yeah i'll extract that
that's not an idiom I've ever seen in clojure
perhaps it's just a sign of whatever language Gitlab's API is written it, then
also, consider this compromise: [:get "/path/:parameter/more_path"]
but I think a named "api-doc -> request data" function is a clean option
:thumbsup::skin-tone-2:
(throw (ex-info "oops" {:a 42}))
Execution error (ExceptionInfo) at user/eval183 (REPL:1).
oops
(ins)user=> *e
#error {...(def known-actions {:get-users "GET /users"
:get-one-pipeline "GET /projects/:id/pipelines/:pipeline_id"
:list-pipeline's-jobs "GET /projects/:id/pipelines/:pipeline_id/jobs"
:get-pipelines "GET /projects/:id/pipelines"
:get-projects "GET /projects"
:get-user's-jobs "GET /projects/:id/jobs/"
:get-user's-pipeline-schedules "GET /projects/:id/pipeline_schedules"
:edit-user's-pipeline-schedule "PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id"
:play-user's-pipeline-schedule "POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/play"
:get-user's-branches "GET /projects/:id/repository/branches"})
(defn- resolve-path-params
"Gitlab documents its endpoints with a method and a URL containing path `:parameters`. This function helps turn the
endpoint as documented into a concrete request path"
[endpoint data]
(let [path-param-regex #":(\w+)"]
(if-let [matches (re-seq path-param-regex endpoint)]
(loop [params (->> matches (map second) (map keyword))
path endpoint]
(if (seq params)
(let [param (first params)
path-param (str param)
value (get data param)]
(if-not value
(throw (Exception. (str "Endpoint requires data we have not supplied:" path-param)))
(recur (rest params) (string/replace path (re-pattern path-param) (str value)))))
path))
endpoint)))
(defn- prepare-request
"Given an action (documented in `known-actions`, and optionally some path parameters and request options), creates a
hash that http-kit can read with its `request` function."
([action]
(prepare-request action {:id platform-id} {}))
([action params opts]
(let [rest-call (get known-actions action)
[method-name endpoint] (string/split rest-call #"\s")
method (-> method-name .toLowerCase keyword)]
(merge opts {:method method
:url (str base-url (resolve-path-params endpoint params))
:headers (merge authentication content-type )}))))
(defn- read
"Given a request (the result of `prepared-request`), return the body of the response parsed into edn. No asynchrony
here, it is a blocking function."
[request]
(-> @(client/request request)
:body
(json/parse-string true)))I should define authentication and content-type:
(def authentication {"PRIVATE-TOKEN" token})
(def content-type {"Content-Type" "application/json" "Accept" "application/json"})my-id and platform-id and token come from a config.edn
I feel like I probably could have done a lot less work but I haven't wrapped many APIs