CT-808 more grant adapter tests #5452

gshank · 2022-07-08T02:32:40Z

resolves #5447

Description

More adapter tests

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

gshank · 2022-07-08T02:33:47Z

I roughed in some tests but mostly they just verify that things don't die. Didn't have time to do more specific grant checks, but wanted to get things going for someone else to work on.

McKnight-42 · 2022-07-08T06:41:23Z

Thank you Gerda this looks great!

gshank · 2022-07-08T13:36:41Z

tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py

+        self.assert_expected_grants_match_actual(project, "my_snapshot", expected)
+
+        # run it again, nothing should have changed
+        (results, log_output) = run_dbt_and_capture(["snapshot"])


I think you might need to use "--debug" for the log lines that would normally contain the select and revoke statements to have a chance to actually be in the log.

gshank · 2022-07-08T13:37:11Z

tests/adapter/dbt/tests/adapter/grants/test_seed_grants.py

+        self.assert_expected_grants_match_actual(project, "my_seed", expected)
+
+        # run it again, nothing should have changed
+        (results, log_output) = run_dbt_and_capture(["seed"])


Do we need to use "--debug" here to capture the right log output?

gshank · 2022-07-08T13:38:22Z

tests/adapter/dbt/tests/adapter/grants/test_incremental_grants.py

+        assert len(test_users) == 3
+
+        # Incremental materialization, single select grant
+        write_file(incremental_model_schema_yml, project.project_root, "models", "schema.yml")


This line isn't needed, since the "models" fixture will write the file out and it hasn't changed.

gshank · 2022-07-08T13:40:02Z

tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py

+
+    def test_nonexistent_grantee(self, project, get_test_users, logs_dir):
+        # failure when grant to a user/role that doesn't exist
+        write_file(


Line isn't needed since file has been written out from models fixture.

tests/adapter/dbt/tests/adapter/grants/test_model_grants.py

core/dbt/context/base.py

ChenyuLInx

Looks great over all, have some minor questions

core/dbt/context/base.py

ChenyuLInx · 2022-07-08T23:35:37Z

core/dbt/include/global_project/macros/adapters/apply_grants.sql

@@ -34,54 +34,76 @@
 {% endmacro %}

 {%- macro default__get_grant_sql(relation, grant_config) -%}
-    {%- for privilege in grant_config.keys() -%}


Doesn't have to adjust in this PR, but have we considered move all these kind of logic to python vs do them in Jinja

I was thinking about this yesterday. Both get_grant_sql and get_revoke_sql now have the same boilerplate, which is unpacking the grant config and looping over them. The piece of this that really wants to be in Jinja is really more like:

{% macro get_grant_sql() %} grant {{ privilege }} on {{ relation }} to {{ grantee }} {% endmacro %}

I could see refactoring to call this simpler macro instead, and move the unpacking/looping/array-building logic elsewhere.

For what it's worth, most of the heavier lifting is really happening in apply_grants. We could reimplemented some pieces of it in Python. Keeping the top-level macro call in Jinja (=user space) does allow people to override its behavior for themselves, though.

I acted on this feedback in c763601, by moving the grant_config unpacking / looping logic into their own separate reusable macro, so that get_grant_sql + get_revoke_sql can be dead simple

@jtcohen6 solid move! I also like @ChenyuLInx's notion of putting more looping logic into Python to make the Jinja simpler.

Totally. It can be fairly tricky today to call macros from within Python models (adapter.execute_macro is the sort-of state-of-art), so when a macro needs to call other macros (as this one does), Jinja can still be easier. But if we can split out a pure function, as we did with diff_of_two_dicts and standardize_grants_dict, Python is wayyyy better for code clarity and to unit testing.

dbeatty10

👍
This modularization turned out nicely!

@dataders note that this PR introduces several bits of functionality that can be overridden by adapters.

dbeatty10 · 2022-07-11T13:36:05Z

core/dbt/context/base.py

+        dict_b_lowered = {k.casefold(): [x.casefold() for x in v] for k, v in dict_b.items()}
        for k in dict_a:
-            if k in dict_b:
-                a_lowered = map(lambda x: x.lower(), dict_a[k])
-                b_lowered = map(lambda x: x.lower(), dict_b[k])
-                diff = list(set(a_lowered) - set(b_lowered))
+            if k.casefold() in dict_b_lowered.keys():
+                diff = []
+                for v in dict_a[k]:
+                    if v.casefold() not in dict_b_lowered[k.casefold()]:
+                        diff.append(v)


casefold, FTW 💪

oh wow thats super cool haven't heard of casefold before.

@nathaniel-may is going to take a look at refactoring this for performance / Pythonic-ness. Not going to consider it a blocker for merging this PR in the meantime.

dbeatty10 · 2022-07-11T13:38:55Z

tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py

+    # Adapters will need to reimplement these methods with the specific
    # language of their database
    def grantee_does_not_exist_error(self):
        return "does not exist"

    def privilege_does_not_exist_error(self):
        return "unrecognized privilege"


@dataders More methods to implement.

can we add a doc string here to say what they're supposed to do? or examples? I'm happy to do this, but i"m not sure of their functionality right now..

Yup, there's a comment to this effect above:

dbt-core/tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py

Lines 34 to 37 in debc867

# The purpose of this test is to understand the user experience when providing

# an invalid 'grants' configuration. dbt will *not* try to intercept or interpret

# the database's own error at runtime -- it will just return those error messages.

# Hopefully they're helpful!

Is this a useful test? It's checking for specific language in the DatabaseError that dbt will be passing back. That may be annoying, as an adapter maintainer: this test is basically guaranteed to fail, until/unless you reimplement grantee_does_not_exist_error + privilege_does_not_exist_error with language from your own database's error message. But I do like that, as dbt maintainers, we're thinking harder about the UX for these common error cases than we have in the past. "dbt is just returning the error from the database" is only as useful as the actual error from the actual database. We do call them "the thorniest errors of all" for good reason.

dbeatty10 · 2022-07-11T13:42:00Z

tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py

@@ -13,7 +13,7 @@
        check_cols='all', unique_key='id', strategy='check',
        target_database=database, target_schema=schema
    ) }}
-    select 1 as id, 'blue' as color
+    select 1 as id, cast('blue' as {{ type_string() }}) as color


💚 from the Redshift adapter

McKnight-42

This is all looking fantastic! LGTM

* init push or ct-660 work * changes to default versions of get_show_grant_sql and get_grant_sql * completing init default versions of all macros being called for look over and collaboration * minor update to should_revoke * post pairing push up (does have log statements to make sure we remove) * minor spacing changes * minor changes, and removal of logs so people can have clean grab of code * minor changes to how get_revoke_sql works * init attempt at applying apply_grants to all materialzations * name change from recipents -> grantee * minor changes * working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log * removing logs from most materializations to better track diff of grants generation logs * starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching * 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql * changes to loop cases * changes after pairing meeting * adding apply_grants to create_or_replace_view.sql * models are building but testing out small issues around revoke statement never building * postgrest must fixes from jeremy's feedback * postgres minor change to standarize_grants_dict * updating after pairing with dough and jeremey incorporating the new version of should revoke logic. * adding ref of diff_of_two_dicts to base keys ref * change of method type for standardize_grants_dict * minor update trying to fix unit test * changes based on morning feedback * change log message in default_apply_grants macro * CT-808 grant adapter tests (#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * rename grant[privilege] -> grant_config[privilege] * postgres macro rename to copy_grants * CT-808 more grant adapter tests (#5452) * Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> * update to main, create changelog, whitespace fixes Co-authored-by: Gerda Shank <gerda@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* init push or ct-660 work * changes to default versions of get_show_grant_sql and get_grant_sql * completing init default versions of all macros being called for look over and collaboration * minor update to should_revoke * post pairing push up (does have log statements to make sure we remove) * minor spacing changes * minor changes, and removal of logs so people can have clean grab of code * minor changes to how get_revoke_sql works * init attempt at applying apply_grants to all materialzations * name change from recipents -> grantee * minor changes * working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log * removing logs from most materializations to better track diff of grants generation logs * starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching * 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql * changes to loop cases * changes after pairing meeting * adding apply_grants to create_or_replace_view.sql * models are building but testing out small issues around revoke statement never building * postgrest must fixes from jeremy's feedback * postgres minor change to standarize_grants_dict * updating after pairing with dough and jeremey incorporating the new version of should revoke logic. * adding ref of diff_of_two_dicts to base keys ref * change of method type for standardize_grants_dict * minor update trying to fix unit test * changes based on morning feedback * change log message in default_apply_grants macro * CT-808 grant adapter tests (dbt-labs#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * rename grant[privilege] -> grant_config[privilege] * postgres macro rename to copy_grants * CT-808 more grant adapter tests (dbt-labs#5452) * Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> * update to main, create changelog, whitespace fixes Co-authored-by: Gerda Shank <gerda@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

gshank added 2 commits July 7, 2022 21:50

Add tests for invalid user and invalid privilege

1486367

Add more grant tests

e9899db

gshank added the Skip Changelog Skips GHA to check for changelog file label Jul 8, 2022

gshank requested a review from a team as a code owner July 8, 2022 02:32

gshank requested review from emmyoop and McKnight-42 July 8, 2022 02:32

cla-bot bot added the cla:yes label Jul 8, 2022

gshank requested a review from jtcohen6 July 8, 2022 02:34

jtcohen6 added 2 commits July 8, 2022 14:56

Macro touch ups

50b26d8

Many more tests

16b0333

gshank requested a review from a team July 8, 2022 12:57

gshank requested a review from a team as a code owner July 8, 2022 12:57

gshank requested a review from ChenyuLInx July 8, 2022 12:57

gshank commented Jul 8, 2022

View reviewed changes

jtcohen6 added 9 commits July 8, 2022 16:49

Allow easily replacing privilege names

03545a7

Keep adding tests

3fb963c

Refactor macros to return lists, fix test

89eabc3

Code checks

1a783f7

Keep tweaking tests

13f3ba6

Merge branch 'ct-660-grant-sql' into ct-808-more_grant_adapter_tests

c9b0ec8

Revert cool grantees join bc Snowflake isnt happy

64afa6b

Use Postgres/BQ as standard for standardize_grants_dict

03078ec

Code checks

39c704b

emmyoop reviewed Jul 8, 2022

View reviewed changes

tests/adapter/dbt/tests/adapter/grants/test_model_grants.py Outdated Show resolved Hide resolved

add missing replace

27b1afd

small replace tweak, add additional dict diffs

6557c11

gshank requested a review from a team as a code owner July 8, 2022 20:58

emmyoop reviewed Jul 8, 2022

View reviewed changes

core/dbt/context/base.py Outdated Show resolved Hide resolved

ChenyuLInx reviewed Jul 8, 2022

View reviewed changes

dbeatty10 mentioned this pull request Jul 8, 2022

[CT-789] Add Grants to Materializations #212 dbt-labs/dbt-redshift#131

Merged

6 tasks

jtcohen6 added 3 commits July 11, 2022 00:16

All tests passing on BQ

debc867

Add type cast to test_snapshot_grants

7f499ba

Refactor for DRYer apply_grants macros

c763601

dbeatty10 approved these changes Jul 11, 2022

View reviewed changes

McKnight-42 approved these changes Jul 11, 2022

View reviewed changes

McKnight-42 merged commit 794f4d1 into ct-660-grant-sql Jul 11, 2022

McKnight-42 deleted the ct-808-more_grant_adapter_tests branch July 11, 2022 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CT-808 more grant adapter tests #5452

CT-808 more grant adapter tests #5452

gshank commented Jul 8, 2022

gshank commented Jul 8, 2022

McKnight-42 commented Jul 8, 2022

gshank Jul 8, 2022

gshank Jul 8, 2022

gshank Jul 8, 2022

gshank Jul 8, 2022

ChenyuLInx left a comment

ChenyuLInx Jul 8, 2022

jtcohen6 Jul 9, 2022 •

edited

Loading

jtcohen6 Jul 11, 2022

dataders Jul 11, 2022

jtcohen6 Jul 11, 2022

dbeatty10 left a comment

dbeatty10 Jul 11, 2022

McKnight-42 Jul 11, 2022

jtcohen6 Jul 11, 2022

dbeatty10 Jul 11, 2022

dataders Jul 11, 2022

jtcohen6 Jul 11, 2022 •

edited

Loading

dbeatty10 Jul 11, 2022

McKnight-42 left a comment

	# The purpose of this test is to understand the user experience when providing
	# an invalid 'grants' configuration. dbt will not try to intercept or interpret
	# the database's own error at runtime -- it will just return those error messages.
	# Hopefully they're helpful!

CT-808 more grant adapter tests #5452

CT-808 more grant adapter tests #5452

Conversation

gshank commented Jul 8, 2022

Description

Checklist

gshank commented Jul 8, 2022

McKnight-42 commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbeatty10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McKnight-42 left a comment

Choose a reason for hiding this comment

jtcohen6 Jul 9, 2022 •

edited

Loading

jtcohen6 Jul 11, 2022 •

edited

Loading