From 2cdae0d68324b47e1e087c363a2d0481fbe3a498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arne=20Schl=C3=BCter?= Date: Wed, 22 Aug 2018 17:58:03 +0200 Subject: [PATCH] Cache API responses and make sure we remember more than just one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #21. Squashed commit of the following: commit 964b29cf127cf51de86543d040bcb6c674b36d7e Author: Arne Schlüter Date: Wed Aug 22 17:56:48 2018 +0200 Pass content for current route nicely to views commit b469a0a4b69457ddf3a679ac1acc82fbaffdc8fd Author: Arne Schlüter Date: Wed Aug 22 16:01:04 2018 +0200 Add response cache in app-db commit da9faf89138f42ee544efc64c2e46787091b3dc7 Author: Arne Schlüter Date: Wed Aug 22 13:40:57 2018 +0200 Move api helpers and tests to own namespace --- src/cljs/airsonic_ui/api/events.cljs | 51 +++++++++++++++++++ .../{utils/api.cljs => api/helpers.cljs} | 5 +- src/cljs/airsonic_ui/api/subs.cljs | 26 ++++++++++ src/cljs/airsonic_ui/core.cljs | 8 ++- src/cljs/airsonic_ui/events.cljs | 42 ++------------- src/cljs/airsonic_ui/routes.cljs | 42 +++++++++++---- src/cljs/airsonic_ui/subs.cljs | 31 +++-------- src/cljs/airsonic_ui/views.cljs | 7 +-- test/cljs/airsonic_ui/api/events_test.cljs | 50 ++++++++++++++++++ .../api_test.cljs => api/helpers_test.cljs} | 4 +- test/cljs/airsonic_ui/api/subs_test.cljs | 29 +++++++++++ test/cljs/airsonic_ui/events_test.cljs | 9 +--- test/cljs/airsonic_ui/subs_test.cljs | 12 ++--- 13 files changed, 222 insertions(+), 94 deletions(-) create mode 100644 src/cljs/airsonic_ui/api/events.cljs rename src/cljs/airsonic_ui/{utils/api.cljs => api/helpers.cljs} (93%) create mode 100644 src/cljs/airsonic_ui/api/subs.cljs create mode 100644 test/cljs/airsonic_ui/api/events_test.cljs rename test/cljs/airsonic_ui/{utils/api_test.cljs => api/helpers_test.cljs} (96%) create mode 100644 test/cljs/airsonic_ui/api/subs_test.cljs diff --git a/src/cljs/airsonic_ui/api/events.cljs b/src/cljs/airsonic_ui/api/events.cljs new file mode 100644 index 0000000..3652c24 --- /dev/null +++ b/src/cljs/airsonic_ui/api/events.cljs @@ -0,0 +1,51 @@ +(ns airsonic-ui.api.events + "This namespace contains all events relevant to API interaction. It contains + an event handler which issues requests as well as the appropriate handlers, + which dispatch :notification events in case of errors." + (:require [re-frame.core :refer [reg-event-fx]] + [ajax.core :as ajax] + [airsonic-ui.api.helpers :as api])) + +(defn- api-url + "Small helper function which makes constructing API URLs a bit easier" + [db endpoint params] + (let [creds (:credentials db)] + (api/url (:server creds) endpoint (merge params (select-keys creds [:u :p]))))) + +(defn- cache-path [endpoint params] [:api/responses [endpoint params]]) + +(defn api-request + "Event handler to issue API request; takes care of authorization based on our + current app state." + [{:keys [db]} [_ endpoint params]] + {:http-xhrio {:method :get + :uri (api-url db endpoint params) + :response-format (ajax/json-response-format {:keywords? true}) + :on-success [:api/good-response endpoint params] + :on-failure [:api/failed-response endpoint params]} + :db (assoc-in db (conj (cache-path endpoint params) :api/is-loading?) true)}) + +(reg-event-fx :api/request api-request) + +(defn good-api-response + "Handles when the server responded. There could still be an error while + processing the request on the server side which we have to account for." + [fx [_ endpoint params response]] + (let [response-cache (cons :db (cache-path endpoint params))] + (try + (assoc-in fx response-cache (api/unwrap-response response)) + (catch ExceptionInfo e + {:dispatch [:notification/show :error (api/error-msg e)] + :db (update-in fx response-cache dissoc :api/is-loading?)})))) + +(reg-event-fx :api/good-response good-api-response) + +(defn failed-api-response + "Handler for catastrophic failures (network errors and such things)" + [fx [ev endpoint params]] + (let [response-cache (cons :db (cache-path endpoint params))] + {:log ["API call gone bad; are CORS headers missing? check for :status 0" ev] ; <- the :log effect is registered in ../events.cljs + :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."] + :db (update-in fx response-cache dissoc :api/is-loading?)})) + +(reg-event-fx :api/failed-response failed-api-response) diff --git a/src/cljs/airsonic_ui/utils/api.cljs b/src/cljs/airsonic_ui/api/helpers.cljs similarity index 93% rename from src/cljs/airsonic_ui/utils/api.cljs rename to src/cljs/airsonic_ui/api/helpers.cljs index ee8ce25..8bf2bcc 100644 --- a/src/cljs/airsonic_ui/utils/api.cljs +++ b/src/cljs/airsonic_ui/api/helpers.cljs @@ -1,6 +1,5 @@ -(ns airsonic-ui.utils.api - (:require [clojure.string :as str] - [airsonic-ui.config :as config])) +(ns airsonic-ui.api.helpers + (:require [clojure.string :as str])) (def default-params {:f "json" :c "airsonic-ui-cljs" diff --git a/src/cljs/airsonic_ui/api/subs.cljs b/src/cljs/airsonic_ui/api/subs.cljs new file mode 100644 index 0000000..01c0d5a --- /dev/null +++ b/src/cljs/airsonic_ui/api/subs.cljs @@ -0,0 +1,26 @@ +(ns airsonic-ui.api.subs + (:require [clojure.string :as str] + [re-frame.core :refer [reg-sub]])) + +(defn endpoint->kw + "Given an endpoint like `getAlbumList2`, returns a cleaned keyword like + `:album-list``. + + Rules: Kebab-case everything, remove prefixes like `get`, `create`, `delete`, + `update` and strip trailing numbers." + [endpoint-str] + (-> (str/replace endpoint-str #"^(get|create|update|delete)" "") + (str/replace #"\d+$" "") + (str/replace #"([a-z])([A-Z])" (fn [[_ a b]] (str a "-" b))) + (str/lower-case) + (keyword))) + +(defn route-data + "Given a list of event vectors, returns that responses for all API requests." + [db [_ route-events]] + (->> (filter #(= :api/request (first %)) route-events) + (mapcat (fn [[_ endpoint params]] + [(endpoint->kw endpoint) (get-in db [:api/responses [endpoint params]])])) + (apply hash-map))) + +(reg-sub :api/route-data route-data) diff --git a/src/cljs/airsonic_ui/core.cljs b/src/cljs/airsonic_ui/core.cljs index 4bb702d..37b9c18 100644 --- a/src/cljs/airsonic_ui/core.cljs +++ b/src/cljs/airsonic_ui/core.cljs @@ -4,8 +4,12 @@ ;; 3rd party effects / coeffects [day8.re-frame.http-fx] [akiroz.re-frame.storage :as storage] - ;; our app - [airsonic-ui.audio.core] ; <- just registers effects here + + ;; our app; namespaces that are just required but not used register + ;; event handlers, effect handlers or subscriptions + [airsonic-ui.audio.core] + [airsonic-ui.api.events] + [airsonic-ui.api.subs] [airsonic-ui.events :as events] [airsonic-ui.views :as views] [airsonic-ui.config :as config])) diff --git a/src/cljs/airsonic_ui/events.cljs b/src/cljs/airsonic_ui/events.cljs index 67599a6..2de320a 100644 --- a/src/cljs/airsonic_ui/events.cljs +++ b/src/cljs/airsonic_ui/events.cljs @@ -3,7 +3,7 @@ [ajax.core :as ajax] [airsonic-ui.routes :as routes] [airsonic-ui.db :as db] - [airsonic-ui.utils.api :as api] + [airsonic-ui.api.helpers :as api] [airsonic-ui.audio.playlist :as playlist])) (re-frame/reg-fx @@ -69,7 +69,7 @@ :uri (api/url (:server credentials) "ping" (select-keys credentials [:u :p])) :response-format (ajax/json-response-format {:keywords? true}) :on-success [:credentials/authentication-response credentials] - :on-failure [:api/bad-response]})) + :on-failure [:api/failed-response]})) ; <- we don't need endpoint and params here because the response is not cached (re-frame/reg-event-fx :credentials/send-authentication-request authentication-request) @@ -135,37 +135,6 @@ (re-frame/reg-event-fx ::logout logout) -;; --- -;; api interaction -;; --- - -(defn- api-url [db endpoint params] - (let [creds (:credentials db)] - (api/url (:server creds) endpoint (merge params (select-keys creds [:u :p]))))) - -(defn api-request [{:keys [db]} [_ endpoint params]] - {:http-xhrio {:method :get - :uri (api-url db endpoint params) - :response-format (ajax/json-response-format {:keywords? true}) - :on-success [:api/good-response] - :on-failure [:api/bad-response]}}) - -(re-frame/reg-event-fx :api/request api-request) - -(defn good-api-response [fx [_ response]] - (try - (assoc-in fx [:db :response] (api/unwrap-response response)) - (catch ExceptionInfo e - {:dispatch [:notification/show :error (api/error-msg e)]}))) - -(re-frame/reg-event-fx :api/good-response good-api-response) - -(defn bad-api-response [db event] - {:log ["API call gone bad; are CORS headers missing? check for :status 0" event] - :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."]}) - -(re-frame/reg-event-fx :api/bad-response bad-api-response) - ;; --- ;; musique ;; --- @@ -244,11 +213,8 @@ (re-frame/reg-event-fx :routes/did-navigate (fn [{:keys [db]} [_ route params query]] - ;; FIXME: This leads to an ugly "unregistered event handler `nil`" error - ;; all the naviagation logic is in routes.cljs; all we need to do here - ;; is say what actually happens once we've navigated succesfully - {:db (assoc db :current-route [route params query]) - :dispatch (routes/route-data route params query)})) + {:db (assoc db :routes/current-route [route params query]) + :dispatch-n (routes/route-events route params query)})) (re-frame/reg-event-fx :routes/unauthorized diff --git a/src/cljs/airsonic_ui/routes.cljs b/src/cljs/airsonic_ui/routes.cljs index 17c0506..e0c0801 100644 --- a/src/cljs/airsonic_ui/routes.cljs +++ b/src/cljs/airsonic_ui/routes.cljs @@ -11,37 +11,61 @@ ["/artist/:id" ::artist-view] ["/album/:id" ::album-view]])) -; use this in views to construct a url +;; use this in views to construct a url (defn url-for ([k] (url-for k {})) ([k params] (str "#" (r/resolve router k params)))) -; which routes need valid login credentials? +;; which routes need valid login credentials? (def protected-routes #{::main ::artist-view ::album-view}) -; which data should be requested for which route? can either be a vector or a function returning a vector +;; which data should be requested for which route? can either be a vector or a function returning a vector -(defmulti route-data +(defmulti -route-events "Returns the events that take care of correct data being fetched." (fn [route-id & _] route-id)) -(defmethod route-data :default [route-id params query] []) ; no data +(defmethod -route-events :default [route-id params query] nil) -(defmethod route-data ::main +(defmethod -route-events ::main [route-id params query] [:api/request "getAlbumList2" {:type "recent" :size 18}]) -(defmethod route-data ::artist-view +(defmethod -route-events ::artist-view [route-id params query] [:api/request "getArtist" (select-keys params [:id])]) -(defmethod route-data ::album-view +(defmethod -route-events ::album-view [route-id params query] [:api/request "getAlbum" (select-keys params [:id])]) ;; shouldn't need to change anything below +(defn- n-events? + "Predicate that tells us whether a vector is suitable for :dispatch-n" + [ev-vec] + (or (vector? (first ev-vec)))) + +(defn route-events + "Returns a normalized list of event vectors for a given route." + [route-id params query] + (let [ev-vec (-route-events route-id params query)] + (if (n-events? ev-vec) ev-vec [ev-vec]))) + +;; subscription returning the matched route for the current hashbang + +(re-frame/reg-sub :routes/current-route (fn [db _] (:routes/current-route db))) + +;; NOTE: There is some duplication here. The route events are provided as a +;; subscription but they are also invoked directly in events.cljs. It didn't +;; seem to justify pulling in a whole library and we need it in our top most view + +(re-frame/reg-sub + :routes/events-for-current-route + (fn [db _] (re-frame/subscribe [:routes/current-route])) + (fn [current-route _] (apply route-events current-route))) + ;; these are helper effects we can use to navigate; the first two manage an atom ;; holding credentials, which is necessary to restrict certain routes, and the ;; last one is used for actual navigation @@ -50,7 +74,7 @@ ;; returned unaltered, we just need access to the current app database for ;; authentication, which we get with an interceptor -(def ^:private credentials (atom nil)) +(defonce ^:private credentials (atom nil)) (def do-navigation "An interceptor which performs the navigation after looking up current diff --git a/src/cljs/airsonic_ui/subs.cljs b/src/cljs/airsonic_ui/subs.cljs index 7af379c..6fe9793 100644 --- a/src/cljs/airsonic_ui/subs.cljs +++ b/src/cljs/airsonic_ui/subs.cljs @@ -1,23 +1,22 @@ (ns airsonic-ui.subs - (:require [re-frame.core :as re-frame :refer [subscribe]] - [airsonic-ui.audio.playlist :as playlist] - [airsonic-ui.utils.api :as api])) + (:require [re-frame.core :refer [reg-sub subscribe]] + [airsonic-ui.api.helpers :as api])) (defn is-booting? "The boot process starts with setting up routing and continues if we found previous credentials and ends when we receive a response from the server." [db _] ;; so either we don't have any credentials or they are not verified - (or (empty? (:current-route db)) + (or (empty? (:routes/current-route db)) (and (not (empty? (:credentials db))) (not (get-in db [:credentials :verified?]))))) -(re-frame/reg-sub ::is-booting? is-booting?) +(reg-sub ::is-booting? is-booting?) (defn credentials [db _] (:credentials db)) -(re-frame/reg-sub ::credentials credentials) +(reg-sub ::credentials credentials) -(re-frame/reg-sub +(reg-sub ::user (fn [_ _] [(subscribe [::credentials])]) (fn [[credentials] _] @@ -29,26 +28,12 @@ [[{:keys [server u p]}] [_ song size]] (api/cover-url server {:u u :p p} song size)) -(re-frame/reg-sub +(reg-sub ::cover-url (fn [_ _] [(subscribe [::credentials])]) cover-url) -;; current hashbang - -(re-frame/reg-sub - ::current-route - (fn [db _] - (:current-route db))) - -;; TODO: Make this nice and clean - -(re-frame/reg-sub - ::current-content - (fn [db _] - (:response db))) - ;; user notifications (defn notifications [db _] (:notifications db)) -(re-frame/reg-sub ::notifications notifications) +(reg-sub ::notifications notifications) diff --git a/src/cljs/airsonic_ui/views.cljs b/src/cljs/airsonic_ui/views.cljs index d483633..ef28732 100644 --- a/src/cljs/airsonic_ui/views.cljs +++ b/src/cljs/airsonic_ui/views.cljs @@ -26,7 +26,7 @@ (defn most-recent [content] [:div [:h2.title "Recently played"] - [album/listing (:album content)]]) + [album/listing (get-in content [:album-list :album])]]) (defn sidebar [user] [:aside.menu.section @@ -49,7 +49,8 @@ (defn app [route-id params query] (let [user @(subscribe [::subs/user]) - content @(subscribe [::subs/current-content])] + route-events @(subscribe [:routes/events-for-current-route]) + content @(subscribe [:api/route-data route-events])] [:div [:main.columns [:div.column.is-2.sidebar @@ -66,7 +67,7 @@ (defn main-panel [] (let [notifications @(subscribe [::subs/notifications]) is-booting? @(subscribe [::subs/is-booting?]) - [route-id params query] @(subscribe [::subs/current-route])] + [route-id params query] @(subscribe [:routes/current-route])] [:div [notification-list notifications] (if is-booting? diff --git a/test/cljs/airsonic_ui/api/events_test.cljs b/test/cljs/airsonic_ui/api/events_test.cljs new file mode 100644 index 0000000..06b6b1c --- /dev/null +++ b/test/cljs/airsonic_ui/api/events_test.cljs @@ -0,0 +1,50 @@ +(ns airsonic-ui.api.events-test + (:require [cljs.test :refer-macros [deftest testing is]] + [airsonic-ui.api.events :as events] + [airsonic-ui.fixtures :as fixtures])) + +(enable-console-print!) + +(deftest api-failure-notifcations + (testing "Should show an error notification when airsonic responds with an error" + (let [fx (events/good-api-response {} [:api/good-response "ping" nil (:error fixtures/responses)]) + ev (:dispatch fx)] + (is (= :notification/show (first ev))) + (is (= :error (second ev)))))) + +(deftest cached-api-requests + (letfn [(cache [fx [endpoint params]] + (get-in fx [:db :api/responses [endpoint params]]))] + (testing "Should be cached" + (testing "when the response was successful" + (let [endpoint "getScanStatus" + successful (events/good-api-response {} [:api/good-response endpoint nil (:ok fixtures/responses)]) + unsuccessful (events/good-api-response {} [:api/good-response endpoint nil (:error fixtures/responses)])] + (is (map? (cache successful [endpoint]))) + (is (nil? (cache unsuccessful [endpoint]))))) + (testing "in an unwrapped format" + (let [endpoint "getScanStatus" + fx (events/good-api-response {} [:api/good-response endpoint nil (:ok fixtures/responses)])] + (is (= #{:count :scanning} (set (keys (cache fx [endpoint])))))))) + (testing "When being issued" + (let [endpoint "getScanStatus" + fx (events/api-request {:db {:credentials (select-keys fixtures/credentials [:server])}} + [:api/request endpoint])] + (testing "should send an http request" + (is (contains? fx :http-xhrio))) + (testing "should indicate that a request is ongoing" + (is (true? (:api/is-loading? (cache fx [endpoint]))) "for non-cached responses") + (is (true? (-> (events/good-api-response fx [:api/good-response endpoint nil (:ok fixtures/responses)]) + (events/api-request [:api/request endpoint]) + (cache [endpoint]) + :api/is-loading?)) "for cached responses")) + (testing "should remove the indication that a request is ongoing when there is a response" + (is (not (:api/is-loading? (-> (events/good-api-response fx [:api/good-response endpoint nil (:ok fixtures/responses)]) + (cache [endpoint])))) "for a good response") + (is (not (:api/is-loading? (-> (merge fx (events/good-api-response fx [:api/good-response endpoint nil (:error fixtures/responses)])) + (cache [endpoint])))) "when an error is returned") + (is (not (:api/is-loading? (-> (merge fx (events/failed-api-response fx [:api/failed-response endpoint])) + (cache [endpoint])))) "when communication with the server failed")))) + (testing "Should be able to avoid the cache" + ;; FIXME: Implement this + ))) diff --git a/test/cljs/airsonic_ui/utils/api_test.cljs b/test/cljs/airsonic_ui/api/helpers_test.cljs similarity index 96% rename from test/cljs/airsonic_ui/utils/api_test.cljs rename to test/cljs/airsonic_ui/api/helpers_test.cljs index c3b9c73..e272f83 100644 --- a/test/cljs/airsonic_ui/utils/api_test.cljs +++ b/test/cljs/airsonic_ui/api/helpers_test.cljs @@ -1,8 +1,8 @@ -(ns airsonic-ui.utils.api-test +(ns airsonic-ui.api.helpers-test (:require [cljs.test :refer [deftest testing is]] [clojure.string :as str] [airsonic-ui.fixtures :refer [responses]] - [airsonic-ui.utils.api :as api])) + [airsonic-ui.api.helpers :as api])) (defn- url "Construct a url with no params" diff --git a/test/cljs/airsonic_ui/api/subs_test.cljs b/test/cljs/airsonic_ui/api/subs_test.cljs new file mode 100644 index 0000000..ded1966 --- /dev/null +++ b/test/cljs/airsonic_ui/api/subs_test.cljs @@ -0,0 +1,29 @@ +(ns airsonic-ui.api.subs-test + (:require [cljs.test :refer-macros [deftest testing is]] + [airsonic-ui.api.subs :as sub])) + +(enable-console-print!) + +(deftest endpoint-keywordification + (testing "Should strip prefixes" + (is (= :artist-info (sub/endpoint->kw "getArtistInfo"))) + (is (= :jukebox-control (sub/endpoint->kw "jukeboxControl")))) + (testing "Should strip trailing numbers" + (is (= :album-list (sub/endpoint->kw "getAlbumList2"))) + (is (= :search (sub/endpoint->kw "search3"))))) + +(deftest responses-for-route + (testing "Should return all cached responses for a route" + (let [route-events [[:api/request "getAlbumList2" {:type "recent", :size 18}] + [:event/should-be-ignored] + [:api/request "getArtistInfo" {:id "128"}]] + db {:api/responses {["getAlbumList2" {:type "recent" :size 18}] + {:album [{:genre "foo", :artistId "12345"} + {:genre "electronic", :artistId "9999"}]} + + ["getArtistInfo" {:id "128"}] + {:biography "Interesting bio" + :largeImageUrl "https://lastfm-img2.akamaized.net/i/u/300x300/fb416b59cd694587aca0b2dec8f41198.png"}}}] + (is (= {:album-list (get-in db [:api/responses ["getAlbumList2" {:type "recent" :size 18}]]) + :artist-info (get-in db [:api/responses ["getArtistInfo" {:id "128"}]])} + (sub/route-data db [:api/route-data route-events])))))) diff --git a/test/cljs/airsonic_ui/events_test.cljs b/test/cljs/airsonic_ui/events_test.cljs index ef8abd9..260e91a 100644 --- a/test/cljs/airsonic_ui/events_test.cljs +++ b/test/cljs/airsonic_ui/events_test.cljs @@ -60,7 +60,7 @@ (testing "invokes correct callback on server response" (is (= [:credentials/authentication-response fixtures/credentials] (:on-success request)))) (testing "invokes correct callback when server is not reachable" - (is (= [:api/bad-response] (:on-failure request)))))) + (is (= [:api/failed-response] (:on-failure request)))))) (deftest authentication-response (testing "On success" @@ -104,13 +104,6 @@ (is (= ::routes/login route-id)) (is (contains? query :redirect)))))) -(deftest api-interaction - (testing "Should show an error notification when airsonic responds with an error" - (let [fx (events/good-api-response {} [:_ (:error fixtures/responses)]) - ev (:dispatch fx)] - (is (= :notification/show (first ev))) - (is (= :error (second ev)))))) - (defn- first-notification [fx] (-> (get-in fx [:db :notifications]) vals first)) diff --git a/test/cljs/airsonic_ui/subs_test.cljs b/test/cljs/airsonic_ui/subs_test.cljs index 684027d..297f36c 100644 --- a/test/cljs/airsonic_ui/subs_test.cljs +++ b/test/cljs/airsonic_ui/subs_test.cljs @@ -1,7 +1,7 @@ (ns airsonic-ui.subs-test (:require [cljs.test :refer [deftest testing is]] [airsonic-ui.fixtures :as fixtures] - [airsonic-ui.utils.api :as api] + [airsonic-ui.api.helpers :as api] [airsonic-ui.subs :as subs])) (deftest booting @@ -10,17 +10,17 @@ is-booting? (fn is-booting? [db] (subs/is-booting? db [:subs/is-booting?]))] (testing "Should be false when we don't have previous credentials" - (is (not (is-booting? {:current-route route}))) - (is (not (is-booting? {:current-route route + (is (not (is-booting? {:routes/current-route route}))) + (is (not (is-booting? {:routes/current-route route :credentials {}}))) ) (testing "Should be true when we have unverified credentials" - (is (true? (is-booting? {:current-route route + (is (true? (is-booting? {:routes/current-route route :credentials fixtures/credentials})))) (testing "Should be false when we have verified credentials" - (is (not (is-booting? {:current-route route + (is (not (is-booting? {:routes/current-route route :credentials verified-credentials})))) (testing "Should be true when routing is not yet set up" - (is (true? (is-booting? {:current-route nil + (is (true? (is-booting? {:routes/current-route nil :credentials verified-credentials})))))) (deftest cover-images