Skip to content

Commit f13b60c

Browse files
maltesanderclaude
andcommitted
fix!: let the backend state its own reserved words
`common_get_info_raw` answered `SQL_KEYWORDS` (89) with the empty string for every backend. By core's own rule -- is empty "unknown", or is it an answer? -- that is an answer. The spec defines the info type as the data source's keywords *excluding* ODBC's own, so `""` says "this data source reserves nothing beyond ODBC", and applications read it to decide which identifiers need quoting. Both existing drivers have plenty; a wrong empty list is how a generated identifier goes unquoted and the statement fails to parse. Add a required `Backend::keywords` for the raw list, and move the half that is spec data rather than data-source data into core: `ODBC_RESERVED_KEYWORDS` (the Appendix C list, transcribed in the order the spec page gives it) now lives in `types/constants.rs`, and the subtraction, sort and comma-join happen once in `data_source_specific_keywords`. Computed per call rather than cached: a `static` cannot be generic over `B`, and this is a linear scan of a short list on a path `SQLGetInfo` reaches a handful of times per connection. A backend whose list is expensive to produce caches on its own side -- said so in the hook's doc comment. Required, not defaulted, for the same reason as the rest of the capability group: an empty list is a falsifiable claim about the data source that core has no way to make. The two classification mocks declare different lists, so `the_two_classification_mocks_share_no_capability_declaration` covers it. Asserted through the full `sql_get_info_w` path: `&[]` still yields `""`, `["SELECT", "UNNEST"]` yields `"UNNEST"`, and `["select"]` yields `""`. BREAKING CHANGE: `Backend::keywords` is a new required trait method. Every backend must implement it; there is no default, because an empty list is the claim that the data source reserves nothing beyond ODBC. Returning `&[]` reproduces the value core previously gave everyone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 29da4d2 commit f13b60c

6 files changed

Lines changed: 592 additions & 10 deletions

File tree

‎AGENTS.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ These deliberately are not:
230230
| `accessible_tables` | `SQL_ACCESSIBLE_TABLES` |
231231
| `data_source_read_only` | `SQL_DATA_SOURCE_READ_ONLY` |
232232
| `search_pattern_escape` | `SQL_SEARCH_PATTERN_ESCAPE` |
233+
| `keywords` | the data source's own reserved words, *before* ODBC's are subtracted (`SQL_KEYWORDS`) |
233234

234235
Each states a **capability**, where any default is a claim the backend author
235236
never made. `0` understates ("this data source cannot do this at all") and
@@ -259,6 +260,12 @@ Two corollaries worth checking when adding an info type:
259260
`info_type_default_response` gives an unhandled `String`-shaped info type
260261
`""`, which is the right *shape* but is not in any Y/N value list. Such a
261262
type needs either a shared `"N"` arm in `default_get_info` or a hook.
263+
- **An empty *list* is an answer too.** `SQL_KEYWORDS` reads as an empty
264+
string just like an unhandled `String`-shaped type, but it means "this data
265+
source reserves nothing beyond ODBC" — which applications act on when
266+
deciding what to quote. It is a `Backend::keywords` hook for that reason;
267+
core owns only the spec's subtraction of `ODBC_RESERVED_KEYWORDS`, which is
268+
the same for every backend.
262269
- **Watch for info types that constrain each other.** `SQL_SQL_CONFORMANCE`
263270
fixes the value of `SQL_GROUP_BY`, `SQL_CORRELATION_NAME`,
264271
`SQL_NON_NULLABLE_COLUMNS`, `SQL_CONCAT_NULL_BEHAVIOR`, `SQL_SUBQUERIES`

‎CHANGELOG.md‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
7070
`set_prepared_statement` and `discard_result_set` helpers that maintain it.
7171
`StatementHandle::statement` no longer doubles as the answer to "is a cursor
7272
open?" — see the `Fixed` entry below.
73+
- `types::ODBC_RESERVED_KEYWORDS`, the reserved-word list from Appendix C of
74+
the ODBC specification. `SQL_KEYWORDS` is defined as the data source's own
75+
keywords *excluding* these, so the list and the subtraction now live in core
76+
once instead of being transcribed into each driver.
7377

7478
### Changed
7579

@@ -108,13 +112,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
108112
`timedate_add_intervals`, `timedate_diff_intervals`, `subqueries`,
109113
`column_alias`, `concat_null_behavior`, `union_support`,
110114
`convert_functions`, `order_by_columns_in_select`, `accessible_tables`,
111-
`data_source_read_only` and `search_pattern_escape`.
115+
`data_source_read_only`, `search_pattern_escape` and `keywords`.
112116
They are required rather than defaulted on purpose:
113117
each states a *capability*, where a defaulted value is a claim the backend
114118
author never made and is unlikely to notice. A defaulted `0` understates and
115119
a defaulted `true` overstates; the compiler asking is what makes the fact
116120
explicit. Every one of them replaces a value core previously invented (see
117121
`Fixed`).
122+
- **Breaking:** `SQL_KEYWORDS` (89) now comes from the new required
123+
`Backend::keywords`, filtered against `ODBC_RESERVED_KEYWORDS`, sorted and
124+
comma-separated. **Every backend must implement it** — there is no default,
125+
because an empty list is the claim that the data source reserves nothing
126+
beyond ODBC. A backend returns its raw reserved words in any order and any
127+
case; core applies the spec's subtraction. It does so on every call rather
128+
than caching (a `static` cannot be generic over the backend); a backend whose
129+
list is expensive to produce should cache on its own side. Returning `&[]`
130+
reproduces the value core previously gave everyone.
118131
- `SQL_ALTER_TABLE` and `SQL_OJ_CAPABILITIES` now come from
119132
`Backend::alter_table_support` / `Backend::outer_join_capabilities` instead
120133
of defaulting to `0`. `0` remains the shared default for the surrounding
@@ -142,7 +155,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
142155
`SQL_ROW_UPDATES` (11) `"N"`, `SQL_PROCEDURES` (21) `"N"`,
143156
`SQL_MULTIPLE_ACTIVE_TXN` (37) `"N"`, `SQL_DATABASE_NAME` (16) `""`,
144157
`SQL_PROCEDURE_TERM` (40) `""` (consistent with `SQL_PROCEDURES = "N"`),
145-
`SQL_TABLE_TERM` (45) `"table"`, and `SQL_KEYWORDS` (89) `""`.
158+
`SQL_TABLE_TERM` (45) `"table"`, and `SQL_KEYWORDS` (89) — the last of these
159+
now carrying the backend's own list rather than `""`, see below.
146160
All come from `common_get_info_raw`, and core's own fallback consults that
147161
helper, so a backend whose `get_info_raw` does not delegate to it gets the
148162
same answer. That last change also fixes `SQL_QUOTED_IDENTIFIER_CASE` for
@@ -187,6 +201,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
187201
- `SQL_SEARCH_PATTERN_ESCAPE` comes from the backend. It describes what
188202
escapes `%` and `_` in catalog-function pattern arguments, which the backend
189203
interprets.
204+
- `SQL_KEYWORDS` is stated by the backend. Core answered it with `""` for
205+
everyone, which is not an absence of an answer but the claim that the data
206+
source reserves nothing beyond ODBC — and applications read it to decide
207+
which identifiers need quoting, so a wrong empty list is how a generated
208+
identifier goes unquoted and the statement fails to parse. Every real
209+
backend has keywords of its own.
190210
- `SQL_EXPRESSIONS_IN_ORDERBY` is stated by the backend. It previously fell to
191211
`""`, which reads as "no" to a tool deciding whether to push an expression
192212
into `ORDER BY`.

‎src/backend.rs‎

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,27 @@ pub trait Backend: Sized + Send + Sync + 'static {
553553
/// cannot distinguish from a backend that simply never set it.
554554
fn search_pattern_escape() -> &'static str;
555555

556+
/// The data source's own reserved words, unfiltered.
557+
///
558+
/// Core reports `SQL_KEYWORDS` (89) from this, after removing everything
559+
/// ODBC already reserves — the spec defines the info type as the data
560+
/// source's keywords *excluding* its own ("This list does not contain
561+
/// keywords specific to ODBC or keywords used by both the data source and
562+
/// ODBC"), so a backend states the raw fact and core applies the rule once
563+
/// against [`ODBC_RESERVED_KEYWORDS`](crate::types::ODBC_RESERVED_KEYWORDS).
564+
///
565+
/// Required, not defaulted: an empty list is the claim that this data
566+
/// source reserves nothing beyond ODBC, which applications act on when
567+
/// deciding what to quote.
568+
///
569+
/// Return the raw names in any order and in any case; core filters
570+
/// case-insensitively, sorts, and joins. Note that it does so on **every**
571+
/// call rather than caching: a `static` cannot be generic over `B`, and
572+
/// `SQLGetInfo(SQL_KEYWORDS)` is not a hot path. A backend whose list is
573+
/// expensive to produce — read out of a linked library, say — should cache
574+
/// behind its own `OnceLock` and return the cached slice from here.
575+
fn keywords() -> &'static [&'static str];
576+
556577
/// The `SQL_TIMEDATE_ADD_INTERVALS` (109) bitmask: the interval units the
557578
/// `TIMESTAMPADD` scalar function accepts, as an OR of the
558579
/// [`SQL_FN_TSI_*`](crate::types::SQL_FN_TSI_SECOND) constants.
@@ -915,6 +936,36 @@ pub fn default_get_info<B: Backend>(
915936
/// Generic over the calling backend so that `SQL_CURSOR_ROLLBACK_BEHAVIOR` can be
916937
/// derived from [`Backend::cursor_rollback_behavior`] -- call it as
917938
/// `common_get_info_raw::<Self>(info_type)`.
939+
/// The `SQL_KEYWORDS` (89) value for `B`: [`Backend::keywords`] minus
940+
/// everything ODBC itself reserves, sorted, comma-separated with no spaces.
941+
///
942+
/// The subtraction is what the spec defines the info type to be — "This list
943+
/// does not contain keywords specific to ODBC or keywords used by both the
944+
/// data source and ODBC" — so it lives here once rather than in every backend.
945+
/// Comparison is ASCII-case-insensitive: the reserved list is upper-case and a
946+
/// data source that spells its keywords in lower case still shares them.
947+
///
948+
/// Sorting is not required by the spec, but makes the value stable for
949+
/// anything that diffs or caches it, whatever order the backend enumerates in.
950+
///
951+
/// Recomputed on every call. A `static` cache cannot be generic over `B`, and
952+
/// this is a linear scan of a short list against a fixed one on a path
953+
/// `SQLGetInfo` reaches at most a handful of times per connection; a backend
954+
/// with an expensive list caches on its own side (see [`Backend::keywords`]).
955+
fn data_source_specific_keywords<B: Backend>() -> String {
956+
let mut names: Vec<&'static str> = B::keywords()
957+
.iter()
958+
.copied()
959+
.filter(|name| {
960+
!crate::types::ODBC_RESERVED_KEYWORDS
961+
.iter()
962+
.any(|reserved| reserved.eq_ignore_ascii_case(name))
963+
})
964+
.collect();
965+
names.sort_unstable();
966+
names.join(",")
967+
}
968+
918969
pub fn common_get_info_raw<B: Backend>(info_type: u16) -> Option<InfoValue> {
919970
use crate::types::{
920971
InfoValue, SQL_CURSOR_ROLLBACK_BEHAVIOR, SQL_DATABASE_NAME, SQL_FILE_USAGE,
@@ -942,15 +993,19 @@ pub fn common_get_info_raw<B: Backend>(info_type: u16) -> Option<InfoValue> {
942993
// Core opens no transaction of its own, so it certainly cannot hold
943994
// two open at once. A backend that can answers before delegating.
944995
SQL_MULTIPLE_ACTIVE_TXN => Some(InfoValue::String("N".into())),
996+
// The data source's own reserved words, minus everything ODBC already
997+
// reserves -- the subtraction the spec defines for this info type,
998+
// applied once here rather than in each backend. The list itself is a
999+
// capability, so it comes from `Backend::keywords`; core only owns the
1000+
// rule.
1001+
SQL_KEYWORDS => Some(InfoValue::String(data_source_specific_keywords::<B>())),
9451002
// The remaining character-string info types with no
946-
// `odbc_sys::InfoType` variant. Empty is a valid value for all but
947-
// SQL_TABLE_TERM: there is no shared name for the current database,
948-
// no shared list of data-source-specific reserved words, and -- given
1003+
// `odbc_sys::InfoType` variant. Empty is a valid value for both:
1004+
// there is no shared name for the current database, and -- given
9491005
// SQL_PROCEDURES above answers "N" -- no procedures to have a vendor
950-
// term for. Every data source has tables, so that one gets the
1006+
// term for. Every data source has tables, so SQL_TABLE_TERM gets the
9511007
// generic term rather than "".
9521008
SQL_DATABASE_NAME => Some(InfoValue::String(String::new())),
953-
SQL_KEYWORDS => Some(InfoValue::String(String::new())),
9541009
SQL_PROCEDURE_TERM => Some(InfoValue::String(String::new())),
9551010
SQL_TABLE_TERM => Some(InfoValue::String("table".into())),
9561011
_ => None,
@@ -1627,6 +1682,7 @@ mod tests {
16271682
accessible_tables,
16281683
data_source_read_only,
16291684
search_pattern_escape,
1685+
keywords,
16301686
cursor_commit_behavior,
16311687
cursor_rollback_behavior,
16321688
);

‎src/ffi/info.rs‎

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,9 +1163,12 @@ mod tests {
11631163
// Every data source has tables, so unlike the catalog and schema terms
11641164
// this one has no "empty if unsupported" case.
11651165
(crate::types::SQL_TABLE_TERM, "table", "SQL_TABLE_TERM"),
1166-
// A comma-separated list of data-source-specific reserved words; empty
1167-
// is a valid list and the only honest shared answer.
1168-
(crate::types::SQL_KEYWORDS, "", "SQL_KEYWORDS"),
1166+
// A comma-separated list of data-source-specific reserved words,
1167+
// derived from `MockBackend::keywords` with ODBC's own subtracted out
1168+
// (`SELECT` is dropped) and the remainder sorted. Asserted here rather
1169+
// than only in `backend.rs` so the value is proven to survive the
1170+
// string-shaped `sql_get_info_w` path, not just the raw dispatch.
1171+
(crate::types::SQL_KEYWORDS, "MOCK_ATTACH,MOCK_PRAGMA", "SQL_KEYWORDS"),
11691172
(crate::types::SQL_DATABASE_NAME, "", "SQL_DATABASE_NAME"),
11701173
];
11711174

@@ -1231,4 +1234,96 @@ mod tests {
12311234
cleanup(env, conn, stmt);
12321235
}
12331236
}
1237+
1238+
/// Runs `SQLGetInfoW(SQL_KEYWORDS)` against `B` through the whole FFI path
1239+
/// — allocate, connect, query, disconnect, free — and returns the text it
1240+
/// wrote. Generic, unlike the shared helpers above, because the point of
1241+
/// these assertions is that the answer moves with the backend.
1242+
unsafe fn sql_keywords_of<B: Backend>() -> String {
1243+
unsafe {
1244+
let mut env: *mut c_void = std::ptr::null_mut();
1245+
let _ = sql_alloc_handle::<B>(HandleType::Env as i16, std::ptr::null_mut(), &mut env);
1246+
let mut conn: *mut c_void = std::ptr::null_mut();
1247+
let _ = sql_alloc_handle::<B>(HandleType::Dbc as i16, env, &mut conn);
1248+
1249+
let wide: Vec<u16> = "Host=localhost;Database=test".encode_utf16().collect();
1250+
assert_eq!(
1251+
crate::ffi::connect::sql_driver_connect_w::<B>(
1252+
conn,
1253+
std::ptr::null_mut(),
1254+
wide.as_ptr(),
1255+
wide.len() as i16,
1256+
std::ptr::null_mut(),
1257+
0,
1258+
std::ptr::null_mut(),
1259+
0,
1260+
),
1261+
SqlReturn::SUCCESS,
1262+
);
1263+
1264+
// Sentinel-filled, so an empty answer is distinguishable from a
1265+
// `U32(0)` one that merely looks empty.
1266+
let mut buf = [0xEEu16; 64];
1267+
let mut str_len: i16 = -1;
1268+
let ret = sql_get_info_w::<B>(
1269+
conn,
1270+
crate::types::SQL_KEYWORDS,
1271+
buf.as_mut_ptr() as *mut c_void,
1272+
(buf.len() * 2) as i16,
1273+
&mut str_len,
1274+
);
1275+
assert_eq!(ret, SqlReturn::SUCCESS, "SQL_KEYWORDS must not error");
1276+
assert!(
1277+
str_len >= 0 && str_len % 2 == 0,
1278+
"SQL_KEYWORDS is a character string, so StringLength is an even \
1279+
byte count; got {str_len}"
1280+
);
1281+
let units = str_len as usize / 2;
1282+
let value = String::from_utf16_lossy(&buf[..units]);
1283+
assert_eq!(buf[units], 0, "SQL_KEYWORDS must be null-terminated");
1284+
1285+
let _ = crate::ffi::connect::sql_disconnect::<B>(conn);
1286+
let _ = sql_free_handle::<B>(HandleType::Dbc as i16, conn);
1287+
let _ = sql_free_handle::<B>(HandleType::Env as i16, env);
1288+
value
1289+
}
1290+
}
1291+
1292+
/// `SQL_KEYWORDS` is the spec's *subtraction*: the data source's reserved
1293+
/// words minus the ones ODBC already reserves. Core owns that rule and
1294+
/// `Backend::keywords` owns the raw list, so what an application sees has
1295+
/// to move with the backend — the empty string core used to return for
1296+
/// everyone is the claim "this data source reserves nothing beyond ODBC",
1297+
/// which is how a generated identifier ends up unquoted.
1298+
///
1299+
/// Asserted through `sql_get_info_w` rather than `common_get_info_raw`, so
1300+
/// the dispatch ordering is covered too.
1301+
#[test]
1302+
fn sql_keywords_is_the_backend_list_minus_odbcs_own() {
1303+
use crate::test_utils::{
1304+
MockAltBackend, MockNoKeywordsBackend, MockOverlappingKeywordsBackend,
1305+
MockReservedOnlyKeywordsBackend,
1306+
};
1307+
1308+
unsafe {
1309+
// Nothing to subtract from: the same value core produced before the
1310+
// hook existed, so the shape is unchanged for a data source that
1311+
// genuinely reserves nothing extra.
1312+
assert_eq!(sql_keywords_of::<MockNoKeywordsBackend>(), "");
1313+
// `SELECT` is ODBC's; `UNNEST` is not.
1314+
assert_eq!(
1315+
sql_keywords_of::<MockOverlappingKeywordsBackend>(),
1316+
"UNNEST"
1317+
);
1318+
// Case-insensitive, so a lower-case list of ODBC's own words leaves
1319+
// nothing behind.
1320+
assert_eq!(sql_keywords_of::<MockReservedOnlyKeywordsBackend>(), "");
1321+
// Sorted and comma-separated with no spaces, whatever order the
1322+
// backend enumerates in — `MockBackend` declares
1323+
// `["MOCK_PRAGMA", "SELECT", "MOCK_ATTACH"]`.
1324+
assert_eq!(sql_keywords_of::<MockBackend>(), "MOCK_ATTACH,MOCK_PRAGMA");
1325+
// And it moves with the backend.
1326+
assert_eq!(sql_keywords_of::<MockAltBackend>(), "ALT_VACUUM");
1327+
}
1328+
}
12341329
}

0 commit comments

Comments
 (0)