Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting the value DbParameter.DbType to DbType.Time property fails after setting the Value property #5

Closed
mikependon opened this issue May 9, 2019 · 9 comments · Fixed by #269
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain. 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label

Comments

@mikependon
Copy link

Describe the bug

Setting the value DbParameter.DbType to DbType.Time property is failing after setting the Value property

No exception and not a breaking issue.

To reproduce

	using Microsoft.Data.SqlClient;
	using System;
	using System.Data;

	namespace DbParameterDbTypeTimeTest
	{
		class Program
		{
			static void Main(string[] args)
			{
				Console.WriteLine("Hello World!");
				TestDbTypeTimeAssignment();
				Console.ReadLine();
			}

			static void TestDbTypeTimeAssignment()
			{
				using (var connection = new SqlConnection(""))
				{
					using (var command = connection.CreateCommand())
					{
						var parameter = command.CreateParameter();
						parameter.ParameterName = "ParameterName";

						// Here I first set the value of the parameter to any TimeStamp
						parameter.Value = DateTime.UtcNow.TimeOfDay;

						// After I assign the value at line 26, the DbType property is "AUTOMATICALLY"
						// set to DbType.Time.
						// The issue is below, I have an explicit call on my ORM to set the DbParameter.DbType
						// value to DbType.Time, and the result afterwards is DbType.DateTime
						parameter.DbType = DbType.Time;

						command.Parameters.Add(parameter);
					}
				}
			}
		}
	}

Expected behavior

I would expect that after setting the DbParameter.DbType property to DbType.Time, the the value of the DbParameter.DbType property should be DbType.Time (not DbType.DateTime).

Further technical details

#region Assembly Microsoft.Data.SqlClient, Version=1.0.19128.1, Culture=neutral, PublicKeyToken=23ec7fc2d6eaa4a5
// C:\Users\MIPEN.nuget\packages\microsoft.data.sqlclient\1.0.19128.1-preview\ref\netcoreapp2.1\Microsoft.Data.SqlClient.dll
#endregion

Additional context
Explained in the code above.

@David-Engel
Copy link
Contributor

This appears to be done for backwards compatibility and is consistent across .NET Framework and Core. You can see the comment in the code at https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlParameter.cs#L326

GetMetaTypeFromDbType returns DateTime for Date, Time, and DateTime:
https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlEnums.cs#L229

Since it doesn't hurt anything, I image it was to ensure compatibility with pre-SQL 2008 servers, which did not have date or time data types.

@mikependon
Copy link
Author

mikependon commented May 10, 2019

@David-Engel , ok. It is failing actually (I can give the codes with DB Schema if you would like to extend the investigation, so just please let me know).

But, since it is purposely made and the developers are aware, then it is there is no point of letting this ticket open. It just happened that when I rewrite some of my DBCommand parameter assignments to compiled-expressions (for performance purposes), then few of my Time and Date related integration tests were failed. So I have to handle it separately as it is really failing when calling the ExecuteNonQuery().

Thanks for the clarity.

@David-Engel
Copy link
Contributor

Yes, if you have a scenario it is breaking, please do share the details. Since SQL Server 2008 and older are no longer supported, I can see us considering changing the functionality.

@mikependon
Copy link
Author

Wow, this is great. I will upload it here soon or maybe link a reference to a project with everything you need.

mikependon added a commit to mikependon/SqlClient that referenced this issue May 10, 2019
@mikependon
Copy link
Author

The project solution is located here: https://github.com/mikependon/SqlClient/tree/master/Issue5

Note: I replicated the issue #7 at NetFramework, so please use the NetCore project instead to replicate this issue.

@David-Engel David-Engel added the 💡 Enhancement Issues that are feature requests for the drivers we maintain. label May 20, 2019
@David-Engel David-Engel added this to the 1.1.0 milestone May 20, 2019
@fredericDelaporte
Copy link

Since it doesn't hurt anything, I image it was to ensure compatibility with pre-SQL 2008 servers, which did not have date or time data types.

It hurts. Setting a parameter to DbType.Date then setting a value below Sql DateTime minvalue (year 1753) causes the execution of the command to fail.

[Test]
public void WhereOnDateParameter()
{
    using (var cnx = new SqlConnection(_connStr))
    {
        cnx.Open();
        using (var cmd = new SqlCommand(@"select count(*) from Types2008 where [Date] = @d", cnx))
        {
            var param = cmd.CreateParameter();
            param.ParameterName = "d";
            param.DbType = DbType.Date;
            param.Value = DateTime.Parse("1500-10-26");
            cmd.Parameters.Add(param);
            Assert.That(() => cmd.ExecuteScalar(), Throws.Nothing);
        }
    }
}

This throws System.Data.SqlTypes.SqlTypeException: SqlDateTime overflow. Must be between 1/1/1753 12:00:00 AM and 12/31/9999 11:59:59 PM..
[Date] column is of sql type date.

There is a work-around: setting the SqlDbType instead. But being bitten by this old pre 2008 legacy is unexpected.

Yes it is consistent with .Net Framework System.Data.SqlClient, but since Microsoft.Data.SqlClient is about not getting crippled with old legacies as far as I understand it, it should not keep this behavior.

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 15, 2019

@cheenamalhotra Is this "up for grabs" ? Considering giving it a go.

@cheenamalhotra
Copy link
Member

Sure! It would be nice to have this one fixed if anyone is willing to contribute!

@cheenamalhotra cheenamalhotra added the 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label label Oct 15, 2019
ErikEJ added a commit to ErikEJ/SqlClient that referenced this issue Oct 16, 2019
Breaks support for SQL Server 2008 (I believe)

fixes dotnet#5
@David-Engel
Copy link
Contributor

Related issues for reference:
https://github.com/dotnet/corefx/issues/21279
dotnet/docs#4474

David-Engel pushed a commit that referenced this issue Nov 4, 2019
* Get and set expected value for DdType.Date and DbType.Time

Breaks support for SQL Server 2008 (I believe)

fixes #5

Extend DateTimeTest.ReaderParameterTest to cover DbType parameters
@David-Engel David-Engel modified the milestones: 1.1.0, 1.1.0-preview2 Nov 4, 2019
@David-Engel David-Engel changed the title Setting the value DbParameter.DbType to DbType.Time property is failing after setting the Value property Setting the value DbParameter.DbType to DbType.Time property fails after setting the Value property Nov 4, 2019
karinazhou pushed a commit that referenced this issue Nov 4, 2019
* Get and set expected value for DdType.Date and DbType.Time

Breaks support for SQL Server 2008 (I believe)

fixes #5

Extend DateTimeTest.ReaderParameterTest to cover DbType parameters
yukiwongky pushed a commit to yukiwongky/SqlClient that referenced this issue Nov 5, 2019
* Get and set expected value for DdType.Date and DbType.Time

Breaks support for SQL Server 2008 (I believe)

fixes dotnet#5

Extend DateTimeTest.ReaderParameterTest to cover DbType parameters
yukiwongky pushed a commit to yukiwongky/SqlClient that referenced this issue Nov 5, 2019
* Get and set expected value for DdType.Date and DbType.Time

Breaks support for SQL Server 2008 (I believe)

fixes dotnet#5

Extend DateTimeTest.ReaderParameterTest to cover DbType parameters
karinazhou added a commit that referenced this issue Mar 13, 2021
* Add event counters

* Add support netstandard 2.1 & fix the conflict in event source

* Support new event source types & add the test unit

* Remove supporting obsolete types

* fix unit test

* Add snippet sample code

* Address comments

* Fix minor typo (#3)

* Reformatting counter methods

* Fix minor typo

* Removed IsEnabled condition and reformatted counter methods

* Unit tests for Microsoft.Data.SqlClient.SqlClientEventSource (#2)

* Implemented tests for Microsoft.Data.SqlClient.SqlClientEventSource

* Updated the EventCounter test to reflect the recent changes in the code

* Working on EventCounter tests access event counters through reflection

* Updated the EventCounterTest to use reflection

* Fixing dangling SqlConnection's left in tests

* EventCountersTest now checks hard/soft connects/disconnects counters

* Reverted the DataTestUtility changes

* Reverted using statements to the old-style in tests

* Reverted the ConnectionPoolTest.ReclaimEmancipatedOnOpenTest()

* Reverted using statements to the old-style in tests

* Reverted using statements to the old-style in tests

* Rewrite the EventCounterTest assertions not to conflict with other tests

* Code review cleanup

* Add more tests (#5)

Added EventCounter_ReclaimedConnectionsCounter_Functional & EventCounter_ConnectionPoolGroupsCounter_Functional tests.

* Address comments

Co-authored-by: Davoud Eshtehari <David.esh@gmail.com>
Co-authored-by: Davoud Eshtehari <v-daesht@microsoft.com>
Co-authored-by: Karina Zhou <v-jizho2@microsoft.com>
Co-authored-by: Nikita Kobzev <win.accuracy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain. 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants