Skip to content
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

adding perm-graph filtering on db or group id #36543

Merged
merged 12 commits into from
Dec 11, 2023

Conversation

escherize
Copy link
Contributor

@escherize escherize commented Dec 7, 2023

resolves #36647.

This PR adds 2 endpoints that will make our permission graph api a lot snappier when graphs get large.

  • GET api/permissions/graph/db/:db-id
  • GET api/permissions/graph/group/:group-id

These return the same graph structure, but filtered such that they return only {db,group}-ids respectively.

Since this is a slice of the entire graph, we query for, post-process, and transmit to the client, much less data.

@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Dec 7, 2023
@escherize escherize marked this pull request as ready for review December 7, 2023 22:19
@escherize escherize changed the title a more refined first crack at adding perm-graph access for db or group id adding perm-graph filtering on db or group id Dec 7, 2023
…e/metabase into bcm-split-data-perm-graph-access
test/metabase/api/permissions_test.clj Show resolved Hide resolved
src/metabase/models/permissions.clj Outdated Show resolved Hide resolved
@escherize escherize mentioned this pull request Dec 8, 2023
2 tasks
(deftest data-graph-for-db-check-all-dbs-test
(doseq [db-id (t2/select-fn-set :id :model/Database)]
(testing (str "testing data perms graph for db graph with db-id: [" db-id "].")
(= #{db-id}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing is

@escherize escherize added the no-backport Do not backport this PR to any branch label Dec 8, 2023
@escherize escherize enabled auto-merge (squash) December 9, 2023 00:27
@escherize escherize merged commit 19401c1 into master Dec 11, 2023
103 checks passed
@escherize escherize deleted the bcm-split-data-perm-graph-access branch December 11, 2023 23:19
Copy link

@escherize Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@escherize escherize added this to the 0.49 milestone Dec 11, 2023
qnkhuat pushed a commit that referenced this pull request Dec 12, 2023
* a more refined first crack at adding perm-graph access for db or group id

* new routes: filter data-perm-graph on db or group

add tests

* remove inline defs

* code review responses

* fix typo

* fix the other tests

* update the tests to use the right malli schema

* reuse private vars in tests

* pull graph checker into test util ns and apply it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/AdminWebapp Admin and Webapp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster permission graph endpoints
2 participants