Honor explicit store type when discovering Dictionary<string,object>#38433
Open
ajcvickers wants to merge 2 commits into
Open
Honor explicit store type when discovering Dictionary<string,object>#38433ajcvickers wants to merge 2 commits into
ajcvickers wants to merge 2 commits into
Conversation
Contributor
Author
|
@AndriySvyryd Most of the changes here are due to making MemberClassifier, etc. public for use by providers. We could leave them pubternal for a much smaller fix if you prefer. |
ajcvickers
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR exposes/extensifies member classification in EF Core by introducing a dependencies record + public IMemberClassifier, and adds a relational override to treat members with an explicitly configured column type as scalar properties (avoiding property-bag navigation discovery), with new regression tests for #26903.
Changes:
- Introduce
MemberClassifierDependenciesand updateMemberClassifierto consume dependencies via DI. - Move
IMemberClassifierto the public metadata surface and adjustMemberClassifiernamespace/docs accordingly. - Add
RelationalMemberClassifier, register it for relational services, and add model validator tests for dictionary JSON columns.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs | Updates test construction to use MemberClassifierDependencies. |
| test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs | Adds regression tests for dictionary + explicit column type behavior and converter scenario. |
| src/EFCore/Metadata/MemberClassifierDependencies.cs | Adds new dependencies record used for DI and extensibility. |
| src/EFCore/Metadata/Internal/MemberClassifier.cs | Refactors MemberClassifier to use dependencies and updates namespace/docs. |
| src/EFCore/Metadata/Internal/IMemberClassifier.cs | Removes old internal interface in favor of public API. |
| src/EFCore/Metadata/IMemberClassifier.cs | Adds public IMemberClassifier with full XML docs. |
| src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs | Registers MemberClassifierDependencies in core service builder. |
| src/EFCore.Relational/Metadata/RelationalMemberClassifier.cs | Adds relational override honoring explicit column type in member classification. |
| src/EFCore.Relational/Infrastructure/EntityFrameworkRelationalServicesBuilder.cs | Registers RelationalMemberClassifier for relational providers. |
| src/EFCore/EFCore.baseline.json | Updates public API baseline for new/moved types. |
| src/EFCore.Relational/EFCore.Relational.baseline.json | Updates relational API baseline for RelationalMemberClassifier. |
… make MemberClassifier public (dotnet#26903) - Dictionary<string,object> with an explicit column type (e.g. [Column(TypeName="jsonb")]) was detected as a property bag, throwing the "shared-type entity type" error. - Root cause: the member classifier treats any type with no type mapping as a navigation to a property-bag entity type; Dictionary<string,object> has no mapping, Dictionary<string,string> does. - Fix: RelationalMemberClassifier honors an explicit [Column(TypeName)] so the member is discovered as a scalar property instead of a property bag. - Providers that map the type (e.g. Npgsql -> jsonb) now work; others report the clear "cannot be mapped, use a value converter" error. - IMemberClassifier, MemberClassifier and RelationalMemberClassifier are now public (moved out of *.Internal) so providers don't have to depend on internal code. Promotes IMemberClassifier/MemberClassifier from Microsoft.EntityFrameworkCore.Metadata.Internal to Microsoft.EntityFrameworkCore.Metadata, with a new public MemberClassifierDependencies parameter object (following the standard EF service-dependencies pattern) and real public XML docs. RelationalMemberClassifier moves to the same public namespace, dropping the EF1001 suppressions that were only needed while the base type was internal. Scope is the core property-bag misdetection plus the API publicization; producing a JSON mapping for Dictionary<string,object> remains the provider's responsibility. Tests in RelationalModelValidatorTest (inherited by SqlServerModelValidatorTest) cover: the member becoming a scalar property (not a navigation) with the column type set; full validation reporting the clear PropertyNotMapped error; the value-converter end-to-end path; and that a [Column] with only a name (no TypeName) is still a property bag. Public API baselines regenerated.
Maintainer (@AndriySvyryd) — reshape IMemberClassifier into classification primitives: - GetNavigationCandidates and GetInverseCandidateTypes are now [EntityFrameworkInternal] default interface implementations built on the primitives. - FindCandidateNavigationPropertyType -> IsCandidateNavigationProperty: returns bool, with out Type? elementType (the former return), out bool? shouldBeOwned (retained — consumed by ModelValidator), and new out bool explicitlyConfigured. - IsCandidatePrimitiveProperty: added out bool explicitlyConfigured. - FindServicePropertyCandidateBindingFactory -> IsCandidateServiceProperty: returns bool, with out IParameterBindingFactory? bindingFactory and out bool explicitlyConfigured. - Extracted protected virtual TypeConfigurationType? GetConfigurationType(Type, IConventionModel); made TypeConfigurationType and TypeConfigurationTypeExtensions public (moved out of *.Internal). - Updated all call sites (ModelValidator, Relationship/Navigation/Service/Property discovery conventions) and the RelationalMemberClassifier overrides; regenerated API baselines. Copilot: - Clarified the RelationalMemberClassifier doc to say it honors an explicitly configured column type (e.g. ColumnAttribute.TypeName) rather than a generic "store type". - Initialized the test entities' Dictionary properties with `= null!;` to match the file's prevalent style. Pushed back (no change): - Copilot CS8618/null concern: the relational test project is <Nullable>disable</Nullable>, so CS8618 cannot fire (build is warning-clean), and the converter lambdas are never executed (model validation only builds the model). - Copilot null-validation in MemberClassifierDependencies: EF dependency records (TypeMappingSourceDependencies, ModelValidatorDependencies, RuntimeModelDependencies, ...) do not null-check in their [EntityFrameworkInternal] DI-only constructors; adding checks would diverge from the established pattern.
ajcvickers
force-pushed
the
fix/26903-dictionary-object-property-bag
branch
from
e6d687e to
4c97d98
Compare
| || propertyInfo.IsCandidateProperty(needsWrite: true)) | ||
| && IsCandidateNavigationPropertyType(targetType, memberInfo, model, useAttributes, out shouldBeOwned, out explicitlyConfigured)) | ||
| { | ||
| elementType = targetType; |
Member
There was a problem hiding this comment.
For non-collection navigations elementType should be null. The callers need to be adjusted to use memberInfo.GetMemberType() in that case
This file contains hidden or 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
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.
Fixes ##26903
Most of the changes here are due to making pubternal types public so then can be used by providers.
Dictionary<string,object>with an explicit column type (e.g.[Column(TypeName="jsonb")]) was detected as a property bag, throwing the "shared-type entity type" error.Dictionary<string,object>has no mapping,Dictionary<string,string>does.[Column(TypeName)]so the member is discovered as a scalar property instead of a property bag.Promotes IMemberClassifier/MemberClassifier from Microsoft.EntityFrameworkCore.Metadata.Internal to Microsoft.EntityFrameworkCore.Metadata, with a new public MemberClassifierDependencies parameter object (following the standard EF service-dependencies pattern) and real public XML docs. RelationalMemberClassifier moves to the same public namespace, dropping the EF1001 suppressions that were only needed while the base type was internal.
Scope is the core property-bag misdetection plus the API publicization; producing a JSON mapping for Dictionary<string,object> remains the provider's responsibility.
Tests in RelationalModelValidatorTest (inherited by SqlServerModelValidatorTest) cover: the member becoming a scalar property (not a navigation) with the column type set; full validation reporting the clear PropertyNotMapped error; the value-converter end-to-end path; and that a [Column] with only a name (no TypeName) is still a property bag. Public API baselines regenerated.