code-reviews

mathpunk 2020-11-19T01:04:27.062100Z

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

2020-11-19T18:53:28.063200Z

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

2020-11-19T18:54:57.063400Z

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

2020-11-19T18:55:46.063600Z

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

2020-11-19T18:58:01.063900Z

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

2020-11-19T18:59:28.064200Z

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 ?

2020-11-19T19:01:30.064400Z

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

mathpunk 2020-11-19T19:49:19.064600Z

> 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

mathpunk 2020-11-19T19:49:36.064800Z

it definitely might be weird

2020-11-19T19:49:54.065Z

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

2020-11-19T19:50:49.065200Z

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"

2020-11-19T19:51:20.065400Z

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

mathpunk 2020-11-19T19:51:26.065600Z

MMM ok i see what you're saying

mathpunk 2020-11-19T19:52:18.065800Z

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"

mathpunk 2020-11-19T19:52:31.066Z

yeah i'll extract that

2020-11-19T19:52:33.066200Z

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

mathpunk 2020-11-19T19:53:05.066400Z

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

2020-11-19T19:53:18.066600Z

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

2020-11-19T19:54:12.066800Z

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

mathpunk 2020-11-19T19:54:35.067Z

:thumbsup::skin-tone-2:

mathpunk 2020-11-19T22:05:26.067200Z

(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

mathpunk 2020-11-19T01:04:43.062200Z

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

mathpunk 2020-11-19T01:06:13.062400Z

I should define authentication and content-type:

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

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

mathpunk 2020-11-19T01:06:20.062600Z

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

mathpunk 2020-11-19T01:08:07.063Z

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

2020-11-19T18:53:28.063200Z

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

2020-11-19T18:54:57.063400Z

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

2020-11-19T18:55:46.063600Z

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

2020-11-19T18:58:01.063900Z

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

2020-11-19T18:59:28.064200Z

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 ?

2020-11-19T19:01:30.064400Z

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

mathpunk 2020-11-19T19:49:19.064600Z

> 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

mathpunk 2020-11-19T19:49:36.064800Z

it definitely might be weird

2020-11-19T19:49:54.065Z

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

2020-11-19T19:50:49.065200Z

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"

2020-11-19T19:51:20.065400Z

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

mathpunk 2020-11-19T19:51:26.065600Z

MMM ok i see what you're saying

mathpunk 2020-11-19T19:52:18.065800Z

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"

mathpunk 2020-11-19T19:52:31.066Z

yeah i'll extract that

2020-11-19T19:52:33.066200Z

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

mathpunk 2020-11-19T19:53:05.066400Z

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

2020-11-19T19:53:18.066600Z

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

2020-11-19T19:54:12.066800Z

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

mathpunk 2020-11-19T19:54:35.067Z

:thumbsup::skin-tone-2:

mathpunk 2020-11-19T22:05:26.067200Z

(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