-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
SQL Server: Switch IDENTITY_INSERT on (and then off) when explicit values are specified for an identity column #703
Comments
I personally think it is fine to let the database server throw. At least the users will know that they are doing something wrong instead of having their specified values being ignored. |
With the implementation we landed on you will get an exception if you try to specify explicit values... but it is possible to make it work with a bit of extra code. We could be smart about this in our provider and actually switch IDENTITY_INESRT on then off when we know that we're going to try and insert and explicit value. |
We should look at what was done for seed data and see if this can be generalized. However, even then it is not clear that the behavior should be switched on by default. |
See #11586 for an API proposal. |
Tests run under a disposable localdb database, with a GUID name. This ensure that SQL constraints will be respected. Tests run in 24 sec now. User now has a Guid identity column in order to avoir IDENTITY_INSERT ON/OFF on localdb database (cf dotnet/efcore#703) I have used a TestFixture to encapsulate the creation/deletion of the database NB : Domain tests could be converted to use InMemoryStorage for faster run
Tests run under a disposable localdb database, with a GUID name. This ensure that SQL constraints will be respected. Tests run in 24 sec now. User now has a Guid identity column in order to avoir IDENTITY_INSERT ON/OFF on localdb database (cf dotnet/efcore#703) I have used a TestFixture to encapsulate the creation/deletion of the database NB : Domain tests could be converted to use InMemoryStorage for faster run
Tests run under a disposable localdb database, with a GUID name. This ensure that SQL constraints will be respected. Tests run in 24 sec now. User now has a Guid identity column in order to avoir IDENTITY_INSERT ON/OFF on localdb database (cf dotnet/efcore#703) I have used a TestFixture to encapsulate the creation/deletion of the database NB : Domain tests could be converted to use InMemoryStorage for faster run
Consider how this would interact with #13575 |
I found this issue linked off of https://docs.microsoft.com/en-us/ef/core/saving/explicit-values-generated-properties#explicit-values-into-sql-server-identity-columns. Not having a full grasp of what "temporary value" means in the larger context, I'll just address automatically setting IDENTITY_INSERT ON/OFF. While I can definitely see this being useful for seed data scenarios in testing (since Sql Server only allows IDENTITY_INSERT ON for one table at a time on the transaction/connection), I think that identity insert should not automatically be switched on/off by default. In most application code I see (outside of seed data), explicitly assigning a value to a field that is mapped to an identity should be considered an application bug, and I would prefer Entity Framework to raise an exception if an attempt to set an explicit identity value is encountered. In some cases, I see the identity field being used to obtain an ordering of when the application inserted records, e.g. because it is immune to system clock "jumps" when time synchronization occurs (understanding that "identity order" is not necessarily "transaction commit order") For me it would be more useful to have explicit control over identity insert on a case-by-case basis, rather than a global configuration switch e.g. what was proposed in #13575 in that regard:
Also there appears to be a bit of overlap with |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
3 years later, can someone explain the range of "the forseeable future" in context of EFCore? |
@Archomeda as always, we have tons of possible work items and a small number of engineers to do the work. At the end of the day there are just 24 votes on this, so it's really far down out request list and doesn't seem to affect many users. Regardless, posting "when will this feature be implemented" doesn't help us prioritize, but rather takes up more of our time in writing answers. |
@roji I came here via the link on this documentation page. I don't find it unreasonable to think that, if there's a link on the official pages mentioning "hey, we have a feature request for this", that it's something it's at least quite a recent request and not a dead one. Because you have to admit, if it's a 10 year old request, and, as you put it, doesn't seem to affect many other users, why is it even on there? I'm sorry to have bothered anyone here for my obvious blunder by asking something I shouldn't have asked. As I'm not a person who just leaves other people in the dark if I found an alternative solution to my problem: it seems that my use case didn't require identity columns in the first place and I could disable them in the context via |
This is quite possibly the most rude and insulting reply I've ever observed from a Microsoft employee on GitHub, but at least it makes very clear their actual attitude towards contributors, i.e. how much they (don't) value our contributions. Unless we're implementing features for Microsoft without being paid, so that Microsoft can make money off that unpaid work, we're not useful to them; we're just annoying mosquitoes to be fobbed off with excuses. Good job on building community trust, Microsoft! |
We indeed mention unimplemented features and limitations in our documentation. One reason for that is to make users aware that something isn't supported - as a heads up. The other is indeed to gather feedback from users who require this feature - but a simple up-vote on the top-most post is the standard and best way to let us know about it. For one thing, when we plan about what to implement in a given release, we look at highly-voted issues; comments posted down below saying "I need this" or "when will this be done" aren't visible and just get lost. It's important to understand that if an issue is open, that means we've decided it makes sense for it to be done at some point; but that doesn't imply anything about when exactly that will happen. Our backlog is enormous, and it's entirely reasonable for an issue to remain open for years, since it receives very little votes or is out-prioritized by other features. We typically only close issues when we think they don't make sense for EF, or that there's some reason we'll never implement them. Finally, @IanKemp and @Archomeda I can see how my answer may have been worded a bit strongly - my intent wasn't to insult anybody, and I apologize for that. First of all, note that there's no big "Microsoft" here - just a handful of engineers working hard on making EF Core better and engaging with the users. We get a constant stream of "how is this not yet done" and "when will this be implemented" messages; many of these are snarky, and some are downright aggressive. Regardless of their tone, these messages honestly do not help us prioritize features in any way - whereas up-votes definitely do. In other words, we're very much interested in user feedback - and get a lot of it - I'm just trying to explain how to do that in a way that's productive. |
When using generated values the state manager will not generate a temporary value if a specific value was already set. The update pipeline should then handle inserting the specific value or throw an exception. This isn't currently implemented for Identity columns in SQL Server. (It is also an option that the update pipeline could somehow special case this situation using metadata information and still generate a new value, just as it did in the old stack, but this would be a break from the expected behavior for value generation.)
The text was updated successfully, but these errors were encountered: