Skip to content

Commit 29da4d2

Browse files
maltesanderclaude
andcommitted
fix!: enforce the backend/SQLGetInfo split with a test
Every item in the last three rounds was found by reading default_get_info by hand and asking whether core had any business knowing the value. That does not scale and it kept missing things. Replace the audit with a test. default_get_info_answers_are_backend_derived_or_declared_core_facts evaluates default_get_info for two mock backends that share no capability declaration, and asks one question of every info type: does the answer move when the backend does? An info type answering identically for both is one core decided, and must appear in the test's CORE_FACTS list with the reason core is entitled to decide it -- a fact about core's own implementation (its fetch really is forward-only, the Backend trait really is synchronous), a limit where the spec defines 0 as "no limit or unknown", or driver identity with no per-backend answer. Hard-coding a claim about the data source now fails a test that names the info type. A second test asserts the two mocks differ in every hook, since the classification is only as strong as that. Running it surfaced what the hand audit had left: - SQL_SUBQUERIES, SQL_COLUMN_ALIAS and SQL_CONCAT_NULL_BEHAVIOR are the other half of the conformance contradiction fixed in 5f797be. The spec names all three as values a SQL-92 Entry level-conformant driver returns and core hard-coded exactly those, so a backend declaring no conformance level was still reported as supporting correlated subqueries, quantified predicates and column aliases. SQL_SUBQUERIES is the one applications act on: claiming SQL_SQ_CORRELATED_SUBQUERIES for a source without them is how a BI tool pushes down SQL the server rejects. SQL_CONCAT_NULL_BEHAVIOR was also a bare 0 literal for a spec-named constant; SQL_CB_NULL and SQL_CB_NON_NULL now exist. - SQL_UNION, SQL_CONVERT_FUNCTIONS, SQL_ORDER_BY_COLUMNS_IN_SELECT, SQL_ACCESSIBLE_TABLES, SQL_DATA_SOURCE_READ_ONLY and SQL_SEARCH_PATTERN_ESCAPE were all statements about the data source core had no way to know. SQL_ACCESSIBLE_TABLES was the sharpest: "Y" guarantees the connected user has SELECT on every table SQLTables returns, which depends on the principal, not the driver. - SQL_IDENTIFIER_QUOTE_CHAR is now derived from EscapeDialect::identifier_quotes instead of hard-coded to `"`. The escape translator already consulted the dialect, so a backend quoting with a backtick had core telling applications something the translator contradicted. No new hook -- the fact was already declared, which is the better fix and is now written into AGENTS.md as a rule. BREAKING CHANGE: Backend gains required methods subqueries, column_alias, concat_null_behavior, union_support, convert_functions, order_by_columns_in_select, accessible_tables, data_source_read_only and search_pattern_escape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 886007b commit 29da4d2

5 files changed

Lines changed: 618 additions & 25 deletions

File tree

AGENTS.md

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,15 @@ These deliberately are not:
221221
| `sql_conformance` | `SQL_SQL_CONFORMANCE` (`SQL_SC_*`) |
222222
| `timedate_add_intervals` | `SQL_TIMEDATE_ADD_INTERVALS` (`SQL_FN_TSI_*`) |
223223
| `timedate_diff_intervals` | `SQL_TIMEDATE_DIFF_INTERVALS` (`SQL_FN_TSI_*`) |
224+
| `subqueries` | `SQL_SUBQUERIES` (`SQL_SQ_*`) |
225+
| `column_alias` | `SQL_COLUMN_ALIAS` |
226+
| `concat_null_behavior` | `SQL_CONCAT_NULL_BEHAVIOR` (`0` = `SQL_CB_NULL`) |
227+
| `union_support` | `SQL_UNION` (`SQL_U_*`) |
228+
| `convert_functions` | `SQL_CONVERT_FUNCTIONS` (`SQL_FN_CVT_*`) |
229+
| `order_by_columns_in_select` | `SQL_ORDER_BY_COLUMNS_IN_SELECT` |
230+
| `accessible_tables` | `SQL_ACCESSIBLE_TABLES` |
231+
| `data_source_read_only` | `SQL_DATA_SOURCE_READ_ONLY` |
232+
| `search_pattern_escape` | `SQL_SEARCH_PATTERN_ESCAPE` |
224233

225234
Each states a **capability**, where any default is a claim the backend author
226235
never made. `0` understates ("this data source cannot do this at all") and
@@ -251,12 +260,37 @@ Two corollaries worth checking when adding an info type:
251260
`""`, which is the right *shape* but is not in any Y/N value list. Such a
252261
type needs either a shared `"N"` arm in `default_get_info` or a hook.
253262
- **Watch for info types that constrain each other.** `SQL_SQL_CONFORMANCE`
254-
fixes the value of `SQL_GROUP_BY`, `SQL_CORRELATION_NAME` and
255-
`SQL_NON_NULLABLE_COLUMNS` (the spec names what an entry-level driver
256-
returns for each); `SQL_TIMEDATE_FUNCTIONS` claiming
263+
fixes the value of `SQL_GROUP_BY`, `SQL_CORRELATION_NAME`,
264+
`SQL_NON_NULLABLE_COLUMNS`, `SQL_CONCAT_NULL_BEHAVIOR`, `SQL_SUBQUERIES`
265+
and `SQL_COLUMN_ALIAS` — the spec names what an entry-level driver returns
266+
for each of those six. `SQL_TIMEDATE_FUNCTIONS` claiming
257267
`SQL_FN_TD_TIMESTAMPADD` obliges `SQL_TIMEDATE_ADD_INTERVALS` to be
258-
non-zero. Core supplying one side of such a pair while the backend supplies
259-
the other is how it ends up contradicting itself.
268+
non-zero. `SQL_CATALOG_NAME` drives the whole catalog group. Core supplying
269+
one side of such a pair while the backend supplies the other is how it ends
270+
up contradicting itself.
271+
- **Prefer deriving over adding a hook when the fact is already declared.**
272+
`SQL_IDENTIFIER_QUOTE_CHAR` comes from `EscapeDialect::identifier_quotes`
273+
and `SQL_CURSOR_COMMIT_BEHAVIOR` from `Backend::cursor_commit_behavior`,
274+
because a second way to state the same fact is a second way to state it
275+
*differently*. Check whether an existing hook already answers the question
276+
before adding one.
277+
278+
#### The rule is enforced by a test, not by review
279+
280+
`default_get_info_answers_are_backend_derived_or_declared_core_facts`
281+
(`src/backend.rs`) asks one question of every info type: **does the answer
282+
move when the backend does?** It evaluates `default_get_info` for two mock
283+
backends that share no capability declaration. An info type answering
284+
identically for both is one core decided, and must appear in that test's
285+
`CORE_FACTS` list together with the reason core is entitled to decide it —
286+
a fact about core's own implementation (its fetch really is forward-only, its
287+
`Backend` trait really is synchronous), a limit where the spec defines `0` as
288+
"no limit or unknown", or driver-level identity with no per-backend answer.
289+
290+
So adding a hard-coded claim to `default_get_info` fails a test that names the
291+
info type. If you find yourself adding an entry to `CORE_FACTS`, the reason
292+
string is the test: if you cannot write one that is about *core* rather than
293+
about the data source, the value belongs on a `Backend` method.
260294

261295
`supports_catalogs` and `supports_schemas` between them drive seven info types
262296
(`SQL_CATALOG_NAME`, `SQL_CATALOG_TERM`, `SQL_CATALOG_NAME_SEPARATOR`,

CHANGELOG.md

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3535
`/usr/include/sql.h` and `sqlext.h` by
3636
`info_type_value_constants_match_sql_headers`. These are values where a typo
3737
cannot look empty, because zero is itself a valid claim for several of them.
38+
- A test that enforces the backend/`SQLGetInfo` split instead of leaving it to
39+
review. `default_get_info_answers_are_backend_derived_or_declared_core_facts`
40+
evaluates `default_get_info` for two mock backends sharing no capability
41+
declaration; any info type answering identically for both is one core
42+
decided, and must be listed with the reason core is entitled to decide it —
43+
a fact about core's own implementation, a limit where the spec defines `0`
44+
as "no limit or unknown", or driver identity. Hard-coding a claim about the
45+
data source now fails a test naming the info type. Every item fixed in this
46+
release was found by hand; this is what stops the next one needing that.
3847
- `EscapeDialect::rewrite_scalar_fn`, which receives a whole
3948
`{fn NAME(args)}` escape and returns the replacement text.
4049
`remap_scalar_fn` only swaps the identifier in front of the parentheses and
@@ -96,7 +105,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
96105
`outer_join_capabilities`, `default_txn_isolation`,
97106
`txn_isolation_options`, `group_by`, `null_collation`, `correlation_name`,
98107
`non_nullable_columns`, `expressions_in_order_by`, `sql_conformance`,
99-
`timedate_add_intervals` and `timedate_diff_intervals`.
108+
`timedate_add_intervals`, `timedate_diff_intervals`, `subqueries`,
109+
`column_alias`, `concat_null_behavior`, `union_support`,
110+
`convert_functions`, `order_by_columns_in_select`, `accessible_tables`,
111+
`data_source_read_only` and `search_pattern_escape`.
100112
They are required rather than defaulted on purpose:
101113
each states a *capability*, where a defaulted value is a claim the backend
102114
author never made and is unlikely to notice. A defaulted `0` understates and
@@ -151,6 +163,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
151163
`SQL_CORRELATION_NAME`, `SQL_NON_NULLABLE_COLUMNS` and `SQL_GROUP_BY` values
152164
the spec says an entry-level driver never returns, so every backend built on
153165
core inherited a contradiction it could not see.
166+
- `SQL_SUBQUERIES`, `SQL_COLUMN_ALIAS` and `SQL_CONCAT_NULL_BEHAVIOR` come
167+
from the backend too — the other half of that same contradiction. The spec
168+
names all three as values a SQL-92 Entry level-conformant driver returns,
169+
and core hard-coded exactly those, so a backend declaring *no* conformance
170+
level was still reported as supporting correlated subqueries, quantified
171+
predicates and column aliases. `SQL_SUBQUERIES` is the one an application
172+
acts on: claiming `SQL_SQ_CORRELATED_SUBQUERIES` for a source without them
173+
is how a BI tool comes to push down SQL the server rejects.
174+
`SQL_CONCAT_NULL_BEHAVIOR` was additionally a bare `0` literal for a
175+
spec-named constant; `SQL_CB_NULL` and `SQL_CB_NON_NULL` now exist.
176+
- `SQL_UNION`, `SQL_CONVERT_FUNCTIONS`, `SQL_ORDER_BY_COLUMNS_IN_SELECT`,
177+
`SQL_ACCESSIBLE_TABLES` and `SQL_DATA_SOURCE_READ_ONLY` come from the
178+
backend. Each was a statement about the data source that core had no way to
179+
know. `SQL_ACCESSIBLE_TABLES` was the sharpest: `"Y"` guarantees the
180+
connected user has `SELECT` on every table `SQLTables` returns, which
181+
depends on the principal, not the driver.
182+
- `SQL_IDENTIFIER_QUOTE_CHAR` is derived from
183+
`EscapeDialect::identifier_quotes` rather than hard-coded to `"`. The escape
184+
translator already consulted the dialect, so a backend quoting identifiers
185+
with a backtick or brackets had core telling applications something the
186+
translator contradicted. No new hook: the fact was already declared.
187+
- `SQL_SEARCH_PATTERN_ESCAPE` comes from the backend. It describes what
188+
escapes `%` and `_` in catalog-function pattern arguments, which the backend
189+
interprets.
154190
- `SQL_EXPRESSIONS_IN_ORDERBY` is stated by the backend. It previously fell to
155191
`""`, which reads as "no" to a tool deciding whether to push an expression
156192
into `ORDER BY`.

0 commit comments

Comments
 (0)