From 9155943d67560f3592b9d1bf370256e23a5acdd8 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Wed, 22 Apr 2026 14:48:56 -0300 Subject: [PATCH] Fix SQL_DBMS_VER to return the Firebird product version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQLGetInfo(SQL_DBMS_VER) was returning the engine / ODS-compat implementation number rather than the Firebird release version. On Firebird 5.0.3 the driver reported `"06.03.1683 WI-V Firebird 5.0"` — atoi-ing that gives 6, so every ODBC consumer (downstream tools, test harnesses, version-gated behaviour) mis-identified the server as Firebird 6. The ODBC specification defines SQL_DBMS_VER as "the version of the current DBMS product", which for Firebird is the release version (5.0.3), not the wire-protocol / ODS level. Root cause is in IscDbc/Attachment.cpp: we request BOTH `isc_info_firebird_version` (the product version, added in FB 2.x) AND `isc_info_version` (the legacy InterBase-style engine string) and parse them into two independent triples — majorFb/minorFb/versionFb vs local major/minor/version — but build `serverVersion` from the wrong triple (the engine one). Switch to the product triple with a defaults-based fallback so very old InterBase / pre-2.0 Firebird is still handled. After the fix, FB 5.0.3 now reports `"05.00.1683 WI-V Firebird 5.0"` and FB 6.0 master reports `"06.00.1910 WI-T Firebird 6.0 …"`. Why the existing tests didn't catch this ---------------------------------------- test_server_version.cpp only asserted SQL_DBMS_VER was non-empty and contained a dot — every possible output satisfied both. It also had a separate test that queried rdb$get_context('ENGINE_VERSION') but never cross-referenced the two values. Three focused regression tests are added: * DBMSVerMajorMatchesEngineVersion — parses the "MM" prefix and requires it to equal the major token of rdb$get_context. * DBMSVerMinorMatchesEngineVersion — same for "mm". * DBMSVerSuffixContainsDBMSName — sanity smoke test. All three fail cleanly against the unfixed driver with a diagnostic showing both version strings; all three pass after the fix. Downstream knock-on: with SQL_DBMS_VER fixed, tests/test_helpers.h::GetServerMajorVersion — which just does atoi(SQL_DBMS_VER) — now returns the correct product major, and the SKIP_ON_FIREBIRD6 macro it gates stops silently firing on FB 3/4/5. Local full test run unblocks ~20 previously-skipping tests on FB 5.0.3. Verified locally: Firebird 5.0.3 (service): full suite — 221 pass, 165 skip, 0 fail. Firebird 6.0.0.1910 (snapshot): full suite — 202 pass, 184 skip, 0 fail. --- IscDbc/Attachment.cpp | 27 +++++++- tests/test_server_version.cpp | 123 +++++++++++++++++++++++++++++++--- 2 files changed, 140 insertions(+), 10 deletions(-) diff --git a/IscDbc/Attachment.cpp b/IscDbc/Attachment.cpp index fe549e1c..06a73e9e 100644 --- a/IscDbc/Attachment.cpp +++ b/IscDbc/Attachment.cpp @@ -477,7 +477,32 @@ void Attachment::openDatabase(const char *dbName, Properties *properties) else beg++; } - serverVersion.Format( "%02d.%02d.%04d %.*s %s",major,minor,version, tmp ? tmp - start : 0, start, (const char*)productName ); + // Build SQL_DBMS_VER from the FIREBIRD PRODUCT version + // (majorFb/minorFb/versionFb, captured from isc_info_firebird_version) + // rather than from the engine / ODS-compat version parsed above. + // + // Background: isc_info_version returns a legacy InterBase-style + // string whose leading digits are the ENGINE implementation + // version (currently "6.3.x" on every supported Firebird — that + // number tracks the wire protocol / ODS compatibility level, not + // the Firebird release), while isc_info_firebird_version (added + // in FB 2.x) returns the actual Firebird product version string. + // The ODBC specification defines SQL_DBMS_VER as "the version of + // the current DBMS product" — so reporting the engine number + // mis-identifies every Firebird server as 6.x to every ODBC + // consumer (downstream tools, version-gated tests, etc.). + // + // Fall back to the engine numbers only when isc_info_firebird_version + // was not returned (very old InterBase / pre-2.0 Firebird), which + // we detect as the defaults set in the Attachment constructor + // (majorFb=1, minorFb=0, versionFb=0). + const bool haveFbVersion = + majorFb > 1 || minorFb > 0 || versionFb > 0; + serverVersion.Format( "%02d.%02d.%04d %.*s %s", + haveFbVersion ? majorFb : major, + haveFbVersion ? minorFb : minor, + haveFbVersion ? versionFb : version, + tmp ? tmp - start : 0, start, (const char*)productName ); } break; diff --git a/tests/test_server_version.cpp b/tests/test_server_version.cpp index a21160ba..fe9f1160 100644 --- a/tests/test_server_version.cpp +++ b/tests/test_server_version.cpp @@ -1,10 +1,31 @@ // test_server_version.cpp — Tests for server version detection and feature-flagging (Task 4.2) #include "test_helpers.h" +#include +#include // ============================================================================ // ServerVersionTest: Verify server version is correctly detected and exposed // ============================================================================ -class ServerVersionTest : public OdbcConnectedTest {}; +class ServerVersionTest : public OdbcConnectedTest { +protected: + // Query the Firebird engine version string (e.g. "5.0.3" / "6.0.0" / "3.0.12") + // straight from the server via rdb$get_context — this is the ground truth we + // compare SQL_DBMS_VER against below. + std::string GetEngineVersionFromServer() { + SQLRETURN ret = SQLExecDirect(hStmt, + (SQLCHAR*)"SELECT rdb$get_context('SYSTEM','ENGINE_VERSION') FROM rdb$database", + SQL_NTS); + EXPECT_TRUE(SQL_SUCCEEDED(ret)); + ret = SQLFetch(hStmt); + EXPECT_TRUE(SQL_SUCCEEDED(ret)); + SQLCHAR buf[64] = {}; + SQLLEN ind = 0; + ret = SQLGetData(hStmt, 1, SQL_C_CHAR, buf, sizeof(buf), &ind); + EXPECT_TRUE(SQL_SUCCEEDED(ret)); + SQLCloseCursor(hStmt); + return std::string((char*)buf); + } +}; TEST_F(ServerVersionTest, SQLGetInfoDBMSVer) { // SQLGetInfo(SQL_DBMS_VER) should return a non-empty version string @@ -32,20 +53,104 @@ TEST_F(ServerVersionTest, SQLGetInfoDBMSName) { TEST_F(ServerVersionTest, EngineVersionFromSQL) { // Query the engine version directly to cross-check - ExecDirect("SELECT rdb$get_context('SYSTEM','ENGINE_VERSION') FROM rdb$database"); - SQLCHAR version[256] = {}; - SQLLEN ind = 0; - SQLRETURN ret = SQLFetch(hStmt); - ASSERT_TRUE(SQL_SUCCEEDED(ret)); - ret = SQLGetData(hStmt, 1, SQL_C_CHAR, version, sizeof(version), &ind); - ASSERT_TRUE(SQL_SUCCEEDED(ret)); - std::string verStr((char*)version); + std::string verStr = GetEngineVersionFromServer(); // Should be like "5.0.1" or "4.0.5" EXPECT_GE(verStr.length(), 5u) << "Engine version too short: " << verStr; // First char should be a digit >= 3 EXPECT_GE(verStr[0], '3') << "Expected Firebird 3.0+: " << verStr; } +// ---------------------------------------------------------------------------- +// Regression tests for the Firebird-product-version-vs-engine-implementation- +// version bug: until #292 + follow-up, SQL_DBMS_VER was built from +// isc_info_version (which reports the ENGINE / ODS-compat implementation +// number, currently "6.3.x" on every supported Firebird) instead of from +// isc_info_firebird_version (the actual product version, e.g. "5.0.3"). On +// Firebird 5.0.3 SQLGetInfo(SQL_DBMS_VER) returned "06.03.1683 WI-V Firebird 5.0" +// — misidentifying every server as Firebird 6.x to any downstream ODBC tool +// or version-gated test (see SKIP_ON_FIREBIRD6 in test_helpers.h, which was +// silently firing on FB 3 / 4 / 5 too). +// +// These tests cross-reference SQL_DBMS_VER against rdb$get_context(ENGINE_VERSION) +// and against SQL_DBMS_NAME. The pre-fix behaviour fails all three of them. +// ---------------------------------------------------------------------------- + +TEST_F(ServerVersionTest, DBMSVerMajorMatchesEngineVersion) { + // SQL_DBMS_VER per ODBC spec is a "##.##.####" prefix followed by an + // implementation-defined suffix. The first two digits are the product + // major version — they must match the first token of the engine version + // reported by rdb$get_context. + SQLCHAR dbmsVer[256] = {}; + SQLSMALLINT len = 0; + SQLRETURN ret = SQLGetInfo(hDbc, SQL_DBMS_VER, dbmsVer, sizeof(dbmsVer), &len); + ASSERT_TRUE(SQL_SUCCEEDED(ret)); + std::string dbmsVerStr((char*)dbmsVer, len); + + std::string engineVer = GetEngineVersionFromServer(); + ASSERT_FALSE(engineVer.empty()); + + // Parse "MM" prefix of SQL_DBMS_VER (ODBC mandates the format; leading + // whitespace is not allowed). + ASSERT_GE(dbmsVerStr.size(), 2u); + const int dbmsMajor = std::atoi(dbmsVerStr.substr(0, 2).c_str()); + + // Parse first token of engine version (split at '.'). + const int engineMajor = std::atoi(engineVer.c_str()); + + EXPECT_EQ(dbmsMajor, engineMajor) + << "SQL_DBMS_VER major must match rdb$get_context('ENGINE_VERSION') major.\n" + << " SQL_DBMS_VER: '" << dbmsVerStr << "'\n" + << " ENGINE_VERSION: '" << engineVer << "'\n" + << " Parsed DBMS_VER major: " << dbmsMajor << "\n" + << " Parsed engine major: " << engineMajor; +} + +TEST_F(ServerVersionTest, DBMSVerMinorMatchesEngineVersion) { + SQLCHAR dbmsVer[256] = {}; + SQLSMALLINT len = 0; + SQLRETURN ret = SQLGetInfo(hDbc, SQL_DBMS_VER, dbmsVer, sizeof(dbmsVer), &len); + ASSERT_TRUE(SQL_SUCCEEDED(ret)); + std::string dbmsVerStr((char*)dbmsVer, len); + + std::string engineVer = GetEngineVersionFromServer(); + ASSERT_FALSE(engineVer.empty()); + + // Parse "MM.mm" minor (bytes 3-4 of SQL_DBMS_VER). + ASSERT_GE(dbmsVerStr.size(), 5u); + const int dbmsMinor = std::atoi(dbmsVerStr.substr(3, 2).c_str()); + + // Parse second token of engine version. + auto firstDot = engineVer.find('.'); + ASSERT_NE(firstDot, std::string::npos); + const int engineMinor = std::atoi(engineVer.substr(firstDot + 1).c_str()); + + EXPECT_EQ(dbmsMinor, engineMinor) + << "SQL_DBMS_VER minor must match rdb$get_context('ENGINE_VERSION') minor.\n" + << " SQL_DBMS_VER: '" << dbmsVerStr << "'\n" + << " ENGINE_VERSION: '" << engineVer << "'"; +} + +TEST_F(ServerVersionTest, DBMSVerSuffixContainsDBMSName) { + // Beyond the "##.##.####" prefix the string is implementation-defined, but + // the driver has always appended " " where the + // product name contains the DBMS_NAME. If that invariant ever slips we + // want to know about it — it's the quickest smoke test that the whole + // string wasn't clobbered. + SQLCHAR dbmsVer[256] = {}, dbmsName[256] = {}; + SQLSMALLINT verLen = 0, nameLen = 0; + ASSERT_TRUE(SQL_SUCCEEDED( + SQLGetInfo(hDbc, SQL_DBMS_VER, dbmsVer, sizeof(dbmsVer), &verLen))); + ASSERT_TRUE(SQL_SUCCEEDED( + SQLGetInfo(hDbc, SQL_DBMS_NAME, dbmsName, sizeof(dbmsName), &nameLen))); + std::string verStr((char*)dbmsVer, verLen); + std::string nameStr((char*)dbmsName, nameLen); + + EXPECT_NE(verStr.find(nameStr), std::string::npos) + << "SQL_DBMS_VER suffix should include the DBMS_NAME.\n" + << " SQL_DBMS_VER: '" << verStr << "'\n" + << " SQL_DBMS_NAME: '" << nameStr << "'"; +} + TEST_F(ServerVersionTest, SQLGetTypeInfoShowsAllBaseTypes) { GTEST_SKIP() << "Requires Phase 4: server version detection for type count"; // SQLGetTypeInfo(SQL_ALL_TYPES) should return at least the 22 base types