UpForGrabs: Insert fails if table has a DATETIME property as part of the key



please looking at my stackoverflow question for detailed explanation.


Closed Dec 12, 2016 at 10:34 PM by RoMiller
EF Team Triage: We are transitioning this project to GitHub (https://github.com/aspnet/EntityFramework6). As part of this transition we are bulk closing a large number of issues in order that our new issue tracker will accurately reflect the work that our team is planning to complete on the EF6.x code base.

Moving forwards, our team will be fixing bugs, implementing small improvements, and accepting community contributions to the EF6.x code base. Larger feature work and innovation will happen in the EF Core code base (https://github.com/aspnet/EntityFramework). Closing a feature request in the EF6.x project does not exclude us implementing the feature in EF Core. In fact, a number of popular feature requests for EF have already been implemented in EF Core (alternate keys, batching in SaveChanges, etc.).

This is a bulk message to indicate that this issue was closed and not ported to the new issue tracker. The reasons for not porting this particular issue to GitHub may include:
  • It was a bug report that does not contain sufficient information for us to be able to reproduce it
  • It was a question, but sufficient time has passed that it's not clear that taking the time to answer it would provide value to the person who asked it
  • It is a feature request that we are realistically not going to implement on the EF6.x code base
    Although this issue was not ported, you may still re-open it in the new issue tracker for our team to reconsider (https://github.com/aspnet/EntityFramework6/issues). We will no longer be monitoring this issue tracker for comments, so please do not reply here.


divega wrote Apr 1, 2014 at 11:02 PM

Note for triage (edited):

Seems to be a legitimate issue although not a regression from either EF5 or EF6.

The root of the problem is a mismatch between a column type and the type of the corresponding parameter used in the commands we generate in the update pipeline for certain types and facets.

In this particular case a property of type System.DateTime is mapped to a DateTime column and the parameter generated for the INSERT command is of type SqlDbType.DateTime2.

The property is assigned an arbitrary DateTime value, e.g. DateTime.Now.

When we execute the INSERT statement the value stored in the DateTime2 parameter @0 will be silently truncated to make it fit in the StartDate column of type DateTime:
INSERT [dbo].[CalendarItems]([StartDate]) VALUES (@0) 
In the subsequent query to evaluate the number of rows affected, SQL Server will transparently upcast the DateTime column to DateTime2 to perform the comparison against the parameter and the values will not match because of the precision loss:
SELECT [Id] FROM [dbo].[CalendarItems] WHERE @@ROWCOUNT > 0 AND [StartDate] = @0
In the end we will receive a rows affected value of 0 and will throw an exception saying that the insert statement affected an unexpected number of rows.

I found that in In SqlProviderServices.CreateSqlParameter() we decide the type of the parameter only based on a C-space type usage in the insert command tree. We generate a parameter of SqlDbType.DateTime2 if supported by the database version for all properties of type DateTime. Looks like an oversight in the design because in some cases the store type might be different and in this particular case it causes loss of precision.
That said I haven’t found any other cases in which this can happen. Although bug #2036 (which is about DateTimeOffset properties as concurrency tokens) seems related I haven’t been able to confirm.

We can consider fixing it if we believe the cost is reasonable and we feel comfortable that it won't cause unintended breaking changes.
There are a few workarounds for this particular case:
  1. Use a surrogate key instead of a DateTime column as a key
  2. Make the type of the column DateTime2 (you can do this easily with a custom convention now)
As a side note, I investigated a few other concerns I had but they turned out to be negative:
  1. This seem only for some types for which we have a precision facet, e.g. it happens for DateTime and apparently for DateTimeOffset, but it doesn't happen for decimal (for keys of type decimal we generate parameters with the same precision as the column).
  2. This does not seem to affect other facets, e.g. I confirmed for string keys that are defined as non-Unicode we produce the right non-Unicode parameter, even in the update pipeline.

divega wrote Apr 1, 2014 at 11:15 PM

An additional workaround is to truncate the value of the System.DateTime property before saving changes.

scda0001 wrote Apr 1, 2014 at 11:25 PM

yes, truncating the time part is a working solution. But this should be only a workaround. This issue has to be fixed for future releases. I cannot see a breaking change (as far as I can evaluate this).

scda0001 wrote Apr 2, 2014 at 10:15 AM

Can this issue have any side effects with optimistic concurrency pattern too!? (I don't tested it yet)

If you don't use any timestamp column for concurrency check and instead compare all table columns, I would assume that this raise the same issue.

divega wrote Apr 2, 2014 at 11:39 AM

I believe it won"t have an effect on concurrency although I haven't tested all my assumptions: in concurrency control the value that needs to match is the original value if the column after a roundtrip. The original value has already been stored in a DATETIME column before and therefore it is already truncated. A DATETIME2 parameter should have no problem holding the value with a lesser precision and the comparison should match.

I suspect in #2036 the root cause is the opposite, i.e. a parameter cannot hold the whole precision of the column.

scda0001 wrote Apr 2, 2014 at 2:40 PM

You are right. For concurrency check original values are used, so no truncation occured.

RoMiller wrote Apr 3, 2014 at 9:24 PM

EF Team Triage: We agree that we should fix this issue. Given that this is not a regression from a previous release we are not going to take a fix in the 6.1.1 patch release. We're moving it to the Future release to reconsider for the next release. Also marking as UpForGrabs in case someone outside of the EF team wants to contribute the fix.