Support null versus empty for owned JSON collections by ajcvickers · Pull Request #39630 · dotnet/efcore

ajcvickers · 2026-11-13T09:44:36Z

Related: #35916 (Cosmos opt-in, not part of this PR), #31831

Summary

Owned JSON collections could not distinguish a null collection from an empty one. ChangeDetector.DetectNavigationChange ignored null-ness, and ModificationCommand.HandleJson forced a write only for empty collections, so changing between null and empty was lost.

This adds an opt-in INavigation.IsNullable facet (collection navigations only):

Metadata: IReadOnlyNavigation.IsNullable (default interface member), SetIsNullable on the mutable and convention interfaces, and a NullableCollection annotation. RuntimeEntityType.AddNavigation has a new overload with a nullable argument, and PropertyCounts tracks NullableCollectionCount.
Convention: NullableJsonCollectionConvention sets the facet from nullable annotations for JSON-mapped collections.
Change tracking: null-ness is tracked through original values plus PropertyFlag.NavigationModified. CollectionEntry.IsModified is now settable (setting it to false restores the original collection, like for properties) and IUpdateEntry.IsModified(INavigation) is added.
Query: the JSON include loader applies stored null-ness to newly tracked entities through two new public QueryContext methods, CanLoadCollectionNullness and SetCollectionNullnessLoaded, so providers (relational and Cosmos) don't need internal APIs for this.
Update: ModificationCommand writes JSON NULL, and writes when replacing an entry with the same key and a null value.
Validation: the facet must be on a collection navigation, and relational requires JSON mapping. The Cosmos validator throws and points to Support nullable collections mapped to JSON #35916, and the in-memory validator throws.

Breaking changes

A stored NULL now loads as null for nullable-annotated JSON collections, even when the CLR property has an initializer. Opt out with SetIsNullable(false).
IUpdateEntry.IsModified(INavigation) is a new abstract member, which is a source break for implementers.
Compiled models must be regenerated to use nullable collections. Existing compiled models keep working, because RuntimeEntityType.AddNavigation keeps its old signature.

Testing

Unit tests for the metadata, convention, tracking and validators, about 20 end-to-end tests in NonSharedModelUpdatesTestBase with Sqlite and SQL Server baselines, and two compiled-model tests. Full test.cmd run passes with 0 failures (Cosmos emulator tests were likely skipped).

Open items

Naming of IsNullable, a convention builder API and fluent opt-out, copying the facet when a relationship is rebuilt in InternalForeignKeyBuilder, and an untested standalone projection of the collection.

🤖 Generated with Claude Code

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Copilot

🟡 Changes recommended

Critical tracking and materialization defects can lose or overwrite nullable-collection changes, while the new abstract IUpdateEntry member breaks existing provider and extension implementations.

6 open issues

4 files not reviewed

src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file
src/EFCore.InMemory/Properties/InMemoryStrings.Designer.cs: Generated file
src/EFCore.Relational/Properties/RelationalStrings.Designer.cs: Generated file
src/EFCore/Properties/CoreStrings.Designer.cs: Generated file

_{Review effort: Balanced}

Adds an opt-in INavigation.IsNullable facet for collection navigations. Relational conventions set it for JSON-mapped collections whose CLR property is annotated as nullable. For such navigations, change tracking now distinguishes a null collection from an empty one, and null round-trips to and from the database as a JSON NULL. Fixes dotnet#33452 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Providers no longer need internal APIs to support nullable collection navigations. - Add QueryContext.CanLoadCollectionNullness and SetCollectionNullnessLoaded, which apply and record the stored null-ness of a collection navigation. These replace the internal QueryContext.StartedTracking and the state manager calls that the relational JSON include shaper made. - Stop using IRuntimeEntityType.NullableCollectionCount in ModificationCommand. - Make IUpdateEntry.IsModified(INavigation) a default interface member returning false, so it is not a source-breaking change for implementers. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

- Keep the existing RuntimeEntityType.AddNavigation signature and add an overload with the nullable parameter, so compiled models built against earlier versions keep working. - Don't run change detection when recording the loaded null-ness, which moved the entry to Modified and back during materialization. - Reset the set of entities that started being tracked when a query is executed again, so unsaved changes made after an earlier execution aren't overwritten. - Look up the state entry once per entity, and merge duplicate branches in the JSON include. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

- Setting CollectionEntry.IsModified to false for a nullable collection navigation now restores the original value, like it does for properties. The query code, which wants the loaded value to become the baseline, passes acceptChanges instead. - Make IUpdateEntry.IsModified(INavigation) abstract again instead of a default interface member, so implementers can't silently report no change. - The in-memory provider now rejects nullable collection navigations in model validation. - Rename the isNull parameter of QueryContext.SetCollectionNullnessLoaded to storedAsNull, which satisfies the API consistency rule on bool parameter names. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

…erload The overload taking `nullable` had a higher overload resolution priority, so source calls omitting it, and RuntimeModelConvention for non-nullable navigations, bypassed subclasses that override the shipped nine-parameter virtual method. The new overload now has a required `nullable` parameter, so calls that omit it bind to the original overload. When `nullable` is false the new overload delegates to the original virtual method. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…ions InternalEntrySubscriber obtained the collection with GetOrCreateCollection when subscribing to, or unsubscribing from, a collection navigation. For a nullable collection navigation that is currently null, this created an empty collection, so Add wrote [] instead of NULL, Attach changed the supplied value, and stored nulls were replaced after a query materialized a tracked notifying entity. A null nullable collection navigation now has nothing to subscribe to, and is subscribed to when a collection is assigned, as for any other change. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

With ChangingAndChangedNotificationsWithOriginalValues the original values are captured lazily from PropertyChanging, but only for properties. Changing a nullable collection navigation before any property meant there was no snapshot to compare with, so the null-ness change was missed and lost by SaveChanges. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…a query An entity that started being tracked by a query stayed eligible for loading the stored null-ness of its nullable collections for the whole query. If a streaming query returned the same owner again, a change made to the collection in the meantime was overwritten and its baseline reset. The completion is now recorded for each (entry, navigation). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…n querying When a query returned an entity that was already tracked with a null nullable JSON collection, the stored elements were still materialized. Tracking them added them back to the owner's collection through fixup, so the null that was about to be saved was lost. The stored collection is now skipped without being materialized in that case. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…entities An entity that uses change notifications and has a null nullable collection no longer has an empty collection created when it starts being tracked. When a query then created the collection while including the stored JSON, nothing subscribed to it, so elements added to or removed from it in place were not detected and not saved. The collection is now subscribed to after the query has loaded it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

… entry CollectionEntry.Load, LoadAsync and Query created an empty collection for the navigation before doing anything else, even when the load was not needed. For a nullable collection navigation that is null, this replaced the null, which was then detected as a change and saved as an empty collection. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Unmodifying a nullable collection navigation restored the original collection reference even when the current collection had the same null-ness, so replacing a collection with another one and then unmodifying the navigation silently undid the replacement. Replacing a collection is not a tracked change, as for collection navigations that are not nullable, so it is no longer reverted. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

ajcvickers · 2026-11-14T15:07:03Z

After addressing the Copilot comments, I had a second, independent review of the whole branch (a separate Opus-model agent, briefed with Copilot's categories and asked to look for other integration gaps and missing coverage). Each finding was checked against the code and fixed test-first. Significant changes from it:

Notifying entities lost in-place Add/Remove on a collection loaded by a query. This was a regression from the subscriber fix above. The collection is now subscribed after the query creates it (e336b56).
CollectionEntry.Load(), LoadAsync() and Query() replaced a null nullable collection with [] (92e2c5c).
Unmodifying a nullable collection reverted a plain replacement that wasn't a null-ness change (8c08b33).

One suspected problem, with TPT/TPC and a derived-type collection, can't occur: model validation already rejects non-TPH inheritance for JSON-mapped entities. One claim, that old compiled models break because the PropertyCounts constructor changed, wasn't substantiated and I made no change for it.

Still open: the IUpdateEntry abstract member (waiting on a decision), and the breaking-change note for stored NULL now loading as null even when the CLR property has an initializer.

Sqlite, SQL Server and the remaining test suites pass.

Copilot

🟡 Changes recommended

Unresolved collection-navigation defects can leave assigned elements untracked and cause SaveChanges failures, break NULL materialization for getter-only collections, and lose data-annotation nullability opt-outs during navigation replacement.

3 open issues

6 resolved since last review

4 files not reviewed

src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file
src/EFCore.InMemory/Properties/InMemoryStrings.Designer.cs: Generated file
src/EFCore.Relational/Properties/RelationalStrings.Designer.cs: Generated file
src/EFCore/Properties/CoreStrings.Designer.cs: Generated file

_{Review effort: Balanced}

+            case INavigation { IsCollection: true, IsNullable: true } nullableCollection:
+                // Collection navigations of notifying entities have no relationship snapshot, so the null-ness
+                // of nullable collections is tracked directly from the notification.


+    bool? SetIsNullable(bool? nullable, bool fromDataAnnotation = false)
+        => (bool?)SetOrRemoveAnnotation(CoreAnnotationNames.NullableCollection, nullable, fromDataAnnotation)?.Value;


+                entry!.SetOriginalValue(navigation, null, skipChangeDetection: true);
+            }
+
+            ((IRuntimePropertyBase)navigation).MaterializationSetter.SetClrValue(entity, null);


AndriySvyryd · 2026-11-15T01:50:49Z

+
+        return null;
+    }
+


Could we extend the existing IsRequired/CanSetIsRequired path for principal-side collection navigations and remove these new builder methods? The nullable facet can follow the existing property pattern (IsRequired(required) sets IsNullable = !required), letting the convention use navigation.Builder.IsRequired(false) and callers opt out through the fluent API.

AndriySvyryd · 2026-11-15T01:50:49Z

+
+namespace Microsoft.EntityFrameworkCore.ChangeTracking;
+
+public class NullableCollectionNavigationTrackingTest


Could we move these tests into the existing ChangeDetectorTest, CollectionEntryTest, and InternalEntrySubscriberTest classes? They already cover the corresponding tracking behaviors and have suitable test infrastructure. The loading/querying cases also overlap with the relational update tests, and several inverse scenarios could be combined as [Theory] cases to reduce duplication.

AndriySvyryd · 2026-11-15T01:50:49Z

+
+namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
+
+public class NullableJsonCollectionConventionTest


Could we put these model-building cases in RelationalOwnedTypesTestBase instead? It already covers owned JSON mapping, and tests there run across the generic/non-generic builders and SQL Server/SQLite. The nullable, non-nullable, JSON/non-JSON, and explicit-override cases could be combined as theory rows.

AndriySvyryd · 2026-11-15T01:50:49Z

+                Assert.Equal(1, ((Microsoft.EntityFrameworkCore.Metadata.Internal.IRuntimeEntityType)derived).NullableCollectionCount);
+                Assert.Equal(0, ((Microsoft.EntityFrameworkCore.Metadata.Internal.IRuntimeEntityType)baseType).NullableCollectionCount);
+#pragma warning restore EF1001 // Internal EF Core API usage.
+            },


Could we extend BigModel_with_JSON_columns instead of adding these two compiled-model tests? It already has a JSON owned collection on a derived type and an owned type using changing/changed notifications, so it can cover most of these facets with a nullable navigation and a few assertions. That would avoid roughly 34 generated baseline files across SQL Server and SQLite.

AndriySvyryd · 2026-11-15T01:50:49Z

    }

+    [Theory, InlineData(true), InlineData(false)] // Issue #33452
+    public virtual async Task Nullable_json_collection_changed_from_empty_to_null_is_saved(bool alsoModifyName)


Could we move the common save/load scenarios into JsonUpdateTestBase or reuse one model across theory cases? Empty/null transitions, nested and derived collections, concurrency, batching, and query projections use nearly identical model shapes; keeping a separate model per test also duplicates entity types and SQL baselines. Reserve separate models for cases that specifically test a different shape, such as coercing setters or notification behavior.

AndriySvyryd · 2026-11-15T01:50:50Z

+            if (memberInfo != null
+                && TryGetNullabilityInfo(modelBuilder, memberInfo, out var nullabilityInfo)
+                && nullabilityInfo.ReadState == NullabilityState.Nullable)
+            {


This convention can enable null materialization for a getter-only navigation with no writable backing field, but SetCollectionNullnessLoaded later unconditionally assigns null through the materialization setter when the stored JSON value is null. Could we skip these navigations here and reject explicit nullable configurations during model validation? Please add a getter-only regression test for the stored-null case.

github-actions Bot added the community-contribution label Nov 13, 2026

ajcvickers marked this pull request as ready for review November 13, 2026 11:12

ajcvickers requested a review from a team as a code owner November 13, 2026 11:12

Copilot AI balanced review requested due to automatic review settings November 13, 2026 11:12

Copilot AI reviewed Nov 13, 2026

AndriySvyryd requested a balanced review from Copilot November 14, 2026 06:05

Copilot started reviewing on behalf of AndriySvyryd November 14, 2026 06:06

View session

Copilot AI reviewed Nov 14, 2026

View reviewed changes

ajcvickers and others added 4 commits November 14, 2026 12:02

ajcvickers force-pushed the feature/33452-nullable-inline-collections branch from 87c4fdb to d45b5ae Compare November 14, 2026 12:07

Copilot AI balanced review requested due to automatic review settings November 14, 2026 12:07

Copilot AI reviewed Nov 14, 2026

ajcvickers and others added 8 commits November 14, 2026 12:33

Copilot AI balanced review requested due to automatic review settings November 14, 2026 14:52

Copilot AI reviewed Nov 14, 2026

ajcvickers mentioned this pull request Nov 14, 2026

Breaking change note: stored NULL JSON collections materialize as null for nullable collection properties dotnet/EntityFramework.Docs#5526

Draft

AndriySvyryd requested a balanced review from Copilot November 14, 2026 19:51

Copilot started reviewing on behalf of AndriySvyryd November 14, 2026 19:53

View session

Copilot AI reviewed Nov 14, 2026

View reviewed changes

AndriySvyryd reviewed Nov 15, 2026

View reviewed changes

		bool? SetIsNullable(bool? nullable, bool fromDataAnnotation = false)
		=> (bool?)SetOrRemoveAnnotation(CoreAnnotationNames.NullableCollection, nullable, fromDataAnnotation)?.Value;


		namespace Microsoft.EntityFrameworkCore.ChangeTracking;

		public class NullableCollectionNavigationTrackingTest


		namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;

		public class NullableJsonCollectionConventionTest

Conversation

ajcvickers commented Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Testing

Open items

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

ajcvickers commented Nov 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Nov 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajcvickers commented Nov 13, 2026 •

edited

Loading