Refactor _grants_collect_schemas macro to accept schema_names and is_…#23
Open
jonhopper-dataengineers wants to merge 4 commits intomainfrom
Open
Refactor _grants_collect_schemas macro to accept schema_names and is_…#23jonhopper-dataengineers wants to merge 4 commits intomainfrom
jonhopper-dataengineers wants to merge 4 commits intomainfrom
Conversation
…exclude_list parameters; update related grant macros for consistency and improved functionality
…and add grant_external_share_read macro for schema permissions
Contributor
Reviewer's GuideRefactors the core schema collection helper to support both include- and exclude-style schema lists and wires existing grant macros to the new interface, while adding a new macro to grant external share read access and bumping the project version and changelog. Sequence diagram for new external share read grant macro using schema include listsequenceDiagram
participant Caller
participant grant_internal_share_read_external as grant_internal_share_read
participant grants_collect_schemas as _grants_collect_schemas
participant Snowflake as Snowflake_DB
Caller->>grant_internal_share_read_external: invoke(share_name, include_schemas, dry_run)
grant_internal_share_read_external->>grant_internal_share_read_external: normalize include_schemas to list
alt no schemas provided
grant_internal_share_read_external->>Caller: return no schemas to grant
else schemas provided
grant_internal_share_read_external->>grants_collect_schemas: _grants_collect_schemas(include_schemas, false)
grants_collect_schemas->>Snowflake: show schemas in database
Snowflake-->>grants_collect_schemas: result set of schemas
grants_collect_schemas->>grants_collect_schemas: filter schemas by include list
grants_collect_schemas-->>grant_internal_share_read_external: list of valid schemas
loop for each schema
alt dry_run is true
grant_internal_share_read_external->>Caller: log grant usage and select statements
else dry_run is false
grant_internal_share_read_external->>Snowflake: grant usage on schema to share
grant_internal_share_read_external->>Snowflake: grant select on all tables in schema
grant_internal_share_read_external->>Snowflake: show views in schema
Snowflake-->>grant_internal_share_read_external: result set of views
loop for each view
grant_internal_share_read_external->>Snowflake: grant select on view to share
end
end
end
grant_internal_share_read_external-->>Caller: finished granting
end
Class diagram for updated grant macros and schema collection helperclassDiagram
class grants_collect_schemas {
<<macro>>
_grants_collect_schemas(schema_names, is_exclude_list=true)
+list include_schemas
}
class grant_internal_share_read_external {
<<macro>>
grant_internal_share_read(share_name, include_schemas, dry_run=false)
}
class grant_internal_share_read_existing {
<<macro>>
grant_internal_share_read(share_name, exclude_schemas, dry_run=false)
}
class grant_procedure_usage {
<<macro>>
grant_schema_procedure_usage(exclude_schemas, grant_roles)
}
class grant_schema_monitor {
<<macro>>
grant_schema_monitor(exclude_schemas, grant_roles)
}
class grant_schema_operate {
<<macro>>
grant_schema_operate(exclude_schemas, grant_roles)
}
class grant_schema_ownership {
<<macro>>
grant_schema_ownership(exclude_schemas, grant_roles)
}
class grant_schema_read {
<<macro>>
grant_schema_read(exclude_schemas, grant_roles)
}
class grant_semantic_views_privileges {
<<macro>>
grant_semantic_views_privileges(exclude_schemas, grant_roles)
}
grants_collect_schemas <.. grant_internal_share_read_existing : uses
grants_collect_schemas <.. grant_internal_share_read_external : uses
grants_collect_schemas <.. grant_procedure_usage : uses
grants_collect_schemas <.. grant_schema_monitor : uses
grants_collect_schemas <.. grant_schema_operate : uses
grants_collect_schemas <.. grant_schema_ownership : uses
grants_collect_schemas <.. grant_schema_read : uses
grants_collect_schemas <.. grant_semantic_views_privileges : uses
grant_internal_share_read_existing : +share_name
grant_internal_share_read_existing : +exclude_schemas
grant_internal_share_read_existing : +dry_run
grant_internal_share_read_external : +share_name
grant_internal_share_read_external : +include_schemas
grant_internal_share_read_external : +dry_run
grants_collect_schemas : +schema_names
grants_collect_schemas : +is_exclude_list
Flow diagram for refactored _grants_collect_schemas macro behaviorflowchart TD
A["start _grants_collect_schemas(schema_names, is_exclude_list)"] --> B{schema_names is iterable}
B -- no --> C[set schema_names to empty list]
B -- yes --> D[keep schema_names as provided]
C --> E{is_exclude_list is true and INFORMATION_SCHEMA not in schema_names}
D --> E
E -- yes --> F[append INFORMATION_SCHEMA to schema_names]
E -- no --> G[do not modify schema_names]
F --> H[run query: show schemas in target.database]
G --> H
H --> I{execute and results present}
I -- no --> J[return empty include_schemas]
I -- yes --> K[loop over each row in results]
K --> L{row.name already in include_schemas}
L -- yes --> K
L -- no --> M{is_exclude_list is true}
M -- yes --> N{row.name not in schema_names}
N -- yes --> O[append row.name to include_schemas]
N -- no --> K
M -- no --> P{row.name in schema_names}
P -- yes --> O
P -- no --> K
O --> K
K --> Q[loop complete]
Q --> R[return include_schemas]
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 3 issues, and left some high level feedback:
- The new
macros/grants/grant_external_share_read.sqlfile defines a macro namedgrant_internal_share_read, which both mismatches the file/feature intent (external share) and collides with the existing internal-share macro name; consider renaming the macro to something likegrant_external_share_readand ensuring no name collisions. - Several callers (e.g.
grant_schema_read,grant_schema_monitor, etc.) still manually appendINFORMATION_SCHEMAtoexclude_schemaseven though_grants_collect_schemasnow enforces this whenis_exclude_list=true; you could simplify those call sites by removing the redundantINFORMATION_SCHEMAhandling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `macros/grants/grant_external_share_read.sql` file defines a macro named `grant_internal_share_read`, which both mismatches the file/feature intent (external share) and collides with the existing internal-share macro name; consider renaming the macro to something like `grant_external_share_read` and ensuring no name collisions.
- Several callers (e.g. `grant_schema_read`, `grant_schema_monitor`, etc.) still manually append `INFORMATION_SCHEMA` to `exclude_schemas` even though `_grants_collect_schemas` now enforces this when `is_exclude_list=true`; you could simplify those call sites by removing the redundant `INFORMATION_SCHEMA` handling.
## Individual Comments
### Comment 1
<location path="macros/grants/grant_external_share_read.sql" line_range="1" />
<code_context>
+{% macro grant_internal_share_read(share_name, include_schemas, dry_run=false) %}
+ {% if flags.WHICH in ['run', 'run-operation'] %}
+ {% if execute %}
</code_context>
<issue_to_address>
**issue (bug_risk):** Macro name and file name are mismatched and appear to duplicate an existing macro, which is likely to override the original implementation.
This file is named `grant_external_share_read.sql` but defines `grant_internal_share_read`, which already exists in `macros/grants/grant_internal_share_read.sql`. In dbt the last definition wins, so this will silently override the original. Please either rename this macro (e.g. to `grant_external_share_read`) or consolidate the logic to avoid unexpected overrides and ambiguity about which implementation is used.
</issue_to_address>
### Comment 2
<location path="macros/grants/_helpers.sql" line_range="10-19" />
<code_context>
#}
-{% macro _grants_collect_schemas(exclude_schemas) %}
+{% macro _grants_collect_schemas(schema_names, is_exclude_list=true) %}
{% set include_schemas = [] %}
- {% if exclude_schemas is not iterable %}
</code_context>
<issue_to_address>
**suggestion:** Callers and helper both manage `INFORMATION_SCHEMA` in exclude mode, leading to duplicated responsibility.
The new `is_exclude_list` flag makes `_grants_collect_schemas` append `INFORMATION_SCHEMA` for exclude lists, but some callers (e.g. `grant_*` macros) still append it before calling `_grants_collect_schemas(..., is_exclude_list=true)`. Even though double-insertion is avoided, the responsibility for excluding `INFORMATION_SCHEMA` is now split. It would be clearer to handle this in a single place (either only in `_grants_collect_schemas` or only in the callers).
Suggested implementation:
```
These ideas intentionally deferred to keep current refactor incremental.
#}
-- NOTE:
-- Responsibility for excluding INFORMATION_SCHEMA is centralized here.
-- Callers MUST NOT add "INFORMATION_SCHEMA" to schema_names themselves when
-- using exclude-mode; instead, pass only the user-specified schemas and
-- rely on this helper to append INFORMATION_SCHEMA when is_exclude_list is true.
{% macro _grants_collect_schemas(schema_names, is_exclude_list=true) %}
{% set include_schemas = [] %}
{% if schema_names is not iterable %}
{% set schema_names = [] %}
{% endif %}
{% if is_exclude_list and "INFORMATION_SCHEMA" not in schema_names %}
{% do schema_names.append("INFORMATION_SCHEMA") %}
{% endif %}
{% set query %}
```
To fully implement the suggestion and avoid duplicated responsibility:
1. Find all callers of `_grants_collect_schemas` (for example, in `grant_*` macros in the grants-related SQL macro files).
2. In exclude-mode callers (where `is_exclude_list` is `true` or omitted to use the default), remove any manual appending of `"INFORMATION_SCHEMA"` to the schema list before calling `_grants_collect_schemas`.
- e.g. change patterns like:
- `{% set exclude_schemas = exclude_schemas + ["INFORMATION_SCHEMA"] %}`
- or `{% do exclude_schemas.append("INFORMATION_SCHEMA") %}`
before `_grants_collect_schemas(exclude_schemas, is_exclude_list=true)`
so that `"INFORMATION_SCHEMA"` is no longer added explicitly.
3. Ensure include-mode callers (where `is_exclude_list` is `false`) do not rely on this helper to manage `"INFORMATION_SCHEMA"`; they should pass the exact include list they want.
</issue_to_address>
### Comment 3
<location path="CHANGELOG.md" line_range="6" />
<code_context>
+## v0.3.11 - 2026-03-24 - External Share Permissions
+
+- refactor the _grants_collect_schemas macro to accept schema_names and is_exclude_list parameters, and updates related grant macros for consistency and improved functionality.
+- added new macro `grant_external_share_read` to apply `select` permissions on all tables and views for specified schemas
+
</code_context>
<issue_to_address>
**suggestion (typo):** Align verb forms in this sentence for correct grammar.
The phrase "refactor ... and updates" mixes verb forms. Please use a consistent tense, e.g., "refactors ... and updates" or "refactored ... and updated."
```suggestion
- refactored the _grants_collect_schemas macro to accept schema_names and is_exclude_list parameters, and updated related grant macros for consistency and improved functionality.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,57 @@ | |||
| {% macro grant_internal_share_read(share_name, include_schemas, dry_run=false) %} | |||
Contributor
There was a problem hiding this comment.
issue (bug_risk): Macro name and file name are mismatched and appear to duplicate an existing macro, which is likely to override the original implementation.
This file is named grant_external_share_read.sql but defines grant_internal_share_read, which already exists in macros/grants/grant_internal_share_read.sql. In dbt the last definition wins, so this will silently override the original. Please either rename this macro (e.g. to grant_external_share_read) or consolidate the logic to avoid unexpected overrides and ambiguity about which implementation is used.
Comment on lines
+10
to
19
| {% macro _grants_collect_schemas(schema_names, is_exclude_list=true) %} | ||
| {% set include_schemas = [] %} | ||
| {% if exclude_schemas is not iterable %} | ||
| {% set exclude_schemas = [] %} | ||
| {% if schema_names is not iterable %} | ||
| {% set schema_names = [] %} | ||
| {% endif %} | ||
| {% if "INFORMATION_SCHEMA" not in exclude_schemas %} | ||
| {% do exclude_schemas.append("INFORMATION_SCHEMA") %} | ||
| {% if "INFORMATION_SCHEMA" not in schema_names and is_exclude_list %} | ||
| {% do schema_names.append("INFORMATION_SCHEMA") %} | ||
| {% endif %} | ||
| {% set query %} | ||
| show schemas in database {{ target.database }}; |
Contributor
There was a problem hiding this comment.
suggestion: Callers and helper both manage INFORMATION_SCHEMA in exclude mode, leading to duplicated responsibility.
The new is_exclude_list flag makes _grants_collect_schemas append INFORMATION_SCHEMA for exclude lists, but some callers (e.g. grant_* macros) still append it before calling _grants_collect_schemas(..., is_exclude_list=true). Even though double-insertion is avoided, the responsibility for excluding INFORMATION_SCHEMA is now split. It would be clearer to handle this in a single place (either only in _grants_collect_schemas or only in the callers).
Suggested implementation:
These ideas intentionally deferred to keep current refactor incremental.
#}
-- NOTE:
-- Responsibility for excluding INFORMATION_SCHEMA is centralized here.
-- Callers MUST NOT add "INFORMATION_SCHEMA" to schema_names themselves when
-- using exclude-mode; instead, pass only the user-specified schemas and
-- rely on this helper to append INFORMATION_SCHEMA when is_exclude_list is true.
{% macro _grants_collect_schemas(schema_names, is_exclude_list=true) %}
{% set include_schemas = [] %}
{% if schema_names is not iterable %}
{% set schema_names = [] %}
{% endif %}
{% if is_exclude_list and "INFORMATION_SCHEMA" not in schema_names %}
{% do schema_names.append("INFORMATION_SCHEMA") %}
{% endif %}
{% set query %}
To fully implement the suggestion and avoid duplicated responsibility:
- Find all callers of
_grants_collect_schemas(for example, ingrant_*macros in the grants-related SQL macro files). - In exclude-mode callers (where
is_exclude_lististrueor omitted to use the default), remove any manual appending of"INFORMATION_SCHEMA"to the schema list before calling_grants_collect_schemas.- e.g. change patterns like:
{% set exclude_schemas = exclude_schemas + ["INFORMATION_SCHEMA"] %}- or
{% do exclude_schemas.append("INFORMATION_SCHEMA") %}
before_grants_collect_schemas(exclude_schemas, is_exclude_list=true)
so that"INFORMATION_SCHEMA"is no longer added explicitly.
- e.g. change patterns like:
- Ensure include-mode callers (where
is_exclude_listisfalse) do not rely on this helper to manage"INFORMATION_SCHEMA"; they should pass the exact include list they want.
…atabase_clone macro
…t internal stages inclusion syntax
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.
…exclude_list parameters; update related grant macros for consistency and improved functionality
Pull Request Purpose
mainmainbranchmainbranchDescription
refactor the _grants_collect_schemas macro to accept schema_names and is_exclude_list parameters, and updates related grant macros for consistency and improved functionality.
Notes for reviewer
Checklist
Summary by Sourcery
Refactor schema collection and grant macros to support both include and exclude schema lists and add external share read-grant support.
New Features:
Enhancements:
Documentation: