Fix #485: SqlFixedString round-trip truncates values that fill capacity by StepKie · Pull Request #486 · LASTRADA-Software/Lightweight

StepKie · 2026-05-05T18:32:58Z

Closes #485.

Problem

BasicStringBinder<...>::OutputColumn and ::GetColumn were passing BufferLength = Capacity (i.e., N) to SQLBindCol / SQLGetData when binding an SqlFixedString<N>-derived field. ODBC's SQL_C_CHAR semantics require BufferLength to include space for the null terminator, so the driver truncated any value of length N to N − 1 data chars + null.

SqlFixedString<N> already backs its data with _data[N + 1] so the terminator slot exists; the binder just wasn't telling ODBC about it.

The bug was masked for almost every consumer because real-world values rarely fill the full capacity with no trailing whitespace. We tripped over it in a downstream codebase (LASTRADA, ticket DEV-6285) via Light::Field<Light::SqlTrimmedFixedString<3>> mapped to a WAEHRUNGEN.NAME CHAR(3) column storing 3-letter ISO 4217 codes (EUR, CHF, GBP, USD, …). The codes came back as 2-char prefixes ("EU", "CH", "GB", "US"), silently breaking ISO 4217 lookup and producing an empty cbc:DocumentCurrencyCode in XRechnung exports for non-Euro invoices.

Approach

TDD — failing test in commit 1, fix in commit 2.

Test (commit 1, `e5ebfc01`)

Adds Trimmed fixed string preserves a value that fills its capacity next to the existing UnicodeTrimmedFixedRow test in DataBinderTests.cpp. It uses SqlTrimmedFixedString<3> and the value "EUR" to hit indicator == N with a non-whitespace last byte — the precise condition the existing SqlFixedString: TrimRight test does not cover (its calls pass indicator < N and so always have slack).

Fix (commit 2, `3a29c5b2`)

Two binder paths and one trim path:

BasicStringBinder.hpp::OutputColumn — for fixed-capacity types, pass Capacity + 1 to SQLBindCol instead of Size(result). Dynamic strings keep their existing behaviour.
BasicStringBinder.hpp::GetColumn (the if constexpr (requires { Capacity; }) branch) — pass Capacity + 1 to SQLGetData.
SqlFixedString.hpp::TrimRight — change the cap on n from N − 1 to N. The previous N − 1 was consistent with the truncated buffer (since ODBC could only have written N − 1 data chars anyway). With BufferLength corrected, capping at N − 1 would leave _size permanently one short of the actual content.

Existing tests are unaffected because they all operate on values shorter than N with trailing whitespace, where both N − 1 and N give the same trim result.

Test matrix

I haven't been able to run Lightweight's full test suite locally (no preconfigured ODBC test DB on this box). CI on this PR will exercise both the new regression test and the rest of the suite across the supported drivers. Happy to iterate if anything else trips.

… capacity Adds a regression test (currently failing) for #485: when an entity field declared as Light::SqlTrimmedFixedString<N> stores a value of length N with no trailing whitespace, the round-trip via dm.QuerySingle returns only N-1 characters. Surfaced downstream as LASTRADA DEV-6285 (empty cbc:DocumentCurrencyCode in XRechnung exports for non-Euro invoices, where 3-letter ISO 4217 codes in CHAR(3) columns came back as 2-char prefixes).

…edString<N> Closes #485. ODBC's SQL_C_CHAR semantics require BufferLength to include space for the null terminator, so passing N (the field's Capacity) caused the driver to truncate any value of length N to N-1 data chars + null. SqlFixedString<N> already allocates _data[N+1] to make room for the terminator, so the fix is to hand ODBC the full N+1 bytes it expects. The N-1 cap inside SqlBasicStringOperations::TrimRight was consistent with the truncated buffer (which never held more than N-1 data chars anyway). Once BufferLength is corrected, the cap must become N as well, otherwise _size would still be limited to N-1 even though the buffer now holds the full value. The bug was previously masked because the existing TrimRight test passes indicator < N (e.g. SqlTrimmedFixedString<20>{"Hello "}, indicator=5). The added regression test exercises indicator == N with a non-whitespace last byte.

StepKie · 2026-05-05T18:39:06Z

+        // Per ODBC spec, BufferLength for SQL_C_CHAR must include space for the
+        // null terminator. SqlFixedString<N> backs its data with _data[N+1] for
+        // exactly this reason; pass Capacity+1 to let the driver write a full
+        // N-char value plus null. Dynamic strings keep the current behaviour.
+        SQLLEN const bufferLength = [result]() -> SQLLEN {
+            if constexpr (requires { AnsiStringType::Capacity; })
+                return static_cast<SQLLEN>(AnsiStringType::Capacity) + 1;
+            else
+                return static_cast<SQLLEN>(StringTraits::Size(result));
+        }();
+
        return SQLBindCol(stmt,
                          column,
                          SQL_C_CHAR,
                          (SQLPOINTER) StringTraits::Data(result),
-                          (SQLLEN) StringTraits::Size(result),
+                          bufferLength,
                          indicator);
    }


This if -else lambda construction does not strike me as very elegant.

this is just what was generated by Claude. Feel free to replace if you find a solution less offensive to the eye that still passes the test.

this is the pattern we use in the code to make variables const

Yaraslaut

Also, this is quite surprising that we get this issue, is it something specific to the trimmed strings? since we have plenty of tests for other string types

Yaraslaut · 2026-05-05T19:05:51Z

+        // Per ODBC spec, BufferLength for SQL_C_CHAR must include space for the
+        // null terminator. SqlFixedString<N> backs its data with _data[N+1] for
+        // exactly this reason; pass Capacity+1 to let the driver write a full
+        // N-char value plus null. Dynamic strings keep the current behaviour.
+        SQLLEN const bufferLength = [result]() -> SQLLEN {
+            if constexpr (requires { AnsiStringType::Capacity; })
+                return static_cast<SQLLEN>(AnsiStringType::Capacity) + 1;
+            else
+                return static_cast<SQLLEN>(StringTraits::Size(result));
+        }();
+
        return SQLBindCol(stmt,
                          column,
                          SQL_C_CHAR,
                          (SQLPOINTER) StringTraits::Data(result),
-                          (SQLLEN) StringTraits::Size(result),
+                          bufferLength,
                          indicator);
    }


this is the pattern we use in the code to make variables const

StepKie · 2026-05-06T07:20:42Z

Also, this is quite surprising that we get this issue, is it something specific to the trimmed strings? since we have plenty of tests for other string types

@Yaraslaut Not specific to trimmed mode — BufferLength = N is passed in OutputColumn regardless of SqlFixedStringMode. The reason it slipped through is that no test in the suite stores a value that fills the full capacity with a non-whitespace last byte:

SqlFixedString: TrimRight calls TrimRight(&str, 5) on SqlTrimmedFixedString<20> / <32> — indicator = 5 vs N = 20, so 15 bytes of slack and the cap at N − 1 never bites.
Trimmed fixed strings strip trailing padding through DataMapper stores "Hello" (5 chars) in a <32> field — same: plenty of slack.

Nothing in the suite probes indicator == N with a non-whitespace last byte. Trimmed mode is just where it's easiest to spot (round-trip "EUR" → get "EU") — for variable-size and non-trimmed fixed mode the same off-by-one chops the last byte too, it just looks less suspicious without an exact-length probe.

🤖 ai-assisted

StepKie · 2026-05-06T07:37:23Z

Apologies @Yaraslaut — when I amended just now to drop the internal ticket reference from the test comment, I force-pushed from a local branch that pre-dated your [clang-tidy] small fixes commit (a05eb9ab), so I silently overwrote your [result] → [=] change. --force-with-lease didn't catch it because my tracking branch was stale (I hadn't fetched).

The latest commit (08147c2e) re-applies an equivalent fix: [result] → [&] (capture-all-by-reference rather than by-value, otherwise same effect — silences -Wunused-lambda-capture under -Werror regardless of which if constexpr branch is selected, and matches the IIFE style used elsewhere in the codebase).

Sorry for the noise. CI should go back green.

🤖 ai-assisted

Stephan Kieburg added 2 commits May 5, 2026 20:30

StepKie requested a review from a team as a code owner May 5, 2026 18:32

github-actions Bot added tests Data Binder SQL Data Binder support labels May 5, 2026

StepKie commented May 5, 2026

View reviewed changes

Yaraslaut reviewed May 5, 2026

View reviewed changes

Comment thread src/tests/DataBinderTests.cpp Outdated

github-actions Bot added the Core API label May 5, 2026

Yaraslaut approved these changes May 5, 2026

View reviewed changes

StepKie force-pushed the regression/issue-485-sqlfixedstring-fullcapacity branch from a05eb9a to 2eb5a00 Compare May 6, 2026 07:20

github-actions Bot removed the Core API label May 6, 2026

style: clang-format SQLBindCol call

08147c2

StepKie force-pushed the regression/issue-485-sqlfixedstring-fullcapacity branch from 2eb5a00 to 08147c2 Compare May 6, 2026 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #485: SqlFixedString round-trip truncates values that fill capacity#486

Fix #485: SqlFixedString round-trip truncates values that fill capacity#486
StepKie wants to merge 3 commits intomasterfrom
regression/issue-485-sqlfixedstring-fullcapacity

StepKie commented May 5, 2026

Uh oh!

StepKie May 5, 2026

Uh oh!

Yaraslaut May 5, 2026

Uh oh!

Yaraslaut left a comment

Uh oh!

Uh oh!

Yaraslaut May 5, 2026

Uh oh!

StepKie commented May 6, 2026

Uh oh!

StepKie commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

StepKie commented May 5, 2026

Problem

Approach

Test (commit 1, e5ebfc01)

Fix (commit 2, 3a29c5b2)

Test matrix

Uh oh!

StepKie May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Yaraslaut May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Yaraslaut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yaraslaut May 5, 2026

Choose a reason for hiding this comment

Uh oh!

StepKie commented May 6, 2026

Uh oh!

StepKie commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Test (commit 1, `e5ebfc01`)

Fix (commit 2, `3a29c5b2`)