Merge pull request #46 from microsoft/dev/saurabh/sync-2026-05-18

Sync ADO main to GitHub main

Author: saurabh500

.pipeline/OneBranch/NonOfficialPythonWheelsPublish.yml

@@ -8,8 +8,10 @@
 # Support:        https://aka.ms/onebranchsup                                   #
 #################################################################################
 
+# CI trigger only on development. Main-branch coverage comes from the
+# Official pipeline (on merge) and the nightly schedule below; no need to
+# also fire NonOfficial on every merge to main.
 trigger:
-- main
 - development
 
 pr:

.pipeline/OneBranch/OfficialPythonWheelsBuild.yml

@@ -10,8 +10,13 @@
 # Support:        https://aka.ms/onebranchsup                                   #
 #################################################################################
 
-# No CI or PR triggers — official builds are manual only
-trigger: none
+# Official build runs on every merge to main; no PR validation (PRs are
+# validated by the separate validation pipeline). Can also be queued manually.
+trigger:
+  batch: true
+  branches:
+    include:
+      - main
 pr: none
 
 variables:

.pipeline/validation-pipeline.yml

@@ -1,5 +1,6 @@
-trigger: 
-- main
+# CI trigger only on development. Main-branch builds are produced by the
+# Official pipeline (OneBranch/OfficialPythonWheelsBuild.yml) on merge.
+trigger:
 - development
 
 parameters:

docs/github-migration-plan.md

@@ -0,0 +1,127 @@
+# GitHub issue [microsoft/mssql-rs#38](https://github.com/microsoft/mssql-rs/issues/38): sp_prepare fails when statement uses a non-int named parameter
+
+## Summary
+
+`TdsClient::execute_sp_prepare` was forwarding the user's named parameter
+values as RPC parameters on the `sp_prepare` call. `sp_prepare`'s signature
+is fixed (`@handle int OUTPUT, @params ntext, @stmt ntext, @options int`)
+and accepts no caller-supplied values; those belong on `sp_execute`,
+`sp_prepexec`, or `sp_executesql`. When the extra named arguments did not
+coincidentally fit the types `sp_prepare` expects, the server returned
+
+  Msg 214: Procedure expects parameter '@options' of type 'int'.
+
+`drain_stream()` collected the ERROR token but the call site dropped the
+returned `Vec<SqlErrorInfo>`. The post-drain shape check then saw
+`ColumnValues::Null` for the `@handle` output and raised the generic
+`ProtocolError("Expected an integer value")` reported in the issue.
+
+## Root cause
+
+In `mssql-tds/src/connection/tds_client.rs`, `execute_sp_prepare` built the
+RPC as:
+
+```rust
+let rpc = SqlRpc::new(
+    RpcType::ProcId(RpcProcs::Prepare),
+    positional_parameters,
+    Some(named_params),    // <-- bug: user values smuggled onto sp_prepare
+    &database_collation,
+    &self.execution_context,
+);
+```
+
+`SqlRpc::write_named_parameters` (`mssql-tds/src/message/rpc.rs`) iterates
+the named slice and serializes every entry on the wire. Because the wire
+contract for `sp_prepare` does not include user values, the server tried to
+bind those as `@options` and rejected anything that wasn't an int.
+
+The same call site also discarded `drain_stream`'s returned errors:
+
+```rust
+self.drain_stream().await?;     // returned Vec<SqlErrorInfo> dropped
+// ... shape check then raised a generic ProtocolError
+```
+
+This collapsed the real diagnostic into an unrelated message and is the
+reason the issue was originally misread as a parameter-declaration bug.
+
+## Fix
+
+`mssql-tds/src/connection/tds_client.rs::execute_sp_prepare`:
+
+- Pass `None` (not `Some(named_params)`) as the RPC named-parameter list.
+  `named_params` is still consumed locally to build the `@params` text
+  string serialized as the second positional argument; only the wire-level
+  RPC named arguments are dropped.
+- Capture the `Vec<SqlErrorInfo>` returned by `drain_stream()` and, when
+  non-empty, return `Error::SqlServerError { errors }` so callers see the
+  actual server diagnostic.
+
+The other RPC paths (`execute_sp_executesql`, `execute_sp_prepexec`) are
+correct as-is: those stored procedures' contracts include the user's
+parameter values after the fixed positional arguments, and the server
+expects them on the wire.
+
+## Tests
+
+Added in `mssql-tds/tests/test_rpc_results.rs` (the file already contains
+the other `sp_prepare` / `sp_prepexec` integration tests):
+
+- `test_sp_prepare_with_named_nvarchar_param_succeeds`: regression test
+  for [microsoft/mssql-rs#38](https://github.com/microsoft/mssql-rs/issues/38);
+  prepares a statement that references
+  `@db_name nvarchar(6)` and asserts a positive handle is returned, then
+  unprepares it.
+- `test_sp_prepare_surfaces_server_error_on_invalid_sql`: verifies the
+  diagnostics fix; preparing an undeclared-variable statement returns
+  `Error::SqlServerError` carrying the populated server message instead
+  of a generic `ProtocolError`.
+
+Both are integration tests; like the rest of the file they require the
+DB environment variables (`DB_HOST`, `DB_USERNAME`, `SQL_PASSWORD`).
+
+## Why the existing tests didn't catch this
+
+- The integration tests in `mssql-tds/tests/test_rpc_results.rs` for
+  `sp_prepare` (`test_sp_prepare_and_unprepare_multi_param`) only use
+  `Int` user parameters. SQL Server tolerates extra named values on
+  `sp_prepare` when their types happen to match what the optional
+  `@options` parameter accepts (int), so all-int tests pass against a
+  live server even though the code is wrong.
+- No existing test exercises `sp_prepare` with `NVarchar`, `NChar`,
+  `Decimal`, `DateTime2`, `Uuid`, `Vector`, or any other non-int user
+  parameter type.
+- No negative test asserts what happens when `sp_prepare` fails. With
+  the drained errors discarded, even a CI run that happened to surface
+  the bug would have reported the misleading
+  `ProtocolError("Expected an integer value")`.
+- No mock-server test exercises the wire-level RPC structure.
+  `mssql-mock-tds` exists but is not used to assert that `sp_prepare` is
+  sent without user named parameters or that ERROR tokens propagate as
+  `SqlServerError`.
+- The integration tests panic when `DB_HOST`/`DB_USERNAME`/`SQL_PASSWORD`
+  are not set (`mssql-tds/tests/common/mod.rs`). They cannot run as
+  unit-only checks, so the suite often runs in environments where the
+  prepare paths simply do not execute.
+
+## Test-coverage follow-ups
+
+These are scoped to the `sp_prepare` regression and would have caught
+this specific class of bug. They are not part of this fix.
+
+- Add per-type integration tests for `sp_prepare`, `sp_prepexec`, and
+  `sp_executesql` that cover at least: `Char`, `NChar`, `Varchar`,
+  `NVarchar`, `VarcharMax`, `NVarcharMax`, `Decimal` with explicit
+  precision and scale, `DateTime2`, `DateTimeOffset`, `Time`, `Uuid`,
+  `VarBinary`, `Vector`. This would have caught the
+  [microsoft/mssql-rs#38](https://github.com/microsoft/mssql-rs/issues/38)
+  regression on the first non-int type added.
+- Add a mock-TDS test that injects an ERROR token into the response
+  stream for an `sp_prepare` RPC and asserts the call surfaces it as
+  `SqlServerError`. This covers the diagnostics path without a live
+  server.
+- Make the integration helpers in `mssql-tds/tests/common/mod.rs` skip
+  cleanly (instead of panicking) when the database environment variables
+  are absent, so CI without a SQL Server can still run the unit-style
+  tests.

docs/issue_38_sp_prepare_named_params.md

@@ -47,10 +47,7 @@ fn get_trace_log_path() -> Option<PathBuf> {
 
         // Security check: warn about insecure paths but allow them
         if is_insecure_path(&path) {
-            eprintln!(
-                "[mssql-js] WARNING: Insecure log directory detected: '{}'",
-                path.display()
-            );
+            eprintln!("[mssql-js] WARNING: Insecure log directory detected: {path:?}",);
             eprintln!("[mssql-js] WARNING: Logs may contain sensitive data (queries, data etc).");
             eprintln!(
                 "[mssql-js] WARNING: Logging to /tmp, /var/tmp, or system temp directories is not recommended."
@@ -68,11 +65,7 @@ fn get_trace_log_path() -> Option<PathBuf> {
         if !path.exists()
             && let Err(e) = create_dir_all(&path)
         {
-            eprintln!(
-                "[mssql-js] ERROR: Could not create log directory '{}': {}",
-                path.display(),
-                e
-            );
+            eprintln!("[mssql-js] ERROR: Could not create log directory '{path:?}': {e}",);
             return None;
         }
         return Some(path.join(TRACE_LOG_FILENAME));
@@ -145,8 +138,7 @@ pub fn init_tracing() {
                             },
                             Err(e) => {
                                 eprintln!(
-                                    "[mssql-js] ERROR: Could not create trace log file '{}': {}. File logging will be disabled.",
-                                    log_path.display(), e
+                                    "[mssql-js] ERROR: Could not create trace log file {log_path:?}: {e}. File logging will be disabled."
                                 );
                             }
                         }

mssql-js/src/tracing_init.rs

@@ -1707,6 +1707,10 @@ impl PythonRowAdapter {
                 let vector = SqlVector::try_from_f32(values)?;
                 Ok(ColumnValues::Vector(vector))
             }
+            VectorBaseType::Float16 => Err(Error::UsageError(format!(
+                "Float16 VECTOR column '{}' is not supported in Python bulk copy yet",
+                target_meta.column_name
+            ))),
         }
     }
 
@@ -1791,6 +1795,10 @@ impl PythonRowAdapter {
                 let vector = SqlVector::try_from_f32(floats)?;
                 Ok(ColumnValues::Vector(vector))
             }
+            VectorBaseType::Float16 => Err(Error::UsageError(format!(
+                "Float16 VECTOR column '{}' is not supported in Python bulk copy JSON coercion",
+                target_meta.column_name
+            ))),
         }
     }
 

mssql-py-core/src/bulkcopy.rs

@@ -4,7 +4,7 @@
 use pyo3::exceptions::PyRuntimeError;
 use pyo3::prelude::*;
 use pyo3::types::PyDict;
-use std::sync::Arc;
+use std::{path::PathBuf, sync::Arc};
 use tokio::runtime::Runtime;
 use tokio::sync::Mutex;
 
@@ -254,7 +254,7 @@ impl PyCoreConnection {
         // ServerCertificate - path to the server certificate file for validation
         let server_certificate = dict
             .get_item("server_certificate")?
-            .and_then(|v| v.extract::<String>().ok());
+            .and_then(|v| v.extract::<PathBuf>().ok());
 
         let encryption_options = EncryptionOptions {
             mode: encryption_mode,

mssql-py-core/src/connection.rs

@@ -550,6 +550,9 @@ impl PyCoreCursor {
                             return list.into_any();
                         }
                     }
+                    VectorBaseType::Float16 => {
+                        // Let it fall through to the string conversion below
+                    }
                 }
                 // Fallback to string if conversion fails
                 format!("{:?}", v)

mssql-py-core/src/cursor.rs

@@ -89,11 +89,7 @@ fn get_trace_log_path() -> Option<PathBuf> {
     if !dir.exists()
         && let Err(e) = create_dir_all(&dir)
     {
-        eprintln!(
-            "[mssql-py-core] ERROR: Could not create log directory '{}': {}",
-            dir.display(),
-            e
-        );
+        eprintln!("[mssql-py-core] ERROR: Could not create log directory {dir:?}: {e}");
         return None;
     }
 
@@ -143,8 +139,7 @@ pub(crate) fn init_tracing() {
                                 .event_format(LogFormatter);
 
                             eprintln!(
-                                "[mssql-py-core] Created log file → {}",
-                                log_path.display()
+                                "[mssql-py-core] Created log file → {log_path:?}"
                             );
 
                             // Store guard to keep async writer alive
@@ -162,9 +157,7 @@ pub(crate) fn init_tracing() {
                         }
                         Err(e) => {
                             eprintln!(
-                                "[mssql-py-core] ERROR: Could not create trace log file '{}': {}. Tracing will be disabled.",
-                                log_path.display(),
-                                e
+                                "[mssql-py-core] ERROR: Could not create trace log file {log_path:?}: {e}. Tracing will be disabled."
                             );
                         }
                     }