Fork me on GitHub
#code-reviews
<
2020-11-19
>
mathpunk01:11:27

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...

mathpunk01:11:43

(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)))

mathpunk01:11:13

I should define authentication and content-type:

(def authentication {"PRIVATE-TOKEN" token})

(def content-type {"Content-Type" "application/json" "Accept" "application/json"})

mathpunk01:11:20

my-id and platform-id and token come from a config.edn

mathpunk01:11:07

I feel like I probably could have done a lot less work but I haven't wrapped many APIs

noisesmith18:11:28

small semantic quibble, "edn" is a text format, you are parsing into native clojure data types

noisesmith18:11:57

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

noisesmith18:11:46

(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}

noisesmith18:11:01

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

noisesmith18:11:28

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 ?

noisesmith19:11:30

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

mathpunk19:11:19

> 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

mathpunk19:11:36

it definitely might be weird

noisesmith19:11:54

oh - so it's effectively auto-generated, but at runtime

noisesmith19:11:49

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"

noisesmith19:11:20

instead of making string-parsing an implementation detail of the request function

mathpunk19:11:26

MMM ok i see what you're saying

mathpunk19:11:18

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"

mathpunk19:11:31

yeah i'll extract that

noisesmith19:11:33

that's not an idiom I've ever seen in clojure

mathpunk19:11:05

perhaps it's just a sign of whatever language Gitlab's API is written it, then

noisesmith19:11:18

also, consider this compromise: [:get "/path/:parameter/more_path"]

noisesmith19:11:12

but I think a named "api-doc -> request data" function is a clean option

mathpunk19:11:35

:thumbsup::skin-tone-2:

mathpunk22:11:26

(throw (ex-info "oops" {:a 42}))
Execution error (ExceptionInfo) at user/eval183 (REPL:1).
oops
(ins)user=> *e
#error {...
i'm just taking this part in now -- heck yeah, this is useful and i have several places where i'll use this pattern