From faf22ccb4f6342503f715bfaa90262b1676704df Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:51:38 -0600 Subject: [PATCH 1/5] feat(py): check warehouse session identity and query access A warehouse decides who may read what, so commons asks with a zero-row query and classifies the failure: an authorization refusal is stable and is cached per relation, a transient one retries on the next touch, and an unrecognized one is neither. The session snapshot pins the principal, roles, and namespace those answers were decided for, and a refusal names which of them moved. Every relation the listing reported is probed before anything is raised, so a name the caller got wrong and a relation they cannot read are reported in one pass rather than one round trip each. The check pairs the selection with the listing by label, which meant the two had to agree on one. They did not for an entry naming a bare table: the warehouse qualifies it from the connection's namespace, so the registry now records the relation's own label rather than the selector's. --- pkg-py/src/commons/_catalog/__init__.py | 30 +- pkg-py/src/commons/_catalog/_core.py | 18 +- pkg-py/src/commons/_catalog/_security.py | 353 +++++++++++++++++++++++ pkg-py/tests/test_catalog.py | 25 ++ pkg-py/tests/test_catalog_security.py | 327 +++++++++++++++++++++ 5 files changed, 745 insertions(+), 8 deletions(-) create mode 100644 pkg-py/src/commons/_catalog/_security.py create mode 100644 pkg-py/tests/test_catalog_security.py diff --git a/pkg-py/src/commons/_catalog/__init__.py b/pkg-py/src/commons/_catalog/__init__.py index 2590ec74..c7f12af4 100644 --- a/pkg-py/src/commons/_catalog/__init__.py +++ b/pkg-py/src/commons/_catalog/__init__.py @@ -4,9 +4,11 @@ types are authoritative, identifier case normalizes per backend, and an ambiguous relative name is an error rather than a guess. -Everything here is a pure function over the rows a warehouse listing returns. -Running the queries that produce those rows belongs to the per-backend -readers, which keeps this testable without a warehouse. +Interpreting a warehouse listing is kept to pure functions over the rows it +returns, and running the queries that produce them belongs to the per-backend +readers, which keeps the interpretation testable without a warehouse. The +session and access checks in `_security` are the exception, since asking the +warehouse is the whole point of them. """ from . import _databricks, _snowflake @@ -23,19 +25,41 @@ search, table_registry, ) +from ._security import ( + CatalogAccessError, + CatalogAuthorizationError, + CatalogSessionChangedError, + CatalogTransientError, + SessionSnapshot, + check_session, + ensure_queryable, + require_queryable, + require_queryable_relations, + session_snapshot, +) __all__ = [ + "CatalogAccessError", + "CatalogAuthorizationError", + "CatalogSessionChangedError", + "CatalogTransientError", "Manifest", "MergedDictionary", "Relation", "Selector", + "SessionSnapshot", "_databricks", "_snowflake", "check_exclude", + "check_session", + "ensure_queryable", "excluded", "id_type", "merge_dictionary", "normalize_identifier", + "require_queryable", + "require_queryable_relations", "search", + "session_snapshot", "table_registry", ] diff --git a/pkg-py/src/commons/_catalog/_core.py b/pkg-py/src/commons/_catalog/_core.py index b43c0657..e0e02f6c 100644 --- a/pkg-py/src/commons/_catalog/_core.py +++ b/pkg-py/src/commons/_catalog/_core.py @@ -184,12 +184,18 @@ def table_registry( # An entry naming a table is kept whether or not the warehouse # has it, and is always validated. Dropping a missing one turns # "that table is not there" into a quietly smaller selection. - table_id = _selector_id(selector) found = exact_relation(selector) - relations.append( - found if found is not None else Relation(id=table_id, discovered=False) + # Keyed by the relation's own id rather than the selector's: an + # entry naming a bare table is qualified with the connection's + # namespace once the warehouse answers, and the two lists have to + # agree on the label or the access check cannot pair them up. + relation = ( + found + if found is not None + else Relation(id=_selector_id(selector), discovered=False) ) - validate.append(table_id) + relations.append(relation) + validate.append(relation.id) continue namespace_selected = True relations.extend(list_relations(selector)) @@ -236,7 +242,9 @@ class Manifest: objects: dict[str, Relation] searchable: bool = False access: dict[str, str] = field(default_factory=dict) - access_errors: dict[str, str] = field(default_factory=dict) + # The driver's own failure, kept for the relations whose refusal is + # cached, so a later refusal can still be raised from what caused it. + access_errors: dict[str, BaseException] = field(default_factory=dict) @classmethod def build( diff --git a/pkg-py/src/commons/_catalog/_security.py b/pkg-py/src/commons/_catalog/_security.py new file mode 100644 index 00000000..4dae817b --- /dev/null +++ b/pkg-py/src/commons/_catalog/_security.py @@ -0,0 +1,353 @@ +"""Session identity and query access for a warehouse catalog. + +A warehouse decides what a principal may read, so commons never tries to +answer that itself: it asks, with a query that returns no rows, and reads the +answer off the failure. The classification matters because it decides what +happens next. An authorization refusal is stable, so it is remembered and the +relation is not probed again; a transient one is not, so the next touch +retries; anything unrecognized is neither cached nor treated as a refusal. + +The session snapshot exists for the same reason. Access was decided for the +principal, role, and namespace in force at discovery, so if any of those +change the answers no longer apply and the source has to be rebuilt. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from typing import Any, NoReturn + +from .._data_source import TableId +from ._core import Manifest, Relation + +__all__ = [ + "CatalogAccessError", + "CatalogAuthorizationError", + "CatalogSessionChangedError", + "CatalogTransientError", + "Probe", + "SessionSnapshot", + "changed_session_fields", + "check_session", + "classify_access_error", + "ensure_queryable", + "probe_relation", + "probe_sql", + "require_queryable", + "require_queryable_relations", + "session_snapshot", +] + +_AUTHORIZATION_MESSAGE = re.compile( + "not authorized|insufficient privilege|permission denied|" + "access denied|does not have.*privilege|not permitted|" + "permission_denied|sql access control error|not allowed to access", + re.IGNORECASE, +) + +_TRANSIENT_MESSAGE = re.compile( + "temporar|timed? ?out|unavailable|connection.*(closed|reset|failed)|" + "warehouse.*(starting|stopped|unavailable)|throttl|rate limit|" + "network|socket|http (429|503)|unexpected eof", + re.IGNORECASE, +) + +_TRANSIENT_SQLSTATE = re.compile("^08|^HYT|^40|^57P01") + + +class CatalogAccessError(Exception): + """Query access to a relation could not be verified.""" + + +class CatalogAuthorizationError(CatalogAccessError): + """The current principal may not query the relation.""" + + +class CatalogTransientError(CatalogAccessError): + """Access could not be verified now, but might be on a later try.""" + + +class CatalogSessionChangedError(Exception): + """The connection's identity moved after the catalog was discovered.""" + + +@dataclass(frozen=True) +class SessionSnapshot: + """The connection identity the catalog's access answers were decided for.""" + + backend: str + principal: str | None + catalog: str | None + schema: str | None + role: str | None = None + secondary_roles: str | None = None + + +@dataclass(frozen=True) +class Probe: + state: str + error: BaseException | None = None + + +def session_snapshot(backend: Any) -> SessionSnapshot | None: + """Read the identity a Snowflake or Databricks connection is acting under. + + Any other backend returns None: nothing else commons supports scopes + catalog access to a role that can change underneath the source. + """ + dialect = backend.dialect() + if dialect == "snowflake": + sql = ( + "SELECT CURRENT_USER() AS principal, CURRENT_ROLE() AS role, " + "CURRENT_SECONDARY_ROLES() AS secondary_roles, " + "CURRENT_DATABASE() AS catalog, CURRENT_SCHEMA() AS schema" + ) + elif dialect == "databricks": + sql = ( + "SELECT CURRENT_USER() AS principal, " + "CURRENT_CATALOG() AS catalog, CURRENT_SCHEMA() AS schema" + ) + else: + return None + + try: + rows = backend.query(sql) + except Exception as error: + raise RuntimeError(f"Failed to read the {dialect} session identity.") from error + return _session_row(rows, dialect) + + +def _session_row(rows: list[dict[str, Any]], dialect: str) -> SessionSnapshot: + has_roles = dialect == "snowflake" + required = ["principal", "catalog", "schema"] + if has_roles: + required += ["role", "secondary_roles"] + row = {str(key).lower(): value for key, value in rows[0].items()} if rows else {} + if len(rows) != 1 or not all(field in row for field in required): + raise ValueError(f"{dialect} returned an invalid session identity response.") + return SessionSnapshot( + backend=dialect, + principal=_session_value(row["principal"]), + catalog=_session_value(row["catalog"]), + schema=_session_value(row["schema"]), + role=_session_value(row["role"]) if has_roles else None, + secondary_roles=(_session_value(row["secondary_roles"]) if has_roles else None), + ) + + +def _session_value(value: Any) -> str | None: + if value is None or str(value) == "": + return None + return str(value) + + +def check_session(backend: Any, snapshot: SessionSnapshot | None) -> None: + """Refuse to go on when the connection is no longer who it was.""" + if snapshot is None: + return + current = session_snapshot(backend) + if current == snapshot: + return + changed = [ + _FIELD_NAMES[field] for field in changed_session_fields(current, snapshot) + ] + raise CatalogSessionChangedError( + f"The connection {_listed(changed) or 'identity'} changed after " + f"catalog discovery; rebuild the data source." + ) + + +def _listed(items: Any) -> str: + """A comma-separated list a person would read out loud.""" + items = list(items) + if len(items) < 2: + return "".join(items) + return f"{', '.join(items[:-1])} and {items[-1]}" + + +# How each snapshot field is worded in the refusal. +_FIELD_NAMES = { + "principal": "principal", + "role": "active role", + "secondary_roles": "secondary roles", + "catalog": "catalog", + "schema": "schema", +} + + +def changed_session_fields( + current: SessionSnapshot | None, snapshot: SessionSnapshot +) -> list[str]: + """Which parts of the session identity differ, in snapshot order. + + The refusal names what moved rather than everything it compares, because + a backend need not have every field: Databricks reports no role, and + naming one there sends the user looking for something that cannot change. + """ + if current is None: + return [] + return [ + field + for field in _FIELD_NAMES + if getattr(current, field) != getattr(snapshot, field) + ] + + +def probe_sql(backend: Any, sql: str) -> Probe: + try: + backend.query(sql) + except Exception as error: # noqa: BLE001 - the failure is the answer + return Probe(classify_access_error(error), error) + return Probe("queryable") + + +def probe_relation(backend: Any, table_id: TableId) -> Probe: + return probe_sql(backend, f"SELECT * FROM {backend.quote(table_id)} WHERE 1 = 0") + + +def classify_access_error(error: BaseException) -> str: + """Read a driver's failure as authorization, transient, or neither. + + Conservative in both directions: a refusal is only called authorization + when the driver said so, and everything unrecognized stays unknown so it + is neither cached nor retried on its own. + """ + sqlstate = _sqlstate(error) + message = str(error) + if ( + sqlstate.startswith("28") + or sqlstate == "42501" + or _AUTHORIZATION_MESSAGE.search(message) + ): + return "authorization" + if _TRANSIENT_SQLSTATE.match(sqlstate) or _TRANSIENT_MESSAGE.search(message): + return "transient" + return "unknown" + + +def _sqlstate(error: BaseException) -> str: + """The SQLSTATE a driver reported, wherever it hung it. + + DBAPI drivers put it on the exception, SQLAlchemy wraps that exception in + one of its own, so the cause is read too. + """ + for candidate in (error, getattr(error, "orig", None), error.__cause__): + if candidate is None: + continue + for attribute in ("sqlstate", "state"): + value = getattr(candidate, attribute, None) + if isinstance(value, str) and value: + return value.upper() + return "" + + +def require_queryable( + backend: Any, table_id: TableId, label: str | None = None +) -> None: + probe = probe_relation(backend, table_id) + if probe.state != "queryable": + _abort_access(probe, label or table_id.label) + + +def require_queryable_relations( + backend: Any, + validate: dict[str, TableId], + relations: dict[str, Relation] | None = None, +) -> None: + """Check every explicitly named relation before the source is built. + + A name the warehouse never reported is missing rather than refused, and + saying so is more use than an access error about a table that is not + there. An unexplained failure is only reported as missing when the + backend can confirm the relation's absence. + + Every relation the listing did report is probed before anything is + raised. Stopping at the first refusal would report one problem at a time, + and would report it ahead of a name the caller simply got wrong. + """ + missing = [ + label + for label in validate + if relations is not None and not relations[label].discovered + ] + # Only the first refusal is raised, so only the first is kept: a + # selection may run to thousands of relations. + refused: tuple[Probe, str] | None = None + for label, table_id in validate.items(): + if label in missing: + continue + probe = probe_relation(backend, table_id) + if probe.state == "queryable": + continue + if probe.state == "unknown" and _relation_exists(backend, table_id) is False: + missing.append(label) + continue + refused = refused or (probe, label) + if missing: + _abort_missing(missing) + if refused is not None: + _abort_access(*refused) + + +def _relation_exists(backend: Any, table_id: TableId) -> bool | None: + inspector = backend.inspector() + if inspector is None: + return None + try: + return bool(inspector(table_id)) + except Exception: # noqa: BLE001 - inconclusive; the caller keeps the probe + return None + + +def ensure_queryable( + backend: Any, manifest: Manifest | None, label: str, table_id: TableId +) -> None: + """Check access to one relation at the moment it is about to be used. + + Construction only probes what the caller named and what the dictionary + matched, so a relation that arrived through a namespace listing is first + checked here. Its caller is the first-touch path that describes a table + to the agent, which lands with the retrieval surface; until then nothing + in commons resolves a label at query time. + """ + if manifest is None: + return + state = manifest.access.get(label, "unknown") + if state == "queryable": + return + if state == "authorization": + _abort_access(Probe(state, manifest.access_errors.get(label)), label) + + probe = probe_relation(backend, table_id) + if probe.state == "queryable": + manifest.access[label] = "queryable" + return + # Only stable authorization failures are cached; other failures retry. + if probe.state == "authorization": + manifest.access[label] = probe.state + if probe.error is not None: + manifest.access_errors[label] = probe.error + _abort_access(probe, label) + + +def _abort_missing(missing: list[str]) -> NoReturn: + noun = "a table" if len(missing) == 1 else "tables" + raise ValueError( + f"tables must not name {noun} the connection does not have: " + f"{_listed(repr(label) for label in missing)}." + ) + + +def _abort_access(probe: Probe, label: str) -> NoReturn: + if probe.state == "authorization": + raise CatalogAuthorizationError( + f"The current principal is not authorized to query {label!r}." + ) from probe.error + if probe.state == "transient": + raise CatalogTransientError( + f"Query access to {label!r} is temporarily unavailable." + ) from probe.error + raise CatalogAccessError( + f"Could not verify query access to {label!r}." + ) from probe.error diff --git a/pkg-py/tests/test_catalog.py b/pkg-py/tests/test_catalog.py index 9da7a08e..699395a4 100644 --- a/pkg-py/tests/test_catalog.py +++ b/pkg-py/tests/test_catalog.py @@ -116,6 +116,31 @@ def test_a_namespace_selection_lists_and_excludes(): assert registry.namespace_selected is True +def test_a_bare_relation_is_validated_under_the_label_it_came_back_with(): + # A selection entry naming a bare table is qualified by the warehouse + # from the connection's namespace. The access check pairs the two lists + # by label, so validate has to carry the qualified one. + registry = table_registry( + selectors=[Selector(table="orders")], + exact_relation=lambda selector: relation("main.sales.orders", kind="table"), + list_relations=lambda selector: [], + ) + + assert list(registry.relations) == ["main.sales.orders"] + assert list(registry.validate) == ["main.sales.orders"] + + +def test_a_bare_relation_the_warehouse_lacks_keeps_the_name_that_was_asked_for(): + registry = table_registry( + selectors=[Selector(table="orders")], + exact_relation=lambda selector: None, + list_relations=lambda selector: [], + ) + + assert list(registry.validate) == ["orders"] + assert registry.relations["orders"].discovered is False + + def test_a_selection_above_the_object_limit_is_refused(): with pytest.raises(ValueError, match="above the supported limit"): table_registry( diff --git a/pkg-py/tests/test_catalog_security.py b/pkg-py/tests/test_catalog_security.py new file mode 100644 index 00000000..f9ec3a1b --- /dev/null +++ b/pkg-py/tests/test_catalog_security.py @@ -0,0 +1,327 @@ +"""Session and access checks for a warehouse catalog. + +A fake backend stands in for the network here, not for a warehouse's access +rules: the point of each test is what commons does with the reply, since the +warehouse is the one deciding whether a principal may read a relation. +""" + +from typing import Any + +import pytest + +from commons._catalog import Manifest, Relation +from commons._catalog._security import ( + CatalogAccessError, + CatalogAuthorizationError, + CatalogSessionChangedError, + CatalogTransientError, + SessionSnapshot, + check_session, + classify_access_error, + ensure_queryable, + probe_relation, + require_queryable, + require_queryable_relations, + session_snapshot, +) +from commons._data_source import TableId + + +class DriverError(Exception): + """An error carrying a SQLSTATE, the way a warehouse driver's does.""" + + def __init__(self, message, sqlstate=None): + super().__init__(message) + self.sqlstate = sqlstate + + +class FakeBackend: + """Replays a canned reply, or raises, for each query it is given.""" + + def __init__(self, replies=None, dialect="snowflake", exists=None): + self._replies = list(replies or []) + self._dialect = dialect + self._exists = exists + self.queries: list[str] = [] + + def query(self, sql: str): + self.queries.append(sql) + reply = self._replies.pop(0) if self._replies else [] + if isinstance(reply, BaseException): + raise reply + return reply + + def quote(self, table_id: TableId) -> str: + return ".".join(f'"{part}"' for part in table_id.parts) + + def dialect(self) -> str: + return self._dialect + + def inspector(self): + if self._exists is None: + return None + return lambda table_id: self._exists + + +SALES = TableId(catalog="ANALYTICS", schema="PUBLIC", table="SALES") + + +def snowflake_row(role="REPORTER") -> dict[str, Any]: + return { + "PRINCIPAL": "ANALYST", + "ROLE": role, + "SECONDARY_ROLES": '{"roles":"READER","value":"ALL"}', + "CATALOG": "ANALYTICS", + "SCHEMA": "PUBLIC", + } + + +def test_session_snapshots_retain_authority_bearing_fields(): + backend = FakeBackend([[snowflake_row()]]) + + snapshot = session_snapshot(backend) + + assert snapshot == SessionSnapshot( + backend="snowflake", + principal="ANALYST", + role="REPORTER", + secondary_roles='{"roles":"READER","value":"ALL"}', + catalog="ANALYTICS", + schema="PUBLIC", + ) + + +def test_databricks_snapshots_carry_no_role(): + rows = [ + [{"principal": "analyst@example.com", "catalog": "main", "schema": "default"}] + ] + backend = FakeBackend(rows, dialect="databricks") + + snapshot = session_snapshot(backend) + + assert snapshot is not None + assert snapshot.role is None + assert snapshot.secondary_roles is None + assert (snapshot.catalog, snapshot.schema) == ("main", "default") + + +def test_other_backends_have_no_session_to_snapshot(): + assert session_snapshot(FakeBackend(dialect="duckdb")) is None + + +def test_an_empty_session_value_is_absent_rather_than_blank(): + row = snowflake_row() + row["ROLE"] = "" + row["SCHEMA"] = None + + snapshot = session_snapshot(FakeBackend([[row]])) + + assert snapshot is not None + assert snapshot.role is None + assert snapshot.schema is None + + +def test_an_unreadable_session_identity_is_an_error(): + backend = FakeBackend([DriverError("connection closed")]) + + with pytest.raises(RuntimeError, match="session identity"): + session_snapshot(backend) + + +def test_an_invalid_session_reply_is_an_error(): + with pytest.raises(ValueError, match="invalid session identity"): + session_snapshot(FakeBackend([[]])) + + with pytest.raises(ValueError, match="invalid session identity"): + session_snapshot(FakeBackend([[{"PRINCIPAL": "ANALYST"}]])) + + +def test_catalog_operations_reject_a_changed_session(): + taken = session_snapshot(FakeBackend([[snowflake_row()]])) + backend = FakeBackend([[snowflake_row(role="ADMIN")]]) + + with pytest.raises(CatalogSessionChangedError, match="active role changed"): + check_session(backend, taken) + + +def test_a_databricks_refusal_never_names_a_role(): + rows = {"principal": "analyst@example.com", "catalog": "main", "schema": "default"} + taken = session_snapshot(FakeBackend([[rows]], dialect="databricks")) + assert taken is not None + moved = FakeBackend([[{**rows, "principal": "other@example.com"}]], "databricks") + + with pytest.raises(CatalogSessionChangedError) as refusal: + check_session(moved, taken) + + assert "principal changed" in str(refusal.value) + assert "role" not in str(refusal.value) + + +def test_an_unchanged_session_passes(): + taken = session_snapshot(FakeBackend([[snowflake_row()]])) + backend = FakeBackend([[snowflake_row()]]) + + check_session(backend, taken) + + +def test_a_source_without_a_session_is_not_checked(): + backend = FakeBackend() + + check_session(backend, None) + + assert backend.queries == [] + + +def test_a_probe_reads_no_rows_from_the_relation(): + backend = FakeBackend() + + probe = probe_relation(backend, SALES) + + assert probe.state == "queryable" + assert backend.queries == ['SELECT * FROM "ANALYTICS"."PUBLIC"."SALES" WHERE 1 = 0'] + + +def test_require_queryable_names_the_relation_it_refused(): + backend = FakeBackend([DriverError("hidden", sqlstate="42501")]) + + with pytest.raises(CatalogAuthorizationError, match="ANALYTICS.PUBLIC.SALES"): + require_queryable(backend, SALES) + + +def test_transient_access_failures_remain_retryable(): + relations = {"sales": Relation(id=SALES)} + manifest = Manifest.build(relations) + backend = FakeBackend([DriverError("timed out"), []]) + + with pytest.raises(CatalogTransientError): + ensure_queryable(backend, manifest, "sales", SALES) + assert manifest.access["sales"] == "unknown" + + ensure_queryable(backend, manifest, "sales", SALES) + assert manifest.access["sales"] == "queryable" + assert len(backend.queries) == 2 + + +def test_authorization_failures_are_cached_per_relation(): + relations = {"sales": Relation(id=SALES)} + manifest = Manifest.build(relations) + backend = FakeBackend([DriverError("permission denied")]) + + for _ in range(2): + with pytest.raises(CatalogAuthorizationError): + ensure_queryable(backend, manifest, "sales", SALES) + + assert manifest.access["sales"] == "authorization" + assert len(backend.queries) == 1 + + +def test_a_relation_already_known_queryable_is_not_probed_again(): + manifest = Manifest.build({"sales": Relation(id=SALES)}) + manifest.access["sales"] = "queryable" + backend = FakeBackend() + + ensure_queryable(backend, manifest, "sales", SALES) + + assert backend.queries == [] + + +def test_a_source_without_a_manifest_has_nothing_to_check(): + backend = FakeBackend() + + ensure_queryable(backend, None, "sales", SALES) + + assert backend.queries == [] + + +def test_exact_relations_use_classified_access_probes(): + backend = FakeBackend([DriverError("warehouse is starting")]) + + with pytest.raises(CatalogTransientError): + require_queryable_relations(backend, {"ANALYTICS.PUBLIC.SALES": SALES}) + + +def test_an_undiscovered_relation_fails_before_the_access_probe(): + backend = FakeBackend() + relations = {"ANALYTICS.PUBLIC.SALES": Relation(id=SALES, discovered=False)} + + with pytest.raises(ValueError, match="must not name"): + require_queryable_relations( + backend, {"ANALYTICS.PUBLIC.SALES": SALES}, relations + ) + + assert backend.queries == [] + + +def test_an_unexplained_failure_on_an_absent_relation_reports_it_missing(): + backend = FakeBackend([DriverError("object not found")], exists=False) + + with pytest.raises(ValueError, match="must not name"): + require_queryable_relations(backend, {"ANALYTICS.PUBLIC.SALES": SALES}) + + +def test_an_unexplained_failure_is_kept_when_the_relation_is_there(): + backend = FakeBackend([DriverError("something odd")], exists=True) + + with pytest.raises(CatalogAccessError, match="Could not verify"): + require_queryable_relations(backend, {"ANALYTICS.PUBLIC.SALES": SALES}) + + +def test_a_backend_that_cannot_answer_existence_keeps_the_access_error(): + backend = FakeBackend([DriverError("something odd")]) + + with pytest.raises(CatalogAccessError, match="Could not verify"): + require_queryable_relations(backend, {"ANALYTICS.PUBLIC.SALES": SALES}) + + +def test_a_relation_the_listing_reported_is_probed_even_beside_a_missing_one(): + orders = TableId(catalog="ANALYTICS", schema="PUBLIC", table="ORDERS") + backend = FakeBackend([DriverError("permission denied")]) + relations = { + "ANALYTICS.PUBLIC.SALES": Relation(id=SALES, discovered=False), + "ANALYTICS.PUBLIC.ORDERS": Relation(id=orders), + } + + with pytest.raises(ValueError, match="ANALYTICS.PUBLIC.SALES"): + require_queryable_relations( + backend, + {"ANALYTICS.PUBLIC.SALES": SALES, "ANALYTICS.PUBLIC.ORDERS": orders}, + relations, + ) + + # The refused relation was still probed, so fixing the missing name is + # not a round trip spent to be told about the next problem. + assert len(backend.queries) == 1 + + +def test_a_missing_relation_is_reported_alongside_a_refused_one(): + orders = TableId(catalog="ANALYTICS", schema="PUBLIC", table="ORDERS") + backend = FakeBackend( + [DriverError("object not found"), DriverError("permission denied")], + exists=False, + ) + + with pytest.raises(ValueError, match="ANALYTICS.PUBLIC.SALES"): + require_queryable_relations( + backend, + {"ANALYTICS.PUBLIC.SALES": SALES, "ANALYTICS.PUBLIC.ORDERS": orders}, + ) + + +def test_a_failing_existence_check_keeps_the_access_error(): + backend = FakeBackend([DriverError("something odd")]) + backend.inspector = lambda: _raise_on_call + + with pytest.raises(CatalogAccessError, match="Could not verify"): + require_queryable_relations(backend, {"ANALYTICS.PUBLIC.SALES": SALES}) + + +def _raise_on_call(table_id): + raise DriverError("the inspector failed too") + + +def test_a_chained_driver_error_still_yields_its_sqlstate(): + inner = DriverError("hidden", sqlstate="42501") + outer = Exception("statement failed") + outer.__cause__ = inner + + assert classify_access_error(outer) == "authorization" From 0054a552f0d3bf78a5c44700d3812f5cf1ef42f0 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:51:39 -0600 Subject: [PATCH 2/5] fix(r): name what a changed session covers, and report every missing relation Two defects the Python implementation of the same contract exposed. The session refusal listed every field the check compares, which told a Databricks user a role had changed on a backend that has none; it now names the fields that differ, and the documentation and the governance vignette scope the role snapshot to Snowflake. And an access refusal on one named relation discarded the missing names already found, so the caller fixed a typo only to be told about the next problem on the next attempt; every relation the listing reported is now probed before anything is raised. --- pkg-r/R/catalog-security.R | 63 ++++++++++++++++++++++++++++++---- pkg-r/R/data-source.R | 9 ++--- pkg-r/man/data_source.Rd | 9 ++--- pkg-r/vignettes/governance.Rmd | 2 +- 4 files changed, 67 insertions(+), 16 deletions(-) diff --git a/pkg-r/R/catalog-security.R b/pkg-r/R/catalog-security.R index 579343c3..5c40dae7 100644 --- a/pkg-r/R/catalog-security.R +++ b/pkg-r/R/catalog-security.R @@ -111,10 +111,16 @@ catalog_check_session_snapshot <- function( ) { current <- catalog_session_snapshot(con, call = call) if (!identical(current, snapshot)) { + fields <- catalog_session_changed_fields(current, snapshot) + # cli reads a character vector out as a list, commas and "and" included. + changed <- unname(catalog_session_field_names[fields]) + if (length(changed) == 0L) { + changed <- "identity" + } cli::cli_abort( paste( - "The connection principal, active role, or namespace changed after", - "catalog discovery; rebuild the data source." + "The connection {changed} changed after catalog discovery;", + "rebuild the data source." ), class = "commons_catalog_session_changed", call = call @@ -123,6 +129,38 @@ catalog_check_session_snapshot <- function( invisible(snapshot) } +# How each snapshot field is worded in the refusal. +catalog_session_field_names <- c( + principal = "principal", + role = "active role", + secondary_roles = "secondary roles", + catalog = "catalog", + schema = "schema" +) + +# Name what moved rather than everything the snapshot compares: Databricks +# has no role, so a fixed list would name a field that backend never had. +catalog_session_changed_fields <- function(current, snapshot) { + fields <- names(catalog_session_field_names) + fields[vapply( + fields, + function(field) { + !identical( + catalog_session_field(current, field), + catalog_session_field(snapshot, field) + ) + }, + logical(1) + )] +} + +catalog_session_field <- function(snapshot, field) { + if (field %in% c("catalog", "schema")) { + return(snapshot$namespace[[field]]) + } + snapshot[[field]] +} + catalog_probe_relation <- function(con, id) { catalog_probe_sql( con, @@ -164,7 +202,7 @@ catalog_access_error_kind <- function(err) { paste( "not authorized|insufficient privilege|permission denied|", "access denied|does not have.*privilege|not permitted|", - "permission_denied|sql access control error", + "permission_denied|sql access control error|not allowed to access", sep = "" ), conditionMessage(err), @@ -220,10 +258,16 @@ catalog_require_queryable_relations <- function( logical(1) )] } - if (length(missing)) { - catalog_abort_missing_relations(missing, call = call) - } + # Every relation the listing did report is probed before anything is + # raised, so a name the caller got wrong and a relation they cannot read + # are found in one pass rather than one round trip each. + # Only the first refusal is raised, so only the first is kept: a selection + # may run to thousands of relations. + refused <- NULL for (i in seq_along(registry$ids)) { + if (registry$labels[[i]] %in% missing) { + next + } probe <- catalog_probe_relation(con, registry$ids[[i]]) if (identical(probe$state, "queryable")) { next @@ -235,11 +279,16 @@ catalog_require_queryable_relations <- function( missing <- c(missing, registry$labels[[i]]) next } - catalog_abort_access(probe, registry$labels[[i]], call = call) + if (is.null(refused)) { + refused <- list(probe = probe, label = registry$labels[[i]]) + } } if (length(missing)) { catalog_abort_missing_relations(missing, call = call) } + if (!is.null(refused)) { + catalog_abort_access(refused$probe, refused$label, call = call) + } invisible(registry) } diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index d7b5839d..28c1cf7e 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -81,10 +81,11 @@ #' data frames, commons additionally disables extension loading and filesystem #' access. These are safeguards, not a sandbox: when you supply your own #' connection, still open it in read-only mode where the backend supports it. -#' Snowflake and Databricks sources snapshot the principal, active role, and -#' namespace at creation, then reject catalog access and trusted calculations -#' after those values change. Authored and native semantic material is exposed -#' only after a zero-row query succeeds for the current principal. +#' Snowflake and Databricks sources snapshot the principal and namespace at +#' creation, and Snowflake its active and secondary roles as well, then reject +#' catalog access and trusted calculations after any of those change. Authored +#' and native semantic material is exposed only after a zero-row query +#' succeeds for the current principal. #' #' @return A `commons_data_source` R6 object. Its internals are private and may #' change without notice. diff --git a/pkg-r/man/data_source.Rd b/pkg-r/man/data_source.Rd index 48cfbb6c..4c60be74 100644 --- a/pkg-r/man/data_source.Rd +++ b/pkg-r/man/data_source.Rd @@ -101,10 +101,11 @@ rejected before reaching the database. For the in-process DuckDB built from data frames, commons additionally disables extension loading and filesystem access. These are safeguards, not a sandbox: when you supply your own connection, still open it in read-only mode where the backend supports it. -Snowflake and Databricks sources snapshot the principal, active role, and -namespace at creation, then reject catalog access and trusted calculations -after those values change. Authored and native semantic material is exposed -only after a zero-row query succeeds for the current principal. +Snowflake and Databricks sources snapshot the principal and namespace at +creation, and Snowflake its active and secondary roles as well, then reject +catalog access and trusted calculations after any of those change. Authored +and native semantic material is exposed only after a zero-row query +succeeds for the current principal. } \examples{ diff --git a/pkg-r/vignettes/governance.Rmd b/pkg-r/vignettes/governance.Rmd index 7f4a5549..75c66bf2 100644 --- a/pkg-r/vignettes/governance.Rmd +++ b/pkg-r/vignettes/governance.Rmd @@ -38,7 +38,7 @@ These checks provide defense in depth, but they are not a SQL parser or a databa The `tables` argument to `data_source()` controls which tables commons describes to the model. It is not an authorization boundary: SQL written by the agent can query any object available to the connection. -On Posit Connect, [viewer OAuth integrations](https://docs.posit.co/connect/admin/access-controls/) can give an interactive application the current viewer's Snowflake or Databricks credentials. If the application creates its connection from those credentials, the warehouse continues to enforce that viewer's existing access policies, including row- and column-level security. commons snapshots the connection's principal, active role, and namespace when it creates a Snowflake or Databricks data source, and rejects subsequent operations if that identity changes. +On Posit Connect, [viewer OAuth integrations](https://docs.posit.co/connect/admin/access-controls/) can give an interactive application the current viewer's Snowflake or Databricks credentials. If the application creates its connection from those credentials, the warehouse continues to enforce that viewer's existing access policies, including row- and column-level security. commons snapshots the connection's principal and namespace when it creates a Snowflake or Databricks data source, and its active and secondary roles as well on Snowflake, and rejects subsequent operations if that identity changes. Viewer credentials are not automatic: commons uses the DBI connection supplied by the application. When using viewer credentials, create the connection and the commons agent inside the Shiny server function so that each session has the correct database identity. From 708c11910d7b38657144d6d65c23d4e4ee53c3ce Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:51:40 -0600 Subject: [PATCH 3/5] test: pin access classification, session comparison, and access precedence Three behaviours both packages have to agree on, each implemented twice and asserted twice until now: how a driver failure classifies, which parts of a session identity a refusal reports, and what construction reports when several named relations fail at once. They move to tests/shared/ and both suites read them. Field names travel rather than message text, since the wording belongs to each language. --- .../tests/test_catalog_security_fixtures.py | 133 ++++++++++++++++++ .../shared/catalog-access-errors.json | 101 +++++++++++++ .../shared/catalog-access-precedence.json | 83 +++++++++++ .../shared/catalog-session-changed.json | 41 ++++++ pkg-r/tests/testthat/helper-catalog-rows.R | 44 ++++++ pkg-r/tests/testthat/test-catalog-security.R | 119 ++++++++++++++-- tests/shared/README.md | 2 + tests/shared/catalog-access-errors.json | 101 +++++++++++++ tests/shared/catalog-access-precedence.json | 83 +++++++++++ tests/shared/catalog-session-changed.json | 41 ++++++ 10 files changed, 738 insertions(+), 10 deletions(-) create mode 100644 pkg-py/tests/test_catalog_security_fixtures.py create mode 100644 pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json create mode 100644 pkg-r/tests/testthat/fixtures/shared/catalog-access-precedence.json create mode 100644 pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json create mode 100644 tests/shared/catalog-access-errors.json create mode 100644 tests/shared/catalog-access-precedence.json create mode 100644 tests/shared/catalog-session-changed.json diff --git a/pkg-py/tests/test_catalog_security_fixtures.py b/pkg-py/tests/test_catalog_security_fixtures.py new file mode 100644 index 00000000..29fadb2a --- /dev/null +++ b/pkg-py/tests/test_catalog_security_fixtures.py @@ -0,0 +1,133 @@ +"""Access-error classification and session comparison, against the shared +fixtures. + +Which failures are authorization, transient, or neither decides what the user +is told and whether the answer is cached, and which parts of a session are +reported as changed decides what they are told to look at, so both are pinned +once and read by both suites. +""" + +import pytest + +from commons._catalog import Relation +from commons._catalog._security import ( + CatalogAccessError, + CatalogAuthorizationError, + CatalogTransientError, + SessionSnapshot, + changed_session_fields, + classify_access_error, + require_queryable_relations, +) +from commons._data_source import TableId +from tests._shared import load_shared_fixture + + +class DriverError(Exception): + def __init__(self, message, sqlstate): + super().__init__(message) + self.sqlstate = sqlstate + + +def test_access_errors_match_the_shared_contract(): + cases = load_shared_fixture("catalog-access-errors")["cases"] + assert cases + + for case in cases: + error = DriverError(case["message"], case["sqlstate"] or None) + assert classify_access_error(error) == case["kind"], case["name"] + + +def test_changed_session_fields_match_the_shared_contract(): + cases = load_shared_fixture("catalog-session-changed")["cases"] + assert cases + + for case in cases: + before, after = _snapshot(case["before"]), _snapshot(case["after"]) + assert changed_session_fields(after, before) == case["changed"], case["name"] + + +def _snapshot(fields): + return SessionSnapshot( + backend=fields["backend"], + principal=fields["principal"], + catalog=fields["catalog"], + schema=fields["schema"], + role=fields["role"], + secondary_roles=fields["secondary_roles"], + ) + + +class _PrecedenceBackend: + """Answers each relation's probe from the fixture's own script.""" + + def __init__(self, relations): + self._script = {item["label"]: item for item in relations} + self.probed: list[str] = [] + + def query(self, sql: str): + label = sql.split('"')[1] + self.probed.append(label) + state = self._script[label]["probe"] + if state == "queryable": + return [] + raise _PROBE_ERRORS[state]() + + def quote(self, table_id): + return f'"{table_id.table}"' + + def dialect(self): + return "snowflake" + + def inspector(self): + def exists(table_id): + answer = self._script[table_id.table].get("exists", "unknown") + if answer == "unknown": + raise RuntimeError("the backend cannot say") + return answer == "true" + + return exists + + +_PROBE_ERRORS = { + "authorization": lambda: DriverError("permission denied", None), + "transient": lambda: DriverError("timed out", None), + "unknown": lambda: DriverError("something odd", None), +} + +_OUTCOMES = { + "missing": ValueError, + "authorization": CatalogAuthorizationError, + "transient": CatalogTransientError, + "access": CatalogAccessError, +} + + +def test_access_precedence_matches_the_shared_contract(): + cases = load_shared_fixture("catalog-access-precedence")["cases"] + assert cases + + for case in cases: + backend = _PrecedenceBackend(case["relations"]) + validate = { + item["label"]: TableId(table=item["label"]) for item in case["relations"] + } + relations = { + item["label"]: Relation( + id=validate[item["label"]], discovered=item["discovered"] == "true" + ) + for item in case["relations"] + } + expected = case["expected"] + if expected["outcome"] == "ok": + require_queryable_relations(backend, validate, relations) + else: + with pytest.raises(_OUTCOMES[expected["outcome"]]) as refusal: + require_queryable_relations(backend, validate, relations) + for label in expected["labels"]: + assert label in str(refusal.value), case["name"] + # The contract is that nothing is raised until every relation the + # listing reported has been probed, which only the probes can show. + assert backend.probed == [ + item["label"] for item in case["relations"] if item["discovered"] == "true" + ], case["name"] diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json b/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json new file mode 100644 index 00000000..bce22ef1 --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json @@ -0,0 +1,101 @@ +{ + "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message.", + "cases": [ + { + "name": "insufficient privilege sqlstate", + "sqlstate": "42501", + "message": "hidden", + "kind": "authorization" + }, + { + "name": "invalid authorization sqlstate class", + "sqlstate": "28000", + "message": "hidden", + "kind": "authorization" + }, + { + "name": "snowflake access control message", + "sqlstate": "", + "message": "SQL access control error: Insufficient privileges to operate on table 'SALES'", + "kind": "authorization" + }, + { + "name": "databricks permission message", + "sqlstate": "", + "message": "PERMISSION_DENIED: User does not have SELECT on Table 'main.default.sales'", + "kind": "authorization" + }, + { + "name": "plain permission denied message", + "sqlstate": "", + "message": "Permission denied on relation sales", + "kind": "authorization" + }, + { + "name": "a network policy refusal is authorization, not a network fault", + "sqlstate": "", + "message": "Incoming request with IP 10.0.0.1 is not allowed to access Snowflake", + "kind": "authorization" + }, + { + "name": "connection exception sqlstate class", + "sqlstate": "08006", + "message": "hidden", + "kind": "transient" + }, + { + "name": "timeout sqlstate", + "sqlstate": "HYT00", + "message": "hidden", + "kind": "transient" + }, + { + "name": "serialization failure sqlstate", + "sqlstate": "40001", + "message": "hidden", + "kind": "transient" + }, + { + "name": "admin shutdown sqlstate", + "sqlstate": "57P01", + "message": "hidden", + "kind": "transient" + }, + { + "name": "warehouse starting message", + "sqlstate": "", + "message": "Warehouse 'COMPUTE_WH' is starting", + "kind": "transient" + }, + { + "name": "a temporarily unavailable warehouse", + "sqlstate": "", + "message": "Warehouse is temporarily unavailable", + "kind": "transient" + }, + { + "name": "rate limited message", + "sqlstate": "", + "message": "HTTP 429 too many requests", + "kind": "transient" + }, + { + "name": "lowercase sqlstate is still matched", + "sqlstate": "hyt00", + "message": "hidden", + "kind": "transient" + }, + { + "name": "syntax error with a sqlstate", + "sqlstate": "42601", + "message": "syntax error at or near SELCT", + "kind": "unknown" + }, + { + "name": "unrecognized failure without a sqlstate", + "sqlstate": "", + "message": "column BOGUS not found", + "kind": "unknown" + } + ] +} diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-access-precedence.json b/pkg-r/tests/testthat/fixtures/shared/catalog-access-precedence.json new file mode 100644 index 00000000..6a9ff6f2 --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-access-precedence.json @@ -0,0 +1,83 @@ +{ + "description": "What a source construction reports when several explicitly named relations fail at once. Every relation the listing did report is probed before anything is raised, so a name the caller got wrong and a relation they cannot read are found in one pass rather than one round trip each. A name the warehouse never listed is reported ahead of a refusal: it is the more actionable problem, and an access error about a table that is not there reads as a permissions problem the caller does not have. Booleans travel as strings so the file reads the same from both JSON readers. `discovered` is whether the listing reported the relation, `probe` is what the zero-row query did, and `exists` is what the backend answers about an unexplained failure.", + "cases": [ + { + "name": "every relation is readable", + "relations": [ + {"label": "A", "discovered": "true", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "queryable"} + ], + "expected": {"outcome": "ok", "labels": []} + }, + { + "name": "a name the listing never reported", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "queryable"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "a missing name is reported ahead of a refusal", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "a relation the probe proves absent is missing too", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "false"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "every missing name is reported at once", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "unknown", "exists": "false"} + ], + "expected": {"outcome": "missing", "labels": ["A", "B"]} + }, + { + "name": "the first refusal is the one raised", + "relations": [ + {"label": "A", "discovered": "true", "probe": "authorization"}, + {"label": "B", "discovered": "true", "probe": "transient"} + ], + "expected": {"outcome": "authorization", "labels": ["A"]} + }, + { + "name": "the first refusal wins even when a later one is an authorization", + "relations": [ + {"label": "A", "discovered": "true", "probe": "transient"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "transient", "labels": ["A"]} + }, + { + "name": "a transient refusal on its own", + "relations": [ + {"label": "A", "discovered": "true", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "transient"} + ], + "expected": {"outcome": "transient", "labels": ["B"]} + }, + { + "name": "an unexplained failure the backend cannot explain away", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "unknown"} + ], + "expected": {"outcome": "access", "labels": ["A"]} + }, + { + "name": "an unexplained failure on a relation that is there", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "true"} + ], + "expected": {"outcome": "access", "labels": ["A"]} + } + ] +} diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json b/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json new file mode 100644 index 00000000..957b0223 --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json @@ -0,0 +1,41 @@ +{ + "description": "Which parts of a warehouse session identity a refusal reports as changed. The comparison runs before every catalog operation, and the answer decides what the user is told to look at, so it is pinned rather than described twice. A field is absent (null) when the backend has none: Databricks reports no role, so a refusal there must never name one. The names here are the snapshot's own fields; each implementation words them for its own message.", + "cases": [ + { + "name": "a snowflake role change", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["role"] + }, + { + "name": "a snowflake secondary role change", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "NONE", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["secondary_roles"] + }, + { + "name": "a databricks principal change names no role", + "before": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "after": {"backend": "databricks", "principal": "other@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "changed": ["principal"] + }, + { + "name": "a databricks namespace change names both parts", + "before": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "after": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "sandbox", "schema": "scratch"}, + "changed": ["catalog", "schema"] + }, + { + "name": "several fields at once, in snapshot order", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ADMIN_USER", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "STAGING"}, + "changed": ["principal", "role", "schema"] + }, + { + "name": "a role that was dropped rather than swapped", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": null, "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["role"] + } + ] +} diff --git a/pkg-r/tests/testthat/helper-catalog-rows.R b/pkg-r/tests/testthat/helper-catalog-rows.R index d3292833..d73df2e8 100644 --- a/pkg-r/tests/testthat/helper-catalog-rows.R +++ b/pkg-r/tests/testthat/helper-catalog-rows.R @@ -54,3 +54,47 @@ catalog_rows_expect_columns <- function(columns, expected, info) { } } } + +# A session snapshot from the shared fixture's field-by-field spelling. +catalog_session_fixture_snapshot <- function(fields) { + list( + backend = fields$backend, + principal = fields$principal, + role = fields$role, + secondary_roles = fields$secondary_roles, + namespace = list(catalog = fields$catalog, schema = fields$schema) + ) +} + +# The access-precedence fixture's relations, keyed by label. +catalog_precedence_script <- function(relations) { + stats::setNames(relations, vapply(relations, `[[`, character(1), "label")) +} + +catalog_precedence_probe <- function(state) { + if (identical(state, "queryable")) { + return(list(state = "queryable", error = NULL)) + } + list(state = state, error = simpleError(state)) +} + +# Every refusal here is a cli condition, so the missing-relation outcome is +# told apart by what it says rather than by a class it shares with the rest. +catalog_precedence_expectation <- function(outcome) { + switch( + outcome, + missing = list(class = "rlang_error", regexp = "not on the connection"), + authorization = list(class = "commons_catalog_authorization_error"), + transient = list(class = "commons_catalog_transient_error"), + list(class = "commons_catalog_access_error") + ) +} + +# The relations a run must have probed: every one the listing reported. +catalog_precedence_probed <- function(script) { + names(script)[vapply( + script, + function(item) identical(item$discovered, "true"), + logical(1) + )] +} diff --git a/pkg-r/tests/testthat/test-catalog-security.R b/pkg-r/tests/testthat/test-catalog-security.R index 8df42beb..3c2d9e3c 100644 --- a/pkg-r/tests/testthat/test-catalog-security.R +++ b/pkg-r/tests/testthat/test-catalog-security.R @@ -73,6 +73,11 @@ test_that("catalog operations reject changed sessions", { catalog_search(source, "sales"), class = "commons_catalog_session_changed" ) + # The refusal names what moved rather than every field it compares. + expect_error( + source_query(source, "SELECT * FROM sales"), + regexp = "active role" + ) }) test_that("transient access failures remain retryable", { @@ -122,23 +127,68 @@ test_that("authorization failures are cached per relation", { expect_equal(calls, 1L) }) -test_that("warehouse access errors are classified conservatively", { - authorization <- structure( - list(message = "hidden", call = NULL, sqlstate = "42501"), - class = c("error", "condition") +test_that("warehouse access errors match the shared contract", { + cases <- shared_fixture("catalog-access-errors")$cases + expect_gt(length(cases), 0L) + + for (case in cases) { + err <- structure( + list(message = case$message, call = NULL, sqlstate = case$sqlstate), + class = c("error", "condition") + ) + expect_equal(catalog_access_error_kind(err), case$kind, info = case$name) + } +}) + +test_that("a databricks refusal never names a role", { + before <- list( + backend = "databricks", + principal = "analyst@example.com", + namespace = list(catalog = "main", schema = "default") ) + local_mocked_bindings( + catalog_session_snapshot = function(...) { + list( + backend = "databricks", + principal = "other@example.com", + namespace = list(catalog = "main", schema = "default") + ) + } + ) + + err <- expect_error( + catalog_check_session_snapshot(DBI::ANSI(), before), + class = "commons_catalog_session_changed" + ) + expect_match(conditionMessage(err), "principal changed") + expect_no_match(conditionMessage(err), "role") +}) - expect_equal(catalog_access_error_kind(authorization), "authorization") +test_that("changed session fields match the shared contract", { + cases <- shared_fixture("catalog-session-changed")$cases + expect_gt(length(cases), 0L) + + for (case in cases) { + expect_equal( + catalog_session_changed_fields( + catalog_session_fixture_snapshot(case$after), + catalog_session_fixture_snapshot(case$before) + ), + unlist(case$changed), + info = case$name + ) + } +}) + +test_that("an NA sqlstate is read as no sqlstate", { + # R-only: the fixture carries an empty string for a driver that reported + # no sqlstate, and only a DBI driver can hand back NA_character_ instead. missing_sqlstate <- structure( list(message = "bad syntax", call = NULL, sqlstate = NA_character_), class = c("error", "condition") ) + expect_equal(catalog_access_error_kind(missing_sqlstate), "unknown") - expect_equal( - catalog_access_error_kind(simpleError("warehouse is temporarily unavailable")), - "transient" - ) - expect_equal(catalog_access_error_kind(simpleError("bad syntax")), "unknown") }) test_that("catalog SQL probes bind typed nulls", { @@ -344,6 +394,55 @@ test_that("exact missing warehouse relations retain their diagnostic", { ) }) +test_that("access precedence matches the shared contract", { + cases <- shared_fixture("catalog-access-precedence")$cases + expect_gt(length(cases), 0L) + + for (case in cases) { + script <- catalog_precedence_script(case$relations) + registry <- list( + labels = names(script), + ids = lapply(names(script), function(label) DBI::Id(table = label)) + ) + relations <- lapply(script, function(item) { + list(id = DBI::Id(table = item$label), discovered = item$discovered == "true") + }) + probed <- character() + local_mocked_bindings( + catalog_probe_relation = function(con, id) { + label <- id@name[["table"]] + probed <<- c(probed, label) + catalog_precedence_probe(script[[label]]$probe) + }, + catalog_relation_exists = function(con, id) { + answer <- script[[id@name[["table"]]]]$exists %||% "unknown" + if (identical(answer, "unknown")) NULL else identical(answer, "true") + } + ) + + expected <- case$expected + if (identical(expected$outcome, "ok")) { + expect_no_error( + catalog_require_queryable_relations(DBI::ANSI(), registry, relations) + ) + expect_equal(probed, catalog_precedence_probed(script), info = case$name) + next + } + expectation <- catalog_precedence_expectation(expected$outcome) + err <- expect_error( + catalog_require_queryable_relations(DBI::ANSI(), registry, relations), + class = expectation$class + ) + if (!is.null(expectation$regexp)) { + expect_match(conditionMessage(err), expectation$regexp, info = case$name) + } + for (label in unlist(expected$labels)) { + expect_match(conditionMessage(err), label, fixed = TRUE, info = case$name) + } + expect_equal(probed, catalog_precedence_probed(script), info = case$name) + } +}) + test_that("discovered relations may have an unknown kind", { registry <- list( labels = "hive_metastore.default.sales", diff --git a/tests/shared/README.md b/tests/shared/README.md index bbc57fa7..7ae77d4a 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -37,6 +37,8 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **Definition expansion and rendering.** `definition-rendering.json` pins what happens to a governed definition after the compiler is done with it: which `{{token}}` queries expand and to what, the one-line gist shown at first touch and in retrieval, and the kind index under a character cap. It carries a bank of export records that each package hydrates into its own shape. A refused query pins the refusal and a reason slug rather than the message, because the wording belongs to each language. - **Catalog rows.** `catalog-rows.json` pins how a warehouse listing becomes relations and columns: which rows are relations at all, what kind each is, which comments count as prose, and where a `DESCRIBE` reply stops being columns. The rows are what Snowflake's `SHOW OBJECTS` and `DESC TABLE`, and Databricks' `system.information_schema.tables` and `DESCRIBE TABLE`, actually return. Running those queries is each language's own business; agreeing on their replies is not. Hand-maintained, since no binary generates it, and booleans travel as strings so the file reads the same from both JSON readers. - **The catalog merge.** `catalog-merge.json` pins the contract between a warehouse listing and an authored dictionary: the three limits (object cap, prompt threshold, search probe bound), exclusion-glob behavior over bare table names, and merge scenarios — authored prose wins, warehouse types and nullability win, identifier case folds per backend, ambiguity is an error — each with the expected merged tables and definition bindings. Error cases pin a slug rather than message text, because the wording belongs to each language. +- **Catalog access errors.** `catalog-access-errors.json` pins how a failed access probe is read: a SQLSTATE and a message become `authorization`, `transient`, or `unknown`, which decides what the user is told and whether the answer is cached per relation. Hand-maintained, since the cases are the failures real drivers report rather than anything a binary emits. An absent SQLSTATE travels as an empty string, because JSON has no way to spell R's `NA_character_`. +- **Session identity and access precedence.** `catalog-session-changed.json` pins which parts of a warehouse session a refusal reports as changed, so neither package tells a Databricks user that a role moved on a backend that has none. `catalog-access-precedence.json` pins what construction reports when several named relations fail at once: every relation the listing reported is probed before anything is raised, and a name the warehouse never listed is reported ahead of a refusal. Both are hand-maintained, and both carry field names rather than message text, since the wording belongs to each language. - **Trace file naming.** `trace(-[0-9]+)?\.jsonl`, one OTLP envelope per line. ## Conventions diff --git a/tests/shared/catalog-access-errors.json b/tests/shared/catalog-access-errors.json new file mode 100644 index 00000000..bce22ef1 --- /dev/null +++ b/tests/shared/catalog-access-errors.json @@ -0,0 +1,101 @@ +{ + "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message.", + "cases": [ + { + "name": "insufficient privilege sqlstate", + "sqlstate": "42501", + "message": "hidden", + "kind": "authorization" + }, + { + "name": "invalid authorization sqlstate class", + "sqlstate": "28000", + "message": "hidden", + "kind": "authorization" + }, + { + "name": "snowflake access control message", + "sqlstate": "", + "message": "SQL access control error: Insufficient privileges to operate on table 'SALES'", + "kind": "authorization" + }, + { + "name": "databricks permission message", + "sqlstate": "", + "message": "PERMISSION_DENIED: User does not have SELECT on Table 'main.default.sales'", + "kind": "authorization" + }, + { + "name": "plain permission denied message", + "sqlstate": "", + "message": "Permission denied on relation sales", + "kind": "authorization" + }, + { + "name": "a network policy refusal is authorization, not a network fault", + "sqlstate": "", + "message": "Incoming request with IP 10.0.0.1 is not allowed to access Snowflake", + "kind": "authorization" + }, + { + "name": "connection exception sqlstate class", + "sqlstate": "08006", + "message": "hidden", + "kind": "transient" + }, + { + "name": "timeout sqlstate", + "sqlstate": "HYT00", + "message": "hidden", + "kind": "transient" + }, + { + "name": "serialization failure sqlstate", + "sqlstate": "40001", + "message": "hidden", + "kind": "transient" + }, + { + "name": "admin shutdown sqlstate", + "sqlstate": "57P01", + "message": "hidden", + "kind": "transient" + }, + { + "name": "warehouse starting message", + "sqlstate": "", + "message": "Warehouse 'COMPUTE_WH' is starting", + "kind": "transient" + }, + { + "name": "a temporarily unavailable warehouse", + "sqlstate": "", + "message": "Warehouse is temporarily unavailable", + "kind": "transient" + }, + { + "name": "rate limited message", + "sqlstate": "", + "message": "HTTP 429 too many requests", + "kind": "transient" + }, + { + "name": "lowercase sqlstate is still matched", + "sqlstate": "hyt00", + "message": "hidden", + "kind": "transient" + }, + { + "name": "syntax error with a sqlstate", + "sqlstate": "42601", + "message": "syntax error at or near SELCT", + "kind": "unknown" + }, + { + "name": "unrecognized failure without a sqlstate", + "sqlstate": "", + "message": "column BOGUS not found", + "kind": "unknown" + } + ] +} diff --git a/tests/shared/catalog-access-precedence.json b/tests/shared/catalog-access-precedence.json new file mode 100644 index 00000000..6a9ff6f2 --- /dev/null +++ b/tests/shared/catalog-access-precedence.json @@ -0,0 +1,83 @@ +{ + "description": "What a source construction reports when several explicitly named relations fail at once. Every relation the listing did report is probed before anything is raised, so a name the caller got wrong and a relation they cannot read are found in one pass rather than one round trip each. A name the warehouse never listed is reported ahead of a refusal: it is the more actionable problem, and an access error about a table that is not there reads as a permissions problem the caller does not have. Booleans travel as strings so the file reads the same from both JSON readers. `discovered` is whether the listing reported the relation, `probe` is what the zero-row query did, and `exists` is what the backend answers about an unexplained failure.", + "cases": [ + { + "name": "every relation is readable", + "relations": [ + {"label": "A", "discovered": "true", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "queryable"} + ], + "expected": {"outcome": "ok", "labels": []} + }, + { + "name": "a name the listing never reported", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "queryable"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "a missing name is reported ahead of a refusal", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "a relation the probe proves absent is missing too", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "false"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "missing", "labels": ["A"]} + }, + { + "name": "every missing name is reported at once", + "relations": [ + {"label": "A", "discovered": "false", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "unknown", "exists": "false"} + ], + "expected": {"outcome": "missing", "labels": ["A", "B"]} + }, + { + "name": "the first refusal is the one raised", + "relations": [ + {"label": "A", "discovered": "true", "probe": "authorization"}, + {"label": "B", "discovered": "true", "probe": "transient"} + ], + "expected": {"outcome": "authorization", "labels": ["A"]} + }, + { + "name": "the first refusal wins even when a later one is an authorization", + "relations": [ + {"label": "A", "discovered": "true", "probe": "transient"}, + {"label": "B", "discovered": "true", "probe": "authorization"} + ], + "expected": {"outcome": "transient", "labels": ["A"]} + }, + { + "name": "a transient refusal on its own", + "relations": [ + {"label": "A", "discovered": "true", "probe": "queryable"}, + {"label": "B", "discovered": "true", "probe": "transient"} + ], + "expected": {"outcome": "transient", "labels": ["B"]} + }, + { + "name": "an unexplained failure the backend cannot explain away", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "unknown"} + ], + "expected": {"outcome": "access", "labels": ["A"]} + }, + { + "name": "an unexplained failure on a relation that is there", + "relations": [ + {"label": "A", "discovered": "true", "probe": "unknown", "exists": "true"} + ], + "expected": {"outcome": "access", "labels": ["A"]} + } + ] +} diff --git a/tests/shared/catalog-session-changed.json b/tests/shared/catalog-session-changed.json new file mode 100644 index 00000000..957b0223 --- /dev/null +++ b/tests/shared/catalog-session-changed.json @@ -0,0 +1,41 @@ +{ + "description": "Which parts of a warehouse session identity a refusal reports as changed. The comparison runs before every catalog operation, and the answer decides what the user is told to look at, so it is pinned rather than described twice. A field is absent (null) when the backend has none: Databricks reports no role, so a refusal there must never name one. The names here are the snapshot's own fields; each implementation words them for its own message.", + "cases": [ + { + "name": "a snowflake role change", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["role"] + }, + { + "name": "a snowflake secondary role change", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "NONE", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["secondary_roles"] + }, + { + "name": "a databricks principal change names no role", + "before": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "after": {"backend": "databricks", "principal": "other@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "changed": ["principal"] + }, + { + "name": "a databricks namespace change names both parts", + "before": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "main", "schema": "default"}, + "after": {"backend": "databricks", "principal": "analyst@example.com", "role": null, "secondary_roles": null, "catalog": "sandbox", "schema": "scratch"}, + "changed": ["catalog", "schema"] + }, + { + "name": "several fields at once, in snapshot order", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ADMIN_USER", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "STAGING"}, + "changed": ["principal", "role", "schema"] + }, + { + "name": "a role that was dropped rather than swapped", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": {"backend": "snowflake", "principal": "ANALYST", "role": null, "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "changed": ["role"] + } + ] +} From 2af2fd40687873d53c946c5c25cb378c03f9e534 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Sun, 6 Sep 2026 13:04:07 -0600 Subject: [PATCH 4/5] fix: read an access failure from the driver's own complaint, and label an exact selection where it resolved Three findings from the review of this branch. The access classifier matched its regexes against the whole exception text. SQLAlchemy appends `[SQL: ...]` to the wrapper it raises, and odbc appends a `` line, so the probe statement, which names the relation, was always inside the text being scanned. An absent `PERMISSION_DENIED_EVENTS` classified as an authorization refusal and was cached as one for the life of the source; an absent `NETWORK_EVENTS` classified as transient. Both packages now read the driver's own message, and `catalog-access-errors.json` grows a `sql` field so each suite attaches the statement the way its own drivers repeat it back. R labelled a confirmed bare `tables` entry with the authored name and Python with the name qualified by the namespace the lookup resolved in. The label is what the agent sees and what every access error names, so the two now agree on Python's: `catalog_match_exact_relation()` takes the resolved id, the registry validates under the relation's own id rather than the authored one, and the warehouse's id stays on `identity`. An entry the warehouse never reported still keeps the authored name, since there is no confirmed namespace to qualify it with. `catalog-relation-labels.json` pins all of it. R named every snapshot field when a connection reported no session at all, which is the same mistake as naming a role on Databricks. It now falls back to the identity as a whole, as Python already did. Also: scope the role claim in the Python module docstring to Snowflake, the way the roxygen and the vignette already do; cache a refusal without the traceback that pins the failing frames and their connection; read the skip test from a set and tolerate a `relations` mapping that lacks a label; and assert the exact refusal class rather than a base one in the precedence runner. Covers the rendered multi-field refusal, the identity fallback, the cached refusal's cause, the `orig` SQLSTATE branch, and the unknown-state retry, none of which any test reached before. --- pkg-py/src/commons/_catalog/_security.py | 67 +++++++++-- pkg-py/tests/test_catalog_security.py | 96 +++++++++++++++ .../tests/test_catalog_security_fixtures.py | 88 +++++++++++++- pkg-r/R/catalog-databricks.R | 2 +- pkg-r/R/catalog-security.R | 27 ++++- pkg-r/R/catalog-snowflake.R | 9 +- pkg-r/R/catalog.R | 14 ++- .../shared/catalog-access-errors.json | 23 +++- .../shared/catalog-relation-labels.json | 37 ++++++ .../shared/catalog-session-changed.json | 8 +- pkg-r/tests/testthat/helper-catalog-rows.R | 12 ++ pkg-r/tests/testthat/test-catalog-security.R | 111 +++++++++++++++++- pkg-r/tests/testthat/test-catalog-snowflake.R | 61 ++++++++++ tests/shared/README.md | 5 +- tests/shared/catalog-access-errors.json | 23 +++- tests/shared/catalog-relation-labels.json | 37 ++++++ tests/shared/catalog-session-changed.json | 8 +- 17 files changed, 598 insertions(+), 30 deletions(-) create mode 100644 pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json create mode 100644 tests/shared/catalog-relation-labels.json diff --git a/pkg-py/src/commons/_catalog/_security.py b/pkg-py/src/commons/_catalog/_security.py index 4dae817b..09140f0b 100644 --- a/pkg-py/src/commons/_catalog/_security.py +++ b/pkg-py/src/commons/_catalog/_security.py @@ -8,8 +8,9 @@ retries; anything unrecognized is neither cached nor treated as a refusal. The session snapshot exists for the same reason. Access was decided for the -principal, role, and namespace in force at discovery, so if any of those -change the answers no longer apply and the source has to be rebuilt. +principal and namespace in force at discovery, and on Snowflake for its +active and secondary roles as well, so if any of those change the answers no +longer apply and the source has to be rebuilt. """ from __future__ import annotations @@ -22,6 +23,7 @@ from ._core import Manifest, Relation __all__ = [ + "CachedRefusal", "CatalogAccessError", "CatalogAuthorizationError", "CatalogSessionChangedError", @@ -55,6 +57,10 @@ _TRANSIENT_SQLSTATE = re.compile("^08|^HYT|^40|^57P01") +# SQLAlchemy appends the statement, and its parameters, to the wrapper it +# raises. +_STATEMENT_TAIL = re.compile(r"\n\[(SQL|parameters): .*", re.DOTALL) + class CatalogAccessError(Exception): """Query access to a relation could not be verified.""" @@ -72,6 +78,21 @@ class CatalogSessionChangedError(Exception): """The connection's identity moved after the catalog was discovered.""" +class CachedRefusal(Exception): + """A driver's refusal, kept without the frames that produced it.""" + + +def _without_frames(error: BaseException) -> BaseException: + """The driver's complaint, detached from the stack that raised it. + + A cached refusal outlives the failure by the life of the data source, and + an exception holds its traceback, which holds every frame below it and + the connection those frames were using. The type name and the message are + what a later `raise ... from` needs; the frames are not. + """ + return CachedRefusal(f"{type(error).__name__}: {error}") + + @dataclass(frozen=True) class SessionSnapshot: """The connection identity the catalog's access answers were decided for.""" @@ -93,8 +114,10 @@ class Probe: def session_snapshot(backend: Any) -> SessionSnapshot | None: """Read the identity a Snowflake or Databricks connection is acting under. - Any other backend returns None: nothing else commons supports scopes - catalog access to a role that can change underneath the source. + Both report a principal and a namespace, and Snowflake its active and + secondary roles as well. Any other backend returns None: nothing else + commons supports scopes catalog access to an identity that can change + underneath the source. """ dialect = backend.dialect() if dialect == "snowflake": @@ -179,7 +202,7 @@ def _listed(items: Any) -> str: def changed_session_fields( current: SessionSnapshot | None, snapshot: SessionSnapshot ) -> list[str]: - """Which parts of the session identity differ, in snapshot order. + """Which parts of the session identity differ, in `_FIELD_NAMES` order. The refusal names what moved rather than everything it compares, because a backend need not have every field: Databricks reports no role, and @@ -214,7 +237,7 @@ def classify_access_error(error: BaseException) -> str: is neither cached nor retried on its own. """ sqlstate = _sqlstate(error) - message = str(error) + message = _driver_message(error) if ( sqlstate.startswith("28") or sqlstate == "42501" @@ -226,6 +249,17 @@ def classify_access_error(error: BaseException) -> str: return "unknown" +def _driver_message(error: BaseException) -> str: + """What the driver complained about, without the statement that caused it. + + The probe names the relation, so classifying the wrapper's text would let + a table decide its own answer: a `permission_denied_events` that is merely + absent would read as a refusal, and be cached as one. + """ + driver = getattr(error, "orig", None) or error.__cause__ or error + return _STATEMENT_TAIL.sub("", str(driver)) + + def _sqlstate(error: BaseException) -> str: """The SQLSTATE a driver reported, wherever it hung it. @@ -269,13 +303,15 @@ def require_queryable_relations( missing = [ label for label in validate - if relations is not None and not relations[label].discovered + if relations is not None and _undiscovered(relations.get(label)) ] - # Only the first refusal is raised, so only the first is kept: a - # selection may run to thousands of relations. + # A selection may run to thousands of relations, so the skip test reads + # from a set and the list is kept only to order the message. + skip = set(missing) + # Only the first refusal is raised, so only the first is kept. refused: tuple[Probe, str] | None = None for label, table_id in validate.items(): - if label in missing: + if label in skip: continue probe = probe_relation(backend, table_id) if probe.state == "queryable": @@ -290,6 +326,15 @@ def require_queryable_relations( _abort_access(*refused) +def _undiscovered(relation: Relation | None) -> bool: + """Whether the listing reported this relation, tolerating an absent entry. + + A caller that passes a `relations` mapping the labels do not line up with + gets its relation probed rather than an error about the mapping. + """ + return relation is not None and not relation.discovered + + def _relation_exists(backend: Any, table_id: TableId) -> bool | None: inspector = backend.inspector() if inspector is None: @@ -327,7 +372,7 @@ def ensure_queryable( if probe.state == "authorization": manifest.access[label] = probe.state if probe.error is not None: - manifest.access_errors[label] = probe.error + manifest.access_errors[label] = _without_frames(probe.error) _abort_access(probe, label) diff --git a/pkg-py/tests/test_catalog_security.py b/pkg-py/tests/test_catalog_security.py index f9ec3a1b..6409f3aa 100644 --- a/pkg-py/tests/test_catalog_security.py +++ b/pkg-py/tests/test_catalog_security.py @@ -8,9 +8,11 @@ from typing import Any import pytest +import sqlalchemy.exc from commons._catalog import Manifest, Relation from commons._catalog._security import ( + CachedRefusal, CatalogAccessError, CatalogAuthorizationError, CatalogSessionChangedError, @@ -157,6 +159,31 @@ def test_a_databricks_refusal_never_names_a_role(): assert "role" not in str(refusal.value) +def test_a_refusal_reads_several_changed_fields_as_a_list(): + taken = session_snapshot(FakeBackend([[snowflake_row()]])) + moved = FakeBackend( + [[{**snowflake_row(role="ADMIN"), "PRINCIPAL": "OTHER", "SCHEMA": "SALES"}]] + ) + + with pytest.raises(CatalogSessionChangedError) as refusal: + check_session(moved, taken) + + assert "principal, active role and schema changed" in str(refusal.value) + + +def test_a_refusal_with_nothing_to_name_falls_back_to_the_identity(): + taken = session_snapshot(FakeBackend([[snowflake_row()]])) + assert taken is not None + # A backend that stops reporting a session at all: nothing compares, so + # there is no field to name and the refusal says so rather than nothing. + moved = FakeBackend(dialect="duckdb") + + with pytest.raises(CatalogSessionChangedError) as refusal: + check_session(moved, taken) + + assert "The connection identity changed" in str(refusal.value) + + def test_an_unchanged_session_passes(): taken = session_snapshot(FakeBackend([[snowflake_row()]])) backend = FakeBackend([[snowflake_row()]]) @@ -215,6 +242,50 @@ def test_authorization_failures_are_cached_per_relation(): assert len(backend.queries) == 1 +def test_a_cached_refusal_still_names_what_the_driver_said(): + manifest = Manifest.build({"sales": Relation(id=SALES)}) + backend = FakeBackend([DriverError("permission denied on relation sales")]) + + with pytest.raises(CatalogAuthorizationError): + ensure_queryable(backend, manifest, "sales", SALES) + with pytest.raises(CatalogAuthorizationError) as cached: + ensure_queryable(backend, manifest, "sales", SALES) + + # The second refusal never touched the warehouse, so what it is raised + # from has to come from the cache rather than from a fresh probe. + assert backend.queries == [backend.queries[0]] + assert isinstance(cached.value.__cause__, CachedRefusal) + assert "permission denied on relation sales" in str(cached.value.__cause__) + + +def test_a_cached_refusal_does_not_hold_on_to_the_failing_stack(): + manifest = Manifest.build({"sales": Relation(id=SALES)}) + backend = FakeBackend([DriverError("permission denied")]) + + with pytest.raises(CatalogAuthorizationError): + ensure_queryable(backend, manifest, "sales", SALES) + + # An exception's traceback pins every frame below it, and the connection + # those frames were using, for as long as the manifest lives. + assert manifest.access_errors["sales"].__traceback__ is None + + +def test_an_unrecognized_failure_is_neither_cached_nor_a_refusal(): + manifest = Manifest.build({"sales": Relation(id=SALES)}) + backend = FakeBackend([DriverError("something odd"), DriverError("something odd")]) + + for _ in range(2): + with pytest.raises(CatalogAccessError) as failure: + ensure_queryable(backend, manifest, "sales", SALES) + assert type(failure.value) is CatalogAccessError + + # Neither remembered nor treated as a refusal, so the next touch retries + # rather than being answered from the cache. + assert manifest.access["sales"] == "unknown" + assert "sales" not in manifest.access_errors + assert len(backend.queries) == 2 + + def test_a_relation_already_known_queryable_is_not_probed_again(): manifest = Manifest.build({"sales": Relation(id=SALES)}) manifest.access["sales"] = "queryable" @@ -325,3 +396,28 @@ def test_a_chained_driver_error_still_yields_its_sqlstate(): outer.__cause__ = inner assert classify_access_error(outer) == "authorization" + + +def test_a_wrapped_driver_error_yields_its_sqlstate_through_orig(): + # The branch real failures take: SQLAlchemy hangs the driver's own + # exception off `orig` rather than chaining it. + wrapped = sqlalchemy.exc.ProgrammingError( + 'SELECT * FROM "ANALYTICS"."PUBLIC"."SALES" WHERE 1 = 0', + {}, + DriverError("hidden", sqlstate="42501"), + ) + + assert classify_access_error(wrapped) == "authorization" + + +def test_the_probe_statement_does_not_decide_the_classification(): + # SQLAlchemy repeats the statement back, and the probe names the + # relation, so a table called permission_denied_events would otherwise + # read as a refusal and be cached as one. + wrapped = sqlalchemy.exc.ProgrammingError( + 'SELECT * FROM "AUDIT"."PUBLIC"."PERMISSION_DENIED_EVENTS" WHERE 1 = 0', + {}, + DriverError("Object does not exist", sqlstate="42S02"), + ) + + assert classify_access_error(wrapped) == "unknown" diff --git a/pkg-py/tests/test_catalog_security_fixtures.py b/pkg-py/tests/test_catalog_security_fixtures.py index 29fadb2a..933973e2 100644 --- a/pkg-py/tests/test_catalog_security_fixtures.py +++ b/pkg-py/tests/test_catalog_security_fixtures.py @@ -7,9 +7,13 @@ once and read by both suites. """ +import functools + import pytest +import sqlalchemy.exc -from commons._catalog import Relation +from commons._catalog import Relation, Selector, table_registry +from commons._catalog import _snowflake as snowflake from commons._catalog._security import ( CatalogAccessError, CatalogAuthorizationError, @@ -34,8 +38,16 @@ def test_access_errors_match_the_shared_contract(): assert cases for case in cases: - error = DriverError(case["message"], case["sqlstate"] or None) - assert classify_access_error(error) == case["kind"], case["name"] + assert classify_access_error(_driver_error(case)) == case["kind"], case["name"] + + +def _driver_error(case): + error = DriverError(case["message"], case["sqlstate"] or None) + if not case.get("sql"): + return error + # SQLAlchemy repeats the statement back in the wrapper it raises, which + # is how the probe's own relation name reaches the classifier. + return sqlalchemy.exc.ProgrammingError(case["sql"], {}, error) def test_changed_session_fields_match_the_shared_contract(): @@ -44,10 +56,14 @@ def test_changed_session_fields_match_the_shared_contract(): for case in cases: before, after = _snapshot(case["before"]), _snapshot(case["after"]) + assert before is not None, case["name"] assert changed_session_fields(after, before) == case["changed"], case["name"] def _snapshot(fields): + # A null case is a connection that reports no session at all. + if fields is None: + return None return SessionSnapshot( backend=fields["backend"], principal=fields["principal"], @@ -124,6 +140,10 @@ def test_access_precedence_matches_the_shared_contract(): else: with pytest.raises(_OUTCOMES[expected["outcome"]]) as refusal: require_queryable_relations(backend, validate, relations) + # The exact class, not a base one: an authorization refusal and an + # unexplained failure are both CatalogAccessError, and the fixture + # is pinning which of the two the caller gets. + assert type(refusal.value) is _OUTCOMES[expected["outcome"]], case["name"] for label in expected["labels"]: assert label in str(refusal.value), case["name"] # The contract is that nothing is raised until every relation the @@ -131,3 +151,65 @@ def test_access_precedence_matches_the_shared_contract(): assert backend.probed == [ item["label"] for item in case["relations"] if item["discovered"] == "true" ], case["name"] + + +class _LabelBackend: + """A Snowflake connection whose listing answers from the fixture case.""" + + def __init__(self, namespace, reported): + self._namespace = namespace + self._reported = reported + + def query(self, sql: str): + if "CURRENT_DATABASE" in sql: + return [ + { + "catalog": self._namespace["catalog"], + "schema": self._namespace["schema"], + } + ] + if self._reported is None: + return [] + return [ + { + "name": self._reported["table"], + "kind": "TABLE", + "database_name": self._reported["catalog"], + "schema_name": self._reported["schema"], + "comment": None, + } + ] + + def dialect(self): + return "snowflake" + + +def test_relation_labels_match_the_shared_contract(): + cases = load_shared_fixture("catalog-relation-labels")["cases"] + assert cases + + for case in cases: + authored = case["authored"] + backend = _LabelBackend(case["namespace"], case["reported"]) + registry = table_registry( + selectors=[ + Selector( + catalog=authored.get("catalog"), + schema=authored.get("schema"), + table=authored["table"], + ) + ], + exact_relation=functools.partial(snowflake.exact_relation, backend), + list_relations=lambda selector: [], + ) + + assert list(registry.relations) == [case["label"]], case["name"] + # The access check pairs the two lists by label, so a selection entry + # has to be validated under the label it ended up with. + assert list(registry.validate) == [case["label"]], case["name"] + + identity = registry.relations[case["label"]].identity + assert (identity.label if identity else None) == case["identity"], case["name"] + assert registry.relations[case["label"]].discovered is ( + case["reported"] is not None + ), case["name"] diff --git a/pkg-r/R/catalog-databricks.R b/pkg-r/R/catalog-databricks.R index 3e73328f..306ed9f4 100644 --- a/pkg-r/R/catalog-databricks.R +++ b/pkg-r/R/catalog-databricks.R @@ -300,7 +300,7 @@ databricks_exact_relation <- function(con, id, call = rlang::caller_env()) { } else { databricks_list_unity_relations(con, complete, call = call) } - catalog_match_exact_relation(relations, id) + catalog_match_exact_relation(relations, id, requested = complete) } databricks_relations_from_information_schema <- function( diff --git a/pkg-r/R/catalog-security.R b/pkg-r/R/catalog-security.R index 5c40dae7..0abbbf34 100644 --- a/pkg-r/R/catalog-security.R +++ b/pkg-r/R/catalog-security.R @@ -141,6 +141,12 @@ catalog_session_field_names <- c( # Name what moved rather than everything the snapshot compares: Databricks # has no role, so a fixed list would name a field that backend never had. catalog_session_changed_fields <- function(current, snapshot) { + # A connection reporting no session at all has no field that moved, and + # naming all of them would be the same mistake as naming a role Databricks + # never had. The refusal falls back to the identity as a whole. + if (is.null(current)) { + return(character()) + } fields <- names(catalog_session_field_names) fields[vapply( fields, @@ -188,13 +194,28 @@ catalog_probe_sql <- function(con, sql, bindings = list()) { ) } +# What the driver complained about, without the statement that caused it. +# The probe names the relation, so classifying the whole message would let a +# table decide its own answer: a permission_denied_events that is merely +# absent would read as a refusal, and be cached as one. odbc appends the +# statement on a line. +catalog_driver_message <- function(err) { + driver <- err$parent %||% err + sub("\n? '.*", "", conditionMessage(driver)) +} + catalog_access_error_kind <- function(err) { sqlstate <- toupper(as.character( - err$sqlstate %||% err$state %||% err$parent$sqlstate %||% "" + err$sqlstate %||% + err$state %||% + err$parent$sqlstate %||% + err$parent$state %||% + "" )) if (length(sqlstate) != 1L || is.na(sqlstate)) { sqlstate <- "" } + message <- catalog_driver_message(err) if ( startsWith(sqlstate, "28") || identical(sqlstate, "42501") || @@ -205,7 +226,7 @@ catalog_access_error_kind <- function(err) { "permission_denied|sql access control error|not allowed to access", sep = "" ), - conditionMessage(err), + message, ignore.case = TRUE ) ) { @@ -221,7 +242,7 @@ catalog_access_error_kind <- function(err) { "network|socket|http (429|503)|unexpected eof", sep = "" ), - conditionMessage(err), + message, ignore.case = TRUE ) ) { diff --git a/pkg-r/R/catalog-snowflake.R b/pkg-r/R/catalog-snowflake.R index 1a1a598c..efb272f3 100644 --- a/pkg-r/R/catalog-snowflake.R +++ b/pkg-r/R/catalog-snowflake.R @@ -428,7 +428,14 @@ snowflake_exact_relation <- function(con, id, call = rlang::caller_env()) { } ) relations <- snowflake_relations_from_show(rows) - catalog_match_exact_relation(relations, id) + catalog_match_exact_relation( + relations, + id, + requested = do.call( + DBI::Id, + as.list(c(namespace@name, table = unname(components[["table"]]))) + ) + ) } snowflake_relations_from_show <- function(rows) { diff --git a/pkg-r/R/catalog.R b/pkg-r/R/catalog.R index 3fb542d7..c98653d9 100644 --- a/pkg-r/R/catalog.R +++ b/pkg-r/R/catalog.R @@ -23,7 +23,11 @@ catalog_table_registry <- function( if (identical(type, "relation")) { relation <- exact_relation(con, id, call = call) relations[[length(relations) + 1L]] <- relation - validate[[length(validate) + 1L]] <- id + # Keyed by the relation's own id rather than the authored one: an entry + # naming a bare table is qualified with the connection's namespace once + # the warehouse answers, and the two lists have to agree on the label + # or the access check cannot pair them up. + validate[[length(validate) + 1L]] <- relation$id next } namespace_selected <- TRUE @@ -284,7 +288,11 @@ catalog_id_type <- function(id, backend, call = rlang::caller_env()) { if ("table" %in% roles) "relation" else "namespace" } -catalog_match_exact_relation <- function(relations, id) { +# `requested` is the authored name qualified with the namespace the lookup +# ran in, and becomes the relation's label once the warehouse confirms it. +# The warehouse's own id is kept as `identity`, since it carries that +# backend's casing and is what later metadata queries have to name. +catalog_match_exact_relation <- function(relations, id, requested = id) { requested_name <- id@name[["table"]] is_requested <- vapply( relations, @@ -304,7 +312,7 @@ catalog_match_exact_relation <- function(relations, id) { relation <- relations[[which(is_requested)[[1]]]] relation$identity <- relation$id - relation$id <- id + relation$id <- requested relation$discovered <- TRUE relation } diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json b/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json index bce22ef1..a608ac72 100644 --- a/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-access-errors.json @@ -1,5 +1,5 @@ { - "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message.", + "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message. A case with a `sql` field asserts that only the driver's own complaint is read: both packages wrap a failing statement in a way that repeats it back, so each suite attaches that statement the way its own drivers do, and the expected kind stays the one the message alone earns.", "cases": [ { "name": "insufficient privilege sqlstate", @@ -96,6 +96,27 @@ "sqlstate": "", "message": "column BOGUS not found", "kind": "unknown" + }, + { + "name": "an absent relation whose name reads as a refusal", + "sqlstate": "42S02", + "message": "Object does not exist, or operation cannot be performed.", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"PERMISSION_DENIED_EVENTS\" WHERE 1 = 0", + "kind": "unknown" + }, + { + "name": "an absent relation whose name reads as transient", + "sqlstate": "42S02", + "message": "Object does not exist, or operation cannot be performed.", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"NETWORK_SOCKET_TIMEOUTS\" WHERE 1 = 0", + "kind": "unknown" + }, + { + "name": "a genuine refusal is still read through the statement", + "sqlstate": "", + "message": "SQL access control error: Insufficient privileges to operate on table 'ORDERS'", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"ORDERS\" WHERE 1 = 0", + "kind": "authorization" } ] } diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json b/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json new file mode 100644 index 00000000..b4534acc --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json @@ -0,0 +1,37 @@ +{ + "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers.", + "cases": [ + { + "name": "a bare entry is qualified with the connection namespace", + "authored": {"table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "PUBLIC", "table": "ORDERS"}, + "label": "ANALYTICS.PUBLIC.ORDERS", + "identity": "ANALYTICS.PUBLIC.ORDERS" + }, + { + "name": "an entry naming its own schema is not given a catalog", + "authored": {"schema": "SALES", "table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "label": "SALES.ORDERS", + "identity": "ANALYTICS.SALES.ORDERS" + }, + { + "name": "a fully qualified entry is already its own label", + "authored": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "label": "ANALYTICS.SALES.ORDERS", + "identity": "ANALYTICS.SALES.ORDERS" + }, + { + "name": "an entry the warehouse never reported keeps the authored name", + "authored": {"table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": null, + "label": "ORDERS", + "identity": null + } + ] +} diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json b/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json index 957b0223..50713e93 100644 --- a/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-session-changed.json @@ -26,7 +26,7 @@ "changed": ["catalog", "schema"] }, { - "name": "several fields at once, in snapshot order", + "name": "several fields at once, in the order the refusal names them", "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "after": {"backend": "snowflake", "principal": "ADMIN_USER", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "STAGING"}, "changed": ["principal", "role", "schema"] @@ -36,6 +36,12 @@ "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "after": {"backend": "snowflake", "principal": "ANALYST", "role": null, "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "changed": ["role"] + }, + { + "name": "a connection reporting no session at all names no field", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": null, + "changed": [] } ] } diff --git a/pkg-r/tests/testthat/helper-catalog-rows.R b/pkg-r/tests/testthat/helper-catalog-rows.R index d73df2e8..54f868a2 100644 --- a/pkg-r/tests/testthat/helper-catalog-rows.R +++ b/pkg-r/tests/testthat/helper-catalog-rows.R @@ -57,6 +57,10 @@ catalog_rows_expect_columns <- function(columns, expected, info) { # A session snapshot from the shared fixture's field-by-field spelling. catalog_session_fixture_snapshot <- function(fields) { + # A null case is a connection that reports no session at all. + if (is.null(fields)) { + return(NULL) + } list( backend = fields$backend, principal = fields$principal, @@ -98,3 +102,11 @@ catalog_precedence_probed <- function(script) { logical(1) )] } + +# The relation-label fixture's authored entry, as a `tables` string. +catalog_labels_authored <- function(authored) { + paste( + c(authored$catalog, authored$schema, authored$table), + collapse = "." + ) +} diff --git a/pkg-r/tests/testthat/test-catalog-security.R b/pkg-r/tests/testthat/test-catalog-security.R index 3c2d9e3c..c5825356 100644 --- a/pkg-r/tests/testthat/test-catalog-security.R +++ b/pkg-r/tests/testthat/test-catalog-security.R @@ -80,6 +80,55 @@ test_that("catalog operations reject changed sessions", { ) }) +test_that("a refusal reads several changed fields as a list", { + before <- list( + backend = "snowflake", + principal = "ANALYST", + role = "REPORTER", + secondary_roles = '{"roles":"READER","value":"ALL"}', + namespace = list(catalog = "ANALYTICS", schema = "PUBLIC") + ) + local_mocked_bindings( + catalog_session_snapshot = function(...) { + list( + backend = "snowflake", + principal = "OTHER", + role = "ADMIN", + secondary_roles = '{"roles":"READER","value":"ALL"}', + namespace = list(catalog = "ANALYTICS", schema = "SALES") + ) + } + ) + + err <- expect_error( + catalog_check_session_snapshot(DBI::ANSI(), before), + class = "commons_catalog_session_changed" + ) + expect_match( + conditionMessage(err), + "principal, active role, and schema changed" + ) +}) + +test_that("a refusal with nothing to name falls back to the identity", { + before <- list( + backend = "snowflake", + principal = "ANALYST", + role = "REPORTER", + secondary_roles = '{"roles":"READER","value":"ALL"}', + namespace = list(catalog = "ANALYTICS", schema = "PUBLIC") + ) + # A connection that stops reporting a session at all: nothing compares, so + # there is no field to name and the refusal says so rather than nothing. + local_mocked_bindings(catalog_session_snapshot = function(...) NULL) + + err <- expect_error( + catalog_check_session_snapshot(DBI::ANSI(), before), + class = "commons_catalog_session_changed" + ) + expect_match(conditionMessage(err), "The connection identity changed") +}) + test_that("transient access failures remain retryable", { source <- catalog_security_test_source() state <- data_source_state(source) @@ -127,13 +176,71 @@ test_that("authorization failures are cached per relation", { expect_equal(calls, 1L) }) +test_that("a cached refusal still names what the driver said", { + source <- catalog_security_test_source() + local_mocked_bindings( + catalog_probe_relation = function(...) { + list( + state = "authorization", + error = simpleError("permission denied on relation sales") + ) + } + ) + + expect_error( + catalog_ensure_queryable(source, "sales"), + class = "commons_catalog_authorization_error" + ) + # The second refusal never touched the warehouse, so what it is raised from + # has to come from the cache rather than from a fresh probe. + cached <- expect_error( + catalog_ensure_queryable(source, "sales"), + class = "commons_catalog_authorization_error" + ) + expect_match( + conditionMessage(cached$parent), + "permission denied on relation sales" + ) +}) + +test_that("an unrecognized failure is neither cached nor a refusal", { + source <- catalog_security_test_source() + state <- data_source_state(source) + calls <- 0L + local_mocked_bindings( + catalog_probe_relation = function(...) { + calls <<- calls + 1L + list(state = "unknown", error = simpleError("something odd")) + } + ) + + for (i in 1:2) { + expect_error( + catalog_ensure_queryable(source, "sales"), + class = "commons_catalog_access_error" + ) + } + + # Neither remembered nor treated as a refusal, so the next touch retries + # rather than being answered from the cache. + expect_equal(state$manifest$access[["sales"]], "unknown") + expect_null(state$manifest$access_errors[["sales"]]) + expect_equal(calls, 2L) +}) + test_that("warehouse access errors match the shared contract", { cases <- shared_fixture("catalog-access-errors")$cases expect_gt(length(cases), 0L) for (case in cases) { + message <- case$message + if (!is.null(case$sql)) { + # odbc repeats the statement back on a line, which is how the + # probe's own relation name reaches the classifier. + message <- paste0(message, "\n '", case$sql, "'") + } err <- structure( - list(message = case$message, call = NULL, sqlstate = case$sqlstate), + list(message = message, call = NULL, sqlstate = case$sqlstate), class = c("error", "condition") ) expect_equal(catalog_access_error_kind(err), case$kind, info = case$name) @@ -174,7 +281,7 @@ test_that("changed session fields match the shared contract", { catalog_session_fixture_snapshot(case$after), catalog_session_fixture_snapshot(case$before) ), - unlist(case$changed), + as.character(unlist(case$changed)), info = case$name ) } diff --git a/pkg-r/tests/testthat/test-catalog-snowflake.R b/pkg-r/tests/testthat/test-catalog-snowflake.R index cdb8456f..b8ba6f0e 100644 --- a/pkg-r/tests/testthat/test-catalog-snowflake.R +++ b/pkg-r/tests/testthat/test-catalog-snowflake.R @@ -530,3 +530,64 @@ test_that("Snowflake current namespace requires a database and schema", { error = TRUE ) }) + +test_that("relation labels match the shared contract", { + cases <- shared_fixture("catalog-relation-labels")$cases + expect_gt(length(cases), 0L) + + for (case in cases) { + local_mocked_bindings( + dbGetQuery = function(conn, statement, ...) { + if (grepl("CURRENT_DATABASE", statement, fixed = TRUE)) { + return(data.frame( + CATALOG = case$namespace$catalog, + SCHEMA = case$namespace$schema + )) + } + if (is.null(case$reported)) { + return(data.frame( + name = character(), + kind = character(), + database_name = character(), + schema_name = character(), + comment = character() + )) + } + data.frame( + name = case$reported$table, + kind = "TABLE", + database_name = case$reported$catalog, + schema_name = case$reported$schema, + comment = NA_character_ + ) + }, + .package = "DBI" + ) + + registry <- catalog_table_registry( + con = DBI::ANSI(), + tables = catalog_labels_authored(case$authored), + current_namespace = snowflake_current_namespace, + id_type = snowflake_id_type, + exact_relation = snowflake_exact_relation, + list_relations = function(...) list() + ) + + expect_equal(registry$labels, case$label, info = case$name) + # The access check pairs the two lists by label, so a selection entry has + # to be validated under the label it ended up with. + expect_equal(registry$validate$labels, case$label, info = case$name) + + relation <- registry$relations[[case$label]] + expect_equal( + if (is.null(relation$identity)) NULL else table_id_label(relation$identity), + case$identity, + info = case$name + ) + expect_equal( + isTRUE(relation$discovered), + !is.null(case$reported), + info = case$name + ) + } +}) diff --git a/tests/shared/README.md b/tests/shared/README.md index 7ae77d4a..71877c4b 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -37,8 +37,9 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **Definition expansion and rendering.** `definition-rendering.json` pins what happens to a governed definition after the compiler is done with it: which `{{token}}` queries expand and to what, the one-line gist shown at first touch and in retrieval, and the kind index under a character cap. It carries a bank of export records that each package hydrates into its own shape. A refused query pins the refusal and a reason slug rather than the message, because the wording belongs to each language. - **Catalog rows.** `catalog-rows.json` pins how a warehouse listing becomes relations and columns: which rows are relations at all, what kind each is, which comments count as prose, and where a `DESCRIBE` reply stops being columns. The rows are what Snowflake's `SHOW OBJECTS` and `DESC TABLE`, and Databricks' `system.information_schema.tables` and `DESCRIBE TABLE`, actually return. Running those queries is each language's own business; agreeing on their replies is not. Hand-maintained, since no binary generates it, and booleans travel as strings so the file reads the same from both JSON readers. - **The catalog merge.** `catalog-merge.json` pins the contract between a warehouse listing and an authored dictionary: the three limits (object cap, prompt threshold, search probe bound), exclusion-glob behavior over bare table names, and merge scenarios — authored prose wins, warehouse types and nullability win, identifier case folds per backend, ambiguity is an error — each with the expected merged tables and definition bindings. Error cases pin a slug rather than message text, because the wording belongs to each language. -- **Catalog access errors.** `catalog-access-errors.json` pins how a failed access probe is read: a SQLSTATE and a message become `authorization`, `transient`, or `unknown`, which decides what the user is told and whether the answer is cached per relation. Hand-maintained, since the cases are the failures real drivers report rather than anything a binary emits. An absent SQLSTATE travels as an empty string, because JSON has no way to spell R's `NA_character_`. -- **Session identity and access precedence.** `catalog-session-changed.json` pins which parts of a warehouse session a refusal reports as changed, so neither package tells a Databricks user that a role moved on a backend that has none. `catalog-access-precedence.json` pins what construction reports when several named relations fail at once: every relation the listing reported is probed before anything is raised, and a name the warehouse never listed is reported ahead of a refusal. Both are hand-maintained, and both carry field names rather than message text, since the wording belongs to each language. +- **Catalog access errors.** `catalog-access-errors.json` pins how a failed access probe is read: a SQLSTATE and a message become `authorization`, `transient`, or `unknown`, which decides what the user is told and whether the answer is cached per relation. A case carrying a `sql` field asserts that only the driver's own complaint is read: each suite attaches that statement the way its own drivers repeat it back, and the expected kind stays the one the message alone earns, so a relation cannot classify itself by its name. Hand-maintained, since the cases are the failures real drivers report rather than anything a binary emits. An absent SQLSTATE travels as an empty string, because JSON has no way to spell R's `NA_character_`. +- **Session identity and access precedence.** `catalog-session-changed.json` pins which parts of a warehouse session a refusal reports as changed, so neither package tells a Databricks user that a role moved on a backend that has none. `catalog-access-precedence.json` pins what construction reports when several named relations fail at once: every relation the listing reported is probed before anything is raised, and a name the warehouse never listed is reported ahead of a refusal. Both are hand-maintained, and neither carries message text, since the wording belongs to each language: the session cases travel as snapshot field names, the precedence cases as relation labels and outcome slugs. +- **Relation labels.** `catalog-relation-labels.json` pins the label a selection entry ends up under, which is what the agent sees in a table listing and what every access error names. A confirmed entry is qualified with the connection's namespace only when it named no schema of its own; an entry the warehouse never reported keeps the authored name whole. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing. Hand-maintained. Identifiers are already case-folded, so these cases pin qualification alone and leave case folding to `catalog-merge.json`. - **Trace file naming.** `trace(-[0-9]+)?\.jsonl`, one OTLP envelope per line. ## Conventions diff --git a/tests/shared/catalog-access-errors.json b/tests/shared/catalog-access-errors.json index bce22ef1..a608ac72 100644 --- a/tests/shared/catalog-access-errors.json +++ b/tests/shared/catalog-access-errors.json @@ -1,5 +1,5 @@ { - "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message.", + "description": "How a failed catalog access probe is classified from the driver's error. The classification decides which error a user sees and whether the failure is cached: an authorization failure is stable and is remembered per relation, a transient one is retried on the next touch, and an unknown one is neither. Drivers differ in what they raise, so each case gives only the two things every driver carries, a SQLSTATE (empty when the driver reported none) and a message. A case with a `sql` field asserts that only the driver's own complaint is read: both packages wrap a failing statement in a way that repeats it back, so each suite attaches that statement the way its own drivers do, and the expected kind stays the one the message alone earns.", "cases": [ { "name": "insufficient privilege sqlstate", @@ -96,6 +96,27 @@ "sqlstate": "", "message": "column BOGUS not found", "kind": "unknown" + }, + { + "name": "an absent relation whose name reads as a refusal", + "sqlstate": "42S02", + "message": "Object does not exist, or operation cannot be performed.", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"PERMISSION_DENIED_EVENTS\" WHERE 1 = 0", + "kind": "unknown" + }, + { + "name": "an absent relation whose name reads as transient", + "sqlstate": "42S02", + "message": "Object does not exist, or operation cannot be performed.", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"NETWORK_SOCKET_TIMEOUTS\" WHERE 1 = 0", + "kind": "unknown" + }, + { + "name": "a genuine refusal is still read through the statement", + "sqlstate": "", + "message": "SQL access control error: Insufficient privileges to operate on table 'ORDERS'", + "sql": "SELECT * FROM \"ANALYTICS\".\"PUBLIC\".\"ORDERS\" WHERE 1 = 0", + "kind": "authorization" } ] } diff --git a/tests/shared/catalog-relation-labels.json b/tests/shared/catalog-relation-labels.json new file mode 100644 index 00000000..b4534acc --- /dev/null +++ b/tests/shared/catalog-relation-labels.json @@ -0,0 +1,37 @@ +{ + "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers.", + "cases": [ + { + "name": "a bare entry is qualified with the connection namespace", + "authored": {"table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "PUBLIC", "table": "ORDERS"}, + "label": "ANALYTICS.PUBLIC.ORDERS", + "identity": "ANALYTICS.PUBLIC.ORDERS" + }, + { + "name": "an entry naming its own schema is not given a catalog", + "authored": {"schema": "SALES", "table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "label": "SALES.ORDERS", + "identity": "ANALYTICS.SALES.ORDERS" + }, + { + "name": "a fully qualified entry is already its own label", + "authored": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, + "label": "ANALYTICS.SALES.ORDERS", + "identity": "ANALYTICS.SALES.ORDERS" + }, + { + "name": "an entry the warehouse never reported keeps the authored name", + "authored": {"table": "ORDERS"}, + "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, + "reported": null, + "label": "ORDERS", + "identity": null + } + ] +} diff --git a/tests/shared/catalog-session-changed.json b/tests/shared/catalog-session-changed.json index 957b0223..50713e93 100644 --- a/tests/shared/catalog-session-changed.json +++ b/tests/shared/catalog-session-changed.json @@ -26,7 +26,7 @@ "changed": ["catalog", "schema"] }, { - "name": "several fields at once, in snapshot order", + "name": "several fields at once, in the order the refusal names them", "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "after": {"backend": "snowflake", "principal": "ADMIN_USER", "role": "ADMIN", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "STAGING"}, "changed": ["principal", "role", "schema"] @@ -36,6 +36,12 @@ "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "after": {"backend": "snowflake", "principal": "ANALYST", "role": null, "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, "changed": ["role"] + }, + { + "name": "a connection reporting no session at all names no field", + "before": {"backend": "snowflake", "principal": "ANALYST", "role": "REPORTER", "secondary_roles": "ALL", "catalog": "ANALYTICS", "schema": "PUBLIC"}, + "after": null, + "changed": [] } ] } From fbdfd59e78cf239b650937960badc4ff59cb8b07 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Sun, 6 Sep 2026 17:14:10 -0600 Subject: [PATCH 5/5] fix: do not give a Databricks temporary view a namespace it has none of `SHOW TABLES` lists the session's temporary views alongside the schema's own tables, and the hive listing stamped the selected namespace onto every row. A temporary view belongs to no schema and answers only to its bare name, so the id it was given named nothing. Labelling an exact selection with the resolved id made that reach the access probe and every later query, which is how the live Databricks quoted-name test started failing. The listing now leaves such a relation bare, and an exact match adopts the resolved label only when the warehouse reported a namespace of its own. The relation-label fixture gains a case for it, and each case now names the backend it runs against: only a `SHOW TABLES` reply can report a relation with no namespace. --- pkg-py/src/commons/_catalog/_core.py | 20 +++++ pkg-py/src/commons/_catalog/_databricks.py | 45 +++++++---- pkg-py/src/commons/_catalog/_snowflake.py | 9 +-- .../tests/test_catalog_security_fixtures.py | 41 +++++++++- pkg-r/R/catalog-databricks.R | 35 +++++++-- pkg-r/R/catalog.R | 7 +- .../shared/catalog-relation-labels.json | 15 +++- pkg-r/tests/testthat/helper-catalog-rows.R | 77 ++++++++++++++++++- pkg-r/tests/testthat/test-catalog-security.R | 38 +++++++++ pkg-r/tests/testthat/test-catalog-snowflake.R | 61 --------------- tests/shared/README.md | 2 +- tests/shared/catalog-relation-labels.json | 15 +++- 12 files changed, 266 insertions(+), 99 deletions(-) diff --git a/pkg-py/src/commons/_catalog/_core.py b/pkg-py/src/commons/_catalog/_core.py index e0e02f6c..0456919e 100644 --- a/pkg-py/src/commons/_catalog/_core.py +++ b/pkg-py/src/commons/_catalog/_core.py @@ -17,6 +17,7 @@ "check_exclude", "excluded", "id_type", + "matched_relation", "merge_dictionary", "normalize_identifier", "search", @@ -84,6 +85,25 @@ class MergedDictionary: definition_bindings: dict[str, Any] | None +def matched_relation(relation: Relation, requested: TableId) -> Relation: + """An exact selection's match, under the label it was asked for. + + `requested` is the authored name qualified with the namespace the lookup + ran in. The warehouse's own id is kept as the identity, since it carries + that backend's casing and is what later metadata queries name. A relation + the warehouse reports without a namespace has none to be labelled with, + and none to be queried under either: a Databricks temporary view answers + only to its bare name. + """ + qualified = relation.id.schema is not None or relation.id.catalog is not None + return Relation( + id=requested if qualified else relation.id, + kind=relation.kind, + description=relation.description, + identity=relation.id, + ) + + def normalize_identifier(value: Any, identifier_case: str | None) -> Any: """Fold an identifier the way the backend does, or leave it alone.""" if isinstance(value, list): diff --git a/pkg-py/src/commons/_catalog/_databricks.py b/pkg-py/src/commons/_catalog/_databricks.py index e02bd6ba..caa09a64 100644 --- a/pkg-py/src/commons/_catalog/_databricks.py +++ b/pkg-py/src/commons/_catalog/_databricks.py @@ -11,7 +11,7 @@ from typing import Any from .._data_source import TableId -from ._core import Relation, Selector, id_type +from ._core import Relation, Selector, id_type, matched_relation __all__ = [ "columns_from_describe", @@ -139,16 +139,36 @@ def _list_hive_relations(backend: Any, selector: Selector) -> list[Relation]: rows = backend.query( f"SHOW TABLES IN {_quote_path([selector.catalog or _HIVE, selector.schema])}" ) - return [ - Relation( - id=TableId( - catalog=selector.catalog, - schema=selector.schema, - table=_lower_keys(row)["tablename"], + relations = [] + for row in rows: + values = _lower_keys(row) + temporary = _is_temporary(values) + relations.append( + Relation( + id=TableId( + catalog=None if temporary else selector.catalog, + schema=None if temporary else selector.schema, + table=values["tablename"], + ) ) ) - for row in rows - ] + return relations + + +def _is_temporary(values: dict[str, Any]) -> bool: + """Whether a `SHOW TABLES` row is a session-scoped temporary view. + + `SHOW TABLES` lists the session's temporary views alongside the schema's + own tables, reporting them with an empty database and `isTemporary` set. A + temporary view belongs to no schema and answers only to its bare name, so + qualifying it with the listed namespace would build an id naming nothing. + """ + if "istemporary" in values: + return bool(values["istemporary"]) + for column in ("database", "namespace"): + if column in values: + return not values[column] + return False def _is_hive(selector: Selector) -> bool: @@ -179,12 +199,7 @@ def exact_relation(backend: Any, selector: Selector) -> Relation | None: ) for relation in relations: if relation.id.table == requested.table: - return Relation( - id=requested, - kind=relation.kind, - description=relation.description, - identity=relation.id, - ) + return matched_relation(relation, requested) return None diff --git a/pkg-py/src/commons/_catalog/_snowflake.py b/pkg-py/src/commons/_catalog/_snowflake.py index 90ee95e3..38844686 100644 --- a/pkg-py/src/commons/_catalog/_snowflake.py +++ b/pkg-py/src/commons/_catalog/_snowflake.py @@ -10,7 +10,7 @@ from typing import Any from .._data_source import TableId -from ._core import Relation, Selector, id_type +from ._core import Relation, Selector, id_type, matched_relation __all__ = [ "SHOW_ROW_LIMIT", @@ -164,12 +164,7 @@ def exact_relation(backend: Any, selector: Selector) -> Relation | None: check_show_complete(rows, "relations") for relation in relations_from_show(rows): if relation.id.table == requested.table: - return Relation( - id=requested, - kind=relation.kind, - description=relation.description, - identity=relation.id, - ) + return matched_relation(relation, requested) return None diff --git a/pkg-py/tests/test_catalog_security_fixtures.py b/pkg-py/tests/test_catalog_security_fixtures.py index 933973e2..13272930 100644 --- a/pkg-py/tests/test_catalog_security_fixtures.py +++ b/pkg-py/tests/test_catalog_security_fixtures.py @@ -13,6 +13,7 @@ import sqlalchemy.exc from commons._catalog import Relation, Selector, table_registry +from commons._catalog import _databricks as databricks from commons._catalog import _snowflake as snowflake from commons._catalog._security import ( CatalogAccessError, @@ -184,13 +185,49 @@ def dialect(self): return "snowflake" +class _HiveLabelBackend: + """A Databricks connection whose `SHOW TABLES` answers from the case. + + Only this reply can report a relation with no namespace of its own, which + is what a session-scoped temporary view is. + """ + + def __init__(self, namespace, reported): + self._namespace = namespace + self._reported = reported + + def query(self, sql: str): + if "CURRENT_CATALOG" in sql: + return [dict(self._namespace)] + if self._reported is None: + return [] + temporary = bool(self._reported.get("temporary")) + return [ + { + "database": "" if temporary else self._namespace["schema"], + "tableName": self._reported["table"], + "isTemporary": temporary, + } + ] + + def dialect(self): + return "databricks" + + +_LABEL_BACKENDS = { + "snowflake": (_LabelBackend, snowflake.exact_relation), + "databricks": (_HiveLabelBackend, databricks.exact_relation), +} + + def test_relation_labels_match_the_shared_contract(): cases = load_shared_fixture("catalog-relation-labels")["cases"] assert cases for case in cases: authored = case["authored"] - backend = _LabelBackend(case["namespace"], case["reported"]) + connection, exact_relation = _LABEL_BACKENDS[case["backend"]] + backend = connection(case["namespace"], case["reported"]) registry = table_registry( selectors=[ Selector( @@ -199,7 +236,7 @@ def test_relation_labels_match_the_shared_contract(): table=authored["table"], ) ], - exact_relation=functools.partial(snowflake.exact_relation, backend), + exact_relation=functools.partial(exact_relation, backend), list_relations=lambda selector: [], ) diff --git a/pkg-r/R/catalog-databricks.R b/pkg-r/R/catalog-databricks.R index 306ed9f4..596b80bb 100644 --- a/pkg-r/R/catalog-databricks.R +++ b/pkg-r/R/catalog-databricks.R @@ -401,19 +401,42 @@ databricks_list_hive_relations <- function( } ) names(rows) <- tolower(names(rows)) - lapply(rows$tablename, function(table) { + temporary <- databricks_hive_temporary(rows) + lapply(seq_along(rows$tablename), function(i) { + table <- rows$tablename[[i]] list( - id = DBI::Id( - catalog = components[["catalog"]], - schema = components[["schema"]], - table = table - ), + id = if (temporary[[i]]) { + DBI::Id(table = table) + } else { + DBI::Id( + catalog = components[["catalog"]], + schema = components[["schema"]], + table = table + ) + }, kind = NULL, description = NULL ) }) } +# `SHOW TABLES` lists the session's temporary views alongside the schema's own +# tables, reporting them with an empty database and `isTemporary` set. A +# temporary view belongs to no schema and answers only to its bare name, so +# qualifying it with the listed namespace would build an id naming nothing. +databricks_hive_temporary <- function(rows) { + if ("istemporary" %in% names(rows)) { + flag <- as.logical(rows$istemporary) + return(!is.na(flag) & flag) + } + column <- intersect(c("database", "namespace"), names(rows)) + if (length(column) == 0L) { + return(rep(FALSE, length(rows$tablename))) + } + values <- as.character(rows[[column[[1]]]]) + is.na(values) | !nzchar(values) +} + databricks_read_semantic_model <- function( view, con, diff --git a/pkg-r/R/catalog.R b/pkg-r/R/catalog.R index c98653d9..6d09e00b 100644 --- a/pkg-r/R/catalog.R +++ b/pkg-r/R/catalog.R @@ -312,7 +312,12 @@ catalog_match_exact_relation <- function(relations, id, requested = id) { relation <- relations[[which(is_requested)[[1]]]] relation$identity <- relation$id - relation$id <- requested + # A relation the warehouse reports without a namespace has none to be + # labelled with, and none to be queried under either: a Databricks + # temporary view answers only to its bare name. + if (any(c("catalog", "schema") %in% names(relation$identity@name))) { + relation$id <- requested + } relation$discovered <- TRUE relation } diff --git a/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json b/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json index b4534acc..054f2878 100644 --- a/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json +++ b/pkg-r/tests/testthat/fixtures/shared/catalog-relation-labels.json @@ -1,8 +1,9 @@ { - "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers.", + "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. A relation the warehouse reports without a namespace of its own keeps its bare name, because the label is also the id every later query names it by, and a namespace it does not belong to would name nothing. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers. Each case names the backend it runs against, because only a Databricks `SHOW TABLES` reply can report a relation with no namespace.", "cases": [ { "name": "a bare entry is qualified with the connection namespace", + "backend": "snowflake", "authored": {"table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "PUBLIC", "table": "ORDERS"}, @@ -11,6 +12,7 @@ }, { "name": "an entry naming its own schema is not given a catalog", + "backend": "snowflake", "authored": {"schema": "SALES", "table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, @@ -19,6 +21,7 @@ }, { "name": "a fully qualified entry is already its own label", + "backend": "snowflake", "authored": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, @@ -27,11 +30,21 @@ }, { "name": "an entry the warehouse never reported keeps the authored name", + "backend": "snowflake", "authored": {"table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": null, "label": "ORDERS", "identity": null + }, + { + "name": "a temporary view is not given the namespace it was listed under", + "backend": "databricks", + "authored": {"table": "commons quoted.table"}, + "namespace": {"catalog": "hive_metastore", "schema": "default"}, + "reported": {"table": "commons quoted.table", "temporary": true}, + "label": "commons quoted.table", + "identity": "commons quoted.table" } ] } diff --git a/pkg-r/tests/testthat/helper-catalog-rows.R b/pkg-r/tests/testthat/helper-catalog-rows.R index 54f868a2..531fe61a 100644 --- a/pkg-r/tests/testthat/helper-catalog-rows.R +++ b/pkg-r/tests/testthat/helper-catalog-rows.R @@ -103,10 +103,79 @@ catalog_precedence_probed <- function(script) { )] } -# The relation-label fixture's authored entry, as a `tables` string. +# The relation-label fixture's authored entry, as a `tables` entry. A +# `DBI::Id` rather than a dotted string, since a case may author a name that +# contains a dot of its own. catalog_labels_authored <- function(authored) { - paste( - c(authored$catalog, authored$schema, authored$table), - collapse = "." + do.call( + DBI::Id, + c( + if (!is.null(authored$catalog)) list(catalog = authored$catalog), + if (!is.null(authored$schema)) list(schema = authored$schema), + list(table = authored$table) + ) + ) +} + +# The backend functions a relation-label case runs against. +catalog_labels_backends <- list( + snowflake = list( + current_namespace = function(...) snowflake_current_namespace(...), + id_type = function(...) snowflake_id_type(...), + exact_relation = function(...) snowflake_exact_relation(...) + ), + databricks = list( + current_namespace = function(...) databricks_current_namespace(...), + id_type = function(...) databricks_id_type(...), + exact_relation = function(...) databricks_exact_relation(...) ) +) + +catalog_labels_binding <- function(case, name) { + catalog_labels_backends[[case$backend]][[name]] +} + +# The listing a relation-label case answers with. Only a Databricks +# `SHOW TABLES` reply can report a relation with no namespace of its own, +# which is what a session-scoped temporary view is. +catalog_labels_reply <- function(case) { + function(conn, statement, ...) { + if (grepl("CURRENT_DATABASE|CURRENT_CATALOG", statement)) { + return(data.frame( + CATALOG = case$namespace$catalog, + SCHEMA = case$namespace$schema + )) + } + if (identical(case$backend, "databricks")) { + if (is.null(case$reported)) { + return(data.frame( + database = character(), + tableName = character(), + isTemporary = logical() + )) + } + temporary <- isTRUE(case$reported$temporary) + return(data.frame( + database = if (temporary) "" else case$namespace$schema, + tableName = case$reported$table, + isTemporary = temporary + )) + } + if (is.null(case$reported)) { + return(data.frame( + name = character(), + kind = character(), + database_name = character(), + schema_name = character(), + comment = character() + )) + } + data.frame( + name = case$reported$table, + kind = "TABLE", + database_name = case$reported$catalog, + schema_name = case$reported$schema, + comment = NA_character_ + ) + } } diff --git a/pkg-r/tests/testthat/test-catalog-security.R b/pkg-r/tests/testthat/test-catalog-security.R index c5825356..073aa89b 100644 --- a/pkg-r/tests/testthat/test-catalog-security.R +++ b/pkg-r/tests/testthat/test-catalog-security.R @@ -576,3 +576,41 @@ test_that("discovered relations may have an unknown kind", { relations = relations )) }) + +test_that("relation labels match the shared contract", { + cases <- shared_fixture("catalog-relation-labels")$cases + expect_gt(length(cases), 0L) + + for (case in cases) { + local_mocked_bindings( + dbGetQuery = catalog_labels_reply(case), + .package = "DBI" + ) + + registry <- catalog_table_registry( + con = DBI::ANSI(), + tables = catalog_labels_authored(case$authored), + current_namespace = catalog_labels_binding(case, "current_namespace"), + id_type = catalog_labels_binding(case, "id_type"), + exact_relation = catalog_labels_binding(case, "exact_relation"), + list_relations = function(...) list() + ) + + expect_equal(registry$labels, case$label, info = case$name) + # The access check pairs the two lists by label, so a selection entry has + # to be validated under the label it ended up with. + expect_equal(registry$validate$labels, case$label, info = case$name) + + relation <- registry$relations[[case$label]] + expect_equal( + if (is.null(relation$identity)) NULL else table_id_label(relation$identity), + case$identity, + info = case$name + ) + expect_equal( + isTRUE(relation$discovered), + !is.null(case$reported), + info = case$name + ) + } +}) diff --git a/pkg-r/tests/testthat/test-catalog-snowflake.R b/pkg-r/tests/testthat/test-catalog-snowflake.R index b8ba6f0e..cdb8456f 100644 --- a/pkg-r/tests/testthat/test-catalog-snowflake.R +++ b/pkg-r/tests/testthat/test-catalog-snowflake.R @@ -530,64 +530,3 @@ test_that("Snowflake current namespace requires a database and schema", { error = TRUE ) }) - -test_that("relation labels match the shared contract", { - cases <- shared_fixture("catalog-relation-labels")$cases - expect_gt(length(cases), 0L) - - for (case in cases) { - local_mocked_bindings( - dbGetQuery = function(conn, statement, ...) { - if (grepl("CURRENT_DATABASE", statement, fixed = TRUE)) { - return(data.frame( - CATALOG = case$namespace$catalog, - SCHEMA = case$namespace$schema - )) - } - if (is.null(case$reported)) { - return(data.frame( - name = character(), - kind = character(), - database_name = character(), - schema_name = character(), - comment = character() - )) - } - data.frame( - name = case$reported$table, - kind = "TABLE", - database_name = case$reported$catalog, - schema_name = case$reported$schema, - comment = NA_character_ - ) - }, - .package = "DBI" - ) - - registry <- catalog_table_registry( - con = DBI::ANSI(), - tables = catalog_labels_authored(case$authored), - current_namespace = snowflake_current_namespace, - id_type = snowflake_id_type, - exact_relation = snowflake_exact_relation, - list_relations = function(...) list() - ) - - expect_equal(registry$labels, case$label, info = case$name) - # The access check pairs the two lists by label, so a selection entry has - # to be validated under the label it ended up with. - expect_equal(registry$validate$labels, case$label, info = case$name) - - relation <- registry$relations[[case$label]] - expect_equal( - if (is.null(relation$identity)) NULL else table_id_label(relation$identity), - case$identity, - info = case$name - ) - expect_equal( - isTRUE(relation$discovered), - !is.null(case$reported), - info = case$name - ) - } -}) diff --git a/tests/shared/README.md b/tests/shared/README.md index 71877c4b..9830bec3 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -39,7 +39,7 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **The catalog merge.** `catalog-merge.json` pins the contract between a warehouse listing and an authored dictionary: the three limits (object cap, prompt threshold, search probe bound), exclusion-glob behavior over bare table names, and merge scenarios — authored prose wins, warehouse types and nullability win, identifier case folds per backend, ambiguity is an error — each with the expected merged tables and definition bindings. Error cases pin a slug rather than message text, because the wording belongs to each language. - **Catalog access errors.** `catalog-access-errors.json` pins how a failed access probe is read: a SQLSTATE and a message become `authorization`, `transient`, or `unknown`, which decides what the user is told and whether the answer is cached per relation. A case carrying a `sql` field asserts that only the driver's own complaint is read: each suite attaches that statement the way its own drivers repeat it back, and the expected kind stays the one the message alone earns, so a relation cannot classify itself by its name. Hand-maintained, since the cases are the failures real drivers report rather than anything a binary emits. An absent SQLSTATE travels as an empty string, because JSON has no way to spell R's `NA_character_`. - **Session identity and access precedence.** `catalog-session-changed.json` pins which parts of a warehouse session a refusal reports as changed, so neither package tells a Databricks user that a role moved on a backend that has none. `catalog-access-precedence.json` pins what construction reports when several named relations fail at once: every relation the listing reported is probed before anything is raised, and a name the warehouse never listed is reported ahead of a refusal. Both are hand-maintained, and neither carries message text, since the wording belongs to each language: the session cases travel as snapshot field names, the precedence cases as relation labels and outcome slugs. -- **Relation labels.** `catalog-relation-labels.json` pins the label a selection entry ends up under, which is what the agent sees in a table listing and what every access error names. A confirmed entry is qualified with the connection's namespace only when it named no schema of its own; an entry the warehouse never reported keeps the authored name whole. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing. Hand-maintained. Identifiers are already case-folded, so these cases pin qualification alone and leave case folding to `catalog-merge.json`. +- **Relation labels.** `catalog-relation-labels.json` pins the label a selection entry ends up under, which is what the agent sees in a table listing and what every access error names. A confirmed entry is qualified with the connection's namespace only when it named no schema of its own; an entry the warehouse never reported keeps the authored name whole; a relation the warehouse reports without a namespace of its own keeps its bare name, since the label is also the id every later query names it by. Each case names the backend it runs against, because only a Databricks `SHOW TABLES` reply can report a relation with no namespace. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing. Hand-maintained. Identifiers are already case-folded, so these cases pin qualification alone and leave case folding to `catalog-merge.json`. - **Trace file naming.** `trace(-[0-9]+)?\.jsonl`, one OTLP envelope per line. ## Conventions diff --git a/tests/shared/catalog-relation-labels.json b/tests/shared/catalog-relation-labels.json index b4534acc..054f2878 100644 --- a/tests/shared/catalog-relation-labels.json +++ b/tests/shared/catalog-relation-labels.json @@ -1,8 +1,9 @@ { - "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers.", + "description": "The label a selection entry ends up under. The label is what the agent sees in the table listing, what a catalog search scores against, and what every access error names, so both packages have to derive it the same way. A confirmed entry takes the name as authored, qualified with the connection's namespace only when the entry named no schema of its own: that is what makes a bare entry name exactly one relation. An entry the warehouse never reported keeps the authored name whole, because there is no confirmed namespace to qualify it with and repeating what the caller typed is what makes the error legible. A relation the warehouse reports without a namespace of its own keeps its bare name, because the label is also the id every later query names it by, and a namespace it does not belong to would name nothing. The warehouse's own id is kept separately as the relation's identity, since it carries that backend's casing and is what later metadata queries name. Identifiers here are already folded to the backend's case, so these cases pin qualification alone and say nothing about case folding, which `catalog-merge.json` covers. Each case names the backend it runs against, because only a Databricks `SHOW TABLES` reply can report a relation with no namespace.", "cases": [ { "name": "a bare entry is qualified with the connection namespace", + "backend": "snowflake", "authored": {"table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "PUBLIC", "table": "ORDERS"}, @@ -11,6 +12,7 @@ }, { "name": "an entry naming its own schema is not given a catalog", + "backend": "snowflake", "authored": {"schema": "SALES", "table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, @@ -19,6 +21,7 @@ }, { "name": "a fully qualified entry is already its own label", + "backend": "snowflake", "authored": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": {"catalog": "ANALYTICS", "schema": "SALES", "table": "ORDERS"}, @@ -27,11 +30,21 @@ }, { "name": "an entry the warehouse never reported keeps the authored name", + "backend": "snowflake", "authored": {"table": "ORDERS"}, "namespace": {"catalog": "ANALYTICS", "schema": "PUBLIC"}, "reported": null, "label": "ORDERS", "identity": null + }, + { + "name": "a temporary view is not given the namespace it was listed under", + "backend": "databricks", + "authored": {"table": "commons quoted.table"}, + "namespace": {"catalog": "hive_metastore", "schema": "default"}, + "reported": {"table": "commons quoted.table", "temporary": true}, + "label": "commons quoted.table", + "identity": "commons quoted.table" } ] }