Improve conflicting member exception message#38222
Open
m-x-shokhzod wants to merge 2 commits intodotnet:mainfrom
Open
Improve conflicting member exception message#38222m-x-shokhzod wants to merge 2 commits intodotnet:mainfrom
m-x-shokhzod wants to merge 2 commits intodotnet:mainfrom
Conversation
When adding a member with a name that already exists, the previous exception only stated "a property or navigation with the same name already exists" without distinguishing what kind of member the conflict was with, and unconditionally included the declaring type even when it was the same as the type being added to. Address the three improvements requested in dotnet#36487: 1. Specify the kind of the conflicting member (property, complex property, navigation, skip navigation, or service property). 2. Include the declaring type only when it differs from the current type — split into two resources, ConflictingPropertyOrNavigation for same-type conflicts and ConflictingPropertyOrNavigationOnBaseType for conflicts inherited from a base type. 3. Add explicit removal guidance: "Remove the existing X first." Add two helpers in PropertyBaseExtensions: - GetMemberKindString returns the human-readable kind label. - FormatConflictingMemberMessage picks the right resource based on declaring-type-vs-current-type and assembles the message. Update all eight throw sites to use the helper (or the new resource directly when the conflicting member is not in scope). Update affected tests to assert against the new messages. Fixes dotnet#36487
m-x-shokhzod
force-pushed
the
fix/conflicting-member-exception-message
branch
from
5379f2a to
9425fcf
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the exception thrown when adding a structural type member whose name conflicts with an existing member, by making the message more specific (conflicting member kind), less redundant (declaring type only when different), and more actionable (explicit removal guidance). It updates the core string resources, introduces a formatting helper, rewires the relevant throw sites, and updates unit/spec tests to assert the new messages.
Changes:
- Updated
CoreStrings.ConflictingPropertyOrNavigationand addedConflictingPropertyOrNavigationOnBaseTypeto differentiate same-type vs inherited conflicts and to include removal guidance. - Added
FormatConflictingMemberMessagehelper and updated all relevant throw sites to use it (or the new resource directly). - Updated affected tests to assert against the new, more detailed exception messages.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Tests/Metadata/Internal/InternalEntityTypeBuilderTest.cs | Updates expectations for the new conflict messages (including kind/base-type distinctions). |
| test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs | Updates many conflict-related assertions to include the conflicting member kind. |
| test/EFCore.Tests/Metadata/Internal/EntityTypeTest.BaseType.cs | Updates base-type conflict tests to assert the new base-type-specific resource. |
| test/EFCore.Specification.Tests/ModelBuilding/ModelBuilderTest.OneToOne.cs | Updates spec test assertion for duplicate self-referencing navigation conflict message. |
| src/EFCore/Properties/CoreStrings.resx | Rewords conflict message, adds base-type-specific resource, and adds removal guidance sentence. |
| src/EFCore/Properties/CoreStrings.Designer.cs | Regenerates resource accessors for updated/new string resources (including signature changes). |
| src/EFCore/Metadata/Internal/TypeBase.cs | Switches conflict throws to use the new formatting helper. |
| src/EFCore/Metadata/Internal/PropertyBaseExtensions.cs | Adds FormatConflictingMemberMessage helper to centralize conflict message formatting. |
| src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs | Routes conflict throw through the new helper. |
| src/EFCore/Metadata/Internal/EntityType.cs | Routes multiple conflict throws through the new helper. |
| src/EFCore/Metadata/Builders/ReferenceNavigationBuilder.cs | Updates conflict throw to use the new resource signature (supplying kind). |
| src/EFCore/EFCore.baseline.json | Updates API baseline for the modified/added CoreStrings members. |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
Comment on lines
+361
to
+366
| var declaringTypeDisplayName = ((IReadOnlyTypeBase)conflictingMember.DeclaringType).DisplayName(); | ||
|
|
||
| return declaringTypeDisplayName == owningTypeDisplayName | ||
| ? CoreStrings.ConflictingPropertyOrNavigation(newMemberName, owningTypeDisplayName, conflictingMemberKind) | ||
| : CoreStrings.ConflictingPropertyOrNavigationOnBaseType( | ||
| newMemberName, owningTypeDisplayName, conflictingMemberKind, declaringTypeDisplayName); |
Comment on lines
+359
to
+361
| var conflictingMemberKind = conflictingMember.GetType().Name; | ||
| var owningTypeDisplayName = owningType.DisplayName(); | ||
| var declaringTypeDisplayName = ((IReadOnlyTypeBase)conflictingMember.DeclaringType).DisplayName(); |
There was a problem hiding this comment.
The exception text now includes {conflictingMemberKind}, but conflictingMemberKind is populated via conflictingMember.GetType().Name. This couples a user-facing message to internal implementation type names (and would produce values like RuntimeProperty/RuntimeSkipNavigation if a runtime-model member ever flows here), and also yields non-humanized labels like SkipNavigation/ComplexProperty. Consider implementing the planned kind-label helper (e.g. map by interface/type to "property", "complex property", "navigation", "skip navigation", "service property") and using that instead of GetType().Name.
- Preserve the public CoreStrings.ConflictingPropertyOrNavigation API by restoring its original (member, type, conflictingType) signature and message, and marking it [Obsolete]. Introduce a new ConflictingPropertyOrNavigationWithKind resource for same-type conflicts so existing extensions are not broken. - Use metadata instance equality (DeclaringType == owningType) instead of comparing DisplayName() strings to decide between the same-type and base-type messages, avoiding misclassification when two distinct types share a simple name. - Replace conflictingMember.GetType().Name with a humanized GetMemberKindString helper (property, complex property, navigation, skip navigation, service property) so user-facing messages do not leak internal CLR class names like RuntimeProperty/SkipNavigation.
m-x-shokhzod
force-pushed
the
fix/conflicting-member-exception-message
branch
from
f4c9993 to
3f0bb65
Compare
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.
When adding a member with a name that already exists, the previous exception only stated "a property or navigation with the same name already exists" without distinguishing what kind of member the conflict was with, and unconditionally included the declaring type even when it was the same as the type being added to.
Address the three improvements requested in #36487:
Add two helpers in PropertyBaseExtensions:
Update all eight throw sites to use the helper (or the new resource directly when the conflicting member is not in scope).
Update affected tests to assert against the new messages.