Skip to content

Commit

Permalink
Rename collection revision (metabase#15616)
Browse files Browse the repository at this point in the history 
* Table collection_revision -> collection_permission_graph_revision

in anticipation of getting collection revisions in, need the table
name as the previous table named this was tracking changes to the
permission graph

* rename collection permission graph revision alias

* Remove unnecessary or for precondition

* Rename sequence in sql

liquibase has a renameSequence changeset but it doesn't work for h2
obviously. However, it doesn't allow this constraint to be honored
with a precondition. Just having this changeset on h2 blows up
  • Loading branch information
@dpsutton
dpsutton authored Apr 23, 2021
1 parent e4e1587 commit a88351d
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 27 deletions.
20 changes: 20 additions & 0 deletions resources/migrations/000_migrations.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8261,6 +8261,26 @@ databaseChangeLog:
columnName: parameters
defaultNullValue: '[]'
tableName: pulse
- changeSet:
id: 300
author: dpsutton
comment: Added 0.40.0
changes:
- renameTable:
oldTableName: collection_revision
newTableName: collection_permission_graph_revision
- changeSet:
id: 301
author: dpsutton
comment: Added 0.40.0 renaming collection_revision to collection_permission_graph_revision
failOnError: false # mysql and h2 don't have this sequence
preConditions:
- onFail: MARK_RAN
- dbms:
type: postgresql
changes:
- sql:
- sql: ALTER SEQUENCE collection_revision_id_seq RENAME TO collection_permission_graph_revision_id_seq

# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<

Expand Down
6 changes: 3 additions & 3 deletions src/metabase/cmd/copy.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
[metabase.db.connection :as mdb.conn]
[metabase.db.data-migrations :refer [DataMigrations]]
[metabase.db.setup :as mdb.setup]
[metabase.models :refer [Activity Card CardFavorite Collection CollectionRevision Dashboard DashboardCard
DashboardCardSeries DashboardFavorite Database Dependency Dimension Field
[metabase.models :refer [Activity Card CardFavorite Collection CollectionPermissionGraphRevision Dashboard
DashboardCard DashboardCardSeries DashboardFavorite Database Dependency Dimension Field
FieldValues LoginHistory Metric MetricImportantField NativeQuerySnippet Permissions
PermissionsGroup PermissionsGroupMembership PermissionsRevision Pulse PulseCard
PulseChannel PulseChannelRecipient Revision Segment Session Setting Table User
Expand Down Expand Up @@ -58,7 +58,7 @@
ViewLog
Session
Collection
CollectionRevision
CollectionPermissionGraphRevision
Dashboard
Card
CardFavorite
Expand Down
6 changes: 3 additions & 3 deletions src/metabase/models.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
[metabase.models.card :as card]
[metabase.models.card-favorite :as card-favorite]
[metabase.models.collection :as collection]
[metabase.models.collection-revision :as collection-revision]
[metabase.models.collection-permission-graph-revision :as c-perm-revision]
[metabase.models.dashboard :as dashboard]
[metabase.models.dashboard-card :as dashboard-card]
[metabase.models.dashboard-card-series :as dashboard-card-series]
Expand Down Expand Up @@ -42,7 +42,7 @@
card/keep-me
card-favorite/keep-me
collection/keep-me
collection-revision/keep-me
c-perm-revision/keep-me
dashboard/keep-me
dashboard-card/keep-me
dashboard-card-series/keep-me
Expand Down Expand Up @@ -80,7 +80,7 @@
[card Card]
[card-favorite CardFavorite]
[collection Collection]
[collection-revision CollectionRevision]
[c-perm-revision CollectionPermissionGraphRevision]
[dashboard Dashboard]
[dashboard-card DashboardCard]
[dashboard-card-series DashboardCardSeries]
Expand Down
7 changes: 4 additions & 3 deletions src/metabase/models/collection/graph.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
(:require [clojure.data :as data]
[metabase.api.common :as api :refer [*current-user-id*]]
[metabase.models.collection :as collection :refer [Collection]]
[metabase.models.collection-revision :as collection-revision :refer [CollectionRevision]]
[metabase.models.collection-permission-graph-revision :as c-perm-revision
:refer [CollectionPermissionGraphRevision]]
[metabase.models.permissions :as perms :refer [Permissions]]
[metabase.models.permissions-group :refer [PermissionsGroup]]
[metabase.util :as u]
Expand Down Expand Up @@ -84,7 +85,7 @@
([collection-namespace :- (s/maybe su/KeywordOrString)]
(let [group-id->perms (group-id->permissions-set)
collection-ids (non-personal-collection-ids collection-namespace)]
{:revision (collection-revision/latest-id)
{:revision (c-perm-revision/latest-id)
:groups (into {} (for [group-id (db/select-ids PermissionsGroup)]
{group-id (group-permissions-graph collection-namespace (group-id->perms group-id) collection-ids)}))})))

Expand Down Expand Up @@ -123,7 +124,7 @@
(when *current-user-id*
;; manually specify ID here so if one was somehow inserted in the meantime in the fraction of a second since we
;; called `check-revision-numbers` the PK constraint will fail and the transaction will abort
(db/insert! CollectionRevision
(db/insert! CollectionPermissionGraphRevision
:id (inc current-revision)
:before (assoc before-graph :namespace collection-namespace)
:after changes
Expand Down
12 changes: 6 additions & 6 deletions src/metabase/models/collection_revision.clj → .../collection_permission_graph_revision.clj
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
(ns metabase.models.collection-revision
(ns metabase.models.collection-permission-graph-revision
(:require [metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[toucan.db :as db]
[toucan.models :as models]))

(models/defmodel CollectionRevision :collection_revision)
(models/defmodel CollectionPermissionGraphRevision :collection_permission_graph_revision)

(defn- pre-insert [revision]
(assoc revision :created_at :%now))

(u/strict-extend (class CollectionRevision)
(u/strict-extend (class CollectionPermissionGraphRevision)
models/IModel
(merge models/IModelDefaults
{:types (constantly {:before :json
:after :json})
:pre-insert pre-insert
:pre-update (fn [& _] (throw (Exception. (tru "You cannot update a CollectionRevision!"))))}))
:pre-update (fn [& _] (throw (Exception. (tru "You cannot update a CollectionPermissionGraphRevision!"))))}))


(defn latest-id
"Return the ID of the newest `CollectionRevision`, or zero if none have been made yet.
"Return the ID of the newest `CollectionPermissionGraphRevision`, or zero if none have been made yet.
(This is used by the collection graph update logic that checks for changes since the original graph was fetched)."
[]
(or (:id (db/select-one [CollectionRevision [:%max.id :id]]))
(or (:id (db/select-one [CollectionPermissionGraphRevision [:%max.id :id]]))
0))
25 changes: 13 additions & 12 deletions test/metabase/models/collection/graph_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
[metabase.api.common :refer [*current-user-id*]]
[metabase.models :refer [User]]
[metabase.models.collection :as collection :refer [Collection]]
[metabase.models.collection-revision :as collection-revision :refer [CollectionRevision]]
[metabase.models.collection-permission-graph-revision :as c-perm-revision
:refer [CollectionPermissionGraphRevision]]
[metabase.models.collection.graph :as graph]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as group :refer [PermissionsGroup]]
Expand Down Expand Up @@ -38,7 +39,7 @@
collection-id))))))))

(defn- clear-graph-revisions! []
(db/delete! CollectionRevision))
(db/delete! CollectionPermissionGraphRevision))

(defn- only-groups
"Remove entries for non-'magic' groups from a fetched perms `graph`."
Expand Down Expand Up @@ -291,7 +292,7 @@

(testing "No revision should have been saved"
(is (= 0
(collection-revision/latest-id)))))))
(c-perm-revision/latest-id)))))))

(testing "Make sure you can't be sneaky and edit descendants of Personal Collections either."
(mt/with-temp Collection [collection {:location (lucky-collection-children-location)}]
Expand Down Expand Up @@ -339,7 +340,7 @@
(is (= {"Currency A" :read, "Currency A -> B" :read, :root :none}
(nice-graph (graph/graph :currency)))))

;; bind a current user so CollectionRevisions get saved.
;; bind a current user so CollectionPermissionGraphRevisions get saved.
(mt/with-test-user :crowberto
(testing "Should be able to update the graph for the default namespace.\n"
(let [before (graph/graph)]
Expand All @@ -351,14 +352,14 @@
(is (= {"Currency A" :read, "Currency A -> B" :read, :root :none}
(nice-graph (graph/graph :currency)))))

(testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
(testing "A CollectionPermissionGraphRevision recording the *changes* to the perms graph should be saved."
(is (schema= {:id su/IntGreaterThanZero
:before (s/eq (mt/obj->json->obj (assoc before :namespace nil)))
:after (s/eq {(keyword (str group-id)) {(keyword (str default-ab)) "write"}})
:user_id (s/eq (mt/user->id :crowberto))
:created_at java.time.temporal.Temporal
s/Keyword s/Any}
(db/select-one CollectionRevision {:order-by [[:id :desc]]}))))))
(db/select-one CollectionPermissionGraphRevision {:order-by [[:id :desc]]}))))))

(testing "Should be able to update the graph for a non-default namespace.\n"
(let [before (graph/graph :currency)]
Expand All @@ -370,14 +371,14 @@
(is (= {"Default A" :read, "Default A -> B" :write, :root :none}
(nice-graph (graph/graph)))))

(testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
(testing "A CollectionPermissionGraphRevision recording the *changes* to the perms graph should be saved."
(is (schema= {:id su/IntGreaterThanZero
:before (s/eq (mt/obj->json->obj (assoc before :namespace "currency")))
:after (s/eq {(keyword (str group-id)) {(keyword (str currency-a)) "write"}})
:user_id (s/eq (mt/user->id :crowberto))
:created_at java.time.temporal.Temporal
s/Keyword s/Any}
(db/select-one CollectionRevision {:order-by [[:id :desc]]}))))))
(db/select-one CollectionPermissionGraphRevision {:order-by [[:id :desc]]}))))))

(testing "should be able to update permissions for the Root Collection in the default namespace via the graph"
(graph/update-graph! (assoc (graph/graph) :groups {group-id {:root :read}}))
Expand All @@ -388,15 +389,15 @@
(is (= {:root :none, "Currency A" :write, "Currency A -> B" :read}
(nice-graph (graph/graph :currency)))))

(testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
(testing "A CollectionPermissionGraphRevision recording the *changes* to the perms graph should be saved."
(is (schema= {:before {:namespace (s/eq nil)
:groups {(keyword (str group-id)) {:root (s/eq "none")
s/Keyword s/Any}
s/Keyword s/Any}
s/Keyword s/Any}
:after {(keyword (str group-id)) {:root (s/eq "read")}}
s/Keyword s/Any}
(db/select-one CollectionRevision {:order-by [[:id :desc]]})))))
(db/select-one CollectionPermissionGraphRevision {:order-by [[:id :desc]]})))))

(testing "should be able to update permissions for Root Collection in non-default namespace"
(graph/update-graph! :currency (assoc (graph/graph :currency) :groups {group-id {:root :write}}))
Expand All @@ -407,15 +408,15 @@
(is (= {:root :read, "Default A" :read, "Default A -> B" :write}
(nice-graph (graph/graph)))))

(testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
(testing "A CollectionPermissionGraphRevision recording the *changes* to the perms graph should be saved."
(is (schema= {:before {:namespace (s/eq "currency")
:groups {(keyword (str group-id)) {:root (s/eq "none")
s/Keyword s/Any}
s/Keyword s/Any}
s/Keyword s/Any}
:after {(keyword (str group-id)) {:root (s/eq "write")}}
s/Keyword s/Any}
(db/select-one CollectionRevision {:order-by [[:id :desc]]})))))))))))
(db/select-one CollectionPermissionGraphRevision {:order-by [[:id :desc]]})))))))))))

(defn- do-with-n-temp-users-with-personal-collections! [num-users thunk]
(mt/with-model-cleanup [User Collection]
Expand Down

0 comments on commit a88351d

Please sign in to comment.