Skip to content

Fix inverting identifying FK using wrong type and short-circuit principal type on attach#38423

Open
Copilot wants to merge 8 commits into
mainfrom
copilot/various-model-building-issues
Open

Fix inverting identifying FK using wrong type and short-circuit principal type on attach#38423
Copilot wants to merge 8 commits into
mainfrom
copilot/various-model-building-issues

Conversation

Copilot AI commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Fixes two long-standing issues in EF Core's relationship-building engine.

1. Pass in principal type when attaching FK

RelationshipSnapshot now captures PrincipalEntityType and forwards it to InternalForeignKeyBuilder.Attach, which gains an optional EntityType? principalEntityType parameter. When the principal is known and still in the model, Attach short-circuits the heuristic re-resolution (InternalForeignKeyBuilder.cs ~L3499–3540), improving perf. The short-circuit matches on ClrType only for non-shared types to avoid colliding distinct shared-type entities; the existing re-resolution remains for the principal-not-in-model case.

Regression tests added in InternalForeignKeyBuilderTest:

  • Attach_uses_provided_principal_type_when_in_model — verifies the short-circuit fires when the captured principal is still in the model.
  • Attach_does_not_bind_to_wrong_shared_type_principal_by_clr_type — registers two shared-type entities with the same CLR type; passing the wrong one must not cause incorrect binding (the !HasSharedClrType guard disables CLR-type matching for shared types).
  • Attach_falls_back_to_model_lookup_when_provided_principal_is_not_in_model — verifies the heuristic fallback correctly resolves the principal when the captured entity is stale.

2. Inverting identifying FK to derived no longer moves the FK to base

The invert branch of SetRelatedTypes (InternalForeignKeyBuilder.cs ~L1440–1441) used LeastDerivedType, which collapsed onto the base when the requested type was a proper base of the old opposite end. This is the bug: inverting an identifying FK between a non-derived type and a derived type produced an FK between two non-derived types. The invert branch now keeps the more-derived end, so the inverted FK stays between the derived and non-derived type. Added regression test Inverting_identifying_relationship_keeps_derived_dependent.

Copilot AI changed the title [WIP] Fix various model building issues in EF Core Investigation: remaining model-building items require non-surgical reworks Jun 13, 2026
Copilot AI requested a review from AndriySvyryd June 13, 2026 00:24
@AndriySvyryd

Copy link
Copy Markdown
Member

@copilot Feel free to enhance the logic in SetOrAddForeignKey/CreateForeignKey, but make sure that this doesn't add code duplication
Pass in principal type when attaching FK when known to short-circuit logic and improve perf.
Find a scenario where there's an identifying FK between a non-derived type and a derived type where inverting it makes it go from non-derived type to non-derived type, this is the bug. The inverted FK should be between a derived type and a non-derived type

…ort-circuit re-resolution

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is presented as an investigation into remaining relationship/model-building behaviors in EF Core, but it also includes code changes that alter the attach/snapshot plumbing for relationships.

Changes:

  • Capture and store the relationship’s principal entity type in RelationshipSnapshot.
  • Thread the captured principal entity type into InternalForeignKeyBuilder.Attach via a new optional parameter.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/EFCore/Metadata/Internal/RelationshipSnapshot.cs Stores PrincipalEntityType on snapshot creation and passes it into Relationship.Attach(...) during reattach.
src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs Extends Attach to accept an optional principalEntityType and prefers it when it appears valid/in-model.

Comment thread src/EFCore/Metadata/Internal/RelationshipSnapshot.cs
Comment thread src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs
Copilot AI and others added 3 commits June 17, 2026 20:43
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>

Copilot AI commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Feel free to enhance the logic in SetOrAddForeignKey/CreateForeignKey, but make sure that this doesn't add code duplication
Pass in principal type when attaching FK when know...

All three items are now implemented (the previous PR shipped no code):

  • 1 (single-step FK creation): InternalEntityTypeBuilder.HasForeignKey threads the dependent properties through HasRelationshipInternal/CreateForeignKey/SetOrAddForeignKey, so the FK is created with the real properties instead of conventional shadow properties followed by a replace. To avoid duplicating the FK matching/promotion in InternalForeignKeyBuilder.HasForeignKey, SetOrAddForeignKey falls back to shadow-property creation when a matching FK already exists in the hierarchy, leaving the match/promotion to the existing path.
  • 2 (pass principal type when attaching): RelationshipSnapshot captures PrincipalEntityType and forwards it to InternalForeignKeyBuilder.Attach, which short-circuits the heuristic re-resolution when the principal is known and still in the model (matching on ClrType only for non-shared types).
  • 3 (invert identifying FK): As you described — when inverting an identifying FK between a non-derived and a derived type, the inverted FK now stays between the derived and non-derived type instead of collapsing onto the base. Added regression test Inverting_identifying_relationship_keeps_derived_dependent.

Validated against the full EFCore.Tests (6917) and EFCore.InMemory.FunctionalTests (27955) suites.

Copilot AI changed the title Investigation: remaining model-building items require non-surgical reworks Implement model-building FK engine reworks: single-step FK creation, Attach principal short-circuit, invert identifying FK fix Jun 17, 2026
Comment thread src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs Outdated
…ionship tests

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI changed the title Implement model-building FK engine reworks: single-step FK creation, Attach principal short-circuit, invert identifying FK fix Implement model-building FK engine reworks: Attach principal short-circuit, invert identifying FK fix Jun 17, 2026
Copilot AI requested a review from AndriySvyryd June 17, 2026 22:15
@AndriySvyryd AndriySvyryd changed the title Implement model-building FK engine reworks: Attach principal short-circuit, invert identifying FK fix Fix inverting identifying FK using wrong type and short-circuit principal type on attach Jun 17, 2026
@AndriySvyryd AndriySvyryd marked this pull request as ready for review June 17, 2026 23:10
@AndriySvyryd AndriySvyryd requested a review from a team as a code owner June 17, 2026 23:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs
Copilot AI and others added 2 commits June 18, 2026 00:43
…e guard

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot June 18, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants