Repository navigation
FIX: Resolve UDT parameter metadata before binding - #818
Merged
Jahnvi Thakkar (jahnvi480) merged 2 commits intoSep 25, 2026
Merged
Jahnvi Thakkar (jahnvi480) merged 2 commits into
Jahnvi Thakkar (jahnvi480) merged 2 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 25, 2026 06:20
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Reviewer assessments flag the change as risky for automated approval, and one documented workaround is misleading.
Review effort: Lite
Findings: None
What changed in this PR
Resolves SQL Server UDT metadata before parameter binding while preserving the public API.
Changes:
- Adds UDT metadata discovery via
SQLDescribeParam. - Preserves streamed
setinputsizes()overrides. - Adds spatial UDT regression tests and documentation.
| File | Summary |
|---|---|
tests/test_017_spatial_types.py |
Adds UDT binding regression coverage. |
mssql_python/pybind/ddbc_bindings.cpp |
Resolves UDT metadata before binding. |
mssql_python/cursor.py |
Documents limitations and preserves streamed overrides. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 457-467 457 for (const auto& info : paramInfos) {
458 if (info.paramSQLType == SQL_SS_UDT) {
459 hasUdt = true;
460 break;
! 461 }
! 462 }
! 463 if (!hasUdt) return SQL_SUCCESS;
464
465 // UDT identity lives in the IPD, not in the scalar describe cache. Describe
466 // unbound records on each execution, including reused statements and DAE rows.
467 SQLRETURN rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 79.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%🔗 Quick Links
|
…und-no-way-to-707313
Jahnvi Thakkar (jahnvi480)
requested review from
ttk (Theekshna),
Gaurav Sharma (bewithgaurav),
Copilot,
gargsaumya and
Sumit Sarabhai (sumitmsft)
September 25, 2026 06:37
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 25, 2026 06:37
View session
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 25, 2026
microsoft/mssql-python#818 resolves SQL_SS_UDT parameter identities by calling SQLFreeStmt(SQL_RESET_PARAMS) and then SQLDescribeParam before binding. That works only because the reset truncates the IPD: a record left from an earlier bind reads as explicitly bound, and refine_ipd leaves those alone. Cover the sequence end to end so the interop does not regress. AB#48248
5 tasks done
Sumit Sarabhai (sumitmsft)
approved these changes
Sep 25, 2026
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
Contributor
There was a problem hiding this comment.
PR #818: No actionable introduced defects in the reviewed changes. UDT metadata resolution is consistent across the affected binding paths. The existing zero-sized UDT batch limitation predates this change and is not worsened by it.
Gaurav Sharma (bewithgaurav)
approved these changes
Sep 25, 2026
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
Collaborator
There was a problem hiding this comment.
this addresses the reported gap and keeps the surrounding execution paths intact. approving.
Jahnvi Thakkar (jahnvi480)
enabled auto-merge (squash)
September 25, 2026 11:59
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 25, 2026
microsoft/mssql-python#818 resolves SQL_SS_UDT parameter identities by calling SQLFreeStmt(SQL_RESET_PARAMS) and then SQLDescribeParam before binding. That works only because the reset truncates the IPD: a record left from an earlier bind reads as explicitly bound, and refine_ipd leaves those alone. Cover the sequence end to end so the interop does not regress. AB#48248
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 26, 2026
refine_ipd replays the cached identity list on every cached SQLDescribeParam answer, and searched it linearly for each marker. A describe-all-before-bind pass over N UDT markers - the sequence microsoft/mssql-python#818 runs on every execution - therefore cost O(N^3) ordinal comparisons: 62,625,000 at 500 placeholders. Sort the sparse list once where it is cached, then binary-search it. sp_describe_undeclared_parameters does not promise ordinal order, so the sort cannot be skipped by assuming push order; a debug_assert guards the invariant for future callers. Operation-count reasoning, not a latency measurement - the per-call name clones the review also noted are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 26, 2026
The cache-served SQLDescribeParam path deep-cloned every cached identity out from under the STMT lock, then refine_ipd rebuilt an owned copy for each marker. A describe-all pass over N UDT markers - what microsoft/mssql-python#818 runs per execution - therefore made O(N^2) string allocations. Hold the cached identities in Arc so releasing the STMT lock costs a refcount bump instead of N deep copies, and skip the descriptor write entirely when a replay finds the three wire parts already in place. Comparing them allocates nothing, so the steady state is now allocation-free; the descriptor still owns its copy and the lock ordering is unchanged. Also narrow parity deviation 19: a zero-filled overflow past the 8000-byte ceiling is trimmed and sent by both drivers (trim_zero_overflow is CheckTrailingZeros, sqlccnvt.cpp:8690), so only a non-zero overflow diverges. The recorded measurement used 0xAB bytes and is labelled as such. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 26, 2026
microsoft/mssql-python#818 resolves SQL_SS_UDT parameter identities by calling SQLFreeStmt(SQL_RESET_PARAMS) and then SQLDescribeParam before binding. That works only because the reset truncates the IPD: a record left from an earlier bind reads as explicitly bound, and refine_ipd leaves those alone. Cover the sequence end to end so the interop does not regress. AB#48248
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 26, 2026
refine_ipd replays the cached identity list on every cached SQLDescribeParam answer, and searched it linearly for each marker. A describe-all-before-bind pass over N UDT markers - the sequence microsoft/mssql-python#818 runs on every execution - therefore cost O(N^3) ordinal comparisons: 62,625,000 at 500 placeholders. Sort the sparse list once where it is cached, then binary-search it. sp_describe_undeclared_parameters does not promise ordinal order, so the sort cannot be skipped by assuming push order; a debug_assert guards the invariant for future callers. Operation-count reasoning, not a latency measurement - the per-call name clones the review also noted are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ttk (Theekshna)
pushed a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 26, 2026
The cache-served SQLDescribeParam path deep-cloned every cached identity out from under the STMT lock, then refine_ipd rebuilt an owned copy for each marker. A describe-all pass over N UDT markers - what microsoft/mssql-python#818 runs per execution - therefore made O(N^2) string allocations. Hold the cached identities in Arc so releasing the STMT lock costs a refcount bump instead of N deep copies, and skip the descriptor write entirely when a replay finds the three wire parts already in place. Comparing them allocates nothing, so the steady state is now allocation-free; the descriptor still owns its copy and the lock ordering is unchanged. Also narrow parity deviation 19: a zero-filled overflow past the 8000-byte ceiling is trimmed and sent by both drivers (trim_zero_overflow is CheckTrailingZeros, sqlccnvt.cpp:8690), so only a non-zero overflow diverges. The recorded measurement used 0xAB bytes and is labelled as such. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ttk (Theekshna)
added a commit
to microsoft/mssql-rs
that referenced
this pull request
Sep 28, 2026
* Bind CLR UDT and binary sql_variant parameters
A UDT parameter had no way in: the conversion matrix gave SQL_SS_UDT no
row, and nothing carried the three-part type name the TDS parameter
header requires. SQL_C_BINARY could not reach sql_variant either.
Character and binary buffers now reach both targets. A UDT's payload is
already its IBinarySerialize form, so it passes through untouched; only
the identity has to be supplied. That comes from the IPD's
SQL_CA_SS_UDT_* fields, or from the server's suggested_user_type_*
columns when the application describes the parameter before binding -
the same two sources msodbcsql uses (AutoFillIPD, sqlcdesc.cpp).
The identity lives on a new per-ordinal ParamSnapshot rather than on
BoundParam, which stays Copy: for_row runs once per parameter per row,
and an owned name there would re-allocate the same strings for every row
of a parameter array.
A UDT is declared to sp_executesql by its own quoted type name. The TDS
type name "udt" is not resolvable by the server, which answers "Cannot
find data type udt" (2715).
AB#48248
* Pin the mssql-python reset-then-describe UDT sequence
microsoft/mssql-python#818 resolves SQL_SS_UDT parameter identities by calling SQLFreeStmt(SQL_RESET_PARAMS) and then SQLDescribeParam before binding. That works only because the reset truncates the IPD: a record left from an earlier bind reads as explicitly bound, and refine_ipd leaves those alone. Cover the sequence end to end so the interop does not regress.
AB#48248
* Address Copilot review on the UDT parameter path
Fixes found by review, each with a regression test:
- fuzz_support.rs still called bound_param_to_rpc with the old arity, so the
cfg(fuzzing) target no longer built.
- The cache-served SQLDescribeParam dropped the UDT identity. After one
describe, SQL_RESET_PARAMS truncates the IPD but leaves the metadata cache,
so the next describe rebuilt the record with no type name and the execute
failed. That is the reset-then-describe cycle mssql-python#818 runs on every
execution. Cached the identities beside the scalar descriptions.
- refine_ipd used udt_names.is_none(), which cannot tell an application's
identity from one it auto-filled earlier. A re-SQLPrepare keeps IPD records,
so a stale name survived onto a different statement. Tracked with
udt_names_auto_filled, mirroring explicitly_bound.
- ParameterDefinition omitted the UDT names, so changing one after the first
execute left a materialized plan in place and reused the old declaration.
The assembly name stays excluded; it never reaches the wire.
- A catalog with no schema invented dbo, silently choosing a different type for
a caller whose default schema is not dbo. msodbcsql quotes the absent schema
to the empty string and emits [db]..[type]; match it.
- Three comments claimed SQLDescribeParam cannot report a UDT name, and one
called an execute-time refusal a bind-time one.
Registered the sql_variant ceiling SQLSTATE divergence as parity deviation 18
with the measured SQL_DRIVER_VER, and corrected deviation 17, which still said
UDT parameter binding was unsupported.
Not taken: the extra payload clone in to_column_value_and_context. The binary,
varbinary and image arms beside it clone the same way, so this is a driver-wide
property of that function rather than something the UDT arm introduces; a
borrowing path belongs in its own change.
AB#48248
* Stop an auto-filled UDT name from outliving its SQL
A name refine_ipd auto-fills describes the text it was described from, not the
binding. A re-SQLPrepare keeps IPD records, so the stale name survived; a later
SQLBindParameter then marked the record explicitly bound, which stops the
self-healing describe path from ever correcting it, and execute sent the
previous statement's type identity.
DescHandle::clear_auto_filled_udt_names drops those on a successful prepare and
keeps application-supplied ones. Verified the regression: with the call
disabled, AnAutoFilledNameDoesNotSurviveARePrepare fails.
Not applied to SQLExecDirect, which the review also asked for. Measured
msodbcsql 18.6.2.1 (SQL_DRIVER_VER 18.06.0002) on SQL Server 2022, 2026-09-25:
it reuses the auto-filled name there and the execute succeeds. Matching it
rather than diverging unilaterally; SQLExecDirectReusesAnAutoFilledName pins
that on both legs.
EachUdtParameterCarriesItsOwnName now binds hierarchyid and geometry instead of
hierarchyid twice, so a driver that reused parameter 1's name for parameter 2
can no longer pass it.
AB#48248
* Reject UDT TVP columns and normalize empty UDT name parts
Two holes the new public SqlType::Udt variant opened.
write_type_info emits only the RPC parameter form of a UDT - three B_VARCHARs.
TvpValue::validate reaches it through sqltypes.rs:1197 on the serialization
path and accepted a UDT column, which would have written COLMETADATA without
the MAX_BYTE_SIZE and assembly-qualified name that form requires, i.e.
malformed TDS. TvpColumnDef::validate now rejects it, alongside the existing
text/ntext restriction.
write_b_varchar encodes an empty name part and an absent one identically, but
format_udt_sql_name matched on Some(_), so a UdtTypeName built with
Some(String::new()) - which the public constructor and public fields both
allow - declared [] against a header that named nothing. Empty parts are now
read as absent, so the declaration and the wire identity agree.
AB#48248
* Fix the mssql-py-core build and address review follow-ups
mssql-py-core is excluded from the workspace, so `cargo clippy --workspace`
never saw that SqlType::Udt made its sql_type_metadata match non-exhaustive.
CI runs scripts/bclippy.ps1, which lints the workspace and then mssql-py-core,
which is why all four legs failed while the workspace was green. Added the arm,
and validated with the CI script rather than a bare workspace clippy.
The UDT name is part of the declaration, so it belongs in the reuse identity.
Unlike the TVP arm beside it, absent parts stay absent: `Point` and `dbo.Point`
are different declarations and must not share a prepared statement.
Also pinned the accept side of the binary length ceiling, and recorded in
`unrelated_c_types_do_not_reach_decimal_xml_or_variant` that the sql_variant
row is narrower than msodbcsql's fValidConversion by deferral, not intent,
tracked in AB#48453.
AB#48248
* Scope UDT name provenance and drop per-row identity clones
Writing SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME marked the whole bundle as
application-supplied, even though that field never reaches the wire. After an
auto-filled name, setting only the assembly field stopped SQLPrepare clearing
the stale wire identity and stopped refine_ipd replacing it, so a later
statement could execute against the previous statement's type. Only the three
wire-relevant parts now claim the identity; the clear and the describe refresh
both preserve an application's assembly name, which no describe supplies.
clear_auto_filled_udt_names ignored a poisoned descriptor mutex and let
SQLPrepare report success with the stale identity still in place. It now
returns SQL_ERROR and SQLPrepare propagates it, per the repository's
mutex-poison rule.
Two identity clones removed. build_positional_params cloned the whole tail into
a temporary Vec once per stored-procedure call; it borrows the slice instead.
The parameter-array validation loop cloned each ParamSnapshot once per row per
parameter, reallocating the boxed name strings - exactly the cost the snapshot
split exists to avoid - and now copies only the Copy half.
AUdtParameterWithoutATypeNameFails asserted only SQL_ERROR, so the diagnostic
could regress to any SQLSTATE unnoticed. It now asserts HY000 as the
application observes it, and that passes on the msodbcsql leg too.
AB#48248
* Keep an assembly-named UDT record refreshable by describe
`clear_auto_filled_udt_names` dropped the auto-filled flag for every
record it visited, but only cleared the record itself when no assembly
name was set. A record kept for its echo-only assembly name was left
`Some(..)` and no longer auto-filled - exactly the state `refine_ipd`'s
gate refuses to touch. The server's `suggested_user_type_*` identity was
never re-applied, so the execute failed with ERR_MISSING_UDT_TYPE_NAME
through a plain describe / set-assembly-name / re-prepare / describe
sequence.
Keep the flag set on the branch that retains the record, and stop
`refine_ipd` from discarding a record whose only remaining content is an
assembly name no describe can restore.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Let the server name a UDT an assembly-only record never claimed
`refine_ipd` gated on `udt_names.is_some()`, which treats any record with
a `UdtNames` as an application claim. Writing only the echo-only
`SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME` on a never-described record produces
`Some(..)` with an empty `type_name` and the provenance bit still false,
so a later describe skipped it and the execute failed with
ERR_MISSING_UDT_TYPE_NAME - route (b) silently broken for an application
that knows its assembly but relies on the server for the type name.
Gate on the record's state instead: an empty `type_name` carries no wire
identity however it was reached, whether by that setter or by
`clear_auto_filled_udt_names` keeping a record for its assembly name
alone. The two histories are now indistinguishable, as the descriptor
contract says they should be, and an application-written `type_name`
still outranks the server.
Also post a diagnostic on `SQLPrepareW`'s poisoned-IPD exit, which
otherwise returned SQL_ERROR with nothing for SQLGetDiagRec to report,
and retarget a `mssql-py-core` TODO this change satisfies at the
remaining Python-side blocker.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Pin UDT PLP framing with byte-level serialization tests
The e2e comment claimed `datatypes::sql_udt::tests` pinned the TYPE_INFO
name block *and* the PLP body framing, but those tests only exercise
`write_udt_type_name` - nothing serialized a `SqlType::Udt`, so the type
byte, the chunk framing and the terminator were unpinned.
Add two tests over the real `serialize` path: one for a populated UDT
covering the type byte, the three B_VARCHARs, the PLP length marker,
chunk and terminator, and one for a NULL UDT, which must still name its
type and then declare the PLP null length. Correct the e2e comment to
cite what each test actually covers.
Recorded while pinning it: this driver declares the PLP body
unknown-length and relies on chunk framing, where msodbcsql's
WriteUDTHeader writes the actual byte count when it knows it. Both are
valid PLP; the note is in the test so the next reader does not take the
difference for a defect.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Record that msodbcsql hex-decodes a character buffer bound to a UDT
The character arm passes the buffer's bytes through verbatim, and the comment beside it read as though that were parity. It is not: rgbTRANSTYPE* maps SQL_UDT_MAPPED to a SQL_C_BINARY transfer type, so ConvertLongData takes its conversion path rather than its pass-through one and hex-decodes, two characters per byte. Same binding, different wire payload.
Source reading only, so this is a cited divergence note rather than a registry entry - the measurement that would settle it is named in the comment, along with the SQL_NTS caveat that makes a character UDT binding need an explicit length.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Refuse character buffers against a UDT until the decode is measured
The matrix admitted SQL_C_CHAR and SQL_C_WCHAR for SQL_SS_UDT and the conversion sent the buffer verbatim, but msodbcsql hex-decodes two characters to the byte. Advertising the rows while diverging on the payload ships a wrong-bytes path that no e2e case covers, since every UDT test binds SQL_C_BINARY.
Drop the two rows instead. The matrix documents itself as a progress list where a missing entry means unbuilt (HYC00), which is the gap category in parity-deviations.md - a code comment plus a work item, not a registry entry. Nothing regresses: this PR is what introduces the capability. The citation chain and the measurement that would close it are recorded at both sites.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Binary-search UDT identities instead of scanning per marker
refine_ipd replays the cached identity list on every cached SQLDescribeParam answer, and searched it linearly for each marker. A describe-all-before-bind pass over N UDT markers - the sequence microsoft/mssql-python#818 runs on every execution - therefore cost O(N^3) ordinal comparisons: 62,625,000 at 500 placeholders.
Sort the sparse list once where it is cached, then binary-search it. sp_describe_undeclared_parameters does not promise ordinal order, so the sort cannot be skipped by assuming push order; a debug_assert guards the invariant for future callers. Operation-count reasoning, not a latency measurement - the per-call name clones the review also noted are unchanged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Share cached UDT identities and skip unchanged descriptor replays
The cache-served SQLDescribeParam path deep-cloned every cached identity out from under the STMT lock, then refine_ipd rebuilt an owned copy for each marker. A describe-all pass over N UDT markers - what microsoft/mssql-python#818 runs per execution - therefore made O(N^2) string allocations.
Hold the cached identities in Arc so releasing the STMT lock costs a refcount bump instead of N deep copies, and skip the descriptor write entirely when a replay finds the three wire parts already in place. Comparing them allocates nothing, so the steady state is now allocation-free; the descriptor still owns its copy and the lock ordering is unchanged.
Also narrow parity deviation 19: a zero-filled overflow past the 8000-byte ceiling is trimmed and sent by both drivers (trim_zero_overflow is CheckTrailingZeros, sqlccnvt.cpp:8690), so only a non-zero overflow diverges. The recorded measurement used 0xAB bytes and is labelled as such.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Stop an assembly-only UDT record from forcing a re-prepare
`parameter_definition()` projected `Some(("", "", ""))` for a record
carrying nothing but the echo-only assembly name, where a record with no
identity projects `None`. Writing `SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME` on
an otherwise-unnamed record therefore orphaned a materialized prepared
handle and forced a re-prepare for a field the declaration never
mentions - contradicting the invariant the adjacent test already stated.
Project the wire parts only when at least one is set, and cover the
assembly-only case. Mutation-checked both directions: over-filtering
breaks the rename-invalidates leg.
Also correct parity deviation 19. Narrowing it last commit replaced an
overstatement with an unmeasured parity claim - that both drivers trim a
zero-filled overflow - which is a source reading only. It is now
labelled as such, with the both-leg measurement that would close it
named, per the evidence rule in mssql-odbc.instructions.md.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Renumber the sql_variant deviation to 20 after rebase
main added its own entry 19 in #599, so the registry entry added here and the e2e comment citing it both move to 20.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Pin the zero-overflow variant case the registry cites
Deviation 20 cited a_binary_variant_payload_past_the_byte_ceiling_is_truncation for the zero-only overflow half, but that test only exercised 0xFF payloads at 8001 and 8000 bytes - the doc comment asserted the zero behaviour without covering it. an_all_zero_binary_overflow_is_trimmed_silently does cover zero padding, but for plain varbinary/binary/image targets rather than a variant.
Add the variant-specific leg so the cited test actually pins what the registry claims.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Scope the UDT projection to records that declare a UDT
The SQL_CA_SS_UDT_* fields are writable on any IPD record - classify_field gates on descriptor kind, not concise type - so writing a UDT name on an int marker changed ParameterDefinition and orphaned a materialized prepared handle, even though an int declaration never references that name.
Filter the projection on SQL_SS_UDT. Switching the record to a UDT later still invalidates through sql_type and brings the name with it, which the new test pins alongside the non-UDT case.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Align the UDT projection gate with refine_ipd's claimed test
The projection admitted a record with any of the three wire parts set, so a catalog-only or schema-only IPD record still orphaned a materialized prepared handle - the same spurious re-prepare the assembly-only fix removed, reachable through a different field. Such a record declares nothing and udt_type_name refuses to execute it at all.
Gate on a non-empty type name, which is exactly refine_ipd's claimed test, so the two definitions of having a wire identity are identical. Catalog and schema still travel with a named UDT, so changing either continues to invalidate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Take descriptor test handles from the TestHandles fixture
mssql-odbc.instructions.md:429-439 requires unit-test ODBC handles to come from crate::test_support::TestHandles. The two clear_auto_filled_udt_names tests constructed a standalone DescHandle instead; in production that method is only ever called on a statement's IPD, so the fixture is also the more faithful shape.
The two pre-existing DescHandle::new calls in this module stay: they test the constructor's own defaults, so they have to call it directly.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Propagate refine_ipd failures out of SQLDescribeParam
refine_ipd was best-effort, which was defensible while it only refined type/size/scale the call already answered from its in-memory descriptions. This PR made it load-bearing: on the describe-before-bind route its IPD write is the only source of SQL_CA_SS_UDT_TYPE_NAME. A poisoned IPD lock or a failed prepared-definition invalidation still returned SQL_SUCCESS, so the application saw the failure later as a misleading missing-name error at execute, or reused a stale declaration.
Return SqlReturn and propagate from both the cached and fresh paths, posting HY000 on the statement. Matches clear_auto_filled_udt_names, which already propagates its own poisoned-mutex failure, and the crate rule that a poisoned mutex returns SQL_ERROR.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Correct the assembly-name rationale and the refine_ipd doc block
sp_describe_undeclared_parameters does return suggested_assembly_qualified_type_name (0-based column 11), so three comments claiming the server never supplies one were wrong as written. msodbcsql even reads it (sqlcdesc.cpp:9155-9162), but only into a scratch FRS_Format field - unlike the catalog/schema/type parts it never reaches the IPD name pool, so the descriptor field reads back empty there too. The merge rule is unchanged; only its justification was wrong. Say instead that this driver chooses not to read the column, and why.
Also repair the refine_ipd doc block: propagating failures left the old best-effort paragraph in place, directly contradicting the new one, and duplicated the locking-order line.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Validate the whole UDT name before writing any of it
UdtTypeName::validate only checked that the type name was non-empty; the 255-unit B_VARCHAR bound was enforced inside write_b_varchar, per part, as each part was reached. The parts are written in sequence and PacketWriter sends on overflow (handle_overflow_if_needed -> populate_header_and_send), so a 255-unit catalog can fill and flush a small packet before an oversized schema or type name returns UsageError. Invalid local input then became a half-sent RPC needing cancel-and-drain instead of a clean local failure.
Check all three parts up front. The per-part check in write_b_varchar stays as the encoding-level guard; this one keeps the rejection local. Pinned by a test asserting nothing reaches the mock network, mutation-checked by disabling the bound.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Validate UDT identities before the RPC writes anything
The preflight added in 96955fb8 sat in write_udt_type_name, which is already too late: RpcParameter::serialize writes the parameter name and status flags first, write_type_info writes the UDT type byte, and in a multi-parameter RPC an earlier parameter can have flushed whole packets. An invalid local identity still became a half-sent request needing cancel-and-drain.
Hoist the check to the message: SqlRpc::validate_parameters runs before serialize_prefix and serialize_batch_command write a single byte.
Also fix both new tests. They passed MockNetworkWriter::new(4096) with Some(512) as PacketWriter::new's third argument, which is the timeout - packet size comes from the writer, so nothing ever overflowed and the no-bytes-sent assertions were vacuous. Found by mutation: removing the preflight left them green. With the packet size set on the mock, both now fail without their fix.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Route every fallible SqlType metadata check through the preflight
The preflight matched only SqlType::Udt, so a sql_variant carrying a UDT slipped past it. validate_variant_inner then rejected that pairing inside write_type_info - after serialize had written the parameter name and status flags, and after an earlier parameter could have flushed whole packets. Same half-sent RPC, reached through a different type.
Move the rule onto SqlType::validate_for_send, which now owns both fallible checks, and have RpcParameter delegate to it. The duplicates inside write_type_info stay: that function has to remain correct for callers reaching it another way. Extended the RPC test to cover a later Variant(Udt(..)) parameter; mutation-checked by dropping the variant arm.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Complete the send preflight with the parameter-name bound
The preflight covered SqlType metadata but not the parameter-name length check in serialize, so an overlong name on a later named parameter could still fail after an earlier one had flushed packets. An incomplete guarantee is worse than none, since readers rely on it.
Extract validate_name_length and share it with the write path so the two cannot drift, and split validate_named_before_send from validate_before_send: serialize only writes and length-checks the name on the named path, so validating names on positional parameters would newly reject an unused overlong name. Mutation-checked by routing named parameters through the value-only preflight.
Recorded while there: the bound is measured in UTF-8 bytes while the payload is written as UTF-16, which agree only for the ASCII names this driver generates. Pre-existing, left as-is and documented rather than changed silently.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Extend the send preflight to vectors and scope its doc honestly
The preflight's _ => Ok(()) arm skipped SqlType::Vector, whose three dimension/base-type checks then failed inside write_type_info after earlier parameters had flushed - the exact failure the preflight exists to remove, and reachable from mssql-py-core's InputSqlType::Vector. Factor them into validate_vector, shared with the write path, and cover a later mismatched vector in the regression.
Also correct the doc claim, which said every fallible check in write_type_info belonged here. TVP validation lives across serialize_table, write_tvp_type_name and write_tvp_column_metadata and is a larger hoist, so it is now named as a known exception rather than implied to be covered. Likewise scope validate_parameters: it guards one RPC message, while a batched prepared execution validates per command - a pre-existing property it shares with reject_data_at_exec and the ForceColumnEncryption check.
Restore validate_variant_inner's doc comment, which an earlier commit orphaned by inserting validate_for_send between it and its function.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Correct a stale count in the preflight test doc
The test's doc said it covered both fallible metadata checks, but 3500cc73 added a third case - a vector whose declaration disagrees with its value - without updating the sentence above the array.
Third instance of this shape in the series, after the describe_param.rs block and the orphaned validate_variant_inner comment: appending beside an existing comment without re-reading it. A sweep of all 30 changed files for count claims found no others.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Share the UDT identity through parameter_definition instead of copying it
The Arc cache removed the per-replay clone of the cached identity list, but parameter_definition still deep-copied catalog, schema and type into an owned tuple - and refine_ipd takes that snapshot twice per record, before and after refinement. A cache-served describe over N UDT markers therefore still made ~2N^2 string allocations, which is the cost the Arc was introduced to remove.
Carry Option<Arc<UdtNames>> through DescRecord, BoundParam and DaeParam so the snapshot is a refcount bump; writers use Arc::make_mut, so a record whose identity is also held by a live snapshot copies once on write rather than on every read.
PartialEq is now hand-written because the shared UdtNames carries assembly_type_name, which must not affect equality: it never reaches the declaration, so a derived impl would orphan a prepared handle on an echo-only write. Mutation-checked - comparing the whole struct fails the_udt_name_is_part_of_the_prepared_parameter_definition on exactly that assertion.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Track UDT name provenance per field, not per record
The three wire parts of a UDT identity are independently writable through
SQLSetDescField, so a single `udt_names_auto_filled` bit could not say which
of them the application owns. Two orders broke:
1. A describe fills the identity, the application overrides only the schema.
The one flag marked the whole identity application-owned, so the
prepare-time clear preserved the server's stale type name and the next
describe refused to refresh it - the new statement executed against the
previous one's type.
2. The application writes only the catalog. `refine_ipd`'s gate keyed off
`type_name` alone, so the describe overwrote the catalog it had set.
Replace the bit with `UdtNameClaims`, one bool per wire part. `set_udt_name`
claims only the part it writes; `clear_auto_filled_udt_names` clears only
unclaimed parts; `refine_ipd` merges the server's values into unclaimed parts
and leaves claimed ones alone. The assembly name still claims nothing - it
never reaches the wire and no describe supplies it.
`refine_ipd` keeps the steady-state early-out and still shares the describe's
`Arc` when the application has claimed nothing, so the per-marker copy the
previous change removed does not come back.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Keep the UDT identity merge off the non-UDT path
`refine_ipd`'s per-part merge ran for every record, so an ordinary scalar
marker entered it with nothing stored and nothing described, allocated an
`Arc<UdtNames>` through `get_or_insert_with`, copied empty into empty, and had
the cleanup below free it again. `refine_ipd` is replayed for every
cache-served describe answer, so that is ~N^2 allocations across a
describe-all pass over N markers - on statements containing no UDT at all,
the same order of cost 53c6de48 removed one level down.
Guard the merge on `described.is_some() || record.udt_names.is_some()` and
move it into `merge_udt_identity`. Skipping is behaviour-preserving: both
merge branches are no-ops when the record and the describe agree there is no
identity, and a claim cannot exist without a record to live on - `set_udt_name`
creates the record before claiming, and both clear paths drop the record only
when nothing is claimed. A `debug_assert!` pins that invariant.
Also filter the IPD identity out of `ParamSnapshot` for non-`SQL_SS_UDT`
parameters. `classify_field` gates the `SQL_CA_SS_UDT_*` fields on
`DescKind::ImpParam` alone, so an application can leave an identity on a record
it later rebinds to a scalar - which contradicted the field's documented
contract and cost a refcount bump per execute. This is the same filter
e433dc25 applied to `parameter_definition`.
Correct one stale cross-reference in `parameter_definition`: it cited
`refine_ipd`'s `claimed` gate, which 32a2c35f replaced with per-field claims.
The test it names is `udt_type_name`'s.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Cover the output direction of a UDT parameter
Binding SQL_SS_UDT with SQL_PARAM_OUTPUT became reachable for the first time
in this PR - before it, SQLBindParameter rejected the type with HYC00 for want
of a conversion-matrix row, so no direction was bindable. `sql_bind_parameter_safe`
accepts every non-streamed direction and `is_output_only` routes straight to
`typed_null`'s SQL_SS_UDT arm, but nothing exercised the leg after that:
`write_back_output_params` had never run against a returned UDT, and all 17
e2e UDT cases bind SQL_PARAM_INPUT.
Add both legs. The unit test pins the write-back: a returned UDT decodes to
`ColumnValues::Bytes` (decoder.rs, the TdsDataType::Udt arm), so delivery goes
through the binary path. The e2e drives a procedure with a `hierarchyid OUTPUT`
parameter and compares the bytes against the same value fetched independently.
Accepting the binding is parity, not a divergence: msodbcsql applies no
direction gate to SQL_UDT_MAPPED - `CheckParamBindInfo` (sqlccmd.cpp:9977)
checks only that a type name is present - so refusing the non-input directions
would have been the departure needing a record.
Correct the `SqlType::Udt` doc, which still called the type "input-only". A
returned UDT never reaches that type; it arrives as plain bytes.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Write the missing-UDT-name diagnostic in this driver's own words
The text of ERR_MISSING_UDT_TYPE_NAME was copied from a comment in the
reference driver's source rather than written here, and the same borrowed
wording was repeated in the `udt_type_name` doc comment and the e2e test
comment. That put proprietary source text into a public repository, and it
also misrepresented what it was: a maintainer's comment, not the message that
error ID actually renders.
Reword the constant independently and reduce all three citations to path and
line, which is how the rest of this PR cites the reference. The parity claim
that survives is the SQLSTATE - HY000, asserted on both legs by
`AUdtParameterWithoutATypeNameFails` - and the doc now says that is the claim.
No test asserted the literal text, so the behavior is unchanged.
One more instance of the same class, not flagged but the same defect: the
character-to-UDT note in `bound_param_to_value_with_outcome` quoted a comment
from `sqlccnvt.cpp`. Replaced with a description of the behavior, keeping the
path and line.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Correct two contracts this PR's edits outgrew
Neither is a behavior change; both are statements that stopped being true when
the code beneath them moved.
`sql_prepare_w_safe` said it holds the DBC and STMT locks "for the whole
body". It now releases both before calling `clear_auto_filled_udt_names`,
because the crate forbids holding a STMT lock while taking a DESC lock. The
invariant the sentence protected still holds - the state check and the store
are under one continuous STMT lock - but the release, and the rule that forces
it, went unmentioned at the point where it bites.
`udt_name_fields_are_writable_on_ipd_only` said every kind other than the IPD
"has no use for" the UDT identity. `SQLColAttribute` already answers all four
parts for a result column out of COLMETADATA's UDT_INFO
(`col_attribute.rs:286-309`), so the IRD does have a use for them and the test
was reading as if it had settled the `SQLGetDescField` route. Reworded to
scope the assertion to writes and leave the IRD read side visibly open.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Say what the IRD assertion decides, and record the deviation it pins
The comment added in 6a418798 said the IRD read route was "deliberately left
open rather than decided here", but the assertion ten lines below asserts
`classify_field(ImpRow, SQL_CA_SS_UDT_*)` is `None`. That gate is the whole
read path - `get_desc_field.rs:140` returns HY091 the moment it yields `None` -
so the route is closed, and the test is what holds it closed. Both readings of
"left open" were wrong.
Reword to say the route is closed and that this assertion pins it.
The parity half, which the previous comment left as an open question, is now
answered: msodbcsql serves all four through that route. SQLGetDescFieldW's
default arm dispatches SQL_HANDLE_IRD to GetIRDField with fSQLGETDESCFIELD
(`sqlcdesc.cpp:2390`), and GetIRDField answers the UDT name parts from the
column's name pool (`sqlcdesc.cpp:6844-6880`) with no gate separating that
caller from SQLColAttribute. So refusing the read is a divergence, recorded as
parity deviation 21 with its evidence limit: source reading only, no
comparison run.
The gap predates this work - these fields were absent from `classify_field`
for every kind before it - so no behavior changes here. Wiring up the IRD read
is result-column work.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Record the IRD getter divergence as a gap, not a deviation
Entry 21, added in ceb4e355, described itself as "a gap rather than a choice"
and then registered it as a deliberate deviation. The registry's own boundary
rules that out: entries there are decisions - "we know what msodbcsql does, we
could match it, and we chose not to" - and a gap is "a code comment plus a
work item, not an entry here". `.github/instructions/mssql-odbc.instructions.md:79-81`
says the same.
Remove the entry and move its full content into the code comment on
`udt_name_fields_are_writable_on_ipd_only`, which is the prescribed half I can
deliver. Nothing is lost: the comment keeps what msodbcsql does
(`sqlcdesc.cpp:2390` -> `GetIRDField`, answering at `:6844-6880`), what this
driver does instead (HY091 via `classify_field`), that the evidence is source
reading rather than a measured run, and that the write-side citation is not
evidence for the read side.
The other half - a tracked work item - still needs filing; the comment says so
rather than implying the gap is recorded somewhere it is not.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Fuzz the UDT arm, and fold an empty name part to absent in the py cache key
The conversion-matrix row this PR adds makes SQL_SS_UDT satisfy
`is_supported_conversion`, so `PARAM_SQL_TYPES` now draws it - but
`fuzz_bound_param` passed `udt_names: None`, and the conversion consults the
identity before it reads the value buffer. Every UDT draw returned
`MissingUdtTypeName` immediately, leaving the newly reachable
`SqlType::Udt` construction with no coverage at all.
Draw the three name parts from the same cursor. A literal would reach the
construction but not the code that parses hostile text, so the lengths are
drawn past the 255-UTF-16-unit bound to reach `UdtTypeName::validate`'s
rejection, and can be zero to reach the absent-part branches of
`format_udt_sql_name`, whose `]` doubling now also sees fuzzer bytes. Lossy
UTF-8 conversion rather than a validity check, so no draw is wasted.
Separately, `sql_type_metadata`'s Udt arm keyed `Some("")` and `None` apart
while `format_udt_sql_name` renders them identically - it filters empties
before choosing its branch. Two values with the same `sp_executesql` text
would have missed the prepared-statement cache. Unreachable today because
`InputSqlType::Udt` is still refused, so this is a latent bug fixed before it
can fire rather than an observable one.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Reach the UDT declaration formatter the fuzz harness claimed to cover
The oversized type-name draw added in 99cdc749 was shaped by a belief I did
not check: that `bound_param_to_rpc` evaluates `UdtTypeName::validate`'s
255-UTF-16-unit bound and `format_udt_sql_name`'s `]` doubling. Neither is on
that path. `udt_type_name` only checks the type name is non-empty; `validate`
runs from `write_udt_type_name` and the RPC send preflight, and the formatter
runs from `sql_declaration`. A probe binding 300-unit parts with a raw `]`
returns Ok and carries the `]` through unescaped.
Two consequences, both fixed here.
The draw cost entropy for nothing. It consumed up to 329 bytes ahead of
`cur.rest()` on *every* draw, including every non-UDT `sql_type`, taking that
budget straight out of the value buffer. The parts are now drawn only when
`sql_type` is SQL_SS_UDT, which is stricter than restoring the old 5 bytes:
non-UDT draws now pay nothing at all.
And the coverage was worth having, so take it rather than dropping the claim.
Rendering the declaration from the converted value reaches
`format_udt_sql_name`, whose `]` doubling is the injection-relevant escaping
and was fuzzed by nothing - mssql-tds's `fuzz_api_inputs` calls `get_sql_name`
but never generates `SqlType::Udt`. Both seams this needs
(`RpcParameter::get_sql_name`, `get_value`) are already `#[cfg(fuzzing)]`
exports.
`UdtTypeName::validate` stays out of reach: it is `pub(crate)` to mssql-tds
and runs at serialization time, so fuzzing it belongs in that crate's targets.
The comments now say that instead of claiming it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Track the character-to-UDT decode in its own work item
The four citations for that deferral pointed at AB#48248, this PR's own task,
which closes when the PR merges and would leave the deferral untracked. They
now point at AB#48815, which carries the msodbcsql source chain, the
measurement that would close it, and the note that AB#48249 needs this on its
explicit unimplemented-feature list so the rows keep HYC00 rather than being
flipped to 07006 by matrix absence.
Comment-only; no behaviour change.
AB#48248
* Bound a UDT parameter ColumnSize, and drop one more borrowed comment
`parameter_column_size_is_valid` had no SQL_SS_UDT arm, so it fell through to
`_ => return true` and accepted any size. msodbcsql rejects `> SQL_PREC_UDT`
(8000) with IDS_S1_104 in `CheckSqlPrecScale`'s `case SQL_UDT_MAPPED`
(sqlcdesc.cpp:11790), which SQLBindParameter reaches at sqlcdesc.cpp:3038;
`FixupColumnSizeDecimalDigits` has no UDT arm, so the application's value
arrives unmodified and that check is genuinely reachable. A probe confirmed
this driver accepted 8001, 65535 and 100000.
This PR is what made the arm reachable - before it the conversion matrix
refused the bind, so the table was never consulted for a UDT. Zero stays legal
because it is the `max` spelling, matching the reference's one-sided test.
Mutation-verified: deleting the arm fails the assertion.
Also removes a comment quoted verbatim from the reference driver's source in
`conversion_matrix.rs`. Three siblings went in eebd82d8; the sweep then used a
single-line pattern and this one spans three lines. The 2-chars-to-1-byte
behaviour it described is restated in this driver's own words with the citation
kept.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add mssql-tds integration tests for the UDT parameter path
The new UDT code in mssql-tds had unit tests only. The ODBC e2e suite does
exercise it against a live server on both driver legs, but only through what
ODBC exposes: it cannot reach a caller that builds an RpcParameter itself -
which is how the JS and Python bindings use this crate - and it cannot reach
the send preflight's rejection arms at all, because SQLBindParameter refuses
those inputs before mssql-tds is called.
Five cases in tests/test_udt_parameters.rs, using hierarchyid so no
CREATE ASSEMBLY is needed: the payload round trip, a NULL UDT still carrying
its name, the two-part schema-qualified name, and both preflight rejections -
an empty type name and a 256-unit name part. The rejection cases assert the
connection still serves the next query, which is the half a unit test on
`validate` cannot show: the preflight exists so locally-invalid input fails
before any bytes reach the wire, rather than half-sending an RPC that needs a
cancel-and-drain.
Also corrects two comments on `TvpTableData::validate`. It has no caller
outside this module's tests - `serialize_table` validates the type name and
then writes the rows - so the UDT-column arm added here states what a TVP
should reject rather than preventing it, and the comment said otherwise. That
predates this branch: validate was already test-only at the merge base.
Wiring it in means a second pass over every row ahead of the writing pass, so
it is recorded as a tracked gap rather than changed in passing.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Measure the UDT ColumnSize ceiling on both legs
The ceiling added in ae5e308f was a new client-side rejection with no test
driving it through SQLBindParameter, and every UDT case in the e2e suite binds
ColumnSize 0, so neither leg exercised the boundary. That matters because
mssql-python's documented setinputsizes([(SQL_SS_UDT, 8000, 0)]) sits exactly
on the inclusive edge.
Adds a both-leg e2e asserting HY104 at 8001 and a successful bind at 8000 and
at 0, so the parity claim is measured rather than source-read, plus a
bind-level unit test covering the same three points through SQLBindParameter
rather than through the predicate alone.
Both use literals instead of SQL_PREC_UDT. Written first in terms of the
constant, the mutation that motivated the test - shrinking SQL_PREC_UDT by one
- left both tests green, because the assertions moved with the constant they
were meant to pin. With literals the same mutation fails both.
Also names the zero after the public contract: it is SQL_SS_LENGTH_UNLIMITED
(msodbcsql.h:564), the only way to bind a UDT larger than the ceiling, not
merely a permissive zero by analogy with varchar(max).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Retract a wrong claim about TVP validation not being on the send path
8d9b12da relabelled `TvpTableData::validate` as dead code and rewrote two
comments around it to say a UDT column in a TVP still writes malformed bytes.
That is wrong. `serialize_table` calls `data.validate()?` in its `Some(data)`
arm (sqltypes.rs:1245), before any column metadata or row bytes are written,
so the guard does prevent the malformed output it describes.
Restore the original meaning and record the boundary that is actually true:
the check runs after the TVP type byte and three-part name, so a rejection
keeps the metadata and rows off the wire but leaves that prefix in the writer -
already sent if it overflowed a packet. That is the partial-request shape
`validate_parameters` exists to avoid for scalar parameters, and it is worth
stating rather than implying a full preflight.
Also corrects the new integration test's own doc, which said the JS and Python
bindings reach this path. They do not yet: `mssql-py-core` refuses a UDT
parameter (types.rs:441) and `mssql-js` has no UDT path. The justification that
survives is narrower and checked - `SqlType`, `UdtTypeName` and `RpcParameter`
are public mssql-tds surface, and the preflight's rejection arms are
unreachable from the ODBC suite because SQLBindParameter refuses an empty or
overlong type name before mssql-tds is called.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Bound a UDT payload by its declared ColumnSize
The binary-to-UDT arm forwarded the whole buffer regardless of the declaration,
so a bounded binding such as ColumnSize 3 accepted and sent a four-byte
payload. msodbcsql raises 22001 when cbData > min(cbColDef, SQL_PREC_UDT)
(sqlcfunc.cpp:2681-2696), and skips the check entirely when ColumnSize is
SQL_SS_LENGTH_UNLIMITED - which is what fIsVarMax means for a UDT
(sqlcfunc.cpp:2577-2584).
Reject rather than trim, which is the part worth reading the neighbouring arm
for: SQL_BINARY/VARBINARY/LONGVARBINARY call CheckTrailingZeros and trim a
zero-only overflow (sqlcfunc.cpp:2606-2616), the behaviour trim_zero_overflow
mirrors here. The UDT arm has no such call, so reusing that helper would have
accepted a zero-padded payload the reference refuses. Both halves are asserted.
Unit coverage over the declaration edge, one byte past it, the zero-padded
overflow and the unlimited case; mutation-verified by deleting the guard. Plus
a both-leg e2e asserting 22001 for a payload past a bounded ColumnSize and a
successful round trip for the same payload at SQL_SS_LENGTH_UNLIMITED, so the
reference side stays measured rather than source-read.
This is the neighbour of the ceiling added in f507f273: that bounded the
declaration, this bounds the payload against it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Measure a buffered UDT as its chunks arrive
The ColumnSize bound added in fa598754 caught a data-at-execution UDT only at
close. `dae_length_limit` returned None for SQL_C_BINARY -> SQL_SS_UDT because
`same_unit` recognised SqlFamily::Binary alone, so nothing bounded the value
per chunk: a bounded UDT could accumulate arbitrarily more than its
at-most-8000-byte declaration across SQLPutData calls, and the 22001 arrived on
SQLParamData rather than on the call that overflowed. The rest of the DAE path
deliberately reports on the overflowing call, mirroring msodbcsql's SQLPutData
arms, so this was inconsistent with its own neighbours as well as the
reference.
Treat a UDT as byte-measurable: a UDT payload is bytes passed through
untouched, which is the same correspondence SqlFamily::Binary has. The bound
follows the materialized arm - min(ColumnSize, SQL_PREC_UDT), unbounded at
SQL_SS_LENGTH_UNLIMITED.
Overflow policy is now explicit rather than implied by pad_unit. Every
character and binary declaration keeps trimming an all-padding overflow; a UDT
does not, because the reference's SQL_UDT_MAPPED arm has no CheckTrailingZeros
call (sqlcfunc.cpp:2681-2696 against the varbinary arm at :2606-2616). Reusing
the binary policy would have accepted a zero-padded payload msodbcsql refuses.
Regression covers the accumulated-total bound, the first overflowing chunk, the
zero-padded overflow and the unlimited case. Mutation-verified twice: dropping
the Udt arm from same_unit, and letting a UDT trim padding, each fail it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Use a multi-byte payload in the bounded-UDT e2e
AUdtPayloadPastABoundedColumnSizeIsRefused parsed a single-level hierarchyid,
which serializes to exactly one byte, so its guard assertion failed and took
the Windows and Linux ARM legs red before either half of the test ran.
The guard was right and the payload was wrong. With a one-byte payload
`payload.size() - 1` is zero, and zero is SQL_SS_LENGTH_UNLIMITED - so even
without the assertion the "bounded" case would have bound an unlimited
parameter and proved nothing. Every other hierarchyid in this suite is
single-level, so there was no precedent to borrow the size from.
Use a ten-level path, keep the assertion as the guard the reviewer asked for,
and give it a message so a future mismatch reports what is wrong rather than
`1 vs 1`.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* State the evidence level on the two new UDT ColumnSize parity claims
Both new e2e cases asserted a reference-leg SQLSTATE off a source reading, and
one of them said "measured on both legs rather than read from source" - which
is the opposite of what backs it. Neither records a SQL_DRIVER_VER or tested
build, and the PR body's both-legs run is dated 2026-09-25, before either test
existed, so nothing in the PR measures them.
Section 2.1 of the ODBC instructions requires a source citation and a
measurement for a behavioral parity claim, and admits a source citation alone
only when the entry states its evidence level and names the measurement that
would close it. These now do that: each carries an EVIDENCE paragraph naming
the --compare-with-msodbcsql run that would close it, matching how
BinaryVariantPayloadPastTheCeilingIsRefused records its own.
The unconditional assertion stays rather than becoming an ODBC_TEST_TARGET
split, and the comment says why: PR validation runs this suite on both legs, so
a wrong expectation fails the reference leg instead of shipping. If that run
shows msodbcsql answering differently, it is a measured divergence needing the
split and a registry entry, not a changed expectation.
Swept the rest of the PR's added measurement claims. The dated ones belong to
tests that existed for the 2026-09-25 run; the remaining "measured on both
legs" in this file predates the branch.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Let a buffered UDT trim zero padding, matching the reference's DAE arm
9b1bbb8d gave the DAE limit a hard-reject overflow policy for a UDT, reasoning
by symmetry with the materialized arm. msodbcsql is not symmetric here.
`ValidatePutDataLength`'s SQL_C_BINARY case tests IsSQLBinary, which includes
SQL_UDT_MAPPED (sqlcprot.h:1294), and on overflow calls CheckTrailingZeros and
shortens cbValue rather than failing (sqlccmd.cpp:11192-11215) - where
ParamToSQLType's SQL_UDT_MAPPED arm has no such call (sqlcfunc.cpp:2681-2696).
So a bounded DAE value with a zero-only overflow is accepted there and was
refused here.
Drop the trims_padding field rather than set it true everywhere: with the UDT
case corrected no caller wants the strict policy, and a field that is always
true is dead weight that invites the wrong conclusion about which paths differ.
The asymmetry is now recorded on both sides - on pad_unit, and on the
materialized arm - so neither gets "fixed" later to agree with the other.
The materialized hard reject from fa598754 is unchanged and still correct.
Evidence: source reading only on both arms; a retail measurement recording
SQL_DRIVER_VER would close it. Regression keeps the per-chunk bound, the
accumulated-total bound and the unlimited case, and now asserts the trim;
mutation-verified by dropping Udt from same_unit.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Assert the zero-padded UDT overflow on both legs, and pin the 8000 clamp
Two gaps, both of which the bar this PR sets should have caught.
The e2e comment said a zero-padded overflow is "asserted here" while the body
had only a non-zero overflow and the unlimited round trip. The zero case was
covered by a unit test that never runs on the msodbcsql leg - and it is
precisely the case read against the reference's *neighbouring* varbinary arm
rather than the UDT arm itself, so it is the least certain reading and the one
most worth measuring. Adds a third phase binding the payload plus a trailing
0x00 at ColumnSize = payload length, expecting 22001 on both legs.
The min(column_size, SQL_PREC_UDT) clamp on both the materialized and DAE arms
was unpinned: removing it from both left the whole crate green. It is not dead
code - SQLBindParameter caps ColumnSize at 8000, but SQL_DESC_LENGTH is
writable on an IPD (classify_field) and set_desc_field stores it without
calling parameter_column_size_is_valid, so bind at 0, set SQL_DESC_LENGTH to
20000, and the clamp is the only thing holding the 8000 ceiling. Covered now on
both arms; that mutation fails them.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Theekshna Kotian <tkotian@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Work Item / Issue Reference
Summary
Resolve explicitly declared SQL_SS_UDT parameter identities through
SQLDescribeParam before binding, without changing the public API.
streamed executemany rows.
Validation
Windows x64, Python 3.13.15, SQL Server 2019:
assertion with passwordless authentication and a Windows path-length limit.
Limitations
Temporary tables and table variables may not support metadata discovery.
Custom CLR assembly UDTs and cross-platform behavior remain unverified.