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.
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
[SIP-29] Add support for row-level security #8699
[SIP-29] Add support for row-level security #8699
Changes from 27 commits
ab6ae45
024912e
4d6789f
4152dc2
13f1381
1100a60
76a314a
86234fa
81c2bdb
ff80add
aafce2f
fd0eaf6
8b6ce72
5f69386
032efcb
2c252d9
7223da6
84a5009
7e0e3c8
253e992
1b3b8f6
c0ac8e9
bdfaff6
041038e
8f0d0d1
b402fc2
e403e9b
9c71f05
78ebc1c
5a0257c
9d4bcad
790f395
8adfc89
2b0ccfc
c68d6e6
b82acf6
d94e6aa
f76e953
3d0822d
db9f20a
524c109
a2e6b67
f581cf7
085a501
a625111
a131a72
2ea460b
1451c05
5eeb296
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
small improvement: list_columns = ["table.table_name", "roles", "clause", "creator", "modified"]
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.
small improvement: order_columns = ["table.table_name", "clause", "modified"]
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.
Let's call this in
superset/app.py
in theinit_views
methodThere 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.
moved
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.
Lots of the same code as
get_rls_filters
- maybe stick this in a generalized method?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.
but.. This is
get_rls_filters
, isn't it?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.
My bad - meant
get_rls_ids
XDThere 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.
generalized
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.
Given that by default
sqlalchemy
lazy-loads relationships, this would emit 1+number_of_rls_in_table queries.Let's write a single
sqlalchemy
query that returns the list of filters with the properIN
clauseThere 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.
Done - this reduces the number of necessary
hasattr
calls as well.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.
Hmm, I know it's a bit more verbose, but I prefer checking for the existence of the attribute using
hasattr
rather than catching theAttributeError
.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.
Isn't
try/except
more "pythonic"? I have no problem changing this, but worry about someone else then requesting it be changed back. Can we get a second opinion on this one?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.
I agree with @serenajiang here; best to be explicit about the attribute we`re after.
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.
done