fix(teradata): refuse a table the credential cannot read - Pylon 3035

[paulkarayan]

What & why

Problem: A customer points a Teradata destination at a table their admin created, and every record silently fails to write, because the non-admin credential the connector authenticates as holds nothing on that table. They find out after a job has run, not before, because the connector check told them it was Successful. The check proved only that the credential could log on. When the data dictionary already listed the configured table, the precheck returned early without ever reading it, and the shared write probe that would have caught it never ran: Teradata's get_table_columns() re-raises the driver error as a UserError, and the classifier that decides whether a schema-read failure is a denial tests the exception's module. A wrapped error is not a driver error, so every denial on this dialect was swallowed, including the unambiguous ones.

Change: When the dictionary says the configured table exists, the precheck now runs the same SELECT TOP 1 * the upload runs first, and refuses a credential the server turns away on the codes that mean "no privilege" and nothing else. A table that is not there yet still passes, because create_destination() builds it.

Blast radius: 3/5 -- one connector, but it turns a passing check into a hard refusal, so a wrong classification would block a destination that works.

Linked ticket

none

Client-facing follow-up: connector check reported Successful for a destination the credential could not write, and every record then failed -- Pylon 3035

Impact

• Customers: anyone configuring a Teradata destination against a table they do not own now learns it at connector-creation time instead of after a job has run and failed. The message names the table and the grants to ask for. Nobody whose credential can write its destination sees any change: a readable table passes, and a table that does not exist yet passes.

• Internal: support stops fielding "the check passed but the job failed" on this connector. No other dialect moves -- sql.py gains only a corrected comment, and the refusal lives entirely in the Teradata uploader.

• Wire contract / clients: a new UserError (422) at precheck for a case that used to return success. It is a user-fault, non-retryable classification, which is what the platform already does with every other precheck refusal in this module.

  One behaviour change beyond the check itself, called out because it is easy to miss: the dictionary lookup now matches TableKind IN ('T','O') rather than 'T' alone. 'O' is a table with no primary index and no partitioning, so an admin-created NoPI table used to read as absent. _table_exists is shared with create_destination(), which means such a table previously got a CREATE and a 3803 "already exists", and now gets written into, the same as any 'T' table. That is the behaviour the connector always intended for an existing destination, but it is a change.

• Deployment target considerations: ships wherever the uploader plugin ships. No target-specific behaviour: the change is one SQL statement and a classification, with no new dependency, config or egress.

Risk / rollback

The refusal is one-sided by construction: it fires only when the dictionary returns a row for the configured table AND the server refuses this credential's read of it. Anything unrecognised is logged and passes, so the failure mode is the old behaviour, not a false refusal. Revert the PR to back it out; there is no migration and no state.

How it was verified

Against a live Teradata Vantage 20.0.0.68, reached through the teradata-dev DI, and a before/after on the same server with the same configuration:

• This branch's precheck(), with the database set to PDCRDATA and the table to PDCR_Table_Retention (a table the dictionary lists and the credential cannot read), raised UserError 422: "The destination credentials can connect to the database but do not have SELECT permission on table 'PDCR_Table_Retention'. Records would fail to write. Grant SELECT on that table to the user this connector authenticates as." No driver text, host, user or password in the message. TD_OFSDB.PURGED_OBJECTS behaved identically.
• Stock 1.11.19 on the identical server and configuration passed with no exception, logging write-permission check skipped, table schema unavailable: UserError(status_code=422). That is the reported defect, reproduced live.
• No false refusal: data_scientist.pp99_table, which that credential can read, passed.
• The SQL connector unit suite passes, and the repo's make check-ruff is clean.

Not verified: the absent-table path was deliberately not driven against that server, because precheck() is not read-only there -- it creates and drops a real table under a throwaway name and then runs zero-row INSERT and DELETE probes, and the server is Teradata's preprod rather than our sandbox. That path is covered by unit tests.

Proof

Repro-first, with one leg WAIVED.

Proof waived (environment) -- no Vantage we can create two principals on. The live run borrows tables an administrator happened to leave unreadable rather than a table an admin created for the test. What unblocks it: any Vantage where we may create an admin and a non-admin user and a table between them.

Repro. A fake-driver simulation drives the real connector code with the customer's configuration: Database field blank, the session's default database is the admin's, the destination table already exists there, and the connector's user holds CREATE TABLE/DROP TABLE on the database and nothing on the table. Plus a unit test that scripts the same answers through precheck() and, deliberately, does not mock get_table_columns() -- mocking it is what hid this defect through two releases.

Red. With the new tests present and the connector reverted to origin/main:

test_teradata_precheck_refuses_a_table_the_credential_cannot_see
E  Failed: DID NOT RAISE <class 'unstructured_ingest.error.UserError'>

A second red, for the false refusal the independent review found, with the connector at the revision that still refused on 3807 and the two new guards present:

test_teradata_read_denial_classifier_does_not_refuse_on_3807            FAILED
test_teradata_precheck_passes_when_an_existing_table_reads_as_missing   FAILED

A third red, for a hole found after the first commit, against that commit:

test_teradata_existence_lookup_covers_a_table_with_no_primary_index
E  assert "TableKind IN ('T','O')" in "SELECT 1 FROM DBC.TablesV WHERE TableName = ?
   AND DatabaseName = ? AND TableKind = 'T'"

Fix. The existing-table branch of check_create_table_permission no longer just returns. It runs get_table_columns()'s own statement and classifies the driver's raw answer: the codes that mean "no privilege" and nothing else refuse, as they do everywhere else in this module. Nothing refuses on 3807.

Why the dictionary row settles it. DBC.TablesV is the non-restricted form of the view. Teradata's Data Dictionary manual: the non-X views "return every row of every column defined on the underlying table", where viewnameX returns only what the requesting user owns, created, was granted, or reaches through a current or nested role. Measured on the live server: 795 tables listed to a credential that can reach 360 of them. So a row in TablesV is an existence fact that does not depend on this credential's rights.

What the server actually answers, which is not what this module assumed. Reading any of the 435 tables that are in TablesV and not in TablesVX returns:

[Version 20.0.0.68] [Session 168281] [Teradata Database] [Error 3523] [SQLState 42000]
The user does not have SELECT access to PDCRDATA.PDCR_Table_Retention.

Every time, on six tables across six databases, DBC and user databases alike, and identically for the qualified form and the unqualified form this connector issues. Not one 3807. Teradata does not hide existing objects behind a non-existence code; it names the database and the table. Three comments in this module asserted the opposite and are corrected here. 3523 was already an unambiguous denial, so what fixes the customer's case is simply that the probe now runs. One unit test pins the verbatim measured 3523 including its [SQLState 42000] segment, which no hand-written mock in the file carried: a regex tightened to expect the message right after [Error NNNN] would pass every other test and drop the refusal on a real server.

Why 3807 refuses nothing, which an independent review changed. An earlier revision of this branch refused on 3807 whenever DBC.TablesV had just returned a row, on the reasoning that the dictionary settles the "object does not exist" half of that code. An independent GPT review found the hole: an admin can drop the table between the dictionary lookup and the read, and the upload that follows would find it absent, have create_destination() build it, and write successfully. Refusing there turns a race into a working destination reported as broken, which is the one outcome this check may not produce. 3807 now refuses at no site, and two tests pin that.

After. The live before/after and the unit results are under "How it was verified".

Dependencies / merge order

none

[cubic-dev-ai]

2 issues found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="test/unit/connectors/sql/test_teradata.py">

<violation number="1" location="test/unit/connectors/sql/test_teradata.py:2764">
P2: These tests do not pass as written: after the inconclusive existing-table probe, `precheck()` retries the same failing `SELECT TOP 1 *` through `check_write_permissions()`, so it raises instead of reaching the “must not raise” assertions. Make the failure one-shot if this test only covers the first probe, or update the precheck flow to reuse the inconclusive result before invoking the write checks.</violation>
</file>

<file name="unstructured_ingest/processes/connectors/sql/teradata.py">

<violation number="1" location="unstructured_ingest/processes/connectors/sql/teradata.py:857">
P2: The existing-table read classifier reports database-level denials 3524 and 5315 as `SELECT` permission missing on the table. Tell the customer to grant the required access on `database` for these codes, otherwise the actionable precheck message sends the DBA to the wrong object.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: These tests do not pass as written: after the inconclusive existing-table probe, precheck() retries the same failing SELECT TOP 1 * through check_write_permissions(), so it raises instead of reaching the “must not raise” assertions. Make the failure one-shot if this test only covers the first probe, or update the precheck flow to reuse the inconclusive result before invoking the write checks.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At test/unit/connectors/sql/test_teradata.py, line 2764:

<comment>These tests do not pass as written: after the inconclusive existing-table probe, `precheck()` retries the same failing `SELECT TOP 1 *` through `check_write_permissions()`, so it raises instead of reaching the “must not raise” assertions. Make the failure one-shot if this test only covers the first probe, or update the precheck flow to reuse the inconclusive result before invoking the write checks.</comment>

<file context>
@@ -2602,3 +2604,306 @@ def test_teradata_precheck_refusal_omits_the_table_name_hint_when_one_is_configu
+    _scripted_cursor(
+        mock_cursor,
+        exists=True,
+        fail={_READ_PROBE: _FakeTeradataDriverError(f"[Teradata Database] [Error {code}] x")},
+    )
+
</file context>

[cubic-dev-ai]

P2: The existing-table read classifier reports database-level denials 3524 and 5315 as SELECT permission missing on the table. Tell the customer to grant the required access on database for these codes, otherwise the actionable precheck message sends the DBA to the wrong object.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_ingest/processes/connectors/sql/teradata.py, line 857:

<comment>The existing-table read classifier reports database-level denials 3524 and 5315 as `SELECT` permission missing on the table. Tell the customer to grant the required access on `database` for these codes, otherwise the actionable precheck message sends the DBA to the wrong object.</comment>

<file context>
@@ -749,6 +784,89 @@ def _probe_table_creation(
+        if not _is_teradata_driver_error(error):
+            return None
+        code = _extract_teradata_error_code(error)
+        if code in _WRITE_DENIAL_TERADATA_CODES:
+            return self._write_denied_message("SELECT") + self._blank_database_hint()
+        if code == _OBJECT_DOES_NOT_EXIST_CODE:
</file context>

Suggested change

	if code in _WRITE_DENIAL_TERADATA_CODES:
	return self._write_denied_message("SELECT") + self._blank_database_hint()
	if code in {3523, 5612}:
	return self._write_denied_message("SELECT") + self._blank_database_hint()
	if code in {3524, 5315}:
	return self._write_denied_message(
	"SELECT", object_kind="database", object_name=database
	) + self._blank_database_hint()

[cubic-dev-ai]

0 issues found across 3 files (changes from recent commits).

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.

Re-trigger cubic</sub>

[tabossert]

Approving. The fix is right for the reported case: the probe runs the same read the upload needs first, so it can't refuse a destination that works today, and an INSERT-only credential was already failing every job. Everything below is non-blocking, roughly in priority order.

1. A transient dictionary-lookup failure still lets the swallow through. If SELECT DATABASE or the DBC.TablesV lookup raises (a timeout or a dropped session, say), check_create_table_permission logs "inconclusive" and returns at teradata.py:683-693 before _probe_existing_table_read runs. check_write_permissions then hits sql.py:462-474, where _classify_schema_read_denial still returns None for the wrapped UserError, so a 3523 on the table passes. A persistent lookup failure isn't the problem, since create_destination() runs the same lookup unguarded at teradata.py:607-610 and every job fails there anyway. The case that matters is the job's own lookup later succeeding and its first read of the table hitting the 3523. Nothing tests it: test_teradata.py:2482-2505 (which predates this PR) fails the lookup but lets the table read succeed. CHANGELOG bullet 3 also reads as if the swallow is fixed outright.

Root-cause fix: override _classify_schema_read_denial in TeradataUploader to classify error first and then error.__context__. get_table_columns() raises the UserError inside its except, so __context__ is the driver error, which the docstring at teradata.py:200-204 already promises. classify_write_denial skips 3807, so the objection in the sql.py:514-518 docstring (re-raising the UserError unconditionally) doesn't apply to that shape. An alternative is to also run the read probe when the lookup fails. That would need its own cursor, since the exception has already left the with block.

2. 5612 isn't a privilege code. Teradata's message catalog has 5612 as "A user, database, role, or zone with the specified name already exists" (https://docs.teradata.com/r/Teradata-VantageCloud-Lake-Analytics-Database-Messages/Database-Messages/5612). The module labels it "user does not have any access to the object" (teradata.py:143) and keeps it in _WRITE_DENIAL_TERADATA_CODES (teradata.py:545), so the new read probe refuses on it (teradata.py:855). Nothing the precheck runs creates a user, database, role or zone, so it shouldn't fire in practice. It predates this PR (#811), so a follow-up is fine, but it doesn't belong in the "no privilege and nothing else" set.

3. Several comments contradict what the PR now says about 3807.

• teradata.py:965-966: "_probe_existing_table_read may still refuse on 3807". It doesn't.
• The _probe_existing_table_read docstring (teradata.py:789-805) still argues from the revision that refused on 3807: "that ordering is the whole reason the answer is readable", "whichever code the refusal carries", and "The one thing it can be wrong about is a table dropped between...". The last one contradicts teradata.py:845-851, where that race passes. "Do not move this probe ahead of the dictionary lookup" is still right, but for a different reason: the lookup is what tells you create_destination() won't build the table.
• sql.py:602-604 (older, untouched by the PR) still says Teradata reports "no such object" and "you may not see this object" with the same code. The PR retracts that at sql.py:513-520 and in the three comments it lists, but misses this one.
• teradata.py:542-543 (also older): "3807 is excluded: it is also Teradata's 'object does not exist'". The "also" keeps the overloaded reading that teradata.py:86-92 now retracts.
• teradata.py:562-566: "NOTHING in this module refuses on it, at any site" is too broad. _USER_FAULT_TERADATA_CODES still turns 3807 into a UserError on the upload path and in TeradataIndexer.precheck (teradata.py:398-418, pinned by test_teradata.py:586-602). Scope it to the destination precheck. The PR body's "3807 now refuses at no site" has the same overbreadth, and it's what becomes the squash commit message here.

4. An inconclusive probe reads the table twice. On a code that isn't a denial (2644, 3807, ...) or a non-driver error, the probe logs "inconclusive" and returns, then check_write_permissions runs the same SELECT TOP 1 * again through get_table_columns(). If that second read also fails with a driver error, teradata.py:984-986 logs "failed to read schema for table ..." at ERROR, so a passing precheck carries an ERROR line. That's also why cubic's "these tests do not pass" note is off: all 191 tests in test_teradata.py pass at head, because sql.py:462-474 catches the retry. Skipping the second read after an inconclusive probe would save the round trip, but it would also drop the retry that today lets a transient first failure still reach the INSERT/DELETE probes. So it's a trade-off, not a clear fix. The ERROR itself predates this PR; it also fires on every precheck of a table that doesn't exist yet.

5. TableKind IN ('T','O') changes the upload path too. _table_exists is shared with create_destination(), so an admin-created NoPI table used to get a CREATE that failed with 3803, and now gets written into. That's a fix, but the only test for it asserts the SQL substring through precheck(). test_teradata_uploader_create_destination_skips_when_table_exists already checks "returns False, no CREATE", but its mock returns the row by script, so it passes with or without 'O'. A create_destination() test that asserts its lookup includes TableKind IN ('T','O') would pin the upload-path side. It's worth a rollback note too: after a revert, those destinations go back to 3803, which isn't in _USER_FAULT_TERADATA_CODES and surfaces as DestinationConnectionError "Failed to connect to server", pointing at the network.

6. Test gaps in the new branch. On the existing-table read path only 3523 goes through precheck(); 3524, 5315 and 5612 are covered only by the direct classifier test (test_teradata.py:2834). Nothing covers a successful probe with cursor.description None or empty, or _columns already being set. The inconclusive-read tests don't assert that the CREATE probe is absent, so a fall-through into the else branch wouldn't be caught.

Smaller stuff

• The read-refusal message (teradata.py:856) doesn't include the Teradata code, while the CREATE 3523 message does ((error {code}, ...), teradata.py:883). All four codes produce the same "do not have SELECT permission" text, so a customer screenshot doesn't show which one the server returned; the code only reaches the ERROR log.
• The outer except handler in check_create_table_permission (teradata.py:683-697) still words its logs for the CREATE branch. If the cursor close or get_connection()'s commit/close fails after the read probe, it logs "CREATE TABLE permission check inconclusive" or "...after the server refused the create; the refusal stands", though no CREATE ran.
• The probe statement (teradata.py:807) and get_table_columns() (teradata.py:979) match only because _quote_identifier quotes the same way. A shared helper would keep them from drifting.
• teradata.py:90-92 says reading "any of the other 435" tables answers 3523, and the commit message says "on all 435 tables". The PR body's own evidence is "on six tables across six databases". If six were read, say six.
• The probe start logs at DEBUG (teradata.py:809) and get_connection() sets no request_timeout. A plain SELECT takes a READ lock, which waits behind another session's WRITE lock (a load in progress, say), so a blocked probe shows up as silence after "...skipping the CREATE TABLE probe". Logging the probe at INFO would help whoever debugs it.

Questions

• VECTOR32: per teradata.py:552-559, a VECTOR32 column needs UDTUSAGE on SYSUDTLIB and refuses with 3523. Does SELECT * on such a table need it too? If so, the read probe tells the DBA to grant SELECT on the table when the missing right is UDTUSAGE. Not a false refusal, just the wrong grant named.
• Views and queue tables: the _table_exists comment (teradata.py:726-730) says they aren't things upload_dataframe can read and INSERT into. My understanding is Teradata allows INSERT through an updatable view and into a queue table, though I haven't confirmed it against the docs. If leaving them out is a product choice, it'd be clearer to say that.

[tabossert]

Approving; non-blocking notes in the comment above.