-
Notifications
You must be signed in to change notification settings - Fork 289
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
Conversation
Breaks support for SQL Server 2008 (I believe) fixes dotnet#5
What happened to the CI builds? |
No changes done on our end, seems like another DevOps issue. Will open a ticket. |
Let me know if you want me to close and resubmit |
No, for SQL 2005. That is SQL 2008 which has introduced (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.) |
Both SQL 2005 and 2008 are now out of support cycle, so I think we're good :) |
Pipeline unblocked! |
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. |
Thanks for the observations, looking forward to see additional testing!
Could you explain the technical term "hobbled" at bit more? 😄 |
I submitted a PR to your branch: ErikEJ#1
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
Branch merged |
* 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
* 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
* 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
Breaks support for SQL Server 2005 (I believe)
fixes #5