fix(sql): refuse a destination credential that cannot write - #811
Conversation
Repro-first proof — WAIVED (environment)
Snowflake: error 003001 / SQLSTATE 42501, "SQL access control error: Insufficient Teradata: codes 3523 / 5315 / 5612, read out of the message by the parser this repo What would unblock it: a Snowflake account and a Teradata instance (the customer DI, or Bugfix — SQL destination precheck asks whether the credential can perform the write, not just connect Reproduced the broken state
The precheck is Second repro, run against the first version of this fix (an INSERT-only probe), which inserter precheck=PASS | real upload=FAILED: DestinationConnectionError: failed to upload: InsufficientPrivilege(pgcode=42501)
Failing test (red)
Fix
Proof it's resolved
BEFORE (origin/main, and -- for inserter -- the insert-only first version of the fix) AFTER (this branch) Three things to read off it. Statement shapes measured directly on postgres 16, to confirm the zero-row DELETE still Two other dialects were exercised live on real servers, same shape: SingleStore (ghcr.io/singlestore-labs/singlestoredb-dev on :33306, real SingleStore) SQLite (a writable .db, the same .db chmod 444, and one with no record_id column) Suites: Auto-generated from this branch's |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
paulkarayan
added
prio:need
paulkarayan
added
the
prio:need
tabossert
left a comment
tabossert
left a comment
There was a problem hiding this comment.
Reviewed this end to end. Requesting changes on one item specifically — the Snowflake regression needs validating before this merges — plus the red CI. Details are inline; everything non-blocking is in a separate comment so it doesn't clutter the merge path.
First, the part that deserves saying: the design here is the strong part of this PR, and all three blockers are mechanical slips rather than problems with the approach.
The one-sided contract is genuinely enforced rather than just asserted — test_write_probes_pass_on_anything_they_cannot_confirm, test_write_probes_pass_when_the_connection_cannot_be_opened, and test_write_probes_pass_when_can_delete_cannot_be_answered (test_sql.py:601-631) are exactly the tests that stop this from becoming a false-refusal generator. The docstring at sql.py:413 cites test_write_probe_columns_match_the_uploaders_insert and that test actually exists at test_sql.py:516. And the postgres integration test (test_postgres.py:239-289) stands up four real roles, asserts the reader is told about both missing rights in one message, asserts the inserter is told DELETE and specifically not INSERT, then asserts count(*) == 0 to prove the probes wrote nothing — that's the test a reviewer would have asked for, already written. I'm not re-litigating any of the documented exclusions (42P01, 3807, 002003, 1045); each one gives its reason.
The ask, specifically on the Snowflake regression:
SnowflakeUploader lost its _embeddings_dimension and _variant_columns dataclass fields in a725337e, where classify_write_denial was inserted in their place. Both properties still read them, so every freshly constructed uploader raises AttributeError on its first batch. This is unrelated to the PR's actual purpose, which is what makes me fairly confident it's an editing slip.
What I'd like before merge is a validation that closes the hole permanently, not just the two lines restored:
- Restore both field declarations.
- Add a test that constructs a
SnowflakeUploaderand readsvariant_columns/embeddings_dimensionwithout the fixture's manual assignment. The fixture attest_snowflake.py:42-43sets both by hand — that's pre-existing onmain, not something this PR introduced, but it's the reason 300+ lines of Snowflake tests stayed green through a total breakage of the write path. As long as every test goes through that fixture, this regression can recur silently.
Worth flagging that the new precheck passes cleanly in this state — check_write_permissions never touches those properties — so the failure only surfaces once real records start writing.
Also blocking: test_ingest_unit is red because three new test files import optional connector drivers that aren't in the base test group. Two fail at collection, one fails at call time; inline threads have the specifics and point at the pytest.importorskip / fake-exception patterns already used elsewhere in the repo (including in this PR's own test_teradata.py).
Happy to re-review quickly once the Snowflake fields are back and CI is green.
|
Non-blocking observations from the same review pass, separated out so they don't clutter the merge path. Several of these are questions rather than requests, and a couple are things I'd be happy to be told are deliberate. The branch needs a rebase, and the version bump is stale
One caveat so this isn't confusing: the Also worth knowing: A credential denied even SELECT still passes the new check
The sharper version of this is on Teradata: Not a regression, and I realise "the table schema is unavailable" genuinely isn't this method's question. But it does mean the fix covers a credential that can't write but can already read, which is narrower than the title suggests. Is routing an already-classified The three untouched connectorsYou documented this in the CHANGELOG verbatim, so this isn't a gap you missed — it's a scope decision. Recording it only so it doesn't become permanent: Two dialects log a denial with no error code
The inconclusive line is the one that matters for debugging a false negative, and for these two dialects it's content-free. SQLite looks like a one-line fix: A crash inside a classifier looks identical to a clean non-matchAt Connection count at precheckEach probe opens a fresh physical connection; there's no pooling in this library. Teradata goes from 1 connection at precheck to 4 ( Small related note: Teradata's Postgres 25006 during a failoverTreating Does the 422 reach the user?The stated rationale for What I couldn't verify is the rest of the path: Was grant introspection considered?The docstring is unusually thorough about tradeoffs, but never mentions the driver's own privilege introspection ( Keeping the two teradata code tables in sync
Nit
|
The SQL destination precheck was `SELECT 1`. That runs against the session,
so it succeeds on any credential the driver can open a connection with and
says nothing about the table the uploader writes to. A credential holding
CONNECT, USAGE and SELECT and no INSERT passed setup and then failed on every
record at write time. Measured on postgres 16: precheck PASS, upload
InsufficientPrivilege(pgcode=42501).
Precheck now also asks the question, using the real statements made harmless.
The probe's privilege surface has to match the upload's exactly -- wider
refuses a credential that works, narrower passes one that cannot write --
and `upload_dataframe` is delete-then-insert, so it is both:
INSERT INTO <table> (<columns>) SELECT <columns> FROM <table> WHERE 1 = 0
DELETE FROM <table> WHERE 1 = 0
The DELETE runs only when `can_delete()` is true, which is the same gate
`upload_dataframe` puts its own DELETE behind: on a table with no record-id
column the upload skips the delete and warns, so asking for DELETE there would
refuse a credential that never needs it. The DELETE names no column on purpose
-- the real one filters on the record-id column, and the INSERT probe already
asks for SELECT on every column because `get_table_columns()` does.
The engine runs the privilege check when it plans the statement, before any
row is produced, so a refusal arrives while the row count is still zero and
nothing is written or removed even where the driver is in autocommit. `1 = 0`
rather than `FALSE` because Teradata has no boolean literal. Measured on
postgres 16: the zero-row DELETE returns rowcount=0 for a credential that
holds the right and 42501 for one that does not.
It lives in the base `SQLUploader` with a per-dialect `classify_write_denial`
hook, rather than as a per-connector probe. The statements, the column
derivation, the can_delete gate, the rollback, the one-sidedness and the raised
type are identical across dialects; the only thing that differs is which driver
answer counts as a refusal, and that is all a connector overrides. postgres,
singlestore, sqlite and snowflake inherit the call from the base precheck;
teradata calls it from its own precheck override and quotes identifiers the way
its uploader does. databricks-delta-tables, vastdb and ibm-watsonx-s3 define
their own precheck and are untouched: the first two do not write through the
column list this derives from, and none of the three has a verified denial code.
The columns are `get_table_columns()`, not `*`, because that is what the
uploader's own INSERT names once `_fit_to_schema` has conformed the frame.
Grants are per-column on every engine here, so a probe asking for rights the
write path does not use refuses credentials that work;
`test_write_probe_columns_match_the_uploaders_insert` pins the two together so
narrowing one without the other fails the suite.
The message names the right that was refused. A credential that can insert and
cannot delete is told it needs DELETE, with no mention of the INSERT it holds;
sending it to grant the wrong thing is its own bug. Both probes run even when
the first is refused, so a credential short of both is told both at once
instead of learning about the second after granting the first.
One-sided by construction. It refuses only on an answer the dialect recognizes
as an unambiguous privilege denial, and passes on everything else: a timeout, a
dropped connection, a missing table, a schema error, an unrecognized code, a
dialect with no classifier, an exception from the classifier itself. Codes that
mean "does not exist OR you may not see it" -- postgres 42P01, teradata 3807,
snowflake 002003 -- are deliberately excluded, so a typo'd table name is never
reported as a permissions problem.
Denial signals, and how each was established. postgres and singlestore both
report a refused DELETE with the same code as a refused INSERT, measured, which
is why the message's privilege comes from the probe and not from the code:
postgres 42501 insufficient_privilege, 25006 read_only_sql_transaction,
read off psycopg2's `pgcode` via `psycopg2.errorcodes`.
Reproduced live on postgres 16.15 for INSERT and for DELETE.
singlestore errno 1044/1142/1143, the MySQL access-denied numbers
SingleStore reuses. Reproduced live against a real SingleStore:
"INSERT command denied to user 'reader'@'%' for table
'elements'" and "DELETE command denied to user 'inserter'@'%'
for table 'elements'", both errno 1142. 1045 is excluded -- that
is authentication.
sqlite SQLITE_READONLY in the low byte of `sqlite_errorcode`.
Reproduced live against a read-only database file, for both
statements. SQLite has no grants, but the
connect-and-read-then-cannot-write shape is the same and the
signal is exact.
snowflake error 003001 / SQLSTATE 42501, from Snowflake's documented access
control error; attribute names from the connector's own
errors.py. Not exercised against a live account.
teradata 3523/5315/5612, read out of the message by the `[Error NNNN]`
parser this module already trusts on the upload path -- the
driver's exceptions carry no code attribute. Not exercised
against a live instance.
A refusal is a `UserError` (422), never a `UserAuthError`. The credential is
valid; telling the customer to rotate it sends them to fix the wrong thing.
Driver text is never surfaced -- it can carry the connection string.
Two rights stay outside what a zero-row statement can ask about, and both still
fail at upload: a rule evaluated per row -- postgres RLS `WITH CHECK`, a
Snowflake row access policy -- and the CREATE TABLE right Teradata's
create_destination() needs, which is reached only when the configured table does
not exist yet, in which case the schema read fails first and the probe reports
unknown.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…top of `classify_write_denial` was inserted exactly where `_embeddings_dimension` and `_variant_columns` were declared, and both readers stayed: `variant_columns` and `embeddings_dimension` each test their cached attribute before filling it. A freshly constructed `SnowflakeUploader` raised `AttributeError: 'SnowflakeUploader' object has no attribute '_embeddings_dimension'` on its first batch, so every Snowflake upload broke while the new precheck, which never reads either property, passed cleanly first. The fixture at test_snowflake.py:42-43 assigned both by hand. That predates this branch and was redundant until the fields went away, at which point it became the reason 300 lines of Snowflake tests stayed green through a broken write path. The fixture no longer sets them, so every test in the file now runs against the fields' own defaults, and two tests cover the regression directly: one constructs an uploader the way the pipeline does and reads both properties, one asserts both fields are declared on the dataclass. Removing the two fields again turns 10 tests in that file red, 8 of which the fixture used to mask. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…roup runs `test_ingest_unit` was red. CI runs `uv sync --group test --locked`, which installs no extras, and three of the new test modules reached for a driver that lives in one: `psycopg2` and `singlestoredb` at module scope, which failed the whole file at collection, and `snowflake.connector` inside a helper, which failed 7 tests at call time. Two of the three now forge the driver error instead of importing the driver, which is what `test_teradata.py` in this same branch already does with `_FakeTeradataDriverError`. Both classifiers read exactly one thing through `getattr` and never the exception type: singlestore reads `errno`, snowflake reads `sqlstate` and `errno`. The fakes carry those and nothing else, and the snowflake fake mirrors the driver's own unset defaults (errno=-1, sqlstate="n/a") so an unset field means the same thing in the test as in production. Each one is paired with an `importorskip` test that asserts the real driver still fills those fields, so the fake cannot quietly drift away from the thing it stands in for. Postgres is skipped rather than forged: `PostgresUploader.classify_write_denial` is `@requires_dependencies(["psycopg2"])` and reads the SQLSTATEs out of `psycopg2.errorcodes`, so it cannot run without the driver at all. `pytest.importorskip` there matches test_gcs.py's skip on gcsfs, and sits after the other imports to satisfy E402. Verified against a venv built the way CI builds it (`uv sync --group test --locked`, all four SQL drivers absent). Before: 2 collection errors and 7 failures. After: 269 passed, 3 skipped, nothing red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…either The check covered a credential that can read and cannot write. A credential with no rights at all failed the schema read instead, got logged as "schema unavailable", and passed -- so the customer this is for, the one whose credential holds nothing useful, still met the old generic upload failure a job later. The schema read is `SELECT * FROM <table> LIMIT 1`, and the write path runs it too, both to conform the frame and to name the columns of its own INSERT. A credential refused there cannot upload. It is now classified through the same one-sided contract as the two write probes: refuse only on an answer the dialect calls an unambiguous denial, pass on everything else, including the table not existing yet. Measured on postgres 16.15 against four roles granted exactly one scenario each: noaccess precheck -> REFUSED (422): ...do not have SELECT permission on table 'elements' reader precheck -> REFUSED (422): ...INSERT... ...DELETE... inserter precheck -> REFUSED (422): ...DELETE... writer precheck -> PASS writer, table name misspelled -> PASS Teradata is deliberately not closed. Its `get_table_columns()` classifies the driver error itself and raises `UserError`, so what arrives is already wrapped. Re-raising that unconditionally is the obvious move and is wrong: 3807 means "object does not exist or user has no privilege on it", and a configured table that does not exist yet is a working Teradata destination, because `create_destination()` builds it at upload time. That would refuse a destination that works today. Also here, from the same review pass: - `sqlite_errorcode` joins the `_SAFE_ERROR_ATTRS` allowlist. It is an int and a pure machine code, which is the allowlist's stated contract, and without it a refused SQLite write logged as a bare `OperationalError`: measured, the inconclusive line now carries `sqlite_errorcode=8`. - Teradata renders the parsed `[Error NNNN]` code into the probe log. `teradatasql.Error` carries no code attribute at all, so attribute sniffing structurally cannot work there and its probe lines were content-free. Only the code crosses, never the message. - A classifier that raises is now named in the log by its exception type. It used to be indistinguishable from a driver error no classifier recognizes. - The docstring's rollback reassurance was wrong and is corrected: `get_connection` commits in its own `finally` on postgres and teradata alike, right after the rollback. Zero rows is what makes the probes harmless, not the rollback. The statement-level trigger case that leaves, and why grant introspection is not the mechanism, are both written down, with the postgres measurement for the first. - `_USER_FAULT_TERADATA_CODES` now cross-references `_WRITE_DENIAL_TERADATA_CODES`, so a code added to one gets a decision about the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0d02b09 to
5788c25
Compare
|
Rebased onto BlockingSnowflake dataclass fields deleted (tabossert snowflake.py:197, cubic snowflake.py:191)FIXED,
The fixture at test_ingest_unit red: per-connector drivers imported in the base test group (tabossert test_postgres.py:1, test_singlestore.py:2, test_snowflake.py:312)FIXED, Singlestore and snowflake now forge the driver error rather than importing the driver, which is Postgres is skipped, not forged: Verified against a venv built the way CI builds it, cubicsql.py:468, statement-level triggers fire on a zero-row probeNOT CHANGED on postgres, where the premise does not hold; documented for teradata, which is the Measured on postgres 16.15 with psycopg2 opens a transaction and this package never sets SQLite has no statement triggers at all ( The other suggestion, a non-DML privilege check, is answered under "grant introspection" below. sql.py:434, the precheck pins the column snapshot in
|
…#815) ## What & why **Problem:** A non-admin Teradata user passes the destination connector check even when it cannot create the table the job is about to create, because the check never asks; the job asks instead, halfway through, and is told the server is unreachable. That happens whenever the configured table does not exist yet or no table is configured: `create_destination()` builds it at upload time, which needs CREATE TABLE, and on `main` the 3524 it gets back surfaces as "Failed to connect to server". With the Database field blank it is worse, because the table goes into whatever database the session defaults to, which can be one the user never meant to write to, and nothing tells anyone which database that was. **Change:** The Teradata destination precheck now resolves the database the write lands in (`SELECT DATABASE`, which is the configured one when set, else the session default), logs it, and creates and drops a throwaway `unstructured_precheck_<hex>` table in that database unless the configured table already exists there. With no table configured the probe always runs, since the platform names the table per workflow only when it calls `create_destination()`. Teradata error 3524 refuses the destination with a `UserError` that names the database; anything else passes. **Blast radius:** 3/5 -- the connector check now runs DDL on the customer's Teradata; one connector, revert-safe. This completes #811, which made the SQL precheck probe INSERT and DELETE on an existing table and called out the missing-table case as the one gap it left. ## Linked ticket none Client-facing follow-up: a customer configured a Teradata destination with a non-admin user and a blank Database field; it passed the connector check and the run auto-created its table in a database the user had not chosen. ## Impact - **Customers:** Teradata destination users whose table is auto-created now find out at connector check time that the user lacks CREATE TABLE, with a message naming the database to grant it on, instead of a job that fails with "Failed to connect to server". Destinations whose configured table already exists see no change. A destination that works today keeps passing: only 3524 refuses. - **Internal:** support gets the resolved database in the uploader's INFO log (`destination writes resolve to Teradata database '<db>'`), which is the fact that was missing when triaging where an auto-created table went. 3524 at upload time is now classified as the user's to fix (422) rather than a connection failure. - **Wire contract / clients:** a new `UserError` (422) from the Teradata destination precheck: "The destination credentials can connect to the database but do not have CREATE TABLE permission on database '<db>'. ...". `SQLUploader._write_denied_message` gained optional `object_kind` / `object_name` arguments; its default output is unchanged for every other dialect. 3524 raised from `create_destination()` at upload time changes from `DestinationConnectionError` to `UserError` "Teradata error 3524 (user does not have CREATE TABLE access to the database)". - **Deployment target considerations:** same on SaaS, DI, in-VPC and on-prem (including the Teradata OEM sites): the check talks only to the customer's own Teradata. Not exercised against a real Teradata on any target. ## Risk / rollback - The precheck is slower on Teradata. It opens one more session and runs up to four more statements (`SELECT DATABASE`, a `DBC.TablesV` lookup when a table is configured, and the CREATE / DROP when that table is absent or none is configured). The preflight controller treats a precheck TIMEOUT as PASS, so on a slow logon this check can now time out and pass where it used to finish. That is the fail-open direction: the job still fails as it does today, it does not refuse a working destination. - The check writes DDL. The CREATE commits under the driver's autocommit. If the DROP fails, a `unstructured_precheck_<hex>` table is left in the destination database and a warning names it; the check still passes. Every connector check on a destination with no table configured creates and drops one probe table, including after the workflow's own table exists. - Only 3524 refuses. A user refused with some other code (5315, perm space, a bad database name) passes the check and fails at upload as before. - With no table configured, a user whose per-workflow table already exists but who has since lost CREATE TABLE is now refused, though the job would have written to the existing table. Precheck cannot know the per-workflow table name, so it asks for the right a first run needs. - Revert the PR to back it out; nothing persists. ## How it was verified - New unit tests cover: table absent and CREATE refused with 3524 (refused, database named, driver text absent); table absent and CREATE allowed (probe created then dropped); table present (no CREATE or DROP; the INSERT/DELETE probe still runs); blank Database (`SELECT DATABASE` names the database in the probe); no table configured (the probe runs without a lookup); the server's spelling of the database is what gets quoted; other CREATE failures pass; a failed DROP passes and names the leftover. All of them fail against `origin/main`'s module. - The full unit suite passes (`pytest -n auto test/unit --ignore test/unit/unstructured`), including the other SQL dialects that share `_write_denied_message`. - A fake-driver simulation drives the real `TeradataUploader` against a modelled session default database and per-database grants; output under Proof. - NOT verified against a live Teradata. The 3524 code and the `DBC.TablesV` lookup come from Teradata's documentation and the existing `create_destination()`, not from a server. ## Proof > **Proof waived (environment)** -- no live Teradata is reachable from this machine. There are no `TERADATA_*` credentials locally (the live integration tests in `test/integration/connectors/sql/test_teradata.py` expect them exported by hand, and CI does not set them), and no Vantage SQL listener answers here. Unblocked by a Teradata Vantage (a ClearScape trial works) with an admin who can create a user lacking CREATE TABLE in one database; then run the CREATE-probe red/green against `main` and this branch. **Repro (local, fake teradatasql driver, real `TeradataUploader` from `origin/main`).** The "new precheck" rows are `main`'s precheck, which already includes the INSERT/DELETE probe (host replaced with `<host>`): ``` B. blank Database, operator's default db IS admin_db, no rights there new precheck connector check: PASS job: FAIL DestinationConnectionError: Failed to connect to server <host> table now in: nowhere E. Database explicitly set to admin_db, no rights there new precheck connector check: PASS job: FAIL DestinationConnectionError: Failed to connect to server <host> table now in: nowhere ``` **Failing tests against `origin/main`'s module:** ``` FAILED test_teradata_uploader_precheck_with_table_name_none FAILED test_teradata_precheck_refuses_a_credential_that_cannot_create_the_missing_table FAILED test_teradata_precheck_creates_and_drops_a_probe_table_when_the_table_is_missing FAILED test_teradata_precheck_issues_no_create_when_the_table_exists FAILED test_teradata_precheck_probes_the_session_database_when_database_is_blank FAILED test_teradata_precheck_passes_when_the_create_probe_fails_for_another_reason[2644] FAILED test_teradata_precheck_passes_when_the_create_probe_fails_for_another_reason[3803] FAILED test_teradata_precheck_passes_when_the_create_probe_fails_for_another_reason[5315] FAILED test_teradata_precheck_passes_when_the_create_probe_fails_for_another_reason[9999] FAILED test_teradata_precheck_passes_and_names_the_leftover_when_the_drop_fails FAILED test_teradata_precheck_quotes_the_database_as_the_server_spells_it ``` **After (same simulation, this branch).** Rows A, C and D do not move; B and E now stop at the connector check (destination table name and host replaced with `<table>` and `<host>`): ``` A. blank Database, operator's default db is its own, no rights in admin_db new precheck connector check: PASS job: PASS table now in: ['operator_db.<table>'] B. blank Database, operator's default db IS admin_db, no rights there new precheck connector check: FAIL UserError: The destination credentials can connect to the database but do not have CREATE TABLE permission on database 'admin_db'. Records would fail to write. Grant CREATE TABLE on that database to the user this connector authenticates as. C. blank Database, default db IS admin_db, rights there (direct or via a role) new precheck connector check: PASS job: PASS table now in: ['admin_db.<table>'] D. blank Database, default db is own, table of that name ALREADY in admin_db new precheck connector check: PASS job: PASS table now in: ['admin_db.<table>', 'operator_db.<table>'] E. Database explicitly set to admin_db, no rights there new precheck connector check: FAIL UserError: The destination credentials can connect to the database but do not have CREATE TABLE permission on database 'admin_db'. Records would fail to write. Grant CREATE TABLE on that database to the user this connector authenticates as. ``` Row C is the case to keep in mind: a user who holds CREATE TABLE in the session's default database still passes and the table still lands there. The check now logs which database that is; it does not stop it. ## Dependencies / merge order none --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/Unstructured-IO/unstructured-ingest/pull/815?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: paulkarayan <pk@unstructured.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
Review follow-up on #815. The CREATE TABLE precheck probe asked the server for less than create_destination() will: a one-INTEGER-column table against a real DDL carrying CLOB, VECTOR32, JSON and a PRIMARY INDEX. A right one of those types needs and CREATE TABLE does not carry was therefore invisible to the check and hit the customer at upload instead. The probe now runs create_destination()'s own statement under the throwaway name, through the shared _elements_schema_sql(), so the rights it asks for are the rights the upload needs: no wider, no narrower, which is the rule #811 set. It runs UNQUALIFIED, the way create_destination() runs it, so it lands in the same session database the check just resolved and no identifier is interpolated into the SQL. That also answers cubic's quoting finding: a database name with a double quote in it no longer reaches a statement. 3523 now refuses alongside 3524. The probe is the real statement, so any answer that means "a right was refused and nothing else" is an answer about the real CREATE, and a right the column types need comes back as 3523 rather than 3524. Its message says which database and which code, and deliberately does not tell the customer CREATE TABLE is what they are missing, because on that code it is not. With the Database field blank both messages also say the named database is only the session default and that the field exists: a DBA who follows the message otherwise grants rights on a database nobody chose, which is the complaint this check came from. A refusal the server has already given now survives the way out of the block: denied is set before the try, and a cursor close or a commit that raises after the CREATE was refused no longer turns the refusal into "inconclusive". Logging: the probe table is named BEFORE it is created, so a process killed between the CREATE and the DROP leaves a traceable name; create_destination() names the database it is creating in, which precheck alone used to report; and each precheck outcome (skipped, refused, created, inconclusive) now has its own line, with inconclusive downgraded to info to match _run_write_probe. Tests cover the real-DDL statement, the 3523 refusal, the blank-Database sentence, the probe name in the log, a non-driver CREATE error, SELECT DATABASE raising and returning nothing, the DBC.TablesV lookup raising, get_cursor() raising, and teardown raising after a confirmed refusal. The live integration test now asserts the precheck left no unstructured_precheck_% table behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
2 participants
What changes
The SQL destination precheck was
SELECT 1. That runs against the session, so it succeeds on any credential the driver can open a connection with and says nothing about the table the uploader writes to. A credential holding CONNECT, USAGE and SELECT and no INSERT passed setup and then failed on every record at write time.Precheck now asks the question, using the real statements made harmless. The probe's privilege surface has to match the upload's exactly -- wider refuses a credential that works, narrower passes one that cannot write -- and
upload_dataframeis delete-then-insert, so it is both:The DELETE runs only when
can_delete()is true, the same gateupload_dataframeputs its own DELETE behind, so a table with no record-id column is never asked for a right the upload does not use. The engine runs the privilege check when it plans the statement, before any row is produced, so a refusal arrives while the row count is still zero and nothing is written or removed even where the driver is in autocommit.The columns are
get_table_columns(), not*, because that is what the uploader's own INSERT names once_fit_to_schemahas conformed the frame. Grants are per-column on every engine here, so a probe asking for rights the write path does not use would refuse credentials that work.The message names the right that was refused. A credential that can insert and cannot delete is told it needs DELETE, with no mention of the INSERT it holds. Both probes run even when the first is refused, so a credential short of both is told both at once.
One-sided by construction: it refuses only on an answer the dialect recognizes as an unambiguous privilege denial, and passes on a timeout, a dropped connection, a missing table, a schema error, an unrecognized code, a dialect with no classifier, or an exception from the classifier itself. Codes meaning "does not exist OR you may not see it" (postgres
42P01, teradata3807, snowflake002003) are excluded, so a typo'd table name is never reported as a permissions problem.The schema read the probes are built from is held to the same contract.
get_table_columns()runs aSELECT * FROM <table> LIMIT 1, which the write path runs too, so a credential refused there cannot upload; that refusal is now classified rather than swallowed, and a credential denied even SELECT is refused at precheck instead of passing it. Teradata is deliberately left out of that one: its 3807 means "does not exist or you may not see it", and a configured table that does not exist yet is a working Teradata destination becausecreate_destination()builds it at upload time.A refusal is a
UserError(422), never aUserAuthError-- the credential is valid, and telling the customer to rotate it sends them to fix the wrong thing.Two dialects used to log a denial with no code to read.
sqlite_errorcodenow crosses the safe-attribute allowlist, andTeradataUploaderappendsteradata_error=NNNNthrough a new_probe_error_detailseam. Only the parsed code crosses, never the message it came from.Covers postgres, singlestore, sqlite, snowflake and teradata. databricks-delta-tables, vastdb and ibm-watsonx-s3 define their own precheck and are untouched.
What it does not cover
Two gaps, both in the direction a one-sided check is allowed to be wrong in: they pass here and can still fail at upload.
create_destination()needs CREATE TABLE, which this does not probe. It is reached only when the configured table does not exist yet, in which case the schema read fails first and the probe reports unknown and passes. Probing a create right is not side-effect free and is a different problem from probing rights on a table that already exists.WITH CHECKpolicy and a Snowflake row access policy are both evaluated per row, so neither is reached.Proof
Reproduced on local postgres 16.15 with roles granted exactly one scenario each. Before:
After:
inserterholds INSERT and not DELETE: it fails every record because the write path deletes first, and it is now refused for DELETE specifically.writerandcolgrantare the rows that did not move --colgrantholds column-level grants only, runs the real upload fine, and is not refused.SingleStore was exercised live against a real server (
errno 1142for both a refused INSERT and a refused DELETE, zero rows left); SQLite against a read-only database file and against a table with no record-id column, where the DELETE probe is correctly skipped.Snowflake and Teradata are a WAIVED environment leg: both signals come from vendor documentation and the drivers' own source and are unit-tested, but no Snowflake account or Teradata instance is reachable from this machine.
Dependencies / merge order
none
Testing
make test-unit). In a CI-shaped venv with all four SQL drivers absent,test/unitis 1740 passed / 69 skipped with no collection errors, which is the group that was red before the rebase.test_postgres_destination_precheck_refuses_a_credential_that_cannot_writepasses, fails onorigin/mainwithDID NOT RAISE UserError, and fails with the DELETE leg alone removed.ruff check .clean.