-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the Trash data model #42845
base: master
Are you sure you want to change the base?
Improve the Trash data model #42845
Conversation
|
d3ae5b7
to
41a9209
Compare
b4b73c6
to
47d1b11
Compare
src/metabase/models/collection.clj
Outdated
(format "%%/%s/" (str parent-id)))] | ||
(format "%%/%s/" (str parent-id))) | ||
collection-ids (if (= collection-ids :all) | ||
(t2/select-pks-set :model/Collection :archived (or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an existing bug here. When looking for collection children, we didn't care at all about :archived
. So with collections like /A/B/C
where B is archived, we'd show A as having no effective children.
I'm a little concerned about performance here and still thinking about whether we can help that.
133fa0c
to
daa41f8
Compare
Ok, so I had a realization at the PERFECT time, immediately after the RC cutoff. Great job, brain! Here's the realization. For the Trash, we need to keep track of two things: - where the item actually is located in the hierarchy, and - what collection we should look at to see what permissions apply to the item. For example, a Card might be in the Trash, but we need to look at Collection 1234 to see that a user has permission to Write that card. My implementation of this was to add a column, `trashed_from_collection_id`, so that we could move a Card or a Dashboard to a new `collection_id`, but keep track of the permissions we actually needed to check. So: - `collection_id` was where the item was located in the collection hierarchy, and - `trashed_from_collection_id` was where we needed to look to check permissions. Today I had the realization that it's much, much more important to get PERMISSIONS right than to get collection hierarchy right. Like if we mess up and show something as in the Trash when it's not in the Trash, or show something in the wrong Collection - that's not great, sure. But if we mess up and show a Card when we shouldn't, or show a Dashboard when we shouldn't, that's Super Duper Bad. So the problem with my initial implementation was that we needed to change everywhere that checked permissions, to make sure they checked BOTH `trashed_from_collection_id` and `collection_id` as appropriate. So... there's a much better solution. Instead of adding a column to represent the *permissions* that we should apply to the dashboard or card, add a column to represent the *location in the hierarchy* that should apply to the dashboard or the card. We can simplify further: the *only time* we want to display something in a different place in the hierarchy than usual is when it was put directly into the trash. If you trash a dashboard as a part of a collection, then we should display it in that collection just like normal. So, we can do the following: - add a `trashed_directly` column to Cards and Dashboards, representing whether they should be displayed in the Trash instead of their actual parent collection - use the `collection_id` column of Cards and Dashboards without modification to represent permissions. There's one main downside of this approach. If you trash a dashboard, and then delete the collection that the dashboard was originally in, what do we do with `dashboard.collection_id`? - we have to change it, because it's a foreign key - we can't set it to null, because that represents the root collection In this initial implementation, I've just cascaded the delete: if you delete a dashboard and then delete a collection, the dashboard will be deleted. This is not ideal. I'm not totally sure what we should do in this situation.
And don't allow deleting collections
1068b64
to
840631a
Compare
e5d26c9
to
b73bb64
Compare
Ok, so I had a realization at the PERFECT time, immediately after the RC cutoff. Great job, brain!
Here's the realization. For the Trash, we need to keep track of two things:
where the item actually is located in the hierarchy, and
what collection we should look at to see what permissions apply to the item.
For example, a Card might be in the Trash, but we need to look at Collection 1234 to see that a user has permission to Write that card.
My implementation of this was to add a column,
trashed_from_collection_id
, so that we could move a Card or a Dashboard to a newcollection_id
, but keep track of the permissions we actually needed to check.So:
collection_id
was where the item was located in the collection hierarchy, andtrashed_from_collection_id
was where we needed to look to check permissions.Today I had the realization that it's much, much more important to get PERMISSIONS right than to get collection hierarchy right. Like if we mess up and show something as in the Trash when it's not in the Trash, or show something in the wrong Collection - that's not great, sure. But if we mess up and show a Card when we shouldn't, or show a Dashboard when we shouldn't, that's Super Duper Bad.
So the problem with my initial implementation was that we needed to change everywhere that checked permissions, to make sure they checked BOTH
trashed_from_collection_id
andcollection_id
as appropriate.So... there's a much better solution. Instead of adding a column to represent the permissions that we should apply to the dashboard or card, add a column to represent the location in the hierarchy that should apply to the dashboard or the card.
We can simplify further: the only time we want to display something in a different place in the hierarchy than usual is when it was put directly into the trash. If you trash a dashboard as a part of a collection, then we should display it in that collection just like normal.
So, we can do the following:
add a
trashed_directly
column to Cards and Dashboards, representing whether they should be displayed in the Trash instead of their actual parent collectionuse the
collection_id
column of Cards and Dashboards without modification to represent permissions.There's one main downside of this approach. If you trash a dashboard, and then delete the collection that the dashboard was originally in, what do we do with
dashboard.collection_id
?we have to change it, because it's a foreign key
we can't set it to null, because that represents the root collection
In this initial implementation, I've just cascaded the delete: if you delete a dashboard and then delete a collection, the dashboard will be deleted. This is not ideal. I'm not totally sure what we should do in this situation.