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

Get and set expected value for DdType.Date and DbType.Time #269

Merged
merged 5 commits into from
Nov 4, 2019

Conversation

ErikEJ
Copy link
Contributor

@ErikEJ ErikEJ commented Oct 16, 2019

Breaks support for SQL Server 2005 (I believe)

fixes #5

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 16, 2019

What happened to the CI builds?

@cheenamalhotra
Copy link
Member

No changes done on our end, seems like another DevOps issue. Will open a ticket.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 16, 2019

Let me know if you want me to close and resubmit

@fredericDelaporte
Copy link

fredericDelaporte commented Oct 17, 2019

Breaks support for SQL Server 2008 (I believe)

No, for SQL 2005. That is SQL 2008 which has introduced date and time types into SQL-Server. So SQL 2008 should be fine with the change.

(I was speaking of a "pre 2008 legacy" for meaning a legacy of SQL-Server versions prior to 2008, aka SQL-Server 2005 or older.)

@cheenamalhotra
Copy link
Member

Both SQL 2005 and 2008 are now out of support cycle, so I think we're good :)
@ErikEJ the pipelines seem to work now, please give it a try by checking in some change.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 18, 2019

Pipeline unblocked!

@cheenamalhotra cheenamalhotra added this to the 1.1.0-preview2 milestone Oct 18, 2019
@David-Engel David-Engel self-requested a review November 1, 2019 22:52
@David-Engel
Copy link
Contributor

I've been trying to think about the behavior change around this to see if we would be introducing any subtle breaking changes to existing applications.

DateTimeTest.ReaderParameterTest has good coverage comparing the interactions of SqlDbType parameters against the DB, so I extended it to perform the same tests using DbType. What I found was that before this change, the "compatibility" code did enable passing DbType date and time data into and out of columns for pre SQL Server 2008 sources. But it also hobbled the use of DbType date and time columns in where parameters post 2008. (SqlDbType data is unaffected by this change.) It seems like it's just plain broken trying to use DbType.Date or DbType.Time as a parameter in a where clause.

So for an application to work around the above, if they kept using DbType (and not the SQL-specific SqlDbType), and they wanted to still use Date or Time columns in the database, I can't think of how they could have coded it. If they used SqlDbType, this change will not break them. If they used a different data type in the database, this change will not break them. I think this change simply fixes broken scenarios for 2008+.

I thought I'd add my analysis in case I'm wrong and this ends up breaking something.

I'll submit my DateTimeTest changes to your branch to be included in this PR.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Nov 2, 2019

Thanks for the observations, looking forward to see additional testing!

But it also hobbled the use of DbType date and time columns in where parameters post 2008.

Could you explain the technical term "hobbled" at bit more? 😄

@David-Engel
Copy link
Contributor

David-Engel commented Nov 4, 2019

Thanks for the observations, looking forward to see additional testing!

I submitted a PR to your branch: ErikEJ#1

But it also hobbled the use of DbType date and time columns in where parameters post 2008.

Could you explain the technical term "hobbled" at bit more? 😄

It seems like the backward compatibility changes that you are removing broke the use of DbType.Date and DbType.Time parameters against 2008+ SQL Servers in the context of a where clause. The database will not implicitly cast between DateTime and Date/Time for comparison. I put comments in the test changes in my PR which show which lines would fail without the removal of the back-compat code.

If you merge my PR into your branch, it should show up in this PR. You probably also need to merge the latest from master here into your branch, too.

Extend DateTimeTest.ReaderParameterTest to cover DbType parameters
@ErikEJ
Copy link
Contributor Author

ErikEJ commented Nov 4, 2019

Branch merged

@David-Engel David-Engel merged commit b1d4b96 into dotnet:master Nov 4, 2019
karinazhou pushed a commit that referenced this pull request 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 pull request 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 pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting the value DbParameter.DbType to DbType.Time property fails after setting the Value property
4 participants