Sjelden har jeg sett ny kunnskap betale seg så direkte, så raskt 😄

Ja, det var snakk om timer 😄

Vi fikk introdusert en trace-service med with-span! som logger til sentry for ca et år siden. Aldri har performance tuning vært lettere.

herlig! Spans ligner veldig på flame graphs for rendring i nettleseren osv

Ikke ha spit nederst i refresh-token-cache 😄

(defn refresh-token-cache [scope]
  (let [token "LOL"]
    (spit "/tmp/lol.cache" token)
    token))

Ja, jeg sitter og lurer på hvordan jeg kan få refresh-token-cache til å returnere path til filen den nettopp har skrevet.

Forøvrig, å kalle denne funksjonen get-cached-token er å dra "get" litt langt 😅 Det er ikke det den gjør

Jeg ville vurder acquire-token! eller noe sånt

Definitivt room for improvement på navnene også, ja! Hehe.

En annen smell: Det ser ut som om du har en funksjon som leser fra denne fila, og en som skriver til den - og begge setter manuelt sammen filnavnet.

Dette er koden som skriver fila:

(defn spit-file [filename data]
  (let [path (get-absolute-path filename)]
    (create-dirs (file/parent path))
    (spit path (pr-str data))))

(defn write-to-files [file-ext data]
  (doseq [[name [token]] data]
    (spit-file (str name file-ext) token)))

(defn refresh-token-cache [scopes]
  (doall
   (->> scopes
        (map add-token)
        (group-by :name)
        (write-to-files ".edn"))))

(defn get-token [name]
  (when-let [token (get-cached-token name)] token))

(get-cached-token name)

Hvorfor brukes token-cache-dir kun ved lesing og ikke når det skrives?

spit-file kaller get-absolute-path som bruker token-cache-dir:

(defn get-absolute-path
  ([filename]
   (get-absolute-path (str (file/cwd) token-cache-dir)
                      filename))

  ([path filename]
   (str path "/" filename)))

Jeg burde kanskje bruke get-absolute-path når fila leses også da.

Er token-cache-dir en global var?

Ja, det er en def helt øverst i fila: (def token-cache-dir "/tmp/tokens")

Skjønner

(Dette er forresten et Babashka skript.)

Du har en del informasjon som egentlig hører sammen spredt litt for alle vinder

Jeg tror jeg ville laget denne:

(defn get-cache-file [n]
  (str (file/cwd) "/tmp/tokens/" n ".edn"))

Ville du gjentatt "/tmp/tokens/" på to steder (der filene skrives og leses) fremfor å gjøre en global def på ett sted?

Nei, jeg ville laget funksjonen jeg nettopp ga deg 😄

to sek så kan jeg sette det sammen i et forslag

Åja, tror jeg skjønner hva du mener!

(defn get-cache-file [n]
  (str (file/cwd) "/tmp/tokens/" n ".edn"))

(defn spit-file [filename data]
  (let [path (get-absolute-path filename)]
    (create-dirs (file/parent path))
    (spit path (pr-str data))))

(defn write-to-files [data]
  (doseq [[name [token]] data]
    (spit-file (get-cache-file name) token)))

(defn refresh-token-cache [scopes]
  (->> scopes
       (map add-token)
       (group-by :name)
       write-to-files))

(defn acquire-token [name]
  (let [file-name (get-cache-file name)]
    (when-not (.exists (io/file file-name))
      (refresh-token-cache (get-scope name)))
    (slurp-file file-name)))

Noe sånt

Med mindre du har tenkt til å gjøre noe mer i din get-token trenger du den ikke

Jeg har (hadde 😅) planer om å sjekke hvorvidt cached token fortsatt er gyldig litt senere.

Typ en is-token-valid? predicate eller noe sånt.

Men hvorfor spre det over to funksjoner?

Du har allerede en funksjon som henter fra cache eller henter nytt om den ikke finnes

Den kan bare sjekke gyldighet i tillegg

Forøvrig trenger du ikke både doall og doseq, en av dem holder

doseq er mest eksplisitt

Også ville jeg som vi diskuterte tidligere droppa den add-funksjonen og heller gjort:

(defn refresh-token-cache [scopes]
  (->> scopes
       (map #(assoc % :token (get-token %)))
       (group-by :name)
       write-to-files))

Aha, jeg misforstod at det burde være add- istedenfor get-

Hvis get-token faktisk henter over nettverk ville jeg kanskje heller kalt den fetch-token e.l.

Det er disse som faktisk henter token:

(defn sh-out->json [opts & args]
  (-> (apply process/sh opts args)
      :out
      (json/parse-string true)))

(defn obtain-token [scope-url]
  (sh-out->json "az" "account" "get-access-token"
                "--scope" scope-url))

Jeg tror jeg ville latt obtain-token få det den trenger for å bygge URL-en selv, og så kalle den fra map-en over: (map #(assoc % :token (obtain-token %)))

Ja, nice! Det gir mening. Jeg har vel kanskje forsøkt å dele opp for mye i egne funksjoner og ikke brukt core greier nok (smartere bruk av assoc i dette tilfellet).

Tusen takk for alle tipsene! Da kan jeg rydde opp ganske bra og gjøre dette mer konsist. Dette må være verdens mest interaktive og dynamiske remote code review ever 😛

😄

Resultatet etter litt opprydning 🙂

(ns myscript (:require [babashka.fs :as file]
                       [babashka.process :as process]
                       [cheshire.core :as json]
                       [clojure.edn :as edn]))

(def scopes
  [{:name "graph"
    :url ""}
   {:name "myservice"
    :url ""}])

(defn sh-out->json [opts & args]
  (-> (apply process/sh opts args)
      :out
      (json/parse-string true)))

(defn fetch-token [url]
  (sh-out->json "az" "account" "get-access-token"
                "--scope" url))

(defn write-file [path data]
  (let [dirs (file/parent path)]
    (when-not (file/exists? dirs)
      (file/create-dirs dirs)))
  (spit (str path) (pr-str data)))

(defn get-filepath [filename]
  (-> (file/cwd)
      (str "/tmp/tokens/" filename ".edn")))

(defn write-to-files [data]
  (doseq [[name [token]] data]
    (-> (get-filepath name)
        (write-file token))))

(defn refresh-token [scopes]
  (->> scopes
       (map #(assoc % :token (fetch-token (:url %))))
       (group-by :name)
       (write-to-files)))

(defn read-file [path]
  (when (file/exists? path)
    (-> (slurp path)
        edn/read-string)))

(defn get-token [name]
  (let [file-name (get-filepath name)]
    (when-not (file/exists? file-name)
      (-> (filter #(= name (:name %)) scopes)
          refresh-token))
    (read-file file-name)))

(comment
  (get-token "graph")

  (->> (map :name scopes)
       (map get-token)))

Og der fant jeg ut hvordan jeg kan sjekke om en token har gått ut på dato også! Dette funker, men det ser ikke spesielt pent ut, og det er nok mye rom for forbedring her også:

(defn has-token-expired? [token]
  (let [format (DateTimeFormatter/ofPattern "yyyy-MM-dd HH:mm:ss.SSSSSS")
        timestamp (LocalDate/parse (:expiresOn token) format)]
    (.isAfter (LocalDate/now) timestamp)))

(defn load-token [name]
  (let [path (get-filepath name)]
    (when-not (file/exists? path)
      (-> (filter #(= name (:name %)) scopes)
          refresh-token))
    (let [token (read-file path)]
      (when (has-token-expired? (:token token))
        (-> (filter #(= name (:name %)) scopes)
            refresh-token))
      token)))

Jeg likker ikke at dette er repetert to ganger:

(-> (filter #(= name (:name %)) scopes)
          refresh-token)

Trur du kan unngå det ved å skrive litt om på koden, for eksempel

(defn get-token [name]
  (let [file-name (get-filepath name)]
    (when-not (file/exists? file-name)
      (-> (filter #(= name (:name %)) scopes)
          refresh-token))))

(defn valid-token? [token]
  (let [format (DateTimeFormatter/ofPattern "yyyy-MM-dd HH:mm:ss.SSSSSS")
        timestamp (LocalDate/parse (:expiresOn token) format)]
    (.isBefore (LocalDate/now) timestamp)))

(defn load-token [name]
  (let [token (read-file (get-filepath name))]
    (if (valid-token? token)
      token
      (-> (filter #(= name (:name %)) scopes)
          refresh-token))))

Takk for tipsene, @U024X3V2YN4! Du satte jeg på rett spor. Jeg endte opp med noe litt annerledes:

(defn token-exists? [name]
  (let [path (get-filepath name)]
    (if (file/exists? path)
      (edn/read-string (slurp path))
      false)))

(defn token-valid? [token]
  (let [format (DateTimeFormatter/ofPattern "yyyy-MM-dd HH:mm:ss.SSSSSS")
        timestamp (LocalDate/parse (:expiresOn token) format)]
    (.isBefore (LocalDate/now) timestamp)))

(defn filter-scope [name]
  (filter #(= name (:name %)) scopes))

(defn refresh-token [scopes]
  (->> scopes
       (map #(assoc % :token (fetch-token (:url %))))
       (group-by :name)
       (write-to-files)))

(defn load-token [name]
  (let [token (token-exists? name)]
    (when-not (and (token-exists? name)
                   (token-valid? (:token token)))
      (refresh-token (filter-scope name)))
    (token-exists? name)))