Skip to content

Commit 9d6b2ee

Browse files
maltesanderclaude
andcommitted
feat!: adapt to core's catalog query types and sealed rows, and correct four info values
The follow-up the previous commit deferred, plus everything else core moved since. Three breaking changes with no compiling intermediate state, so they land together. Catalog query types. The six catalog methods take a sealed query object instead of five to eight positional arguments — `SQLForeignKeys` alone took six `Option<&str>` in a row, where crossing a primary-key argument with its foreign-key counterpart compiled without complaint. The query travels all the way into `metadata.rs` rather than being unpacked at the trait boundary, which would reintroduce that hazard one layer down. `TablesQuery::table_types()` is a `&[String]` core has already split on commas and stripped the quotes from, so `metadata::tables` loses its own parsing; a lone "%" still arrives, because the `SQL_ALL_TABLE_TYPES` enumeration core answers itself additionally requires the other three arguments to be empty strings, and is still read as no filter. Sealed rows. Every catalog row type is `#[non_exhaustive]`, so the eight struct literals become `Default` plus the consuming setter per column. A column this driver does not populate is now unnamed rather than spelled `None`, which is the point — it makes a column added to a spec result set a core-only change — so each site says in a comment which columns it leaves NULL and why. Ten new required capability hooks. Six are values this driver already stated and that now move out of `sqlite_get_info` into the hook, because answering in both places is the "declare it once" violation AGENTS.md describes: `driver_name`, `driver_version`, `dbms_name`, `dbms_version`, `integrity` and `txn_capable`. The snapshot pins all six regardless of which layer answers, which is what made moving them safe. Two more were already pinned at core's default and are now claims this driver makes on purpose: `accessible_procedures` "N" and `txn_capable` SQL_TC_DML. `driver_name`/`driver_version` take no connection — the Windows DM asks for driver identity before `SQLDriverConnectW` — while `dbms_name`/`dbms_version` describe what was connected to and take one. Two info values were wrong, and both are now live-probed rather than read off the documentation. `SQL_QUOTED_IDENTIFIER_CASE` claimed SQL_IC_SENSITIVE, telling an application that "T" and "t" are different tables; in SQLite double quotes are a delimiter, not a case-sensitivity switch, so it is SQL_IC_MIXED, and the probe asserts both halves of that — case-insensitive matching and mixed-case storage. `SQL_SPECIAL_CHARACTERS` claimed "", which was core's old default rather than a claim this driver ever made; SQLite parses `$` in an undelimited identifier, so it is "$", probed over 31 candidates with the rejected ones asserted too. The negative half is what stops it understating again, the same lesson `alter_table_capabilities_are_each_live_probed` records. SQLRowCount. Core now reads a zero-column statement reporting `Some(0)` as SQL_NO_DATA, per SQLExecDirect's Comments, which surfaced that this driver answered `Some(0)` for DDL — so every `CREATE TABLE` it ran returned SQL_NO_DATA to the application, and 60 of the 66 initial test failures were that. `row_count` now distinguishes "counted zero" from "no count applies". SQLite exposes no predicate for this (`sqlite3_stmt_readonly` is false for DDL too), so `is_searched_dml` decides from the leading keyword, past whitespace and both comment forms, counting REPLACE and WITH alongside the obvious three. That also removes a stale count: `sqlite3_changes()` reports the most recently completed INSERT, UPDATE or DELETE, so a `CREATE TABLE` run after a three-row INSERT was handed that 3 and reported it. Two values changed underneath us, both core-owned and both describing core's fetch path rather than SQLite, so the snapshot follows: `SQL_CURSOR_SENSITIVITY` to SQL_UNSPECIFIED, and `SQL_FORWARD_ONLY_CURSOR_ATTRIBUTES2` to SQL_CA2_READ_ONLY_CONCURRENCY. Tests. The two `get_info_every_named_info_type_has_the_declared_shape_*` tests asserted SQL_SUCCESS where core now documents that the shape probe's zero-length buffer is total truncation; they assert "not SQL_ERROR", which is what their own messages always claimed. `dbms_ver_is_well_formed` and `driver_ver_is_well_formed` read through the hooks, the first via `test_connection` since it needs a data source. `SQL_MULTIPLE_ACTIVE_TXN` has no `odbc_sys::InfoType` variant, so it is pinned through the raw path — the snapshot iterates named types only. Also fixes a pre-existing rustdoc failure that only surfaced once the crate compiled again: a public doc comment linked to the `pub(crate)` `SqliteConnection::interrupt`. Verified against stackable-odbc-core dd25a22: cargo test 280 passing, clippy clean, `pre-commit run --all-files` green across all 15 hooks, and the pyodbc suite 23/23 through real unixODBC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9c081d5 commit 9d6b2ee

7 files changed

Lines changed: 962 additions & 389 deletions

File tree

AGENTS.md

Lines changed: 87 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ the 73 C ABI entry points — lives in
2020
| [Declaring capabilities](#declaring-capabilities) | Adding or changing any `SQLGetInfo` value |
2121
| [Transactions](#transactions) | Touching `SQLEndTran`, autocommit or cursor behaviour |
2222
| [Cancellation](#cancellation) | Touching `SQLCancel` or `SQL_ATTR_QUERY_TIMEOUT` |
23+
| [`row_count` has three answers](#row_count-has-three-answers-not-two) | Touching `SQLRowCount` or the execute path |
2324
| [Catalog functions](#catalog-functions) | Touching anything in `metadata.rs` |
2425
| [Architecture](#architecture-of-this-crate) | Understanding the module layout |
2526
| [Connection string keys](#connection-string-keys) | Adding or changing a parameter |
@@ -179,26 +180,35 @@ database file is `08001`. Failures after that point are `08S01`.
179180

180181
### Declaring capabilities
181182

182-
`Backend` has around two dozen **required** methods that state what SQLite can
183+
`Backend` has around thirty **required** methods that state what SQLite can
183184
do — `alter_table_support`, `outer_join_capabilities`, `subqueries`,
184185
`sql_conformance`, `supports_catalogs`, `identifier_case`,
185-
`txn_isolation_options`, `table_types` and the rest. They are required, with no
186-
default, deliberately: a defaulted capability is a claim no backend ever made,
187-
and every one of them was a bug here before core made it a compile error.
188-
`table_types` is required for the same reason and one of its own: an empty
189-
table-type list is an *answer* ("this data source has no table types"), not
190-
"unknown", and unlike catalogs and schemas there is no `supports_*` method for
191-
core to derive it from.
186+
`quoted_identifier_case`, `txn_capable`, `txn_isolation_options`, `integrity`,
187+
`multiple_active_txn`, `special_characters`, `accessible_procedures`,
188+
`dbms_name`, `dbms_version`, `table_types` and the rest. They are required,
189+
with no default, deliberately: a defaulted capability is a claim no backend
190+
ever made, and every one of them was a bug here before core made it a compile
191+
error. `table_types` is required for the same reason and one of its own: an
192+
empty table-type list is an *answer* ("this data source has no table types"),
193+
not "unknown", and unlike catalogs and schemas there is no `supports_*` method
194+
for core to derive it from. `special_characters` is required on that same
195+
principle — `""` asserts that nothing beyond the alphanumerics and underscore
196+
is legal unquoted, which is a claim, not an absence, and inheriting it as a
197+
default is how this driver came to under-report `$`.
192198

193199
They all take `&Self::Connection`, because `SQLGetInfo` is a per-connection
194200
call and a data source's capabilities can differ by server. Every one this
195201
driver declares is a property of the SQLite `rusqlite` links, not of the file
196202
opened, so each ignores the argument — but the answer must still be read
197203
through a connection, and the tests do that via `info::tests::test_connection`
198204
rather than calling the hook as a free function. `cursor_commit_behavior`,
199-
`cursor_rollback_behavior` and `catalog_result_column_widths` are the
200-
exceptions and take none: `SQLGetInfo` must answer the first two before a
201-
connection exists.
205+
`cursor_rollback_behavior`, `catalog_result_column_widths`, `driver_name` and
206+
`driver_version` are the exceptions and take none: `SQLGetInfo` must answer the
207+
first three before a connection exists, and the Windows Driver Manager asks for
208+
driver identity before `SQLDriverConnectW`. Note the split within the identity
209+
group — `driver_name`/`driver_version` describe the driver and take no
210+
connection, while `dbms_name`/`dbms_version` describe what was connected to and
211+
take one.
202212

203213
The same split runs through `get_info`. `sqlite_get_info` takes
204214
`Option<&SqliteConnection>``None` on the pre-connect path — and hands it to
@@ -366,14 +376,77 @@ This is load-bearing well beyond memory use. It is why the cursor-behaviour
366376
hooks report `Preserve`, why `SQLEndTran` cannot disturb a cursor, and why
367377
concurrency is a non-issue. Changing it is not a local optimisation.
368378

379+
### `row_count` has three answers, not two
380+
381+
`StatementBackend::row_count` returns `Option<i64>`, and core reads all three
382+
possibilities differently:
383+
384+
| Answer | Means | Here |
385+
|--------|-------|------|
386+
| `Some(n)` | the backend counted | a searched INSERT / UPDATE / DELETE, or a materialised result set |
387+
| `Some(-1)` | `SQL_NO_TOTAL`, cannot determine | a count exceeding `i64`; unreachable in practice |
388+
| `None` | not applicable to this statement | DDL, transaction control, `PRAGMA`, an unexecuted prepared statement |
389+
390+
The distinction between the last two is not cosmetic. Core turns a statement
391+
with **zero columns** reporting **`Some(0)`** into `SQL_NO_DATA`, which is
392+
`SQLExecDirect`'s documented behaviour for "a searched update, insert, or
393+
delete statement that doesn't affect any rows". Answering `Some(0)` for DDL
394+
therefore made every `CREATE TABLE` return `SQL_NO_DATA`.
395+
396+
SQLite offers no predicate for "is this DML" — `sqlite3_stmt_readonly` is false
397+
for DDL too — so `execute::is_searched_dml` decides it from the statement's
398+
leading keyword, past whitespace and both comment forms. `REPLACE` and `WITH`
399+
count alongside the obvious three: the first is an `INSERT OR REPLACE` alias,
400+
and the second fronts a CTE, which is only ever consulted for a zero-column
401+
statement, so a `WITH` that declared no columns cannot be a `WITH ... SELECT`.
402+
Being wrong is not symmetric, so an unrecognised keyword answers "no count":
403+
withholding a count leaves `SQLRowCount` at -1, while inventing one fabricates
404+
`SQL_NO_DATA`.
405+
406+
Do **not** replace this with the number `rusqlite`'s `execute()` returns.
407+
`sqlite3_changes()` reports the rows touched by the *most recently completed*
408+
INSERT, UPDATE or DELETE, so a `CREATE TABLE` run after a three-row `INSERT` is
409+
handed that `3`. `ddl_after_dml_does_not_inherit_the_dml_row_count` pins it.
410+
369411
### Catalog functions
370412

371-
The six catalog methods return **typed row vectors**`Vec<TableRow>`,
413+
The six catalog methods take a **typed query object**`&TablesQuery`,
414+
`&ColumnsQuery`, `&PrimaryKeysQuery`, `&ForeignKeysQuery`, `&StatisticsQuery`,
415+
`&SpecialColumnsQuery` — and return **typed row vectors**`Vec<TableRow>`,
372416
`Vec<ColumnRow>`, `Vec<PrimaryKeyRow>`, `Vec<ForeignKeyRow>`,
373417
`Vec<StatisticsRow>`, `Vec<SpecialColumnRow>` — not a `Self::Statement`. Core
374418
converts each row to the spec's column layout, sorts the set into the order
375-
that function's spec page mandates, and serves it. Three consequences for
376-
anything changed in `metadata.rs`:
419+
that function's spec page mandates, and serves it.
420+
421+
Both sides are core's types and both are sealed, which is what a change in
422+
`metadata.rs` has to work with:
423+
424+
- **Neither has a struct expression here.** Every row type is
425+
`#[non_exhaustive]`, so a row is built from `Default` and the consuming
426+
setter per column: `TableRow::default().name(n).table_type(t)`. Each setter
427+
takes `impl Into<T>`, so an `Option<String>` column accepts a bare `String`.
428+
A column a driver does not populate is simply not named — which is the point,
429+
since it makes a column added to a spec result set a core-only change instead
430+
of a break in every driver. The query types are sealed the same way, with
431+
crate-private fields, an accessor and a `with_*` setter per field, and a
432+
`new()` for the arguments that have no honest default (`StatisticsQuery`'s
433+
`unique_only`, `SpecialColumnsQuery`'s `identifier_type`/`scope`/`nullable`).
434+
- **Read the filters off the query, do not destructure it.** The run of
435+
same-typed `Option<&str>` arguments these hooks used to take is exactly what
436+
the query types exist to remove: `SQLForeignKeys` took six in a row, where
437+
swapping a primary-key argument for its foreign-key counterpart compiled
438+
without complaint. Unpacking a query back into positional arguments at the
439+
trait boundary reintroduces that hazard one layer down, so the query travels
440+
all the way into `metadata.rs`.
441+
- **`TablesQuery::table_types()` is already parsed.** Core splits `TableType`
442+
on commas and strips the optional single quotes — it is a value list, not a
443+
pattern, and `SQL_ATTR_METADATA_ID` never applies to it — so a backend gets a
444+
`&[String]` and never parses it. Empty means no filter. A lone `"%"` does
445+
still arrive, because the `SQL_ALL_TABLE_TYPES` enumeration core answers
446+
itself additionally requires the other three arguments to be empty strings;
447+
`metadata::tables` reads that as no filter.
448+
449+
Three further consequences for anything changed in `metadata.rs`:
377450

378451
- **Do not sort, and do not add an `ORDER BY` for ODBC's sake.** Core sorts,
379452
stably, on the spec's keys. A second ordering in the backend is one more

CHANGELOG.md

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
7070

7171
### Changed
7272

73+
- `SQL_QUOTED_IDENTIFIER_CASE` reports `SQL_IC_MIXED` instead of
74+
`SQL_IC_SENSITIVE`. In SQLite, double quotes are a *delimiter* — they let a
75+
keyword or a name with punctuation be used as an identifier — and do not
76+
switch on case-sensitive matching the way they do in a SQL-92 conformant
77+
DBMS: a table created as `"MixedCase"` is found by `"mixedcase"`, and the
78+
catalog stores the name with the case it was written in. The old value told
79+
an application that `"T"` and `"t"` were different tables. Both halves of the
80+
new claim — case-insensitive matching and mixed-case storage — are probed
81+
against the bundled library rather than read off the documentation.
82+
83+
- `SQL_SPECIAL_CHARACTERS` reports `$` instead of the empty string. SQLite's
84+
tokenizer treats `$` as an identifier character, so `a$b` parses undelimited
85+
and round-trips through `sqlite_master` unchanged. An application reads this
86+
info type to decide when it must quote, and the empty string had it quoting a
87+
name that needs no quoting. The empty string was `stackable-odbc-core`'s
88+
default rather than a claim this driver ever made; it is now a per-connection
89+
`Backend` hook, and every candidate character is executed against the bundled
90+
library, the rejected ones included.
91+
92+
- `SQL_CURSOR_SENSITIVITY` reports `SQL_UNSPECIFIED` instead of
93+
`SQL_INSENSITIVE`, and `SQL_FORWARD_ONLY_CURSOR_ATTRIBUTES2` reports
94+
`SQL_CA2_READ_ONLY_CONCURRENCY` instead of `0`. Both describe
95+
`stackable-odbc-core`'s own fetch path rather than SQLite, and both now come
96+
from core: insensitivity would be a promise that no other cursor's changes
97+
become visible, which core does not make about rows it has not read yet,
98+
while `0` for the second denied the one concurrency
99+
`SQLSetStmtAttr(SQL_ATTR_CONCURRENCY)` actually accepts. This follows a
100+
`stackable-odbc-core` change.
101+
73102
- `SQLDescribeCol` and `SQLColAttribute` report each result column's real
74103
nullability instead of claiming every column is nullable. A column declared
75104
`NOT NULL` is now `SQL_NO_NULLS`, a plain table column `SQL_NULLABLE`, and a
@@ -224,6 +253,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
224253

225254
### Fixed
226255

256+
- `SQLRowCount` reported `0` after a `CREATE TABLE`, `DROP TABLE`, `ALTER
257+
TABLE`, `BEGIN`, `COMMIT`, `PRAGMA` or `VACUUM`, where the spec's
258+
affected-row count does not apply at all. The three answers are now distinct:
259+
a count for a searched INSERT / UPDATE / DELETE, the materialised size of a
260+
result set, and *no count* for everything else. This matters beyond
261+
tidiness — `stackable-odbc-core` reads a zero-column statement reporting a
262+
counted zero as `SQL_NO_DATA`, per `SQLExecDirect`'s Comments, so every DDL
263+
statement this driver ran returned `SQL_NO_DATA` to the application instead
264+
of `SQL_SUCCESS`. A searched DELETE that matches nothing still reports `0`,
265+
which is the case the spec reserves `SQL_NO_DATA` for.
266+
267+
The same fix removes a stale count: `sqlite3_changes()` reports the rows
268+
touched by the *most recently completed* INSERT, UPDATE or DELETE, so a
269+
`CREATE TABLE` run straight after a three-row `INSERT` was handed that `3`
270+
and reported it.
271+
227272
- `SQLForeignKeys` reported `PKCOLUMN_NAME` as NULL for a foreign key declared
228273
without an explicit column list (`REFERENCES parent`), a column the spec
229274
marks "not NULL". SQLite defines the implicit target as the parent table's

0 commit comments

Comments
 (0)