Skip to content

Feature | Write TimeOnly instances as time values - #4604

Open
edwardneal wants to merge 2 commits into
dotnet:mainfrom
edwardneal:feat/timeonly-variant
Open

edwardneal wants to merge 2 commits into
dotnet:mainfrom
edwardneal:feat/timeonly-variant

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Description

This PR follows up the earlier #4439. At the conclusion of that PR, DateOnly instances were always written as a date type. This one now raises TimeOnly to parity with DateOnly in three cases:

  1. SqlBulkCopy, based upon a TimeOnly instance within a DataRow or a DataTable.
  2. A simple SqlParameter of SqlDbType Variant containing a TimeOnly instance.
  3. When run in a Debug configuration, a user-defined table type parameter, specified via a SqlParameter with its value being an array of SqlDataRecord objects, all with a column of SqlDbType Variant and a row containing a TimeOnly instance.

I also noticed while in the area that although DateTimeVariantTests was performing a number of validity checks against invalid time values, checks against a valid time value were missing. I've added these.

Notes

  • This only opens the DataRow and DataTable paths in SqlBulkCopy; the SqlDataReader path was already open. I opened this path to the other two situations by making an explicit type check in TdsParser.WriteSqlVariantDataRowValue.
  • The simpler case (a Variant-typed SqlParameter) is opened by the change to TdsParser.WriteSqlVariantValue.
  • When run in Debug mode, an assertion was tripping based upon the contents of s_canAccessSetterDirectly. I added a mapping confirming that a variant type can contain a TimeOnly instance.

Issues

None. Repeats the style of fix in #4439 for TimeOnly instances.

Testing

New test cases have been added which verify functionality, existing tests continue to pass.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

4 participants