diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 5c0ebe97..c9ca6cc1 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -872,6 +872,13 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par // Special handling for Numeric type - // https://learn.microsoft.com/en-us/sql/odbc/reference/appendixes/retrieve-numeric-data-sql-numeric-struct-kb222831?view=sql-server-ver16#sql_c_numeric-overview if (paramInfo.paramCType == SQL_C_NUMERIC) { + // The APD record number is the 1-based parameter position, matching the + // SQLBindParameter call above. It was previously hardcoded to 1, so a + // SQL_C_NUMERIC parameter in any position other than the first had its + // precision/scale/data pointer written onto record 1 instead of its own. + // The driver then read the numeric struct with the wrong descriptor and + // raised "Numeric value out of range" (GH-740). + const SQLSMALLINT descRecNum = static_cast(paramIndex + 1); SQLHDESC hDesc = nullptr; rc = SQLGetStmtAttr_ptr(hStmt, SQL_ATTR_APP_PARAM_DESC, &hDesc, 0, NULL); if (!SQL_SUCCEEDED(rc)) { @@ -880,7 +887,8 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par paramIndex, rc); return rc; } - rc = SQLSetDescField_ptr(hDesc, 1, SQL_DESC_TYPE, (SQLPOINTER)SQL_C_NUMERIC, 0); + rc = SQLSetDescField_ptr(hDesc, descRecNum, SQL_DESC_TYPE, + (SQLPOINTER)SQL_C_NUMERIC, 0); if (!SQL_SUCCEEDED(rc)) { LOG("BindParameters: SQLSetDescField(SQL_DESC_TYPE) failed for " "param[%d] - SQLRETURN=%d", @@ -889,7 +897,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par } SQL_NUMERIC_STRUCT* numericPtr = reinterpret_cast(dataPtr); rc = SQLSetDescField_ptr( - hDesc, 1, SQL_DESC_PRECISION, + hDesc, descRecNum, SQL_DESC_PRECISION, reinterpret_cast(static_cast(numericPtr->precision)), 0); if (!SQL_SUCCEEDED(rc)) { LOG("BindParameters: SQLSetDescField(SQL_DESC_PRECISION) " @@ -899,7 +907,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par } rc = SQLSetDescField_ptr( - hDesc, 1, SQL_DESC_SCALE, + hDesc, descRecNum, SQL_DESC_SCALE, reinterpret_cast(static_cast(numericPtr->scale)), 0); if (!SQL_SUCCEEDED(rc)) { LOG("BindParameters: SQLSetDescField(SQL_DESC_SCALE) failed " @@ -908,7 +916,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par return rc; } - rc = SQLSetDescField_ptr(hDesc, 1, SQL_DESC_DATA_PTR, + rc = SQLSetDescField_ptr(hDesc, descRecNum, SQL_DESC_DATA_PTR, reinterpret_cast(numericPtr), 0); if (!SQL_SUCCEEDED(rc)) { LOG("BindParameters: SQLSetDescField(SQL_DESC_DATA_PTR) failed " diff --git a/mssql_python/pybind/param_detect.hpp b/mssql_python/pybind/param_detect.hpp index 6c5d4338..62d06de3 100644 --- a/mssql_python/pybind/param_detect.hpp +++ b/mssql_python/pybind/param_detect.hpp @@ -194,8 +194,9 @@ inline bool StartsWithAscii(unsigned int kind, const void* data, Py_ssize_t leng // storage engine's range exactly (TINYINT: 0-255, SMALLINT: -32768..32767, etc.) // 4. String handling inspects UCS kind directly for O(1) ASCII detection rather than // scanning content — critical for bulk insert scenarios with thousands of params. -// 5. MONEY/SMALLMONEY uses exact Decimal comparison (PyObject_RichCompareBool) to avoid -// double-precision boundary errors (e.g., 214748.3647 would round incorrectly as double). +// 5. Every finite Decimal binds as SQL_NUMERIC with its own precision/scale; the value's +// magnitude does not change the bind type, so a comparison against a smaller numeric +// column returns no match instead of a server-side varchar->numeric overflow (GH-740). // --------------------------------------------------------------------------- // // ORDERING MATTERS: @@ -511,45 +512,11 @@ inline std::vector DetectParamTypes(PyObject* params) { std::to_string(precision) + "."); } - // Check SMALLMONEY first, then widen to MONEY, so common small values keep the narrowest - // exact range while still accepting larger fixed-point values supported by SQL Server. - // MONEY/SMALLMONEY: SQL Server stores these as fixed-point integers internally. - // We bind as formatted VARCHAR (e.g., "214748.3647") because SQL_C_NUMERIC can't - // represent the exact money range without precision loss on certain ODBC drivers. - // Use exact Decimal comparison (not double) to avoid boundary misclassification. - bool in_money_range = false; - int cmp_ge = PyObject_RichCompareBool(obj, PyTypeCache::smallmoney_min, Py_GE); - int cmp_le = PyObject_RichCompareBool(obj, PyTypeCache::smallmoney_max, Py_LE); - if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); - if (cmp_ge == 1 && cmp_le == 1) { - in_money_range = true; - } else { - cmp_ge = PyObject_RichCompareBool(obj, PyTypeCache::money_min, Py_GE); - cmp_le = PyObject_RichCompareBool(obj, PyTypeCache::money_max, Py_LE); - if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); - if (cmp_ge == 1 && cmp_le == 1) { - in_money_range = true; - } - } - - if (in_money_range) { - py::object formatted = steal(PyObject_CallMethod(obj, "__format__", "s", "f")); - if (!formatted) throw py::error_already_set(); - info.paramSQLType = SQL_VARCHAR; - info.paramCType = PARAM_C_TYPE_TEXT; - info.columnSize = PyUnicode_GET_LENGTH(formatted.ptr()); - info.decimalDigits = 0; - PyObject* raw = formatted.release().ptr(); - if (PyList_SetItem(params, i, raw) != 0) { - // PyList_SetItem steals (decrefs) the item even on failure, - // so raw is already freed — do NOT Py_DECREF here. - throw py::error_already_set(); - } - continue; - } - - // Build SQL_NUMERIC_STRUCT from the Decimal object. Store as a pybind11-castable - // object in the param list so BindParameters can extract it as NumericData. + // Bind every finite Decimal as SQL_NUMERIC using its own precision and scale, + // regardless of value. The previous MONEY/SMALLMONEY VARCHAR shortcut chose the + // bind type from the value alone, ignoring the target column, so an in-range value + // compared against a smaller numeric column triggered a server-side varchar->numeric + // overflow instead of simply not matching (GH-740). info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; NumericData nd = build_numeric_data(as_tuple_ptr.ptr(), digits_obj.ptr(), exponent); diff --git a/mssql_python/pybind/py_type_cache.hpp b/mssql_python/pybind/py_type_cache.hpp index 19a9c1f0..df1bb5d6 100644 --- a/mssql_python/pybind/py_type_cache.hpp +++ b/mssql_python/pybind/py_type_cache.hpp @@ -1,4 +1,4 @@ -// py_type_cache.hpp — One-time cache of Python type objects and MONEY boundary constants. +// py_type_cache.hpp — One-time cache of Python type objects. // // Called on first execute(). Uses raw CPython API (not pybind11) because // these cached PyObject* are compared via PyObject_IsInstance in the @@ -24,10 +24,6 @@ inline PyObject* date_class = nullptr; inline PyObject* time_class = nullptr; inline PyObject* decimal_class = nullptr; inline PyObject* uuid_class = nullptr; -inline PyObject* money_min = nullptr; -inline PyObject* money_max = nullptr; -inline PyObject* smallmoney_min = nullptr; -inline PyObject* smallmoney_max = nullptr; inline bool cache_initialized = false; // Import a module and extract an attribute. Returns a new reference. @@ -69,24 +65,12 @@ inline void initialize() { py::object dec_cls = steal(import_attr("decimal", "Decimal")); py::object uuid_cls = steal(import_attr("uuid", "UUID")); - // Pre-compute MONEY/SMALLMONEY boundary Decimals for exact comparison - // in DetectParamTypes (avoids double-precision boundary errors). - py::object sm_min = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "-214748.3648")); - py::object sm_max = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "214748.3647")); - py::object m_min = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "-922337203685477.5808")); - py::object m_max = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "922337203685477.5807")); - if (!sm_min || !sm_max || !m_min || !m_max) throw py::error_already_set(); - // Commit to globals — all acquisitions succeeded. datetime_class = dt_cls.release().ptr(); date_class = date_cls.release().ptr(); time_class = time_cls.release().ptr(); decimal_class = dec_cls.release().ptr(); uuid_class = uuid_cls.release().ptr(); - smallmoney_min = sm_min.release().ptr(); - smallmoney_max = sm_max.release().ptr(); - money_min = m_min.release().ptr(); - money_max = m_max.release().ptr(); cache_initialized = true; } diff --git a/tests/test_020_money_smallmoney.py b/tests/test_020_money_smallmoney.py index 912944c4..3846f8ee 100644 --- a/tests/test_020_money_smallmoney.py +++ b/tests/test_020_money_smallmoney.py @@ -4,9 +4,12 @@ Validates that Python Decimal values are correctly bound and round-tripped through MONEY, SMALLMONEY, and DECIMAL columns with proper precision handling. -Key implementation detail: MONEY-range Decimals use string binding (SQL_VARCHAR) -because SQL_NUMERIC binding fails with ODBC "Numeric value out of range" error. -String binding preserves full precision and SQL Server handles conversion. +Key implementation detail: on the execute() path every finite Decimal binds as +SQL_NUMERIC using its own precision and scale, regardless of value. Binding no longer +depends on whether the value falls in the MONEY/SMALLMONEY range, so an in-range value +compared against a smaller numeric column returns no match instead of a varchar->numeric +overflow (GH-740). executemany still string-binds Decimals (SQL_VARCHAR) to preserve +scale-38 precision (GH-503), so that path is unchanged here. """ import pytest @@ -640,3 +643,125 @@ def test_both_null(cursor, db_connection): finally: drop_table_if_exists(cursor, table_name) db_connection.commit() + + +# ============================================================================= +# GH-740: in-range Decimal must bind as SQL_NUMERIC, not VARCHAR +# ============================================================================= + + +def test_gh740_in_range_decimal_numeric_comparison_no_overflow(cursor, db_connection): + """A money-range Decimal compared against a smaller numeric column must not raise. + + Before the fix the value was bound as VARCHAR, so SQL Server did a + varchar->numeric conversion that overflowed instead of simply not matching. + """ + table_name = "#pytest_gh740_cmp" + try: + drop_table_if_exists(cursor, table_name) + cursor.execute(f"CREATE TABLE {table_name} (v numeric(5,2))") # max 999.99 + cursor.execute(f"INSERT INTO {table_name} VALUES (?)", [Decimal("12.34")]) + db_connection.commit() + + # Both probes sit inside the MONEY range but exceed numeric(5,2); they must + # return no rows rather than overflow. + cursor.execute(f"SELECT COUNT(*) FROM {table_name} WHERE v = ?", [Decimal("12345.6789")]) + assert cursor.fetchone()[0] == 0 + cursor.execute(f"SELECT COUNT(*) FROM {table_name} WHERE v = ?", [Decimal("300000.00")]) + assert cursor.fetchone()[0] == 0 + # The matching value still matches. + cursor.execute(f"SELECT COUNT(*) FROM {table_name} WHERE v = ?", [Decimal("12.34")]) + assert cursor.fetchone()[0] == 1 + finally: + drop_table_if_exists(cursor, table_name) + db_connection.commit() + + +def test_gh740_numeric_param_not_in_first_position(cursor, db_connection): + """A numeric param at position 2+ must not corrupt the parameter bound before it. + + Guards the descriptor-record fix: the APD record number was hardcoded to 1, so a + numeric param at any later position wrote its type/precision/scale/data-ptr onto + record 1, clobbering the FIRST parameter's binding as collateral. Putting a non-null + value first pins that collateral corruption - the first value must round-trip intact, + not just the numeric's own value. A NULL first would mask it (record 1 held no data). + """ + table_name = "#pytest_gh740_pos" + try: + drop_table_if_exists(cursor, table_name) + cursor.execute(f"CREATE TABLE {table_name} (a int, b varchar(10), c numeric(6,4))") + cursor.execute( + f"INSERT INTO {table_name} VALUES (?, ?, ?)", + [12345, "keep", Decimal("67.8900")], + ) + db_connection.commit() + + cursor.execute(f"SELECT a, b, c FROM {table_name}") + row = cursor.fetchone() + assert row[0] == 12345 # first param intact despite the later numeric + assert row[1] == "keep" + assert row[2] == Decimal("67.8900") + finally: + drop_table_if_exists(cursor, table_name) + db_connection.commit() + + +def test_gh740_multiple_numerics_with_null_between(cursor, db_connection): + """Multiple SQL_NUMERIC params with differing scales and a NULL between them.""" + table_name = "#pytest_gh740_multi" + try: + drop_table_if_exists(cursor, table_name) + cursor.execute(f"CREATE TABLE {table_name} (a numeric(10,4), b int, c numeric(8,2))") + cursor.execute( + f"INSERT INTO {table_name} VALUES (?, ?, ?)", + [Decimal("1.2300"), None, Decimal("999999.99")], + ) + db_connection.commit() + + cursor.execute(f"SELECT a, b, c FROM {table_name}") + row = cursor.fetchone() + assert row[0] == Decimal("1.2300") + assert row[1] is None + assert row[2] == Decimal("999999.99") + finally: + drop_table_if_exists(cursor, table_name) + db_connection.commit() + + +def test_gh740_money_boundary_still_round_trips(cursor, db_connection): + """MONEY/SMALLMONEY boundary values still insert exactly after the binding change.""" + for coltype, value in [ + ("MONEY", Decimal("922337203685477.5807")), + ("MONEY", Decimal("-922337203685477.5808")), + ("SMALLMONEY", Decimal("214748.3647")), + ("SMALLMONEY", Decimal("-214748.3648")), + ]: + table_name = "#pytest_gh740_bound" + try: + drop_table_if_exists(cursor, table_name) + cursor.execute(f"CREATE TABLE {table_name} (v {coltype})") + cursor.execute(f"INSERT INTO {table_name} VALUES (?)", [value]) + db_connection.commit() + cursor.execute(f"SELECT v FROM {table_name}") + assert cursor.fetchone()[0] == value + finally: + drop_table_if_exists(cursor, table_name) + db_connection.commit() + + +def test_gh740_same_statement_changing_precision(cursor, db_connection): + """Re-executing the same statement with Decimals of different precision/scale works.""" + table_name = "#pytest_gh740_reexec" + try: + drop_table_if_exists(cursor, table_name) + cursor.execute(f"CREATE TABLE {table_name} (v numeric(20,6))") + for value in [Decimal("1.5"), Decimal("123456.789012"), Decimal("0.000001")]: + cursor.execute(f"INSERT INTO {table_name} VALUES (?)", [value]) + db_connection.commit() + + cursor.execute(f"SELECT v FROM {table_name} ORDER BY v") + rows = [r[0] for r in cursor.fetchall()] + assert rows == [Decimal("0.000001"), Decimal("1.500000"), Decimal("123456.789012")] + finally: + drop_table_if_exists(cursor, table_name) + db_connection.commit() diff --git a/tests/test_023_execute_path_parity.py b/tests/test_023_execute_path_parity.py index 65f533aa..9fdafcb9 100644 --- a/tests/test_023_execute_path_parity.py +++ b/tests/test_023_execute_path_parity.py @@ -513,9 +513,16 @@ def test_time_param_binds_wide(cursor): def test_money_range_decimal_binds_wide(cursor): - """Decimals inside the MONEY range are formatted to text and bound with the text - C type, the third consumer of the platform-dependent constant.""" + """On the native path a money-range Decimal now binds as NUMERIC (GH-740), not text. + + This asserts the declared base type via sql_variant, not just a value round-trip, + so a wrong-but-convertible C type cannot pass silently. Note the native path here + intentionally diverges from ``_map_sql_type`` (which still text-binds money-range + Decimals to protect executemany's string binding); see + ``test_map_sql_type_money_range_binds_as_text``. + """ value = decimal.Decimal("214748.3647") + assert _param_basetype(cursor, value) == "numeric" cursor.execute("SELECT CAST(? AS MONEY)", [value]) assert cursor.fetchone()[0] == value @@ -730,7 +737,13 @@ def test_map_sql_type_aware_datetime(cursor): ) def test_map_sql_type_money_range_binds_as_text(cursor, value): """MONEY / SMALLMONEY range Decimals are formatted to text and the slot is - replaced with that formatted string.""" + replaced with that formatted string. + + This is the Python reference path (legacy execute via setinputsizes, and + executemany). It intentionally diverges from the native path, which binds these + as NUMERIC after GH-740; the text binding is retained here because executemany + string-binds Decimals and relies on the server to coerce mixed-scale batches. + """ params = [value] sql_type, c_type, column_size, decimal_digits, is_dae = cursor._map_sql_type(value, params, 0) assert (sql_type, c_type, decimal_digits, is_dae) == (