Fix NRE in AddJsonNavigationBindings when combining DbFunction with OwnsOne/OwnsMany ToJson#37855
Fix NRE in AddJsonNavigationBindings when combining DbFunction with OwnsOne/OwnsMany ToJson#37855
Conversation
…g DbFunction with OwnsOne/OwnsMany ToJson Add JSON container column creation to CreateFunctionMapping in RelationalModel, matching how tables and views already handle JSON container columns. Pass IRelationalTypeMappingSource through AddMappedFunctions, AddTvfs and CreateFunctionMapping to enable container column creation. Co-authored-by: roji <1862641+roji@users.noreply.github.com>
|
@copilot what was the exact cause of the regression from 9.0 to 10.0? |
In EF Core 9, // EF Core 9
var containerColumn = (entityType.GetViewOrTableMappings().SingleOrDefault()?.Table
?? entityType.GetDefaultMappings().Single().Table)
.FindColumn(containerColumnName)!;So even when querying through a TVF, the container column was always resolved from the regular In EF Core 10, this was refactored to look up the container column via the // EF Core 10
var (containerColumn, tableAlias) = tableMap
.Select(kvp => (Column: kvp.Key.FindColumn(containerColumnName), TableAlias: kvp.Value))
.SingleOrDefault(c => c.Column is not null);For TVFs, the The fix adds the missing |
|
@AndriySvyryd this looks like a relational model (owned) JSON issue, triggered by a query change I did in 10.0. Assigning this and the issue to you for now, but let's discuss if you think query should be doing something else. |
…nctionName ownership fallback Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
| { | ||
| var modelBuilder = CreateConventionModelBuilder(); | ||
|
|
||
| modelBuilder.Entity<EntityWithJsonOwnedType>(cb => |
There was a problem hiding this comment.
@copilot Try to use existing classes instead of adding EntityWithJsonOwnedType.
Also assert the non-json columns to ensure no extra ones are addded
There was a problem hiding this comment.
Done in 4bfd2db. Replaced EntityWithJsonOwnedType with existing Order class using OwnsOne(c => c.DateDetails, o => o.ToJson("date_details")), removed the three custom classes and GetEntitiesWithJsonOwnedType method, and added column name assertions to verify no extra columns are present.
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a NullReferenceException when building relational models that combine table-valued DbFunctions with owned JSON types (OwnsOne/OwnsMany + ToJson), ensuring function mappings include the necessary JSON container columns and owned-type mappings.
Changes:
- Pass
IRelationalTypeMappingSourcethrough function-mapping code paths and create function container columns for JSON-mapped types. - Update
GetFunctionNameto resolve function names from owners for owned (including JSON-mapped) entity types. - Extend TVF mapping to include owned JSON entity types and add a regression test covering the scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/EFCore.Relational.Tests/Metadata/RelationalModelTest.cs | Adds a regression test ensuring TVFs include owned JSON container columns and don’t introduce extra columns. |
| src/EFCore.Relational/Metadata/Internal/RelationalModel.cs | Adds type-mapping source to function mapping, creates JSON container columns for functions, and maps owned JSON types for TVFs. |
| src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs | Updates GetFunctionName to inherit function names from owners for owned/JSON-mapped entity types. |
You can also share your feedback on Copilot code review. Take the survey.
| if (ownedType.FindRuntimeAnnotationValue(RelationalAnnotationNames.FunctionMappings) | ||
| is not List<FunctionMapping> ownedFunctionMappings) | ||
| { | ||
| ownedFunctionMappings = []; | ||
| ownedType.AddRuntimeAnnotation(RelationalAnnotationNames.FunctionMappings, ownedFunctionMappings); | ||
| } | ||
|
|
||
| ownedFunctionMappings.Add(ownedFunctionMapping); | ||
| ((StoreFunction)ownedFunctionMapping.StoreFunction).EntityTypeMappings.Add(ownedFunctionMapping); |
There was a problem hiding this comment.
The annotation-list initialization + mapping registration logic is duplicated (similar logic exists for the root entityType earlier in AddTvfs). Consider extracting this into a small helper to keep the mapping registration consistent and reduce copy/paste (e.g., a local function that “gets/creates function mappings list, adds mapping, and registers it on the StoreFunction”).
There was a problem hiding this comment.
@copilot The annotation-list initialization + mapping registration logic is duplicated (similar logic exists for the root entityType earlier in AddTvfs). Consider extracting this into a small helper to keep the mapping registration consistent and reduce duplication
There was a problem hiding this comment.
Extracted AddTvfMapping helper in e3ee4f0. The duplicated annotation-list initialization + mapping registration logic in AddTvfs is now a single method called for both the root entity and each owned JSON type.
There was a problem hiding this comment.
Extracted AddTvfMapping helper in e3ee4f0.
| if (ownedType.FindRuntimeAnnotationValue(RelationalAnnotationNames.FunctionMappings) | ||
| is not List<FunctionMapping> ownedFunctionMappings) | ||
| { | ||
| ownedFunctionMappings = []; | ||
| ownedType.AddRuntimeAnnotation(RelationalAnnotationNames.FunctionMappings, ownedFunctionMappings); | ||
| } | ||
|
|
||
| ownedFunctionMappings.Add(ownedFunctionMapping); | ||
| ((StoreFunction)ownedFunctionMapping.StoreFunction).EntityTypeMappings.Add(ownedFunctionMapping); |
There was a problem hiding this comment.
@copilot The annotation-list initialization + mapping registration logic is duplicated (similar logic exists for the root entityType earlier in AddTvfs). Consider extracting this into a small helper to keep the mapping registration consistent and reduce duplication
| public string Value { get; set; } | ||
| public int Number { get; set; } | ||
| } | ||
|
|
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
CreateFunctionMappinginRelationalModelnever added JSON container columns toStoreFunction, soAddJsonNavigationBindingshit a null deref when resolving the container column viaIStoreFunction.FindColumn(). Tables and views already handled this viaCreateContainerColumn; functions were simply missing it.Fixes #37842
Changes
RelationalEntityTypeExtensions.cs:GetFunctionNamenow falls back to the ownership principal's function name for owned types — consistent withGetViewName/GetTableNamewhich already do this viaGetDefaultViewName/GetDefaultTableNameRelationalModel.cs: RestructuredCreateFunctionMappingto checkmappedType.GetContainerColumnName()and create the container column when non-null, falling to the property loop otherwise — matching the existing pattern inCreateTableMappingandCreateViewMapping. Also added owned JSON type processing inAddTvfsvia a newAddTvfMappinghelper method so that TVF-mapped entities (which lack aFunctionNameannotation) get function mappings for their owned JSON types, eliminating duplicated mapping registration logic.RelationalModelTest.cs: AddCan_use_relational_model_with_functions_and_json_owned_typesusing existingOrderclass with bothOwnsOne(DateDetails).ToJson()andOwnsMany(Addresses).ToJson()on a TVF-mapped entity, asserting both JSON container and non-JSON columns to ensure no extra columns are added✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.