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

SQL Server: Switch IDENTITY_INSERT on (and then off) when explicit values are specified for an identity column #703

Open
ajcvickers opened this issue Sep 25, 2014 · 14 comments

Comments

@ajcvickers
Copy link
Member

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.)

@divega
Copy link
Contributor

divega commented Sep 26, 2014

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.

@rowanmiller
Copy link
Contributor

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.

@rowanmiller rowanmiller changed the title Support explicitly set values for Identity columns in SQL Server update pipeline SQL Server: Switch IDENTITY_INSERT on (and then off) when explicit values are specified for an identity column Dec 3, 2015
@ajcvickers
Copy link
Member Author

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.

@ajcvickers
Copy link
Member Author

See #11586 for an API proposal.

nfaugout-lucca added a commit to LuccaSA/RestDrivenDomain that referenced this issue Oct 3, 2018
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
nfaugout-lucca added a commit to LuccaSA/RestDrivenDomain that referenced this issue Oct 3, 2018
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
nfaugout-lucca added a commit to LuccaSA/RestDrivenDomain that referenced this issue Oct 3, 2018
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
@AndriySvyryd
Copy link
Member

Consider how this would interact with #13575

@davisnw
Copy link

davisnw commented Jan 21, 2020

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.
This issue description also mentions that "state manager will not generate a temporary value".

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:

using (context.ChangeTracker.EnableExplicitKeys())
{
    context.Add(new Blog { Id = 77 });
}

Also there appears to be a bit of overlap with HasData, but that is a different use case. Notably, based on my limited testing in EfCore 2.2, it appears that HasData bypasses the normal db context hooks (that is I don't see e.g. SaveChangesAsync being called in the derived override), I would expect entities added under e.g. EnableExplicitKeys to go through all the normal DbContext hooks when SaveChanges / SaveChangesAsync is called.

@IanKemp

This comment was marked as resolved.

@DawidIzydor

This comment was marked as resolved.

@AndriySvyryd

This comment was marked as resolved.

@Archomeda
Copy link

3 years later, can someone explain the range of "the forseeable future" in context of EFCore?

@roji
Copy link
Member

roji commented Jul 27, 2023

@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.

@Archomeda
Copy link

@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 .ValueGeneratedNever(). It seems that the internet put me on a wild goose chase of IDENTITY_INSERT instead for a couple of hours, because this method wasn't easy to find at all.
Maybe this solution will help other people looking for an alternative for the same use case as mine.

@IanKemp
Copy link

IanKemp commented Jul 28, 2023

Regardless, posting "when will this feature be implemented" doesn't help us prioritize, but rather takes up more of our time in writing answers.

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!

@roji
Copy link
Member

roji commented Jul 28, 2023

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?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants