Fully integrate JSON mapping into the relational model#38038
Fully integrate JSON mapping into the relational model#38038AndriySvyryd wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR advances EF Core’s relational JSON support by modeling JSON elements (objects/arrays/scalars) directly in the relational model and by replacing string JSONPATH usage in partial updates with a structured JsonPath, enabling providers (e.g., PostgreSQL) to consume path components more flexibly.
Changes:
- Introduces
JsonPath/JsonPathSegmentand updates JSON partial update SQL generation to use the structured path instead of a string. - Adds relational-model JSON element types (
IRelationalJsonElement+ object/array/scalar), JSON element mappings, and populates JSON element trees on JSON columns. - Extends
FindColumnAPIs to acceptIPropertyBase(including navigations and complex properties) and updates compiled model/scaffolding baselines and tests accordingly.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Tests/Infrastructure/JsonPathTest.cs | Adds unit tests for JsonPath and JsonPathSegment formatting/behavior. |
| test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/ComplexTypes/DbContextModelBuilder.cs | Updates scaffolding baseline to emit JSON element tree initialization. |
| test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/DbContextModelBuilder.cs | Updates scaffolding baseline to emit JSON element tree initialization (big model). |
| test/EFCore.Sqlite.Tests/Migrations/SqliteMigrationAnnotationProviderTest.cs | Adjusts test for updated column mapping typing and removes BOM. |
| test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/DbContextModelBuilder.cs | Updates scaffolding baseline to emit JSON element tree initialization (SQLite). |
| test/EFCore.Relational.Tests/Metadata/RelationalModelTest.cs | Adds tests asserting JSON element trees/mappings and FindColumn behavior. |
| test/EFCore.Relational.Specification.Tests/Scaffolding/CompiledModelRelationalTestBase.cs | Adds assertions around JSON element metadata in compiled model scenarios. |
| src/EFCore/Infrastructure/JsonPathSegment.cs | Introduces structured JSON path segment (property vs array placeholder). |
| src/EFCore/Infrastructure/JsonPath.cs | Introduces structured JSON path (segments + ordinals) with formatting helpers. |
| src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs | Uses JsonPath.AppendTo and structured root detection for JSON updates. |
| src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs | Switches JSON container column lookup to FindColumn. |
| src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs | Uses JsonPath.AppendTo and structured root detection for JSON updates. |
| src/EFCore.Relational/Update/ModificationCommand.cs | Builds structured JsonPath for JSON partial updates using element metadata/mappings. |
| src/EFCore.Relational/Update/IColumnModification.cs | Changes JsonPath from string to JsonPath?. |
| src/EFCore.Relational/Update/ColumnModificationParameters.cs | Changes JsonPath from string to JsonPath? and updates ctor signature. |
| src/EFCore.Relational/Update/ColumnModification.cs | Changes JsonPath from string to JsonPath?. |
| src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.ExecuteUpdate.cs | Updates container column resolution to FindColumn with null handling. |
| src/EFCore.Relational/Metadata/RelationalAnnotationNames.cs | Adds JsonElementMappings annotation name. |
| src/EFCore.Relational/Metadata/JsonValueType.cs | Adds enum to capture scalar JSON value kind. |
| src/EFCore.Relational/Metadata/IView.cs | Broadens FindColumn parameter type to IPropertyBase. |
| src/EFCore.Relational/Metadata/ITableBase.cs | Broadens FindColumn parameter type to IPropertyBase. |
| src/EFCore.Relational/Metadata/ITable.cs | Broadens FindColumn parameter type to IPropertyBase. |
| src/EFCore.Relational/Metadata/IStoreFunction.cs | Broadens FindColumn parameter type to IPropertyBase. |
| src/EFCore.Relational/Metadata/ISqlQuery.cs | Broadens FindColumn parameter type to IPropertyBase. |
| src/EFCore.Relational/Metadata/IRelationalJsonScalar.cs | Adds public interface for scalar JSON elements. |
| src/EFCore.Relational/Metadata/IRelationalJsonObject.cs | Adds public interface for object JSON elements. |
| src/EFCore.Relational/Metadata/IRelationalJsonElement.cs | Adds public interface for JSON elements (name/path/column/mappings). |
| src/EFCore.Relational/Metadata/IRelationalJsonArray.cs | Adds public interface for array JSON elements. |
| src/EFCore.Relational/Metadata/Internal/View.cs | Implements FindColumn(IPropertyBase) with navigation/complex-property support. |
| src/EFCore.Relational/Metadata/Internal/TableBase.cs | Implements FindColumn(IPropertyBase) with navigation/complex-property support. |
| src/EFCore.Relational/Metadata/Internal/Table.cs | Implements FindColumn(IPropertyBase) with navigation/complex-property support. |
| src/EFCore.Relational/Metadata/Internal/StoreStoredProcedure.cs | Updates FindColumn override for broadened IPropertyBase signature. |
| src/EFCore.Relational/Metadata/Internal/StoreFunction.cs | Updates FindColumn override for broadened IPropertyBase signature. |
| src/EFCore.Relational/Metadata/Internal/SqlQuery.cs | Updates FindColumn override for broadened IPropertyBase signature. |
| src/EFCore.Relational/Metadata/Internal/RelationalModel.cs | Builds JSON element trees, adds JSON element mappings, and sets JsonElement on JSON columns. |
| src/EFCore.Relational/Metadata/Internal/RelationalJsonScalar.cs | Implements internal scalar JSON element. |
| src/EFCore.Relational/Metadata/Internal/RelationalJsonObject.cs | Implements internal object JSON element with ordered properties. |
| src/EFCore.Relational/Metadata/Internal/RelationalJsonElement.cs | Implements internal base type for JSON elements (path/nullable/mappings). |
| src/EFCore.Relational/Metadata/Internal/RelationalJsonArray.cs | Implements internal array JSON element with element-type linkage. |
| src/EFCore.Relational/Metadata/Internal/JsonElementMapping.cs | Implements internal mapping between model property and JSON element. |
| src/EFCore.Relational/Metadata/Internal/ColumnBase.cs | Adds JsonElement property slot to columns. |
| src/EFCore.Relational/Metadata/IJsonElementMapping.cs | Adds public interface for property-to-JSON-element mappings. |
| src/EFCore.Relational/Metadata/IColumnBase.cs | Exposes JsonElement on columns (default null for non-JSON columns). |
| src/EFCore.Relational/Metadata/Conventions/RelationalRuntimeModelConvention.cs | Removes JSON element mapping annotations from runtime annotations dictionary (consistent with other mapping annotations). |
| src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs | Generalizes mapping accessors to IPropertyBase and adds GetJsonElementMappings(). |
| src/EFCore.Relational/Design/Internal/RelationalCSharpRuntimeAnnotationCodeGenerator.cs | Emits compiled-model code to recreate JSON element trees on columns. |
92c7cbb to
3f990fc
Compare
roji
left a comment
There was a problem hiding this comment.
Here's a first review @AndriySvyryd.
- From this PR specifically it's difficult to see the immediate, concrete value (in terms of simplifying/improving code in update/query). Are you planning more significant work on update (since the current changes seem minimal and slightly orthogonal too)?
- I'm noting that we're kinda redoing a version of JSON Schema here, though we have our own concerns (our model is fully traversable in all directions, contains mapping to IPropertyBase...).
- I'd want to understand why JSON scalars have a only string/number/bool typing, as opposed to our full known type mapping.
- Ideally we'd have JSON-mapped primitive collections using the same JSON modeling in the relational model, what do you think?
- I can't help but think of other non-JSON document-style mappings (e.g. PG arrays, composite types...). Since the relational model isn't extensible (I think?), the PG provider can't integrate that into the relational model and has to make do with the current way. Which maybe raises the question of how useful/necessary JSON modeling really is in the relational model... We can discuss.
| /// <remarks> | ||
| /// See <see href="https://aka.ms/efcore-docs-modeling">Modeling entity types and relationships</see> for more information and examples. | ||
| /// </remarks> | ||
| public class JsonPath |
There was a problem hiding this comment.
Am noting that in query we already have PathSegment, and e.g. JsonScalarExpression has an IReadOnlyList<PathSegment> to hold a full JSON path. These are indeed different - the query concepts are for actual paths (not models of paths), and contains Expressions.
But I'm wondering - do JsonPath and JsonPathSegment actually make sense as modeling concepts? We already have full-traversabilty in your new JSON model, so it seems like the JsonPath of any element can be derived upon-demand from the model without needing to store it; and at least in query I'm not sure I can think of a scenario where I'd use them (but maybe I haven't thought it through).
So right now this basically seems like an update-pipeline-only caching mechanism - is that what it's supposed to be? If so, do you think the perf/caching aspects justify putting this into the relational model?
What's odder to me that these are in core, as opposed to relational (and they don't seem to actually be used in any core code); do you see them somehow being used in e.g. Cosmos?
| targetColumnModel = complexType.ContainingEntityType.GetTableMappings() | ||
| .SelectMany(m => m.Table.Columns) | ||
| .SingleOrDefault(c => c.Name == containerColumnName); | ||
| if (containerColumnName != null) |
There was a problem hiding this comment.
Am assuming that in this PR you intentionally didn't (yet) do any query adjustments to the new modeling, right? But presumably there's usage/cleanup potential there?
| .SelectMany(m => m.Table.Columns) | ||
| .SingleOrDefault(c => c.Name == containerColumnName); | ||
| if (containerColumnName != null) | ||
| { |
| } | ||
| } | ||
|
|
||
| private static JsonValueType GetJsonValueType(Type clrType) |
There was a problem hiding this comment.
I'm generally confused by JsonValueType and what it's supposed to be represent exactly...
First, don't want want/need the full type mapping represented by properties inside JSON documents, rather than just string/number/bool? If regular columns in the relational model have a full type mapping (including SQLite ones, where the actual column types supported by SQLite is also a very short list, like JSON), why not here too?
Second, deriving the type purely from the CLR type seems odd - what about value converters?
| /// Gets the child elements (objects, arrays, and scalar properties) of this JSON object, | ||
| /// in their declaration order. | ||
| /// </summary> | ||
| IReadOnlyList<IRelationalJsonElement> Properties { get; } |
There was a problem hiding this comment.
I'm assuming that this is a list (as opposed to a dictionary) in order to produce deterministic ordering when e.g. query/update generate JSON properties in SQL and similar?
| .FirstOrDefault(cm => cm.TableMapping.Table == this)?.Column, | ||
| INavigation nav when nav.TargetEntityType.GetContainerColumnName() is { Length: > 0 } containerColumnName | ||
| => FindColumn(containerColumnName), | ||
| INavigation => null, |
There was a problem hiding this comment.
This logic also seems to functionally duplciate the logic in TableBase (the INavigation => null and IComplexProperty => null arms seem unnecessary)
| AddJsonElementMapping(navigation, rootElement, tableMapping); | ||
| } | ||
|
|
||
| private static void BuildJsonElementTree( |
There was a problem hiding this comment.
Can we unify the owned and complex paths here, just passing in ITypeBase?
| { | ||
| switch (column) | ||
| { | ||
| case JsonColumn jsonCol: |
There was a problem hiding this comment.
I might be confusing things, but do we still need the JsonColumnBase/JsonColumn/JsonViewColumn hierarchy, given that ColumnBase now has JsonElement? Would be really nice to cut down the types/hierarchies/concept count/complexity.
| string? schema) | ||
| { | ||
| if (columnModification.JsonPath is null or "$") | ||
| if (columnModification.JsonPath is not { IsRoot: false }) |
| /// In case of JSON column modification, the JSON path leading to the JSON element that needs to be updated. | ||
| /// </summary> | ||
| public string? JsonPath { get; } | ||
| public JsonPath? JsonPath { get; } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Provider-facing breaking change (with ApiChief we might be able to set up automation to automatically flag and label such PRs, and generate docs...)
Change partial update JSON path to be structured instead of a string JSONPATH
Fixes #36646
Fixes #32185