Optimize grant macros for performance and case-insensitive role matching#33
Merged
jonhopper-dataengineers merged 2 commits intoJun 25, 2026
Merged
Conversation
…atching Grant macros now check existing state before issuing SQL statements, skipping schemas/objects where grants are already in place. This reduces runtime from 20-30 minutes to under 2-3 minutes on large databases where grants are already applied. Also makes all role comparisons case-insensitive via normalization to uppercase. Adds integration tests for grant helpers and idempotency. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Contributor
Reviewer's GuideOptimizes all major grant macros for Snowflake by introducing bulk state-check helpers, case-insensitive role/application normalization, and grant-only behavior, plus new tests and version bump. Sequence diagram for optimized grant_schema_read_specific flowsequenceDiagram
actor DbtRunner
participant grant_schema_read_specific
participant GrantsHelpers
participant Snowflake
DbtRunner->>grant_schema_read_specific: invoke with schemas, grant_roles
grant_schema_read_specific->>GrantsHelpers: _grants_normalize_roles(grant_roles)
grant_schema_read_specific->>GrantsHelpers: _grants_collect_roles()
loop for each schema
grant_schema_read_specific->>GrantsHelpers: _grants_get_schema_grants(schema, USAGE, ROLE)
grant_schema_read_specific->>GrantsHelpers: _grants_get_schema_object_privs(schema, [SELECT,REFERENCES,REBUILD,READ], grant_roles)
alt include_future_grants
grant_schema_read_specific->>GrantsHelpers: _grants_get_future_grants(schema)
end
alt revoke_current_grants
grant_schema_read_specific->>Snowflake: run_query(revoke_query)
Snowflake-->>grant_schema_read_specific: tbl_privs
end
alt no missing privileges
grant_schema_read_specific->>DbtRunner: log("schema skipped, no changes")
else missing privileges
grant_schema_read_specific->>Snowflake: run_query(grant statements)
end
end
grant_schema_read_specific->>DbtRunner: log(summary with schemas skipped)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
_grants_get_schema_object_privs, the_rolevariable is constructed but never used; consider removing it or using it to make the intent clearer and avoid confusion. - The
grant_schema_object_privilegesmacro’s revoke_query filters only onobject_schemaandprivilege_typebut not onobject_type, which may cause revokes on unintended object types; consider narrowing the query to the specificobject_typebeing processed. - The
schemas_skippedtracking ingrant_schema_read_specificuses a list with a dummy initial element andlength - 1arithmetic; replacing this with a simple integer counter would make the skip-count logic easier to read and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_grants_get_schema_object_privs`, the `_role` variable is constructed but never used; consider removing it or using it to make the intent clearer and avoid confusion.
- The `grant_schema_object_privileges` macro’s revoke_query filters only on `object_schema` and `privilege_type` but not on `object_type`, which may cause revokes on unintended object types; consider narrowing the query to the specific `object_type` being processed.
- The `schemas_skipped` tracking in `grant_schema_read_specific` uses a list with a dummy initial element and `length - 1` arithmetic; replacing this with a simple integer counter would make the skip-count logic easier to read and maintain.
## Individual Comments
### Comment 1
<location path="macros/grants/grant_schema_read.sql" line_range="61-70" />
<code_context>
+ {% set existing_privs = dbt_dataengineers_utils._grants_get_schema_object_privs(schema, ['SELECT', 'REFERENCES', 'REBUILD', 'READ'], grant_roles) %}
</code_context>
<issue_to_address>
**issue (bug_risk):** Existing object privileges are checked per-role only, which may skip granting SELECT on some object types.
`_grants_get_schema_object_privs` aggregates privileges per grantee without distinguishing object types or whether they apply to all objects. This code treats any existing `SELECT` for a role as meaning no further `SELECT` grants are needed, and then skips granting on views, materialized views, tables, external tables, dynamic tables, and streams.
As a result, roles that currently have `SELECT` on only some objects may never receive `SELECT` on the remaining ones, changing behavior from the previous `grant ... on all ...` pattern. If you want to maintain full read coverage while keeping the operation idempotent, consider tracking existing privileges per object type or reverting to unconditional `grant ... on all ...` and relying on Snowflake’s idempotence.
</issue_to_address>
### Comment 2
<location path="macros/grants/grant_schema_object_privileges.sql" line_range="58-67" />
<code_context>
+ {% set existing_privs = dbt_dataengineers_utils._grants_get_schema_object_privs(schema_name, permission_list, role_list) %}
</code_context>
<issue_to_address>
**issue (bug_risk):** The bulk helper ignores `object_type`, so revokes/grants may consider privileges on unrelated objects in the schema.
In `grant_schema_object_privileges`, the bulk path calls `_grants_get_schema_object_privs(schema_name, permission_list, role_list)` and builds a revoke query on `information_schema.object_privileges` filtered only by `object_schema` and `privilege_type`, ignoring the `object_type` argument.
As a result:
- A call with `object_type='TABLE'` may treat privileges on views, streams, etc. as if they applied to tables when deciding which roles already have a privilege.
- The revoke logic may target roles based on privileges they hold on other object types, even though the generated statement is `REVOKE ... ON ALL <object_type>s IN SCHEMA ...`.
To avoid incorrect grant/revoke behavior, the queries should either filter by `object_type` or the bulk helper should be limited to cases where its semantics match the object-type-specific logic.
</issue_to_address>
### Comment 3
<location path="macros/grants/_helpers.sql" line_range="125-134" />
<code_context>
+{% macro _grants_get_schema_object_privs(schema, privilege_types, grantees) %}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unused `_role` variable and mixed casing in the grantee filter can be cleaned up for clarity.
Inside `_grants_get_schema_object_privs`:
- `_role = row[0] ~ '::' ~ row[1]` is computed but never used.
- `priv_filter` is uppercased, but `grantees` is not, even though callers currently pass uppercased roles via `_grants_normalize_roles`.
Please remove `_role`, and either normalize `grantees` here or clearly document/enforce the expectation that callers pass uppercased values (e.g. via a `grantees_upper` parameter name) to avoid subtle casing-related bugs.
Suggested implementation:
```
{# Bulk check: returns dict {role: [privs]} for a given schema from information_schema.object_privileges.
Filters by optional privilege_types list and grantee list. Grantees are normalized to UPPER() to avoid casing issues. #}
{% macro _grants_get_schema_object_privs(schema, privilege_types, grantees) %}
{% set result_map = {} %}
{% set priv_filter = privilege_types | map('upper') | list %}
{% set grantees_upper = grantees | map('upper') | list %}
{% set query %}
select privilege_type, grantee
from information_schema.object_privileges
where object_schema = '{{ schema }}'
{% if priv_filter | length > 0 %}
and privilege_type in ({{ priv_filter | map('tojson') | join(', ') }})
{% endif %}
{% if grantees_upper | length > 0 %}
```
1. Remove the unused `_role = row[0] ~ '::' ~ row[1]` assignment inside `_grants_get_schema_object_privs`; the local `_role` variable is not referenced anywhere and should simply be deleted.
2. Anywhere in `_grants_get_schema_object_privs` that currently uses `grantees` to build the SQL (e.g. an `and grantee in (...)` clause) or to filter results in Python/Jinja should be updated to use `grantees_upper` instead:
- In the SQL, compare against `UPPER(grantee)` or ensure grantees are uppercased consistently with `grantees_upper`.
- In any subsequent logic that checks membership or builds `result_map`, use `grantees_upper` for comparison to ensure casing-safe behavior.
3. If there is helper documentation or comments elsewhere that mention that callers must pass uppercased grantees (e.g. via `_grants_normalize_roles`), update those comments to reflect that `_grants_get_schema_object_privs` now performs its own uppercasing internally.
</issue_to_address>
### Comment 4
<location path="macros/grants/grant_schema_operate.sql" line_range="21-30" />
<code_context>
+{% macro _grants_get_schema_object_privs(schema, privilege_types, grantees) %}
+ {% set result_map = {} %}
+ {% set priv_filter = privilege_types | map('upper') | list %}
+ {% set query %}
+ select privilege_type, grantee
+ from information_schema.object_privileges
</code_context>
<issue_to_address>
**question (bug_risk):** Revoke statements now use bulk `revoke operate on all tasks/pipes` which may be broader than the previous per-object revokes.
Previously, revokes were per object:
```sql
revoke operate on <object_type> in schema <db>.<object> from role <role>;
```
Now, for non-managed roles, you use bulk revokes:
```sql
revoke operate on all tasks in schema ... from role ...;
revoke operate on all pipes in schema ... from role ...;
```
This changes semantics: a role that had OPERATE on only some tasks/pipes will now lose it on *all* tasks/pipes in the schema, including grants not originally managed by this macro.
Please confirm whether the macro is intended to fully own OPERATE on tasks/pipes for the schema. If not, consider reverting to per-object revokes or scoping bulk revokes to objects discovered by a dedicated query to avoid unintentionally removing other grants.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
21
to
30
| {% set query %} | ||
| select object_type, concat(object_schema, '.', object_name) as object_name, privilege_type, grantee | ||
| select privilege_type, grantee | ||
| from information_schema.object_privileges | ||
| where privilege_type = 'OPERATE' and object_schema = '{{ schema }}' | ||
| {% endset %} | ||
| {% set results = run_query(query) %} | ||
| {% if execute and results %} | ||
| {% for row in results %} | ||
| {% set priv = row[2] %}{% set grantee = row[3] %} | ||
| {% set priv = row[0] %}{% set grantee = row[1] %} | ||
| {% if priv == 'OPERATE' %} |
Contributor
There was a problem hiding this comment.
question (bug_risk): Revoke statements now use bulk revoke operate on all tasks/pipes which may be broader than the previous per-object revokes.
Previously, revokes were per object:
revoke operate on <object_type> in schema <db>.<object> from role <role>;Now, for non-managed roles, you use bulk revokes:
revoke operate on all tasks in schema ... from role ...;
revoke operate on all pipes in schema ... from role ...;This changes semantics: a role that had OPERATE on only some tasks/pipes will now lose it on all tasks/pipes in the schema, including grants not originally managed by this macro.
Please confirm whether the macro is intended to fully own OPERATE on tasks/pipes for the schema. If not, consider reverting to per-object revokes or scoping bulk revokes to objects discovered by a dedicated query to avoid unintentionally removing other grants.
…idempotency - _grants_get_schema_object_privs: removed unused _role variable, added internal grantees uppercase normalization, added optional object_type parameter to filter by specific object type - grant_schema_read_specific: removed incorrect per-privilege skip logic that aggregated across object types. GRANT ON ALL is idempotent in Snowflake so always issue it. Only skip schema USAGE (single check) and future grants (which error on duplicates) - grant_schema_object_privileges: now passes object_type to both the helper query and the revoke query to avoid cross-object-type leakage .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Test plan
dbt run-operation test_grants_helpers— validates helper macro logic (no Snowflake connection needed)dbt run-operation test_grants_idempotency— validates helpers return correct types and skip-logic works against live datadbt run-operation grants_smoke_test --vars '{"grants_dry_run": true}'— exercises all major grant macros in dry-run modedbt run-operation grant_privilegeson a test environment and verify reduced statement count in logs.... Generated with Cortex Code
Summary by Sourcery
Optimize Snowflake grant macros for faster, idempotent execution and case-insensitive role handling while making grant_object grant-only.
Enhancements:
Documentation:
Tests:
Chores: