Improve the Trash data model #42845
Open
Improve the Trash data model #42845
+1,557
−1,148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
johnswanson
force-pushed
the
investigate-better-perms-for-trash
branch
5 times, most recently
from
d3ae5b7
to
41a9209
Compare
johnswanson
added
the
backport
johnswanson
force-pushed
the
investigate-better-perms-for-trash
branch
4 times, most recently
from
b4b73c6
to
47d1b11
Compare
johnswanson
commented
May 23, 2024
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.
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.
johnswanson
force-pushed
the
investigate-better-perms-for-trash
branch
11 times, most recently
from
e5d26c9
to
b73bb64
Compare
johnswanson
force-pushed
the
investigate-better-perms-for-trash
branch
from
165e51d
to
060aaf1
Compare
Previously named `move-on-archive-or-unarchive`, which was no longer accurate since we're not moving anything!
The Trash exists as a real collection so that we can list items in and such without needing special cases. But we don't want anything to _actually_ get moved to it.
Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Also consolidated it into a single function. It takes the trash-collection-id as an argument to prevent a circular dependency.
In the product, we want to move to "trashed" as the descriptor for things that are currently-known-as-archived. It'd be nice to switch to that on the backend as well, but that'll require quite a lot of churn on the frontend as we'd need to change the API (or have an awkward translation layer, where we call it `trashed` on the backend, turn it into `archived` when sending data out via the API, and then the FE turns around and called it "trashed" again). For now, let's just be consistent on the backend and call it `archived` everywhere. So: - `archived_directly` instead of `trashed_directly`, and - `archive_operation_id` instead of `trash_operation_id`
for `collection/permissions-set->visible-collection-ids`: - `:include-archived` => `:include-archived-items` - `:include-trash?` => `:include-trash-collection`
This used to be a possible return value for `permissions-set->visible-collection-ids` indicating that the user could view all collections. Now that function always returns a set of collection IDs (possibly including "root" for the root collection) so we don't need to deal with that anymore.
Dispatch on the model of each item (but still keep them in batches for efficiency.)
It's slightly different (TTL instead of exactly per-request) but that should be fine.
johnswanson
force-pushed
the
investigate-better-perms-for-trash
branch
from
c16930f
to
f2e0d87
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_idwas where the item was located in the collection hierarchy, andtrashed_from_collection_idwas 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_idandcollection_idas 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_directlycolumn to Cards and Dashboards, representing whether they should be displayed in the Trash instead of their actual parent collectionuse the
collection_idcolumn 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.
Fixes #43494