From dc39729d565b95dfb09fc65f3e069288c3b801cb Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 29 Oct 2020 19:48:47 -0700
Subject: [PATCH] Fix collections graph stack overflow (#13618)
* Fix collections/graph stack overflow
* Test fixes :wrench: [ci postgres] [ci mysql]
* Show engine in Task trouble shooting logs (#13608)
* Show engine in Task trouble shooting logs
in an effort to get better feedback when people raise issues about
long running sync processes, include the database engine in the logs.
Also wrapped `th` in a `tr` inside the `thead` to remove react warning
* Const intead of let on the db_id_to_engine
* Swap db id for db name
* Move comment out of jsx
* fixup! Merge branch 'release-x.37.x' into fix-collections-graph-stack-overflow
Co-authored-by: dpsutton <dan@dpsutton.com>
---
resources/log4j2.xml | 1 +
resources/migrations/000_migrations.yaml | 22 ++++++++++
src/metabase/models/collection/graph.clj | 22 +++++-----
src/metabase/util/honeysql_extensions.clj | 4 +-
.../metabase/models/collection/graph_test.clj | 42 ++++++++++++++++++-
test/metabase/test/util.clj | 2 +-
6 files changed, 76 insertions(+), 17 deletions(-)
diff --git a/resources/log4j2.xml b/resources/log4j2.xml
index f71bc3a827a..cc2636dd4c9 100644
--- a/resources/log4j2.xml
+++ b/resources/log4j2.xml
@@ -23,6 +23,7 @@
<Loggers>
<Logger name="metabase" level="INFO"/>
+ <Logger name="metabase-enterprise" level="INFO"/>
<Logger name="metabase.plugins" level="DEBUG"/>
<Logger name="metabase.middleware" level="DEBUG"/>
<Logger name="metabase.query-processor.async" level="DEBUG"/>
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index b8c0fae938b..6a9fc3a0c83 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -7722,3 +7722,25 @@ databaseChangeLog:
tableName: core_user
columnName: email
newDataType: varchar_ignorecase(254)
+
+ - changeSet:
+ id: 273
+ author: camsaul
+ comment: Added 0.37.1
+ changes:
+ - addDefaultValue:
+ tableName: core_user
+ columnName: is_superuser
+ columnDataType: boolean
+ defaultValueBoolean: false
+
+ - changeSet:
+ id: 274
+ author: camsaul
+ comment: Added 0.37.1
+ changes:
+ - addDefaultValue:
+ tableName: core_user
+ columnName: is_active
+ columnDataType: boolean
+ defaultValueBoolean: true
diff --git a/src/metabase/models/collection/graph.clj b/src/metabase/models/collection/graph.clj
index 338ce8a5f54..18abbcc90a3 100644
--- a/src/metabase/models/collection/graph.clj
+++ b/src/metabase/models/collection/graph.clj
@@ -1,6 +1,5 @@
(ns metabase.models.collection.graph
(:require [clojure.data :as data]
- [honeysql.helpers :as h]
[metabase.api.common :as api :refer [*current-user-id*]]
[metabase.models
[collection :as collection :refer [Collection]]
@@ -8,7 +7,9 @@
[permissions :as perms :refer [Permissions]]
[permissions-group :refer [PermissionsGroup]]]
[metabase.util :as u]
- [metabase.util.schema :as su]
+ [metabase.util
+ [honeysql-extensions :as hx]
+ [schema :as su]]
[schema.core :as s]
[toucan.db :as db]))
@@ -57,16 +58,13 @@
Collections (i.e., things that you can set Permissions for, and that should go in the graph.)"
[collection-namespace :- (s/maybe su/KeywordOrString)]
(let [personal-collection-ids (db/select-ids Collection :personal_owner_id [:not= nil])
- honeysql-form (cond-> {:select [[:id :id]]
- :from [Collection]
- :where [:= :namespace (u/qualified-name collection-namespace)]}
- (seq personal-collection-ids)
- (h/merge-where [:not-in :id (set personal-collection-ids)]))
- honeysql-form (reduce
- (fn [honeysql-form collection-id]
- (h/merge-where honeysql-form [:not [:like :location (format "/%d/%%" collection-id)]]))
- honeysql-form
- personal-collection-ids)]
+ honeysql-form {:select [[:id :id]]
+ :from [Collection]
+ :where (into [:and
+ [:= :namespace (u/qualified-name collection-namespace)]
+ [:= :personal_owner_id nil]]
+ (for [collection-id personal-collection-ids]
+ [:not [:like :location (hx/literal (format "/%d/%%" collection-id))]]))}]
(set (map :id (db/query honeysql-form)))))
(s/defn graph :- PermissionsGraph
diff --git a/src/metabase/util/honeysql_extensions.clj b/src/metabase/util/honeysql_extensions.clj
index 4d2bda5f79b..edc518a8718 100644
--- a/src/metabase/util/honeysql_extensions.clj
+++ b/src/metabase/util/honeysql_extensions.clj
@@ -186,8 +186,8 @@
(def ^{:arglists '([& exprs])} concat "SQL `concat` function." (partial hsql/call :concat))
;; Etc (Dev Stuff)
-(alter-meta! #'honeysql.core/format assoc :style/indent 1)
-(alter-meta! #'honeysql.core/call assoc :style/indent 1)
+(alter-meta! #'honeysql.core/format assoc :style/indent :defn)
+(alter-meta! #'honeysql.core/call assoc :style/indent :defn)
(require 'honeysql.types)
(extend-protocol PrettyPrintable
diff --git a/test/metabase/models/collection/graph_test.clj b/test/metabase/models/collection/graph_test.clj
index 17cb9a66ef3..f9c6cfcc140 100644
--- a/test/metabase/models/collection/graph_test.clj
+++ b/test/metabase/models/collection/graph_test.clj
@@ -1,7 +1,9 @@
(ns metabase.models.collection.graph-test
- (:require [clojure.test :refer :all]
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.test :refer :all]
[medley.core :as m]
[metabase
+ [models :refer [User]]
[test :as mt]
[util :as u]]
[metabase.api.common :refer [*current-user-id*]]
@@ -14,7 +16,8 @@
[metabase.test.fixtures :as fixtures]
[metabase.util.schema :as su]
[schema.core :as s]
- [toucan.db :as db]))
+ [toucan.db :as db]
+ [toucan.util.test :as tt]))
(use-fixtures :once (fixtures/initialize :db :test-users :test-users-personal-collections))
@@ -415,3 +418,38 @@
:after {(keyword (str group-id)) {:root (s/eq "write")}}
s/Keyword s/Any}
(db/select-one CollectionRevision {:order-by [[:id :desc]]})))))))))))
+
+(defn- do-with-n-temp-users-with-personal-collections! [num-users thunk]
+ (mt/with-model-cleanup [User Collection]
+ ;; insert all the users
+ (let [user-ids (jdbc/execute!
+ (db/connection)
+ (db/honeysql->sql
+ {:insert-into User
+ :values (repeatedly num-users #(assoc (tt/with-temp-defaults User) :date_joined :%now))}))
+ max-id (:max-id (db/select-one [User [:%max.id :max-id]]))
+ ;; determine the range of IDs we inserted -- MySQL doesn't support INSERT INTO ... RETURNING like Postgres
+ ;; so this is the fastest way to do this
+ user-ids (range (inc (- max-id num-users)) (inc max-id))]
+ (assert (= (count user-ids) num-users))
+ ;; insert the Collections
+ (jdbc/execute!
+ (db/connection)
+ (db/honeysql->sql
+ {:insert-into Collection
+ :values (for [user-id user-ids
+ :let [collection (tt/with-temp-defaults Collection)]]
+ (assoc collection
+ :personal_owner_id user-id
+ :slug "my_collection"))})))
+ ;; now run the thunk
+ (thunk)))
+
+(defmacro ^:private with-n-temp-users-with-personal-collections [num-users & body]
+ `(do-with-n-temp-users-with-personal-collections! ~num-users (fn [] ~@body)))
+
+(deftest mega-graph-test
+ (testing "A truly insane amount of Personal Collections shouldn't cause a Stack Overflow (#13211)"
+ (with-n-temp-users-with-personal-collections 2000
+ (is (>= (db/count Collection :personal_owner_id [:not= nil]) 2000))
+ (is (map? (graph/graph))))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index aba9e2a1c28..741b651af5c 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -561,7 +561,7 @@
;; might not have an old max ID if this is the first time the macro is used in this test run.
:let [old-max-id (or (get model->old-max-id model)
0)]]
- (db/delete! model :id [:> old-max-id]))))))
+ (db/simple-delete! model :id [:> old-max-id]))))))
(defmacro with-model-cleanup
"Execute `body`, then delete any *new* rows created for each model in `models`. Calls `delete!`, so if the model has
--
GitLab