Fix #485: SqlFixedString round-trip truncates values that fill capacity#486
Open
Fix #485: SqlFixedString round-trip truncates values that fill capacity#486
Conversation
added 2 commits
May 5, 2026 20:30
… 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
commented
May 5, 2026
Comment on lines
+289
to
306
| // 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); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
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.
Member
There was a problem hiding this comment.
this is the pattern we use in the code to make variables const
Yaraslaut
reviewed
May 5, 2026
Member
Yaraslaut
left a comment
Yaraslaut
left a comment
There was a problem hiding this comment.
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
Comment on lines
+1457
to
+1458
| // ISO 4217 codes in a CHAR(3) column. Surfaced downstream as LASTRADA DEV-6285 | ||
| // (empty cbc:DocumentCurrencyCode in XRechnung exports for non-Euro invoices). |
Member
There was a problem hiding this comment.
internal ticket mentioned in public repo
Yaraslaut
approved these changes
May 5, 2026
Comment on lines
+289
to
306
| // 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); | ||
| } |
Member
There was a problem hiding this comment.
this is the pattern we use in the code to make variables const
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #485.
Problem
BasicStringBinder<...>::OutputColumnand::GetColumnwere passingBufferLength = Capacity(i.e.,N) toSQLBindCol/SQLGetDatawhen binding anSqlFixedString<N>-derived field. ODBC'sSQL_C_CHARsemantics requireBufferLengthto include space for the null terminator, so the driver truncated any value of lengthNtoN − 1data 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 aWAEHRUNGEN.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 emptycbc:DocumentCurrencyCodein 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 capacitynext to the existingUnicodeTrimmedFixedRowtest inDataBinderTests.cpp. It usesSqlTrimmedFixedString<3>and the value"EUR"to hitindicator == Nwith a non-whitespace last byte — the precise condition the existingSqlFixedString: TrimRighttest does not cover (its calls passindicator < Nand so always have slack).Fix (commit 2,
3a29c5b2)Two binder paths and one trim path:
BasicStringBinder.hpp::OutputColumn— for fixed-capacity types, passCapacity + 1toSQLBindColinstead ofSize(result). Dynamic strings keep their existing behaviour.BasicStringBinder.hpp::GetColumn(theif constexpr (requires { Capacity; })branch) — passCapacity + 1toSQLGetData.SqlFixedString.hpp::TrimRight— change the cap onnfromN − 1toN. The previousN − 1was consistent with the truncated buffer (since ODBC could only have writtenN − 1data chars anyway). WithBufferLengthcorrected, capping atN − 1would leave_sizepermanently one short of the actual content.Existing tests are unaffected because they all operate on values shorter than
Nwith trailing whitespace, where bothN − 1andNgive 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.