-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Store temporary key values in state entry rather than setting them on entity instances #12378
Comments
@ajcvickers @optiks It will be great if EFCore can get away from setting the properties to negative numbers. While you think through the possibility, can you please advise if there are any workarounds/ideas that may help us ? Thanks, |
@avinash-phaniraj-readify I don't have any great ideas for a workaround other than do to what I already saw in some other code from you guys where you instead check for negative values instead of zero--maybe you can move this into a common helper that you then call whenever this logic exists. |
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Fixes #12378, #10167 The main breaking change here is that application logic that relies on the temporary value being set into the actual entity instance will be broken. Possibly the only place this is used is to trigger fixup between Added entities (before they are saved) using FK values only. This is pretty uncommon. The case where both navigation properties and FKs are excessively set will still work because the navigations will cause fixup to happen. Anyone hitting this can still make it work by grabbing temporary values from the state manager--tests that were hitting this have been updated to work this way. Functionally, the change works like this: * If a property is set to a non-default value, then it cannot be temporary or store-generated * This is kind of a new restriction, but functionally shouldn't change anything that worked before * If a property has a default value, then it can have a temporary value set, which is stored in a new "sidecar". * When the update pipeline sets a store-generated value, it uses the same mechanism. * So, real values hide store generated values, which hide temporary values. * This means that if SaveChanges fails and the store-generated values are discarded, then the temporary values take over again. Setting a store-generated or temporary value has been made explicit. Transparently setting store-generated values wasn't actually very useful.
Re-opening to ensure breaking change is announced. |
@divega Draft breaking change announcement for review Entity Framework Core breaking change: Temporary values are no longer stored in entity instancesSee issue #12378 SummaryWhen a new entity instance is tracked by the context it needs a primary key value. If the primary key value is specified or can be generated immediately, then this permanent key value is used immediately. However, if the entity will get a store-generated key value from the database, then the entity needs a temporary key value until Prior to EF Core 3.0, this temporary key value was set into the primary key property of the entity. Starting with EF Core 3.0, the temporary key value is now stored with EF's tracking information while the value of primary key property itself will not be changed. Where this helpsThe primary motivation for this change is to allow entities that have been previously tracked by some context instance to be moved to being tracked on another instance without the temporary key values erroneously becoming permanent. Where this may break applicationsApplications may be using the temporary key values to form associations between entities. For example, the temporary primary key value may have been used to set an FK value. If this is happening in your application, then there are two approaches to avoid this while still using store-generated keys:
|
@ajcvickers concerning the idea of using navigation properties exclusively … will EF set the foreign key of an Added dependent entity if the principal entity is not attached to the context? Correspondingly for a non-attached principal entity will EF add the dependent entity to the principle entity's navigation collection? Would be interested to know if there is any detail document talking about this kind of thing. |
@sjb-sjb In general, EF will not take a value from an un-tracked entity and propagate to one that is tracked. |
@ajcvickers So … if I have two entities and I want to make one of them refer to the other, then I should (a) set the navigation property and (b) check the primary key of the principle entity; if it is > 0 then set the foreign key into the dependent entity. |
@sjb-sjb You should track them both. |
I think this change was a really bad one for few reasons:
This is a rant because it's taking me forever to migrate a project to aspnet core 3.1 due to EF's 56 breaking changes. and because of this I'm still running on top of an unsupported dotnet core version in production! |
What if there was setting to set the primary key properties on add to a negative number? This would at least give people an option to choose |
@ajcvickers I am facing a similar issue like @ilmax above, where I am trying to avoid having navigation properties between entities of different modules of my codebase (aka between different "aggregate roots"). I also use plain foreign keys instead. The reason is to prevent issues like these:
The behaviour prior to this change, where EF assigned temporary primary key values, was thus very appreciated. This made it possible to have module A create an instance of entity A, module B create an instance of entity B, and link them together using their (temporary) primary keys. But now, their primary keys are all left at their default values, making this impossible without exposing the unwanted navigation properties between the entities. As @chrisevans9629 mentions, an option for re-enabling this behaviour would be appreciated. You do mention a workaround with using |
@Zero3 This should still work because the temporary values still exist. But you'll need to use the EF Core APIs (e.g. |
To answer my own question: No, it is not safe to set So it appears like there is no way to restore the old behaviour. It would be really nice if the new behaviour was opt-in/opt-out, for the reasons already described above. |
@ajcvickers is there any chance that the EF team could reconsider this change, or at least make it optional? Based on the 11 upvotes above, it seems like we are at least a bunch of people that got hit by this change in a negative way, and believe the previous behaviour was better. There is likely people who believe the opposite, of course, so maybe there is no solution that suits both sides. But it would be great if you could reconsider, and in case you decide to keep the new behaviour, and not making it optional, let us know about any possible workarounds for those of us that got hit. Thanks :). |
@Zero3 As far as I am aware it is still possible to use temporary values as before, just with a bit more work. For example, you are free to set the temporary value into the entity if you want it there, but EF Core doesn't put it there by default. See https://docs.microsoft.com/en-us/ef/core/change-tracking/miscellaneous#temporary-values for up-to-date docs on temporary values. |
I tried this, but as mentioned above, EF will then save the temporary value to database. And if one manually does Here is an example of a custom
Output:
|
Shame this changed without a way to revert to the old behaviour. The current approach requires you to more tightly couple with ef core, while the negative id's allowed your code to be completely indifferent to whether it was a real or temp id. @ajcvickers Is there a way to populate this temporary key back in the fields in a universal way? Suppose I'm adding an entity with a collection of sub entities to the context. (eg, all the Blog<->Posts examples in the docs) Can I 'plug-in' to a certain event, func, callback, ... to write code once that fills all fields that have a temp key generated? |
@BjornDeRijcke It's not clear to me what you are trying to achieve. Can you open a new issue, and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate? |
@ajcvickers I logged a new ticket here: #34200 Basically, what I want is to be able to setup relationships for newly added entities (not yet persisted) without having to |
Similar to the way store-generated values are stored during SaveChanges. This would mean that temporary values would not be stored in or visible in the key property, which would have a few advantages:
The reasons that we didn't do this before were:
The text was updated successfully, but these errors were encountered: