Skip to content

feat(mssql): navigational write (append/set/flush/delete) for the native TDS backend#123

Closed
Admnwk wants to merge 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/mssql-native-write-nav
Closed

feat(mssql): navigational write (append/set/flush/delete) for the native TDS backend#123
Admnwk wants to merge 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/mssql-native-write-nav

Conversation

@Admnwk

@Admnwk Admnwk commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

The native MS SQL Server (TDS) backend was read-only (MssqlTable declared
"write not available in v1"); AdsAppendRecord / AdsSetString /
AdsWriteRecord / AdsDeleteRecord returned AE_FUNCTION_NOT_AVAILABLE.
This adds navigational write so the native TDS backend matches the MariaDB /
PostgreSQL / Firebird / ODBC write contract.

Changes

  • MssqlTable: retain the owning connection + table name; discover the
    primary-key columns at open time (INFORMATION_SCHEMA); add a staging buffer.
  • MssqlConnection: implement append_blank / set_field /
    flush_record / delete_record. flush_record emits an INSERT (pending
    append) or a PK-keyed UPDATE; delete_record a PK-keyed DELETE, via the
    existing TDS query() path. Values are emitted as N'...' literals (SQL
    Server implicit-converts to the target column type); the result set is
    re-fetched after each write so record count and navigation stay consistent.
    Write requires a discovered primary key.
  • ace_exports.cpp: dispatch the four ABI write entry points to the MSSQL
    backend (the three former "not available in v1" stubs + a new AdsSetString
    branch).
  • New live test (abi_plus_mssql_write_test, gated on
    OPENADS_TEST_MSSQL_CONNSTR): seeds a CLIENTES table via the connection,
    then drives append/update/delete through the ACE ABI.

Coordination

This touches src/abi/ace_exports.cpp (the ABI write dispatch). The additions
are additive and mirror the existing per-backend blocks.

Testing

Against SQL Server 2025 (native TDS over TLS, OPENADS_WITH_MSSQL=ON):

  • New write test: 27/27 assertions (append + update + delete round-trip).
  • /WX (warnings-as-errors) clean, MSVC x64.

Note: the pre-existing live read test (abi_plus_mssql_read_test) asserts that
NVARCHAR values are blank-padded to their declared width; against this server
they come back unpadded. That is a read-path matter independent of this change
(it only surfaces now that a live server makes the gated read test run) and is
not touched here.

🤖 Generated with Claude Code (Opus 4.8)

…ive TDS backend

The native MS SQL Server backend was read-only ("write not available in v1");
AdsAppendRecord / AdsSetString / AdsWriteRecord / AdsDeleteRecord returned
AE_FUNCTION_NOT_AVAILABLE. This adds navigational write so the native TDS
backend matches the MariaDB / PostgreSQL / Firebird / ODBC write contract.

- MssqlTable: retain the owning connection + table name; discover the
  primary-key columns at open (INFORMATION_SCHEMA); add a staging buffer.
- MssqlConnection: implement append_blank / set_field / flush_record /
  delete_record. flush emits an INSERT (pending append) or a PK-keyed UPDATE;
  delete a PK-keyed DELETE, via the existing TDS query() path. Values are
  emitted as N'...' literals (SQL Server implicit-converts to the column
  type); the result set is re-fetched after each write so record count and
  navigation stay consistent. Write requires a discovered primary key.
- ace_exports: dispatch the four ABI write entry points to the MSSQL backend
  (the three former "not available in v1" stubs + a new AdsSetString branch).
- New live test (abi_plus_mssql_write_test, gated on OPENADS_TEST_MSSQL_CONNSTR):
  seeds a CLIENTES table via the connection, then drives append/update/delete
  through the ACE ABI.

Testing (SQL Server 2025, native TDS over TLS, OPENADS_WITH_MSSQL=ON):
  new write test 27/27 assertions pass; /WX (-Werror) clean, MSVC x64.

Note: the pre-existing live read test (abi_plus_mssql_read_test) asserts that
NVARCHAR values are blank-padded to their declared width; against this server
they come back unpadded. That is a read-path matter independent of this write
change (it only surfaces now that a live server makes the gated read test run);
it is not touched here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements navigational write operations (append, set field, flush, and delete) for the MS SQL Server (TDS) backend, mirroring the behavior of other SQL backends. It also introduces a live write test suite to verify these operations. The reviewer identified several critical issues with the implementation: quote_ident does not correctly handle schema-qualified identifiers; the refetch function unconditionally resets the cursor to BOF, which can lead to subsequent edits targeting the wrong record; the primary key discovery query is vulnerable to SQL injection and fails on schema-qualified tables; and set_field lacks early validation when there is no current record.

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.

Comment on lines +21 to +26
std::string quote_ident(const std::string& name) {
std::string out = "[";
for (char c : name) { if (c == ']') out += ']'; out += c; }
out += ']';
return out;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The quote_ident function currently wraps the entire identifier in brackets (e.g., [dbo.CLIENTES]). In SQL Server, schema-qualified identifiers must be quoted individually (e.g., [dbo].[CLIENTES]). Wrapping the entire string with a dot inside brackets will cause SQL Server to look for a table literally named dbo.CLIENTES in the default schema, which will fail. We should split the identifier by . and quote each part individually.

std::string quote_ident(const std::string& name) {
    std::string out;
    std::string part;
    for (char c : name) {
        if (c == '.') {
            if (!part.empty()) {
                out += "[";
                for (char pc : part) { if (pc == ']') out += ']'; out += pc; }
                out += "].";
                part.clear();
            }
        } else {
            part += c;
        }
    }
    if (!part.empty()) {
        out += "[";
        for (char pc : part) { if (pc == ']') out += ']'; out += pc; }
        out += "]";
    }
    return out;
}

Comment on lines +186 to +198
util::Result<void> refetch(MssqlConnection& c, MssqlTable* tbl) {
auto qr = c.query("SELECT * FROM " + quote_ident(tbl->table_name));
if (!qr) return qr.error();
if (!qr.value().ok) {
return util::Error{static_cast<std::int32_t>(qr.value().error_number),
0, qr.value().message, ""};
}
tbl->data = std::move(qr).value();
tbl->pos = 0;
tbl->bof = true;
tbl->eof = tbl->data.rows.empty();
return util::Result<void>{};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The refetch function unconditionally resets the cursor position to BOF (tbl->pos = 0). In a navigational database, after performing an UPDATE or INSERT (via flush_record), the cursor is expected to remain on the written record. Resetting to BOF will cause subsequent edits or deletes to silently target the wrong record. We should accept the target primary key values and restore the cursor position to the matching row after refetching.

util::Result<void> refetch(MssqlConnection& c, MssqlTable* tbl, const std::vector<std::string>& target_pk = {}) {
    auto qr = c.query("SELECT * FROM " + quote_ident(tbl->table_name));
    if (!qr) return qr.error();
    if (!qr.value().ok) {
        return util::Error{static_cast<std::int32_t>(qr.value().error_number),
                           0, qr.value().message, ""};
    }
    tbl->data = std::move(qr).value();
    tbl->pos  = 0;
    tbl->bof  = true;
    tbl->eof  = tbl->data.rows.empty();

    if (!target_pk.empty() && !tbl->pk_cols.empty()) {
        for (std::size_t r = 0; r < tbl->data.rows.size(); ++r) {
            const auto& row = tbl->data.rows[r];
            bool match = true;
            for (std::size_t i = 0; i < tbl->pk_cols.size(); ++i) {
                std::size_t col_idx = tbl->pk_cols[i];
                if (col_idx >= row.size() || row[col_idx].value != target_pk[i]) {
                    match = false;
                    break;
                }
            }
            if (match) {
                tbl->pos = r;
                tbl->bof = false;
                tbl->eof = false;
                break;
            }
        }
    }
    return util::Result<void>{};
}

Comment on lines +245 to +311
util::Result<void> MssqlConnection::flush_record(MssqlTable* tbl) {
if (!valid() || tbl == nullptr) {
return util::Error{5001, 0, "invalid mssql flush", ""};
}
if (!tbl->row_dirty && !tbl->pending_append) return util::Result<void>{};
const std::size_t n = tbl->data.columns.size();

if (tbl->pending_append) {
std::string cols, vals;
bool any = false;
for (std::size_t i = 0; i < n; ++i) {
if (i < tbl->staging_nulls.size() && tbl->staging_nulls[i]) continue;
if (any) { cols += ", "; vals += ", "; }
cols += quote_ident(tbl->data.columns[i].name);
vals += quote_lit(tbl->staging_row[i]);
any = true;
}
if (!any) {
return util::Error{5001, 0, "insert has no columns", tbl->table_name};
}
const std::string sqlq = "INSERT INTO " + quote_ident(tbl->table_name) +
" (" + cols + ") VALUES (" + vals + ")";
auto r = query(sqlq);
if (!r) return r.error();
if (!r.value().ok) {
return util::Error{static_cast<std::int32_t>(r.value().error_number),
0, r.value().message, sqlq};
}
tbl->pending_append = false;
tbl->row_dirty = false;
return refetch(*this, tbl);
}

// UPDATE the current row, keyed on its primary key.
if (tbl->pk_cols.empty()) {
return util::Error{5004, 0, "mssql update requires a primary key",
tbl->table_name};
}
if (tbl->pos >= tbl->data.rows.size()) {
return util::Error{5026, 0, "no current record", ""};
}
const std::string where = pk_where(*tbl, tbl->data.rows[tbl->pos]);
std::vector<bool> is_pk(n, false);
for (std::size_t i : tbl->pk_cols) if (i < n) is_pk[i] = true;
std::string setc;
bool any = false;
for (std::size_t i = 0; i < n; ++i) {
if (is_pk[i] || i >= tbl->staging_row.size()) continue;
if (any) setc += ", ";
setc += quote_ident(tbl->data.columns[i].name) + " = " +
((i < tbl->staging_nulls.size() && tbl->staging_nulls[i])
? std::string("NULL")
: quote_lit(tbl->staging_row[i]));
any = true;
}
if (!any) { tbl->row_dirty = false; return refetch(*this, tbl); }
const std::string sqlq = "UPDATE " + quote_ident(tbl->table_name) +
" SET " + setc + " WHERE " + where;
auto r = query(sqlq);
if (!r) return r.error();
if (!r.value().ok) {
return util::Error{static_cast<std::int32_t>(r.value().error_number),
0, r.value().message, sqlq};
}
tbl->row_dirty = false;
return refetch(*this, tbl);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Update flush_record to extract the primary key values of the record being inserted or updated, and pass them to refetch so that the cursor position is correctly preserved after the write operation.

util::Result<void> MssqlConnection::flush_record(MssqlTable* tbl) {
    if (!valid() || tbl == nullptr) {
        return util::Error{5001, 0, "invalid mssql flush", ""};
    }
    if (!tbl->row_dirty && !tbl->pending_append) return util::Result<void>{};
    const std::size_t n = tbl->data.columns.size();

    std::vector<std::string> target_pk;
    if (tbl->pending_append) {
        for (std::size_t idx : tbl->pk_cols) {
            if (idx < tbl->staging_row.size()) {
                target_pk.push_back(tbl->staging_row[idx]);
            } else {
                target_pk.push_back("");
            }
        }

        std::string cols, vals;
        bool any = false;
        for (std::size_t i = 0; i < n; ++i) {
            if (i < tbl->staging_nulls.size() && tbl->staging_nulls[i]) continue;
            if (any) { cols += ", "; vals += ", "; }
            cols += quote_ident(tbl->data.columns[i].name);
            vals += quote_lit(tbl->staging_row[i]);
            any = true;
        }
        if (!any) {
            return util::Error{5001, 0, "insert has no columns", tbl->table_name};
        }
        const std::string sqlq = "INSERT INTO " + quote_ident(tbl->table_name) +
                                 " (" + cols + ") VALUES (" + vals + ")";
        auto r = query(sqlq);
        if (!r) return r.error();
        if (!r.value().ok) {
            return util::Error{static_cast<std::int32_t>(r.value().error_number),
                               0, r.value().message, sqlq};
        }
        tbl->pending_append = false;
        tbl->row_dirty      = false;
        return refetch(*this, tbl, target_pk);
    }

    // UPDATE the current row, keyed on its primary key.
    if (tbl->pk_cols.empty()) {
        return util::Error{5004, 0, "mssql update requires a primary key",
                           tbl->table_name};
    }
    if (tbl->pos >= tbl->data.rows.size()) {
        return util::Error{5026, 0, "no current record", ""};
    }
    for (std::size_t idx : tbl->pk_cols) {
        if (idx < tbl->data.rows[tbl->pos].size()) {
            target_pk.push_back(tbl->data.rows[tbl->pos][idx].value);
        } else {
            target_pk.push_back("");
        }
    }
    const std::string where = pk_where(*tbl, tbl->data.rows[tbl->pos]);
    std::vector<bool> is_pk(n, false);
    for (std::size_t i : tbl->pk_cols) if (i < n) is_pk[i] = true;
    std::string setc;
    bool any = false;
    for (std::size_t i = 0; i < n; ++i) {
        if (is_pk[i] || i >= tbl->staging_row.size()) continue;
        if (any) setc += ", ";
        setc += quote_ident(tbl->data.columns[i].name) + " = " +
                ((i < tbl->staging_nulls.size() && tbl->staging_nulls[i])
                     ? std::string("NULL")
                     : quote_lit(tbl->staging_row[i]));
        any = true;
    }
    if (!any) { tbl->row_dirty = false; return refetch(*this, tbl, target_pk); }
    const std::string sqlq = "UPDATE " + quote_ident(tbl->table_name) +
                             " SET " + setc + " WHERE " + where;
    auto r = query(sqlq);
    if (!r) return r.error();
    if (!r.value().ok) {
        return util::Error{static_cast<std::int32_t>(r.value().error_number),
                           0, r.value().message, sqlq};
    }
    tbl->row_dirty = false;
    return refetch(*this, tbl, target_pk);
}

Comment on lines +176 to +184
const std::string pk_sql =
"SELECT kcu.COLUMN_NAME "
"FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc "
"JOIN INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu "
" ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME "
" AND tc.TABLE_NAME = kcu.TABLE_NAME "
"WHERE tc.CONSTRAINT_TYPE = 'PRIMARY KEY' "
" AND tc.TABLE_NAME = '" + table_name + "' "
"ORDER BY kcu.ORDINAL_POSITION";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The primary key discovery query directly concatenates table_name into the SQL string. If table_name contains single quotes, this will cause a SQL syntax error or potential SQL injection. Additionally, if table_name is schema-qualified (e.g., dbo.CLIENTES), the query will fail to find the primary key because INFORMATION_SCHEMA.TABLE_CONSTRAINTS.TABLE_NAME does not include the schema. We should escape single quotes and use SQL Server's built-in PARSENAME function to correctly handle schema-qualified and bracketed table names.

    std::string escaped_table_name;
    for (char c : table_name) {
        if (c == '\'') escaped_table_name += "''";
        else escaped_table_name += c;
    }
    const std::string pk_sql =
        "SELECT kcu.COLUMN_NAME "
        "FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc "
        "JOIN INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu "
        "  ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME "
        " AND tc.TABLE_NAME = kcu.TABLE_NAME "
        "WHERE tc.CONSTRAINT_TYPE = 'PRIMARY KEY' "
        "  AND tc.TABLE_NAME = COALESCE(PARSENAME('" + escaped_table_name + "', 1), '" + escaped_table_name + "') "
        "  AND (PARSENAME('" + escaped_table_name + "', 2) IS NULL OR tc.TABLE_SCHEMA = PARSENAME('" + escaped_table_name + "', 2)) "
        "ORDER BY kcu.ORDINAL_POSITION";

Comment on lines +213 to +221
util::Result<void> MssqlConnection::set_field(
MssqlTable* tbl, const std::string& field_name, const std::string& value) {
if (!valid() || tbl == nullptr) {
return util::Error{5001, 0, "invalid mssql set_field", ""};
}
const std::size_t idx = col_index_ci(*tbl, field_name);
if (idx == static_cast<std::size_t>(-1)) {
return util::Error{5063, 0, "column not found", field_name};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In set_field, if we are not pending an append and there is no current record (e.g., cursor is at EOF), we should fail early with AE_NO_CURRENT_RECORD (error code 5026) instead of staging values on an invalid cursor state and failing only during flush_record.

Suggested change
util::Result<void> MssqlConnection::set_field(
MssqlTable* tbl, const std::string& field_name, const std::string& value) {
if (!valid() || tbl == nullptr) {
return util::Error{5001, 0, "invalid mssql set_field", ""};
}
const std::size_t idx = col_index_ci(*tbl, field_name);
if (idx == static_cast<std::size_t>(-1)) {
return util::Error{5063, 0, "column not found", field_name};
}
util::Result<void> MssqlConnection::set_field(
MssqlTable* tbl, const std::string& field_name, const std::string& value) {
if (!valid() || tbl == nullptr) {
return util::Error{5001, 0, "invalid mssql set_field", ""};
}
if (!tbl->pending_append && tbl->pos >= tbl->data.rows.size()) {
return util::Error{5026, 0, "no current record", ""};
}
const std::size_t idx = col_index_ci(*tbl, field_name);

@Admnwk

Admnwk commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Withdrawing to re-verify against real-world data before resubmitting.

@Admnwk Admnwk closed this Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant