Skip to content

fix(data_provider): correct fetch_grant matching for FUTURE and integration grants - #66

Merged
noel merged 3 commits into
datacoves:mainfrom
usbrandon:fix/grant-fetch-phantom-creates
Aug 24, 2026
Merged

fix(data_provider): correct fetch_grant matching for FUTURE and integration grants#66
noel merged 3 commits into
datacoves:mainfrom
usbrandon:fix/grant-fetch-phantom-creates

Conversation

@usbrandon

Copy link
Copy Markdown
Contributor

Problem

Two grant-matching bugs in fetch_grant cause phantom creates during plan/apply: grants that exist remotely are reported missing, so snowcap re-creates (or in sync mode, mishandles) them on every run.

  1. ALL + FUTURE grants query the wrong SHOW command. A FUTURE grant (e.g. GRANT ALL ON FUTURE SCHEMAS IN DATABASE) was looked up via SHOW GRANTS TO ROLE instead of SHOW FUTURE GRANTS, so it never matched its remote counterpart.

  2. Multi-word on_type never matches SHOW GRANTS output. Snowflake reports granted_on='CATALOG INTEGRATION' / 'STORAGE INTEGRATION' (and collapses all *-integration types to INTEGRATION in some query forms), while the URN uses catalog_integration. The underscore form was compared verbatim, so integration grants never matched.

Note: a third fix originally on this branch (drop uncovered object grants during grant sync) was dropped — PR #58 already restructured that logic and added equivalent regression coverage (test_uncovered_object_grants_are_still_dropped).

Fix

  • Normalize on_type to Snowflake's SHOW GRANTS form (upper().replace('_', ' ')) before filtering.
  • Route FUTURE grants through _show_future_grants_to_role / _show_future_grants_to_database_role.
  • Map integration types to the collapsed INTEGRATION label where Snowflake requires it.

Tests

  • 16 new TestFetchGrant cases: ALL+FUTURE schemas, catalog/storage integration matching, and regression checks that ACCOUNT and SCHEMA grants still match.
  • Full suite green: 2059 passed; ruff, black, and mypy clean per Makefile targets.

…n grants

Live verification showed Snowflake's SHOW GRANTS collapses all *-integration
types to granted_on='INTEGRATION' (not the spaced label), while
GIT_REPOSITORY keeps its underscore — the initial space-normalization fix
was wrong for both. fetch_grant now keeps a separate query form (bare
'INTEGRATION' for *_integration labels, uppercased label otherwise) from
the canonical spaced form returned for diffing. Tests updated to assert
the real live forms; all four cases verified matching against the live
account.
@noel

noel commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Superpowers code review + full Ponytail review

Verdict: changes required before merge.

Superpowers — Important

  • snowcap/data_provider.py:893, :923, and :2412: FUTURE grant container inference uses name.split("."). This is not quote-aware, so a valid grant such as ALL ON FUTURE SCHEMAS IN DATABASE "DB.WITH.DOT" is misclassified and fetch_grant returns None, preserving the phantom create this PR is intended to fix. snowcap/parse.py:736 has the same raw-split limitation when parsing returned collection state.

    Required fix: use a shared quote-aware collection parser for future-grant normalization, container inference, and returned-state parsing. Add database- and schema-container regression tests using quoted identifiers containing dots.

Superpowers — Required test coverage

  • tests/test_data_provider.py:2806: cover the ALL + FUTURE database-role branch at snowcap/data_provider.py:2413.
  • tests/test_data_provider.py:2830: add an ALL integration or Git-repository case exercising the direct _filter_result branch.

The targeted tests/test_data_provider.py suite passed: 239 tests. Hosted CI is green, but it does not cover these cases.

Ponytail — Required simplifications

  • tests/test_data_provider.py:2780: shrink: the two new grant-row factories duplicate the existing _grant_to_role_row fixture. Reuse it with keyword overrides.
  • tests/test_data_provider.py:2829: shrink: five structurally identical object-type round-trip tests repeat setup, patching, execution, and assertions. Parameterize privilege, encoded type, reported type, name, and expected canonical type while retaining every regression case.
  • net: -60 lines possible.

Please address both the correctness and Ponytail findings before merge.

Address PR review:
- FUTURE grant container inference and parse_collection_string now use
  smart_split, so quoted identifiers containing dots
  ('"DB.WITH.DOT".<SCHEMA>') classify correctly instead of
  mis-parsing and preserving the phantom create.
- Cover the ALL+FUTURE database-role branch and the ALL-on-integration
  _filter_result branch.
- Reuse the _grant_to_role_row fixture and parameterize the five
  object-type round-trip tests.
- Regression tests for quoted identifiers at both database and schema
  container level, and for parse_collection_string.
@usbrandon

Copy link
Copy Markdown
Contributor Author

All findings addressed in 352d54f.

Quote-aware parsing (Important): Verified — the raw split(".") at data_provider.py:893/:923 and parse.py:736 misclassified quoted identifiers containing dots. All three now use the existing smart_split from identifiers.py (pyparsing-based, respects double-quoted identifiers). Regression coverage added at both container levels: test_show_future_grants_container_inference_is_quote_aware ('"DB.WITH.DOT".<SCHEMA>' → DATABASE, '"DB.WITH.DOT"."SCH.WITH.DOT".<TABLE>' → SCHEMA) and test_parse_quoted_identifier_containing_dots for parse_collection_string.

Test coverage: Added test_fetch_grant_all_future_to_database_role covering the database-role branch at data_provider.py:2413 (asserts _show_future_grants_to_database_role is called and the role variants are not), and test_fetch_grant_all_on_integration exercising the ALL + direct _filter_result branch with the collapsed INTEGRATION label.

Ponytail simplifications: Dropped both new row factories — _future_grant_row now builds on the existing _grant_to_role_row with keyword overrides, and the five object-type round-trip tests are one parameterized test (test_fetch_grant_object_type_round_trip) retaining every regression case plus the git-repository case.

Full suite green: 2063 passed. ruff, black, and mypy clean per the Makefile targets.

@noel noel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superpowers re-review + Ponytail

Verdict: approved.

Superpowers

No Critical or Important findings. The current head (352d54f) addresses the prior review:

  • routes ALL + FUTURE through the correct role/database-role SHOW helpers
  • keeps Snowflake query labels separate from canonical returned object types
  • uses quote-aware splitting for FUTURE container inference and collection parsing
  • covers the missing database-role and integration branches

Fresh focused verification: 363 passed (tests/test_data_provider.py, tests/test_parse.py, tests/test_identifiers.py). Hosted Python 3.10/3.11/3.12 and security checks are also green.

Minor, non-blocking

  • tests/test_data_provider.py:2882: quote-aware inference directly exercises the role helper but not the duplicated database-role helper. Both production helpers contain the same change; parameterize this test over both only if branch-specific regression coverage becomes necessary.

Ponytail

Lean already. Ship.

@noel
noel merged commit 1ca5554 into datacoves:main Aug 24, 2026
6 checks passed
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.

2 participants