Skip to content

fix(grant): stop phantom CREATE for database-role-to-database-role grants - #75

Merged
noel merged 2 commits into
mainfrom
fix/database-role-grant-qualification
Sep 9, 2026
Merged

noel merged 2 commits into
mainfrom
fix/database-role-grant-qualification

Conversation

@noel

@noel noel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

snowcap plan reported a phantom + CREATE for every database-role-to-database-role grant on every run, because Snowflake reports a DATABASE_ROLE grantee unqualified when it's in the same database as the granting role, while the manifest side always builds a fully qualified DB.ROLE string for comparison. Root cause and repro are in the original bug report; see commit message for the fix breakdown.

  • fetch_database_role_grant and both list_database_role_grants paths (SHOW fallback and ACCOUNT_USAGE) now qualify-then-compare grantee names via a shared _database_role_grantee_fqn() helper instead of comparing raw strings.
  • _fetch_grants_from_account_usage now normalizes GRANTED_ON the same way it already normalizes GRANTED_TO (DATABASE_ROLE -> DATABASE ROLE), fixing a dead filter that made the ACCOUNT_USAGE path for list_database_role_grants always return zero results, plus a dead qualification branch that left NAME unqualified.

Test plan

  • Added unit tests covering: same-database unqualified grantee, cross-database qualified grantee, the false-match guard (OTHERDB.ROLE must not match DB.ROLE), the account-role regression case, both list paths individually, AU/SHOW path agreement, and an end-to-end check that the fetched FQN equals the FQN the manifest builds for the same declared grant.
  • Verified the new tests fail against the pre-fix code and pass after.
  • Full non-integration suite passes: python -m pytest tests/ --ignore=tests/integration (2215 passed, 1 skipped, 1 xfailed).
  • Ran ponytail-review (no findings) and code-review (one finding on duplicated spelling-match logic, applied by normalizing at the source instead).

…ants

Snowflake reports a DATABASE_ROLE grantee unqualified when it's in the
same database as the granting role, but the manifest side always
compares against a fully qualified DB.ROLE string. That mismatch made
fetch_database_role_grant and list_database_role_grants never find the
existing grant, so plan re-issued it as a no-op CREATE on every run.

- fetch_database_role_grant and both list_database_role_grants paths
  now qualify-then-compare grantee names via a shared
  _database_role_grantee_fqn() helper instead of comparing raw strings.
- _fetch_grants_from_account_usage now normalizes GRANTED_ON the same
  way it already normalizes GRANTED_TO (DATABASE_ROLE -> DATABASE
  ROLE), fixing a dead filter that made the ACCOUNT_USAGE path for
  list_database_role_grants always return zero results.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review of PR #75

No issues found.

What the change does: snowcap/data_provider.py fixes a phantom-CREATE bug for DatabaseRoleGrant where the grantee is another database role. Snowflake's SHOW GRANTS OF DATABASE ROLE (and the ACCOUNT_USAGE.GRANTS_TO_ROLES equivalent) reports the grantee's grantee_name unqualified when it lives in the same database as the granting role, but the manifest side (database_role_grant_fqn in snowcap/resources/grant.py:867-878) always builds a fully-qualified DB.ROLE string. The previous exact-string match in fetch_database_role_grant (via _filter_result, snowcap/data_provider.py:482-498) never matched in the same-database case, so plan treated an existing grant as absent and re-issued a no-op CREATE every run.

Fix: new helper _database_role_grantee_fqn (snowcap/data_provider.py:2112-2125) parses the grantee name as an FQN and fills in the granting role's own database only when the parsed name has none, making both the unqualified (same-db) and qualified (cross-db) forms comparable to the manifest's always-qualified form. It's applied consistently across all three producers: fetch_database_role_grant (2128-2161), and both branches of list_database_role_grants — the ACCOUNT_USAGE path (4308-4322) and the SHOW-fallback path (4351-4365). The granted_on tuple→equality change at 1524-1531 is a no-op cosmetic simplification.

Verification:

  • Traced that fqn.database/db_name/db passed as default_database in each of the three call sites is consistently the granting role's own database, matching the semantics documented in _database_role_grantee_fqn's docstring.
  • Confirmed FQN.__eq__ (snowcap/identifiers.py:36-45) and ResourceName.__eq__ (snowcap/resource_name.py:64-78) give the needed case-insensitive/quoted-aware comparison for the parsed grantee vs. the manifest's target FQN.
  • New tests in tests/test_data_provider.py (TestDatabaseRoleGrantToDatabaseRole) cover: same-db unqualified grantee, cross-db qualified grantee, a same-name-different-db non-match (regression guard against a false-positive match on bare role name), the pre-existing to_role path (no regression), both list paths individually, agreement between the two list paths for the same underlying grant, and an end-to-end check that the fetched FQN equals the manifest-declared FQN for the reported symptom.
  • Ran the full tests/test_data_provider.py suite (304 tests) and black --check locally against the changed files; all pass.

No correctness, security, or architecture-fit concerns found in this diff.

…base param

FQN.database is Optional[ResourceName]; the helper's parameter was typed
as the non-optional ResourceName, which mypy flagged at both call sites
in fetch_database_role_grant where fqn.database is passed straight
through.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review of PR #75

No issues found. The change is a narrowly-scoped, well-tested bug fix.

What it does: snowcap/data_provider.py fixes a phantom-CREATE bug for DATABASE_ROLE-to-DATABASE_ROLE grants. Snowflake's SHOW GRANTS OF DATABASE ROLE reports the grantee unqualified when it lives in the same database as the granting role, but the manifest side (database_role_grant_fqn in snowcap/resources/grant.py:867) always builds a fully-qualified DB.ROLE string. Comparing the two directly never matched, so plan kept re-issuing a no-op CREATE. The new helper _database_role_grantee_fqn (snowcap/data_provider.py:2112) parses the grantee name and fills in the granting role's database only when the parsed FQN has no database, making both forms comparable. It's applied consistently across the three read paths: fetch_database_role_grant (data_provider.py:2128), the SHOW-based list_database_role_grants loop (data_provider.py:4347), and the ACCOUNT_USAGE-based path (data_provider.py:4292). A separate one-line fix normalizes ACCOUNT_USAGE's GRANTED_ON = 'DATABASE_ROLE' (underscore) to 'DATABASE ROLE' (space) to match SHOW GRANTS output (data_provider.py:1528), mirroring the existing granted_to normalization already in the same function.

Correctness check: verified FQN.__eq__/ResourceName equality semantics used for the target comparison at data_provider.py:2138, confirmed to_database_role's DatabaseRole field always carries a database (required constructor arg / DatabaseScope), so database_role_grant_fqn's output is reliably fully qualified and a safe basis for comparison. No unsafe string interpolation into generated SQL was introduced, and no change to grant/privilege semantics — this only affects how remote state is matched to the manifest, not what gets granted.

Tests: tests/test_data_provider.py adds TestDatabaseRoleGrantToDatabaseRole with 8 cases covering same-database unqualified grantees, cross-database qualified grantees, a same-role-name-different-database false-positive guard, an account-role regression check, both list paths (SHOW and ACCOUNT_USAGE), path-parity between the two, and an end-to-end manifest-vs-fetched FQN equality test that reproduces the originally reported symptom. Good coverage, no gaps noted.

@noel
noel merged commit 3ad1aef into main Sep 9, 2026
6 checks passed
@noel
noel deleted the fix/database-role-grant-qualification branch September 9, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant