Skip to content
Snippets Groups Projects
Commit ef201a9e authored by Cam Saül's avatar Cam Saül Committed by GitHub
Browse files

Merge pull request #2907 from metabase/fix-horrifying-code 

Fix n+1 DB calls in GET /activity :disappointed: :unamused:
parents 14af3702 a9bc0e8d
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/activity.clj
+ 47
− 9
View file @ ef201a9e
(ns metabase.api.activity
(:require [compojure.core :refer [GET]]
(:require [clojure.set :as set]
[compojure.core :refer [GET]]
[metabase.api.common :refer :all]
[metabase.db :as db]
(metabase.models [activity :refer [Activity]]
......@@ -8,19 +9,56 @@
[hydrate :refer [hydrate]]
[view-log :refer [ViewLog]])))
(defn- dashcard-exists [{:keys [topic] :as activity}]
(if-not (contains? #{:dashboard-add-cards :dashboard-remove-cards} topic)
activity
(update-in activity [:details :dashcards] (fn [dashcards]
(for [dashcard dashcards]
(assoc dashcard :exists (db/exists? Card :id (:card_id dashcard))))))))
(defn- dashcard-activity? [activity]
(contains? #{:dashboard-add-cards :dashboard-remove-cards}
(:topic activity)))
(defn- activities->referenced-objects
"Get a map of model name to a set of referenced IDs in these ACTIVITIES.
(activities->referenced-objects <some-activities>) -> {\"dashboard\" #{41 42 43}, \"card\" #{100 101}, ...}"
[activities]
(apply merge-with set/union (for [{:keys [model model_id], :as activity} activities
:when model]
(merge {model #{model_id}}
;; pull the referenced card IDs out of the dashcards for dashboard activites that involve adding/removing cards
(when (dashcard-activity? activity)
{"card" (set (for [dashcard (get-in activity [:details :dashcards])]
(:card_id dashcard)))})))))
(defn- referenced-objects->existing-objects
"Given a map of existing objects like the one returned by `activities->referenced-objects`, return a similar map of models to IDs of objects *that exist*.
(referenced-objects->existing-objects {\"dashboard\" #{41 42 43}, \"card\" #{100 101}, ...}) -> {\"dashboard\" #{41 43}, \"card\" #{101}, ...}"
[referenced-objects]
(into {} (for [[model ids] referenced-objects
:when (seq ids)]
{model (case model
"card" (db/select-ids 'Card, :id [:in ids])
"dashboard" (db/select-ids 'Dashboard, :id [:in ids])
"metric" (db/select-ids 'Metric, :id [:in ids], :is_active true)
"pulse" (db/select-ids 'Pulse, :id [:in ids])
"segment" (db/select-ids 'Segment, :id [:in ids], :is_active true)
nil)}))) ; don't care about other models
(defn- add-model-exists-info
"Add `:model_exists` keys to ACTIVITIES, and `:exists` keys to nested dashcards where appropriate."
[activities]
(let [existing-objects (-> activities activities->referenced-objects referenced-objects->existing-objects)]
(for [{:keys [model model_id], :as activity} activities]
(let [activity (assoc activity :model_exists (contains? (get existing-objects model) model_id))]
(if-not (dashcard-activity? activity)
activity
(update-in activity [:details :dashcards] (fn [dashcards]
(for [dashcard dashcards]
(assoc dashcard :exists (contains? (get existing-objects "card") (:card_id dashcard)))))))))))
(defendpoint GET "/"
"Get recent activity."
[]
(-> (db/select Activity, {:order-by [[:timestamp :desc]], :limit 40})
(hydrate :user :table :database :model_exists)
(->> (mapv dashcard-exists))))
(hydrate :user :table :database)
add-model-exists-info))
(defendpoint GET "/recent_views"
"Get the list of 15 things the current user has been viewing most recently."
......
src/metabase/models/activity.clj
+ 0
− 12
View file @ ef201a9e
......@@ -17,18 +17,6 @@
:details {}}]
(merge defaults activity)))
(defn model-exists?
"Does the object associated with this `Activity` exist in the DB?"
{:hydrate :model_exists, :arglists '([activity])}
[{:keys [model model_id]}]
(case model
"card" (db/exists? Card, :id model_id)
"dashboard" (db/exists? Dashboard, :id model_id)
"metric" (db/exists? Metric, :id model_id, :is_active true)
"pulse" (db/exists? Pulse, :id model_id)
"segment" (db/exists? Segment, :id model_id, :is_active true)
nil))
(u/strict-extend (class Activity)
i/IEntity
(merge i/IEntityDefaults
......
src/metabase/models/hydrate.clj
+ 8
− 5
View file @ ef201a9e
......@@ -178,11 +178,14 @@
(let [entity (@hydration-key->entity dest-key)
source-key (k->k_id dest-key)
ids (set (for [result results
:when (not (get result dest-key))]
(source-key result)))
objs (when (seq ids)
(u/key-by :id (db/select entity, :id [:in ids])))]
(for [{source-id source-key :as result} results]
:when (not (get result dest-key))
:let [k (source-key result)]
:when k]
k))
objs (if (seq ids)
(u/key-by :id (db/select entity, :id [:in ids]))
(constantly nil))]
(for [{source-id source-key, :as result} results]
(if (get result dest-key)
result
(assoc result dest-key (objs source-id)))))))
......
test/metabase/api/activity_test.clj
+ 47
− 3
View file @ ef201a9e
......@@ -9,7 +9,7 @@
[view-log :refer [ViewLog]])
[metabase.test.data :refer :all]
[metabase.test.data.users :refer :all]
[metabase.test.util :refer [match-$ random-name expect-with-temp]]
[metabase.test.util :refer [match-$ random-name expect-with-temp resolve-private-fns]]
[metabase.util :as u]))
;; GET /
......@@ -78,7 +78,7 @@
:common_name $})
:model $
:model_id $
:model_exists nil
:model_exists false
:database_id nil
:database nil
:table_id nil
......@@ -92,7 +92,7 @@
:user nil
:model $
:model_id $
:model_exists nil
:model_exists false
:database_id nil
:database nil
:table_id nil
......@@ -168,3 +168,47 @@
(create-view (user->id :rasta) "card" (:id card1))
(->> ((user->client :crowberto) :get 200 "activity/recent_views")
(map #(dissoc % :max_ts))))))
;;; activities->referenced-objects, referenced-objects->existing-objects, add-model-exists-info
(resolve-private-fns metabase.api.activity activities->referenced-objects referenced-objects->existing-objects add-model-exists-info)
(def ^:private ^:const fake-activities
[{:model "dashboard", :model_id 43, :topic :dashboard-create, :details {}}
{:model "dashboard", :model_id 42, :topic :dashboard-create, :details {}}
{:model "card", :model_id 114, :topic :card-create, :details {}}
{:model "card", :model_id 113, :topic :card-create, :details {}}
{:model "card", :model_id 112, :topic :card-create, :details {}}
{:model "card", :model_id 111, :topic :card-create, :details {}}
{:model "dashboard", :model_id 41, :topic :dashboard-add-cards, :details {:dashcards [{:card_id 109}]}}
{:model "card", :model_id 109, :topic :card-create, :details {}}
{:model "dashboard", :model_id 41, :topic :dashboard-add-cards, :details {:dashcards [{:card_id 108}]}}
{:model "dashboard", :model_id 41, :topic :dashboard-create, :details {}}
{:model "card", :model_id 108, :topic :card-create, :details {}}
{:model "user", :model_id 90, :topic :user-joined, :details {}}
{:model nil, :model_id nil, :topic :install, :details {}}])
(expect
{"dashboard" #{41 43 42}
"card" #{113 108 109 111 112 114}
"user" #{90}}
(activities->referenced-objects fake-activities))
(expect-with-temp [Dashboard [{dashboard-id :id}]]
{"dashboard" #{dashboard-id}, "card" nil}
(referenced-objects->existing-objects {"dashboard" #{dashboard-id 0}
"card" #{0}}))
(expect-with-temp [Dashboard [{dashboard-id :id}]
Card [{card-id :id}]]
[{:model "dashboard", :model_id dashboard-id, :model_exists true}
{:model "card", :model_id 0, :model_exists false}
{:model "dashboard", :model_id 0, :model_exists false, :topic :dashboard-remove-cards, :details {:dashcards [{:card_id card-id, :exists true}
{:card_id 0, :exists false}]}}]
(add-model-exists-info [{:model "dashboard", :model_id dashboard-id}
{:model "card", :model_id 0}
{:model "dashboard", :model_id 0, :topic :dashboard-remove-cards, :details {:dashcards [{:card_id card-id}
{:card_id 0}]}}]))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment