Fix | Write DateOnly instances to sql_variant columns of table-valued parameters as date values - #4439
Conversation
Instances of DateOnly stored within a SqlDataRecord field of type Variant will now be sent as dates, not datetimes. By extension: sending DateOnly instances with values outside the acceptable range for a datetime will no longer throw overflow exceptions.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4439 +/- ##
==========================================
- Coverage 65.83% 62.64% -3.20%
==========================================
Files 287 283 -4
Lines 43763 66949 +23186
==========================================
+ Hits 28812 41938 +13126
- Misses 14951 25011 +10060
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
benrr101
left a comment
There was a problem hiding this comment.
Small change but seems pretty important!
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness issue when sending DateOnly values into sql_variant columns in table-valued parameters (TVPs): the value was being transported as datetime rather than date, which can overflow for DateOnly min/max values and yields an incorrect base type.
Changes:
- Updates TVP/SMI value-setting logic so
DateOnlywritten tosql_variantis transported with base typedate(modern .NET only). - Relaxes variant metadata assertions to allow
SqlDbType.Datewhere variant-type metadata is tracked. - Updates manual variant tests to remove the prior overflow/“datetime” exemptions now that the scenario should succeed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/DateTimeVariantTests.cs | Removes expected overflow exceptions / base-type overrides for DateOnly in sql_variant TVP scenarios. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs | Routes DateOnly through date-specific variant metadata handling for TVP writes; also adjusts sql-value retrieval logic for SqlDbType.Date. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlRecordBuffer.cs | Allows SqlDbType.Date in the debug assertion for variant metadata. |
Ensure that SqlDataRecord.GetSqlValue doesn't return a DateOnly instance. Apply the same pattern as SqlDataReader - directly expose the original variant type metadata, and base the decision on that.
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Both are user-facing fixes merged into the preview3 milestone that were not yet captured in the release notes or CHANGELOG: - #4439: DateOnly values in sql_variant TVP columns were sent as datetime instead of date, overflowing for values outside the datetime range. - #4445: ServerCertificate pin validation was skipped when the platform reported no TLS policy errors, and an unloadable certificate file fell back to host-name validation instead of failing closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1
* Add release notes for 7.1.0-preview3 Adds release notes for Microsoft.Data.SqlClient 7.1.0-preview3 and its four aligned companion packages, updates the per-version README index tables, and adds the corresponding CHANGELOG entry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7425f3e-f6b8-439b-b74f-e25f8406fbf6 * Capture remaining closed 7.1.0-preview3 milestone items in release notes Adds coverage for milestone items that closed after the initial draft: - #4529 leaked-connection reclamation in ChannelDbConnectionPool - #4535 SqlBulkCopy graph column alias mapping bypass - #4521 / #4496 Entra ID tenant parsing for multi-segment STSURL authorities - #4540 async key store provider APIs in the AzureKeyVaultProvider Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c485150e-f46e-4c55-8d06-e0491a10e7e8 * Add #4439 and #4445 to 7.1.0-preview3 release notes Both are user-facing fixes merged into the preview3 milestone that were not yet captured in the release notes or CHANGELOG: - #4439: DateOnly values in sql_variant TVP columns were sent as datetime instead of date, overflowing for values outside the datetime range. - #4445: ServerCertificate pin validation was skipped when the platform reported no TLS policy errors, and an unloadable certificate file fell back to host-name validation instead of failing closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1 * Add #4474 to 7.1.0-preview3 cross-platform build notes PR #4474 (Remove OS-Specific Builds) removed OS-specific build targets and output paths, and rewrote the MDS nuspec to source a single OS-agnostic assembly for both the win and unix runtime folders. Verified the nuspec change is src-path-only: all 52 file entries have identical target= values before and after, so the produced package layout is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1 * Clarify #4536 allocation fix applies to the default async read path Addresses review feedback on PR #4565: the PacketData node-reuse fix is not gated behind UseCompatibilityAsyncBehaviour or any other AppContext switch, and the perf validation was measured on the default path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1 * Scope preview3 release notes to customer-facing changes Remove entries with no customer-visible effect, and reframe entries that led with implementation detail rather than customer impact. Removed: - #3862 ConnectionCapabilities consolidation - internal refactor; the GetSchema("DataTypes") fix it enables is deferred to a later PR. - #3700/#3741 SSRP scaffolding - adds no parsing code and no behavior change. - #4517 CodeQL findings - PKCS#1 half is suppression comments for declared false positives; the SHA-1 removal is a no-op in practice. - #4421 Extensions.Azure APIScan remediation - no public API change and managed-identity behavior preserved exactly; the removed assignment was already a no-op. Extensions.Azure now has no Changed section. Promoted: - #4504 counter fixes moved from a pool V2 sub-bullet into Fixed. These affect the default pool that customers use without opting in, and the previous text understated them as two fixes limited to Count. Reframed: - Cross-platform build collapsed to the one customer-visible outcome (trimming on Linux/macOS); package contents were always unchanged. - #4528 now leads with the allocation regression rather than call-site count. - AKV cache fixes now lead with symptoms (unbounded signature cache growth, duplicate CryptographyClient per key) rather than GetOrCreate/GetOrAdd. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7425f3e-f6b8-439b-b74f-e25f8406fbf6 Copilot-Session: c485150e-f46e-4c55-8d06-e0491a10e7e8 Copilot-Session: 8c691319-4c66-40f6-a88e-b453937052f1
Description
This builds on #4294, and handles the only other issue I saw while investigating #3934.
If a user-defined table type exists with a column of type
sql_variant, client applications can insert aDateOnlyinstance into this. It's supposed to be sent with a type ofdate, but is actually sent as adatetime.Besides a point around correctness, it presents an issue when sending
DateOnlyinstances which are valid values fordatebut not fordatetime: these values overflow.This PR fixes the issue, transporting all
DateOnlyinstances with thedatetype on modern .NET. On .NET Framework, there's noDateOnlytype - and thus there's no way for the client to encapsulate adatevalue within asql_variantcolumn.@ErikEJ, this covers the only other way to write
DateOnlyinstances to SQL Server and should hopefully completely close #3934. SqlClient will continue to readDateTimeinstances by default, but this is needed for backwards compatibility purposes.NB: similar issues also exist with
TimeOnlyinstances. TdsParser assumes that onlyTimeSpaninstances will be written as times, so there are failures to cast in a few different places.Issues
Builds on #4294. Possible resolution to #3934.
Testing
#4294 added the relevant tests, with an exemption carved out for this use case. I've removed the exemption and verified that it covers the test case.