Add tests for checking partial table privileges on PG #40549
Conversation
qnkhuat
added
the
backport
| @@ -965,7 +965,7 @@ | |||
| ")" | |||
| "select t.*" | |||
| "from table_privileges t"])) | |||
| (filter #(or (:select %) (:update %) (:delete %) (:update %))))) | |||
| (filter #(or (:select %) (:update %) (:delete %) (:insert %))))) | |||
There was a problem hiding this comment.
oopsie, caught a bug!
|
| (is (= (into #{} | ||
| (map #(merge {:role nil | ||
| :schema "dotted.schema" | ||
| :table "dotted.materialized_view" |
There was a problem hiding this comment.
I'd rather exclude this default table name, could cause a lot of head scratching at test output if we forgot to override it.
There was a problem hiding this comment.
Also nit, I think it'd be a little bit more obvious what's going on if you bound this map to something like base-privileges.
There was a problem hiding this comment.
I think it'd be a little bit more obvious what's going on if you bound this map to something like base-privileges
I agree with that if we reuse this function elsewhere, but in this context I like that I could see the default value in a single map opreration
There was a problem hiding this comment.
It took my eyes a second to realise what's going on - checking which way around the merge is happening, then inferring the purpose of the merge. It wasn't helped by the :table key in both though.
Oh one more nit is that you could use (set ...) instead of (into #{} ...).
|
@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
…40555) * Add tests for checking partial table privileges on PG (#40549) * Include postgresql tables that are visible through column grants instead of full table grants for simple questions (#40034) Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> --------- Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Eric Jensen <ericcj@gmail.com>
Follow up from this request change I made in an external PR.
the test is quite similar to what I did in this fix for redshift