Stop setting JSON array entities' state to Modified in SaveChanges #28926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajcvickers
approved these changes
Aug 31, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue is when saving entities within JSON arrays. When generating column modifications in SaveChanges, we rebuild the entire JSON document in CreateJson; any entities which are contained in a JSON array get their "ordinal key property" recalculated and assigned (see this line); this changes the entity's state to Modified. Since these entities are unrelated to the original change, we don't accept changes on them (not in entriesToSave in StateManager.SaveChanges), and so they remain Modified after SaveChanges completes.
Note that something similar actually happens when we save a regular entity with a computed property: propagating the computed property marks the entry as Modified. But in that case it's already Modified anyway, and accepting the changes reverts it back to Unchanged.
I tried changing SetStoreGeneratedValue to pass setModified=false to SetStoreGeneratedValue, with the thought that propagating a value back from the database shouldn't change the entity's state; that could even be a slight optimization for regular non-JSON propagation. But that caused test failures in TableSplittingTestBase.Can_use_optional_dependents_with_shared_concurrency_tokens, OptimisticConcurrencySqlServerTestBase.Database_concurrency_token_value_is_updated_for_all_sharing_entities, OptimisticConcurrencySqlServerTestBase.Database_concurrency_token_value_is_updated_for_all_sharing_entities.
So for now I've simply exposed setModified on SetStoreGeneratedValue, which seems minimally-invasive and low risk (but a small public API change. Let me know what you think.
Fixes #28813