feat: ODBC-backed table driver (read + write) behind the ACE ABI (builds on #18)#24
feat: ODBC-backed table driver (read + write) behind the ACE ABI (builds on #18)#24Admnwk wants to merge 7 commits into
Conversation
…Soft#18) Adds an optional, read-only ODBC table driver so a Harbour rddads app (USE / SKIP / SEEK / FieldGet) can talk to any data source that has an ODBC driver -- SQL Server, Oracle, Firebird, PostgreSQL, MariaDB, DB2, Access, ... -- without recompiling. Mirrors the existing SQL backends: a thin OdbcConnection/OdbcTable pair under src/sql_backend, HandleKind::Odbc*, and additive dispatch at the ACE border (get_odbc_table alongside get_remote_table). Off by default; enable with -DOPENADS_WITH_ODBC=ON, which links the system ODBC driver manager (odbc32 on Windows, unixODBC on Linux) -- no external dependency, the headers and import library ship with the platform SDK. Navigation uses an in-memory primary-key snapshot and only ever issues SELECT / WHERE / ORDER BY / COUNT -- no LIMIT/OFFSET/TOP and no scrollable-cursor dependency -- so it is portable across drivers. The key is discovered via SQLPrimaryKeys, falling back to the first UNIQUE index reported by SQLStatistics for drivers that do not implement SQLPrimaryKeys. Column metadata comes from SQLColumns; numeric literals are emitted unquoted (type-aware) so integer/double key comparisons work on strict engines. String literals use standard quote doubling and all identifiers are validated, so the dynamic SQL is injection-safe. Scope: read + seek + navigate. Write (append/update/delete) is a later slice; until then a write on an ODBC-backed table is rejected at the border rather than misrouted. Tests: odbc_uri_test (always built when the backend is on), plus abi_plus_odbc_{read,seek}_test which drive the ABI against an ODBC connection string in OPENADS_TEST_ODBC_CONNSTR and self-skip when it is unset. tools/scripts/run_odbc_tests.ps1 seeds a throwaway fixture and runs them; build_msvc_x64_odbc.bat configures an MSVC build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a read-only ODBC backend to OpenADS, allowing connection, schema discovery, and navigation (such as seeking and skipping) on ODBC-backed tables. Key feedback from the review highlights several critical issues, including a potential crash in read_all_rows due to unhandled negative indicator values, potential null pointer dereferences when pucField is null, and missing checks on the return value of SQLSetEnvAttr. Additionally, the reviewer noted that schema filtering is required when calling SQLPrimaryKeys, SQLStatistics, and SQLColumns to prevent mixing columns from duplicate table names across different schemas. Finally, several performance optimizations were suggested, such as caching uppercase field names to avoid repeated conversions and adding operator< to PkRow to enable faster O(log N) lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| std::size_t chunk; | ||
| if (ind == SQL_NO_TOTAL || | ||
| ind >= static_cast<SQLLEN>(sizeof(buf))) { | ||
| chunk = sizeof(buf) - 1; // truncated; NUL eats one byte | ||
| } else { | ||
| chunk = static_cast<std::size_t>(ind); | ||
| } | ||
| val.append(buf, chunk); |
There was a problem hiding this comment.
If ind is negative (e.g., representing a driver-specific indicator or unexpected status) and is not SQL_NULL_DATA or SQL_NO_TOTAL, casting it directly to std::size_t will result in a extremely large value. This will cause a crash or std::length_error when calling val.append(buf, chunk). We should explicitly check for any other negative values and handle them safely.
| std::size_t chunk; | |
| if (ind == SQL_NO_TOTAL || | |
| ind >= static_cast<SQLLEN>(sizeof(buf))) { | |
| chunk = sizeof(buf) - 1; // truncated; NUL eats one byte | |
| } else { | |
| chunk = static_cast<std::size_t>(ind); | |
| } | |
| val.append(buf, chunk); | |
| std::size_t chunk; | |
| if (ind == SQL_NO_TOTAL || | |
| ind >= static_cast<SQLLEN>(sizeof(buf))) { | |
| chunk = sizeof(buf) - 1; // truncated; NUL eats one byte | |
| } else if (ind < 0) { | |
| chunk = 0; | |
| } else { | |
| chunk = static_cast<std::size_t>(ind); | |
| } | |
| val.append(buf, chunk); |
There was a problem hiding this comment.
Fixed in b36de76: a negative indicator that is neither SQL_NULL_DATA nor SQL_NO_TOTAL now maps to chunk=0 instead of being cast to a huge size_t.
| for (auto& row : rows) { | ||
| std::string col = row.size() >= 4 ? row[3] : std::string(); | ||
| int seq = row.size() >= 5 ? std::atoi(row[4].c_str()) : 0; | ||
| if (!col.empty()) keyed.emplace_back(seq, col); | ||
| } |
There was a problem hiding this comment.
In databases that support schemas (such as SQL Server, PostgreSQL, or Oracle), calling SQLPrimaryKeys with a null schema name can return primary key columns for multiple tables with the same name across different schemas. This will mix primary keys from different tables and result in an incorrect PK snapshot. We should filter the returned rows to only keep columns belonging to the first schema encountered.
std::string chosen_schema;
bool has_schema = false;
for (auto& row : rows) {
std::string schem = row.size() >= 2 ? row[1] : std::string();
std::string col = row.size() >= 4 ? row[3] : std::string();
int seq = row.size() >= 5 ? std::atoi(row[4].c_str()) : 0;
if (!has_schema) {
chosen_schema = schem;
has_schema = true;
}
if (schem != chosen_schema) continue;
if (!col.empty()) keyed.emplace_back(seq, col);
}There was a problem hiding this comment.
Fixed in b36de76: SQLPrimaryKeys rows are now pinned to the first TABLE_SCHEM seen so same-named tables across schemas cannot be mixed.
| for (auto& row : rows) { | ||
| const std::string non_unique = row.size() >= 4 ? row[3] : ""; | ||
| const std::string idx_name = row.size() >= 6 ? row[5] : ""; | ||
| const std::string ord_s = row.size() >= 8 ? row[7] : ""; | ||
| const std::string col_name = row.size() >= 9 ? row[8] : ""; | ||
| if (col_name.empty()) continue; // table-statistic row | ||
| if (non_unique != "0") continue; // only unique indexes | ||
| if (chosen.empty()) chosen = idx_name; | ||
| if (idx_name != chosen) continue; | ||
| keyed.emplace_back(std::atoi(ord_s.c_str()), col_name); | ||
| } |
There was a problem hiding this comment.
Similarly to SQLPrimaryKeys, calling SQLStatistics with a null schema name can return index columns for multiple tables with the same name across different schemas. We should filter the returned rows to only keep columns belonging to the first schema encountered.
std::string chosen_schema;
bool has_schema = false;
std::string chosen;
for (auto& row : rows) {
const std::string schem = row.size() >= 2 ? row[1] : "";
const std::string non_unique = row.size() >= 4 ? row[3] : "";
const std::string idx_name = row.size() >= 6 ? row[5] : "";
const std::string ord_s = row.size() >= 8 ? row[7] : "";
const std::string col_name = row.size() >= 9 ? row[8] : "";
if (col_name.empty()) continue; // table-statistic row
if (non_unique != "0") continue; // only unique indexes
if (!has_schema) {
chosen_schema = schem;
has_schema = true;
}
if (schem != chosen_schema) continue;
if (chosen.empty()) chosen = idx_name;
if (idx_name != chosen) continue;
keyed.emplace_back(std::atoi(ord_s.c_str()), col_name);
}There was a problem hiding this comment.
Fixed in b36de76: same first-schema pinning applied to the SQLStatistics unique-index path.
| std::vector<OdbcTable::FieldDesc> out; | ||
| out.reserve(rows.size()); | ||
| for (auto& row : rows) { | ||
| auto cell = [&](std::size_t one_based) -> std::string { | ||
| std::size_t i = one_based - 1; | ||
| return i < row.size() ? row[i] : std::string(); | ||
| }; | ||
| const std::string cname = cell(4); | ||
| const int dtype = std::atoi(cell(5).c_str()); | ||
| const int csize = std::atoi(cell(7).c_str()); | ||
| const int ddig = std::atoi(cell(9).c_str()); | ||
| const int ncode = std::atoi(cell(11).c_str()); // 0=NO_NULLS,1=NULLABLE | ||
| out.push_back(map_odbc_column(cname, dtype, ncode != 0, csize, ddig)); | ||
| } |
There was a problem hiding this comment.
Calling SQLColumns with a null schema name can return columns for multiple tables with the same name across different schemas. This will result in duplicate columns in the table's field list. We should filter the returned rows to only keep columns belonging to the first schema encountered.
std::string chosen_schema;
bool has_schema = false;
std::vector<OdbcTable::FieldDesc> out;
out.reserve(rows.size());
for (auto& row : rows) {
std::string schem = row.size() >= 2 ? row[1] : "";
if (!has_schema) {
chosen_schema = schem;
has_schema = true;
}
if (schem != chosen_schema) {
continue;
}
auto cell = [&](std::size_t one_based) -> std::string {
std::size_t i = one_based - 1;
return i < row.size() ? row[i] : std::string();
};
const std::string cname = cell(4);
const int dtype = std::atoi(cell(5).c_str());
const int csize = std::atoi(cell(7).c_str());
const int ddig = std::atoi(cell(9).c_str());
const int ncode = std::atoi(cell(11).c_str()); // 0=NO_NULLS,1=NULLABLE
out.push_back(map_odbc_column(cname, dtype, ncode != 0, csize, ddig));
}There was a problem hiding this comment.
Fixed in b36de76: same first-schema pinning applied to SQLColumns so the field list cannot mix same-named tables across schemas.
| std::size_t odbc_field_index(openads::sql_backend::OdbcTable* st, | ||
| UNSIGNED8* pucField) { | ||
| if (!st->fields_cached) { | ||
| if (st->conn == nullptr) { | ||
| return std::numeric_limits<std::size_t>::max(); | ||
| } | ||
| auto r = st->conn->describe_table(st); | ||
| if (!r) return std::numeric_limits<std::size_t>::max(); | ||
| } |
There was a problem hiding this comment.
If pucField is null, calling openads::abi::to_internal(pucField, 0) will dereference a null pointer and crash. We must add a null check at the beginning of odbc_field_index.
std::size_t odbc_field_index(openads::sql_backend::OdbcTable* st,
UNSIGNED8* pucField) {
if (pucField == nullptr) {
return std::numeric_limits<std::size_t>::max();
}
if (!st->fields_cached) {
if (st->conn == nullptr) {
return std::numeric_limits<std::size_t>::max();
}
auto r = st->conn->describe_table(st);
if (!r) return std::numeric_limits<std::size_t>::max();
}There was a problem hiding this comment.
No change: to_internal is null-safe (src/abi/charset.cpp: if (p == nullptr) return {};), so a null pucField yields field name "" -> not found (SIZE_MAX), not a dereference. This mirrors the sibling remote_field_index, which relies on the same guarantee.
| if (!SQL_SUCCEEDED(SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, | ||
| &conn.impl_->env))) { | ||
| return util::Error{5001, 0, "odbc: SQLAllocHandle(ENV) failed", ""}; | ||
| } | ||
| SQLSetEnvAttr(conn.impl_->env, SQL_ATTR_ODBC_VERSION, | ||
| reinterpret_cast<SQLPOINTER>(SQL_OV_ODBC3), 0); |
There was a problem hiding this comment.
The return value of SQLSetEnvAttr is not checked. If it fails, the environment might not be set to ODBC 3, leading to undefined behavior or failures in subsequent ODBC calls. We should check the return value and handle any failure.
if (!SQL_SUCCEEDED(SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE,
&conn.impl_->env))) {
return util::Error{5001, 0, "odbc: SQLAllocHandle(ENV) failed", ""};
}
if (!SQL_SUCCEEDED(SQLSetEnvAttr(conn.impl_->env, SQL_ATTR_ODBC_VERSION,
reinterpret_cast<SQLPOINTER>(SQL_OV_ODBC3), 0))) {
return odbc_error("odbc: SQLSetEnvAttr(ODBC_VERSION)",
odbc_diag(SQL_HANDLE_ENV, conn.impl_->env));
}There was a problem hiding this comment.
Fixed in b36de76: the SQLSetEnvAttr(ODBC_VERSION) return is now checked and surfaced as an error.
| struct FieldDesc { | ||
| std::string name; | ||
| std::uint16_t type = 0; | ||
| std::uint32_t length = 0; | ||
| std::uint16_t decimals = 0; | ||
| bool nullable = true; | ||
| }; |
There was a problem hiding this comment.
To avoid converting field names to uppercase on every single lookup in field_index_ci and odbc_field_index, we should cache the uppercase version of the field name in FieldDesc during schema discovery.
| struct FieldDesc { | |
| std::string name; | |
| std::uint16_t type = 0; | |
| std::uint32_t length = 0; | |
| std::uint16_t decimals = 0; | |
| bool nullable = true; | |
| }; | |
| struct FieldDesc { | |
| std::string name; | |
| std::string name_upper; | |
| std::uint16_t type = 0; | |
| std::uint32_t length = 0; | |
| std::uint16_t decimals = 0; | |
| bool nullable = true; | |
| }; |
There was a problem hiding this comment.
Deferring: field lookup is not a hot path, and the sibling SQL backends (sqlite/pg/maria) keep FieldDesc without a cached uppercase name. Holding the struct parallel across backends for maintainability; can revisit if profiling shows it matters.
| OdbcTable::FieldDesc fd; | ||
| fd.name = name; | ||
| fd.nullable = nullable; |
There was a problem hiding this comment.
Populate the cached uppercase field name during column mapping.
OdbcTable::FieldDesc fd;
fd.name = name;
fd.name_upper = name;
for (auto& c : fd.name_upper) {
c = static_cast<char>(std::toupper(static_cast<unsigned char>(c)));
}
fd.nullable = nullable;There was a problem hiding this comment.
Deferring with the FieldDesc change above (kept parallel with the other SQL backends).
| std::size_t field_index_ci(const OdbcTable& tbl, const std::string& name) { | ||
| std::string want = name; | ||
| for (auto& c : want) { | ||
| c = static_cast<char>(std::toupper(static_cast<unsigned char>(c))); | ||
| } | ||
| for (std::size_t i = 0; i < tbl.fields.size(); ++i) { | ||
| std::string have = tbl.fields[i].name; | ||
| for (auto& c : have) { | ||
| c = static_cast<char>(std::toupper(static_cast<unsigned char>(c))); | ||
| } | ||
| if (have == want) return i; | ||
| } | ||
| return static_cast<std::size_t>(-1); | ||
| } |
There was a problem hiding this comment.
Use the cached uppercase field name to avoid repeated string allocations and uppercase conversions in the lookup loop.
std::size_t field_index_ci(const OdbcTable& tbl, const std::string& name) {
std::string want = name;
for (auto& c : want) {
c = static_cast<char>(std::toupper(static_cast<unsigned char>(c)));
}
for (std::size_t i = 0; i < tbl.fields.size(); ++i) {
if (tbl.fields[i].name_upper == want) return i;
}
return static_cast<std::size_t>(-1);
}There was a problem hiding this comment.
Deferring with the FieldDesc change above.
| struct PkRow { | ||
| std::vector<std::string> values; | ||
| bool operator==(const PkRow& o) const { return values == o.values; } | ||
| }; |
There was a problem hiding this comment.
Add operator< to PkRow to allow storing it in a std::map or using it with standard algorithms. This will enable replacing the seek_index with a fast
| struct PkRow { | |
| std::vector<std::string> values; | |
| bool operator==(const PkRow& o) const { return values == o.values; } | |
| }; | |
| struct PkRow { | |
| std::vector<std::string> values; | |
| bool operator==(const PkRow& o) const { return values == o.values; } | |
| bool operator<(const PkRow& o) const { return values < o.values; } | |
| }; |
There was a problem hiding this comment.
Deferring: the PK snapshot is ordered by the database ORDER BY (collation / numeric), not lexicographically, so a std::lower_bound via operator< on the snapshot would be incorrect (numeric PK 1,2,10 vs string '1','10','2'). An exact-match std::map is viable, but seek is dominated by its SQL round-trip and the in-memory scan is secondary. Noted for a later optimization pass.
- read_all_rows: ignore stray negative SQLGetData indicator values instead of casting them to a huge size_t (defensive crash guard for a misbehaving driver). - discover_pk / describe_columns: pin the SQLPrimaryKeys, SQLStatistics and SQLColumns result sets to the first schema seen (column 2, TABLE_SCHEM). With a null schema argument a driver that supports schemas can return rows for same-named tables across several schemas; mixing them would corrupt the key or the field list. - OdbcConnection::open: check the SQLSetEnvAttr(ODBC_VERSION) return so a failure to select ODBC 3 is surfaced rather than ignored. The full unit suite stays green (528/528) and the live Access e2e (4/4) is unaffected -- the Access driver reports a null schema, so the schema filter is a no-op there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use driver-reported TABLE_NAME for FROM clauses and canonical column names in seek SQL. Firebird stores unquoted identifiers uppercase; quoting the ABI lowercase name broke AdsOpenTable/AdsSeek. Add run_firebird_odbc_tests.ps1 for the portable devai_test.fdb fixture. Co-Authored-By: Grok Code
Use HKLM-registered driver pointing at SSD odbc\bin; fallback to system driver with warning. Co-Authored-By: Grok Code
|
The automated review comments on this PR were generated against an earlier revision of the branch — the current HEAD (
Verified on this HEAD: unit suite 528/528 (44,666 assertions), plus the live ODBC e2e against a real Access |
Reproducible verification — so this can be merged without taking anyone's wordBuild (the ODBC driver-manager import lib ships with the Windows SDK — no external dependency): Full unit suite: Live ODBC end-to-end — creates a throwaway Access ( The three flagged items each map to a guard already present at HEAD (
So the items are verifiable two ways: read the guard, or run the suite. Happy to add any extra fixture you'd like to see covered. |
|
Heads-up: this inline-dispatch version conflicts pairwise with the other backend PRs (each adds its own block to the same ~17 ABI functions, so combining N backends produces ~17xN conflicting hunks in ace_exports.cpp). #31 supersedes that approach: it lifts this driver onto a small pluggable backend-ops registry so each backend becomes one ops struct + one registration line, the 17 ABI functions stay backend-agnostic, and the native / tcp:// path is the unchanged fall-through. #31 bundles SQLite + PostgreSQL + MariaDB + ODBC together, all behavior-preserving and e2e-verified. Suggested path: merge #31 as the combined line, or land the registry first and rebase this PR onto it. (ODBC e2e there: 59/59 vs an Access fixture.) |
Add a connection-string-driven harness (run_odbc_tests_live.ps1) that runs the existing ODBC ABI cases against any live SQL data source, plus a note on verified targets. The unmodified backend passes the same 4 cases / 59 assertions against SQL Server 2022 (ODBC Driver 18) that it passes against the Access fixture -- no dialect-specific code: SQLPrimaryKeys is honoured, the identifier quote char is discovered via SQLGetInfo, and numeric literals are emitted type-aware. When the connection string is unset the live cases skip, so the suite stays green without a configured data source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ODBC backend was read-only; a write on an ODBC-backed table fell through to the native-engine path and failed as an unknown handle. Add navigational write so a Harbour rddads app (APPEND BLANK + field assigns + commit, or DELETE) works unmodified against any ODBC data source. - OdbcTable gains a staged field list + an `appending` flag. - OdbcConnection: append_blank / set_field / flush_table / delete_record, mirroring the remote (tcp://) backend's write surface. flush emits one INSERT (appending) or UPDATE (positioned edit) built from the staged values via the existing type-aware literal formatter, then reloads the PK snapshot and repositions the cursor on the written row; delete issues a DELETE by primary key. - ABI: AdsAppendRecord / AdsWriteRecord / AdsDeleteRecord / AdsSetString / AdsSetDouble / AdsSetLogical route ODBC handles to the new methods, alongside the existing remote and native branches. - read_all_rows returns empty when the statement has no result set, so the same execute helper serves DML. v1 expects the caller to supply the primary key on append (no IDENTITY round-trip yet) and emits SQL literals (parameter binding is a later hardening slice). New live test abi_plus_odbc_write_test.cpp appends, updates and deletes a row; it is self-restoring (ends back at 3 rows) so it composes with the read/seek cases. Green on both the Access CI fixture and live SQL Server 2022 (ODBC Driver 18): 5 ODBC cases / 83 assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Admnwk
changed the title
It hardcoded local toolchain paths and a fixture password. The generic connection-string-driven runner (run_odbc_tests_live.ps1) already covers Firebird: pass -ConnStr 'Driver={Firebird ODBC Driver};Database=...;Uid=SYSDBA;Pwd=...'.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Adds an optional ODBC table driver behind the ACE ABI, so a Harbour
rddadsapplication can read and write any data source that has an ODBCdriver — SQL Server, Oracle, Firebird, PostgreSQL, MariaDB, DB2, Access, … —
without recompiling. Reads navigate (
USE/SKIP/SEEK/FieldGet);writes go through
APPEND BLANK+ field assigns + commit, andDELETE.It mirrors the existing optional SQL backends (SQLite #18, PostgreSQL #22,
MariaDB #23): a thin
OdbcConnection/OdbcTablepair undersrc/sql_backend/,HandleKind::Odbc*, and additive dispatch at the ACE border (get_odbc_table()alongside
get_remote_table()). Builds on #18.Design
-DOPENADS_WITH_ODBC=ONlinks the system ODBC drivermanager —
odbc32on Windows,unixODBC(odbc) on Linux. No externaldependency: the headers and import library ship with the platform SDK.
ever issues
SELECT/WHERE/ORDER BY/COUNT— noLIMIT/OFFSET/TOPand no scrollable-cursor dependency — so it behaves the same on everydriver.
SQLPrimaryKeys, falling back to the firstUNIQUEindexreported by
SQLStatisticsfor drivers that do not implementSQLPrimaryKeys.SQLColumns; numeric literals areemitted unquoted (type-aware) so integer/double comparisons work on strict
engines. String literals use standard quote doubling, the identifier quote
character is discovered via
SQLGetInfo, and all identifiers are validated —the generated SQL is injection-safe.
AdsAppendRecordstages a new row;AdsSetString/AdsSetDouble/AdsSetLogicalstage field values;AdsWriteRecordflushesone
INSERT(append) orUPDATE(positioned edit) and repositions the cursoron the written row;
AdsDeleteRecordissues aDELETEby primary key. v1expects the caller to supply the primary key on append (no IDENTITY round-trip
yet) and emits SQL literals (parameter binding is a later hardening slice).
Tests
odbc_uri_test— URI parsing (always built when the backend is on).abi_plus_odbc_read_test/abi_plus_odbc_seek_test/abi_plus_odbc_write_test— drive the ABI (AdsConnect60→AdsOpenTable→navigate /
AdsSeek/ append-update-delete) against an ODBC connection stringin
OPENADS_TEST_ODBC_CONNSTR; they self-skip when it is unset. The writecase is self-restoring (appends a row, then deletes it), so it composes with
the read/seek cases.
tools/scripts/run_odbc_tests.ps1seeds a throwaway Microsoft Access fixtureand runs them (zero-server, available out of the box on Windows).
tools/scripts/run_odbc_tests_live.ps1 -ConnStr '…'runs the same casesagainst any live server.
tools/scripts/build_msvc_x64_odbc.batconfigures anMSVC build.
Verified live: the unmodified backend passes the same 5 ODBC cases /
83 assertions against both the Access CI fixture and SQL Server 2022 (ODBC
Driver 18) — same binary, no dialect-specific code. See
docs/openads-plus/ODBC_LIVE_TARGETS.mdfor how to reproduce.The full unit suite stays green with the backend enabled.
🤖 Generated with Claude Code