EF-293: Fix entity states for owned entity collections#281
EF-293: Fix entity states for owned entity collections#281damieng merged 4 commits intomongodb:mainfrom
Conversation
|
I tested your branch against my solution as a direct dependency. |
|
Thanks for your other PR - it also worked but I had concerns about the way it tackled the restoring or state and updating the keys being too different from what we and Cosmos do today. I don't want to deviate too much from that not just because of potential regressions in areas we don't have test coverage but also because it's harder to stay aligned with EF changes if we're not using the same technique (the synthetic key code is based on the Cosmos providers hence they they have the same bug) |
|
Yeah, I understand that. I was trying to figure out these problems for a few days and it's not so easy to see how it all fits together. Now finally, it works the same as with other providers! |
…evel where we do partials.
There was a problem hiding this comment.
Pull request overview
This PR addresses EF-293 by ensuring EF Core’s AcceptAllChanges sees entities whose state becomes Modified during MongoDB save preparation (root promotion + FK cascade from ordinal reassignment), and by preventing ordinal key updates from incorrectly flagging owned entries as modified. It also expands functional test coverage for repeated saves and nested owned collections.
Changes:
- Update save pipeline to materialize Mongo updates and add any entries promoted to
Modifiedduring save prep into EF’sentrieslist for correctAcceptAllChangesbehavior. - Prevent ordinal shadow key assignment from marking owned entries as modified (
setModified: false). - Add/expand functional tests (sync + async) for owned collection adjustments, nested owned collections, and avoiding unintended root property overwrites; adjust Evergreen CI matrix to split macOS into its own variant.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/MongoDB.EntityFrameworkCore/Storage/MongoDatabaseWrapper.cs |
Ensures promoted roots and other entries promoted during save prep are included for AcceptAllChanges. |
src/MongoDB.EntityFrameworkCore/Storage/MongoUpdate.cs |
Avoids marking entries modified when assigning store-generated ordinal keys. |
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Query/OwnedEntityTests.cs |
Adds theory-based sync/async coverage for repeated saves and nested owned collection modifications. |
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Design/Generated/EF10/SimpleContextModelBuilder.cs |
Updates generated model ID to match current model snapshot. |
evergreen/evergreen.yml |
Splits macOS testing into a separate buildvariant and adjusts OS matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes EF-293 which reported owned entities were causing an exception if saved more than once after some types of modification to the collection.
There were three issue involved in this:
Issue 1: Promoted root entities
When only an owned entity changes (e.g. removing a collection item), the root document is
Unchangedfrom EF's perspective. OurGetAllChangedRootEntriespromotes it toModifiedfor writing, but EF'sAcceptAllChangesonly processes its original entries list with the root staying permanentlyModified.Fix: Add promoted roots to EF's
entrieslist soAcceptAllChangesprocesses them. This matches how the EF Cosmos provider handles the same scenario in (CosmosDatabaseWrapper.cs:76-82).Issue 2: Ordinal key marking
SetStoreGeneratedValueon ordinal key properties used the default overload which set the modified as true, which could mark owned entity entries asModifiedeven though they wereUnchangedbefore save.Fix: Pass
setModified: false, matching the intent already expressed inSetTemporaryOrdinals. Safe because ordinal keys are immutable shadow key properties as EF doesn't run change detection on them.Note that Cosmos has this issue too.
Issue 3: FK cascade to nested owned entities
When ordinal keys are reassigned on collection items (e.g. after removing an item), EF's relationship fixup cascades the key change to FK properties on nested owned entities (e.g. a
Referenceowned by aThirdLevelin a collection). This marks those nested entries asModified, but they aren't in EF's entries list either. I discovered this while adding an extra test for nested owned entity updates.Fix: After materializing updates (which triggers ordinal reassignment and FK cascade), scan the state manager for any non-root entries promoted to
Modifiedand add them to the entries list. The scan is scoped to non-root entities only to avoid interfering with entries EF manages directly.