From e94b7498060441ce2b9e2e577bca084e9bb7e882 Mon Sep 17 00:00:00 2001 From: = Date: Thu, 16 Jul 2026 21:17:03 +0200 Subject: [PATCH 1/5] refactor(ef): single source for the owned-type effective-table rule The effective-table rule (owned type shares its owner's table unless it declares its own) was implemented verbatim in both FluentOwnedTypeWalker and MermaidErdRenderer, so a future change to EF table-sharing shapes could silently drift the capture and render paths apart. Hoist it onto EfEntity.EffectiveTable alongside EffectiveKey and point both call sites at it. Pure refactor - no golden moves. Closes #164 Co-Authored-By: Claude Fable 5 --- src/ProjGraph.Core/Models/EfModel.cs | 8 ++++++++ .../Infrastructure/FluentOwnedTypeWalker.cs | 9 ++------- .../Rendering/MermaidErdRenderer.cs | 10 +--------- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/ProjGraph.Core/Models/EfModel.cs b/src/ProjGraph.Core/Models/EfModel.cs index 540a60c..1c0f538 100644 --- a/src/ProjGraph.Core/Models/EfModel.cs +++ b/src/ProjGraph.Core/Models/EfModel.cs @@ -62,6 +62,14 @@ public class EfEntity /// public string EffectiveKey => string.IsNullOrEmpty(Key) ? Name : Key; + /// + /// Gets the table this entity effectively maps to: its explicit , or its + /// when unmapped (EF's default). The single source of the table-sharing rule — + /// an owned type shares its owner's table exactly when their effective tables are equal — used by + /// both the capture side (owned-type table resolution) and the render side (inline-vs-box). + /// + public string EffectiveTable => string.IsNullOrEmpty(TableName) ? Name : TableName; + /// /// Gets or initializes a value indicating whether the entity is an EF Core owned type /// (configured via OwnsOne/OwnsMany) rather than a root entity. diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs index 34512cd..84bb18f 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs @@ -331,7 +331,7 @@ public static void ResolveTables(Dictionary entities, EfModel continue; } - var ownerTable = EffectiveTable(owner); + var ownerTable = owner.EffectiveTable; var table = owned.IsCollection ? $"{ownerTable}_{owned.NavigationName}" : ownerTable; var updated = EfEntityFactory.CopyWith(owned, table); @@ -365,7 +365,7 @@ public static void StripShadowKeys(Dictionary entities, EfMode { var sharesOwnerTable = owned.OwnerEntity is not null && entities.TryGetValue(owned.OwnerEntity, out var owner) - && EffectiveTable(owner) == EffectiveTable(owned); + && owner.EffectiveTable == owned.EffectiveTable; var stripped = EfEntityFactory.CopyWith(owned); stripped.Properties.Clear(); @@ -386,9 +386,4 @@ public static void StripShadowKeys(Dictionary entities, EfMode EfEntityFactory.ReplaceModelSlot(model, stripped); } } - - /// Returns an entity's effective table: its explicit table name, or its entity name when unmapped. - /// The entity. - private static string EffectiveTable(EfEntity entity) - => string.IsNullOrEmpty(entity.TableName) ? entity.Name : entity.TableName; } diff --git a/src/ProjGraph.Lib.EntityFramework/Rendering/MermaidErdRenderer.cs b/src/ProjGraph.Lib.EntityFramework/Rendering/MermaidErdRenderer.cs index d763c32..7225a68 100644 --- a/src/ProjGraph.Lib.EntityFramework/Rendering/MermaidErdRenderer.cs +++ b/src/ProjGraph.Lib.EntityFramework/Rendering/MermaidErdRenderer.cs @@ -69,14 +69,6 @@ private static void RenderEntities(EfModel model, StringBuilder sb, DiagramOptio } } - /// - /// Returns the table an entity effectively maps to: its explicit table name, or its entity name - /// when unmapped (EF's default). - /// - /// The entity. - private static string EffectiveTable(EfEntity entity) - => string.IsNullOrEmpty(entity.TableName) ? entity.Name : entity.TableName; - /// /// Determines whether an owned entity's columns are folded into its owner rather than drawn as their /// own box: true when it shares the owner's table (EF table-splitting). Always false in Classic mode. @@ -114,7 +106,7 @@ private static bool IsInlined(EfEntity entity, EfModel model, DiagramOptions? op } var owner = model.Entities.FirstOrDefault(e => e.EffectiveKey == entity.OwnerEntity); - return owner is not null && EffectiveTable(owner) == EffectiveTable(entity); + return owner is not null && owner.EffectiveTable == entity.EffectiveTable; } /// From 33737881f0e1003c70575bb00ea07c69465ccbca Mon Sep 17 00:00:00 2001 From: = Date: Thu, 16 Jul 2026 21:21:27 +0200 Subject: [PATCH 2/5] chore(ef): minor debt roundup from PR #161 reviews - Document ResolveSourceEntity's unbounded ancestor walk precondition: it is safe only while relationship chains are never captured inside owned builders; bounding is required if that ever changes. - Consolidate the nested-builder boundary rule into one FluentSyntax.IsNestedBuilderFence predicate shared by IsInsideNestedBuilderScope and ResolveOwningEntity's ancestor search. - Keep (not remove) ForeignKeyPropertyNames' lambda branch: contrary to the issue premise, OwnershipBuilder HAS an Expression HasForeignKey overload (EF Core 3.0+), so the branch is live; documented and pinned with a regression test instead. - Reword the OwnsMany because-string to drop the doubled-brace CA2241 workaround that rendered literally in failure messages. - Close test-coverage boundaries: assert the Address box exists before splitting in Classic_OwnerDoesNotGainNavigationColumn; new test that an inlined undrawn same-named sibling does not force a qualified label onto the sole rendered box; new OwnedFkConfigContext fixture exercising StripShadowKeys against a config-class owned type with explicit WithOwner().HasForeignKey/HasKey. Closes #165 Co-Authored-By: Claude Fable 5 --- .../Infrastructure/FluentOwnedTypeWalker.cs | 7 ++- .../FluentRelationshipWalker.cs | 8 +++ .../Infrastructure/FluentSyntax.cs | 35 +++++++----- .../Golden/fixtures/OwnedFkConfigContext.cs | 54 +++++++++++++++++++ .../FluentOwnedTypeWalkerTests.cs | 39 +++++++++++++- .../Rendering/OwnedTypeRenderingTests.cs | 54 +++++++++++++++++++ 6 files changed, 182 insertions(+), 15 deletions(-) create mode 100644 tests/ProjGraph.Tests.Unit.EntityFramework/Golden/fixtures/OwnedFkConfigContext.cs diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs index 84bb18f..a977658 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs @@ -197,7 +197,12 @@ private static void ApplyOwnedForeignKey(SyntaxNode ownedScope, DictionaryExtracts the property names from a HasForeignKey call (lambda member access or string literals). + /// + /// Extracts the property names from a HasForeignKey call. Both branches are live: + /// snapshots emit the params string[] form, while on the DbContext path the generic + /// OwnershipBuilder<TEntity,TDependentEntity> a builder-lambda's WithOwner() + /// returns also has an Expression overload (HasForeignKey(x => x.OwnerId)). + /// /// The HasForeignKey invocation. private static IEnumerable ForeignKeyPropertyNames(InvocationExpressionSyntax invocation) { diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs index 5850729..3baa852 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs @@ -108,6 +108,14 @@ private static bool IsInsideUsingEntity(SyntaxNode node) } // Entity(e => e.HasMany(...)): the Has call is inside the Entity configuration lambda. + // + // PRECONDITION: this ancestor walk is unbounded — it climbs past owned-type / join-entity + // builder fences without stopping, unlike FluentSyntax.ResolveOwningEntity. That is safe only + // because FindRelationshipRoots never yields a Has call from inside such a builder (UsingEntity + // is fenced there, and relationships inside OwnsOne/OwnsMany builders are not captured at all). + // If HasOne/HasMany capture inside owned builders is ever added, this walk must stop at + // nested-builder boundaries too, or it will reattribute the relationship to whatever + // Entity() lies beyond the fence. var enclosingEntity = chain.HasNode.Ancestors() .OfType() .FirstOrDefault(inv => inv.Expression is MemberAccessExpressionSyntax ma diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentSyntax.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentSyntax.cs index a64f8f2..53724b8 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentSyntax.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentSyntax.cs @@ -63,9 +63,24 @@ public static bool IsInsideNestedBuilderScope(SyntaxNode node, SyntaxNode scope) return node.Ancestors() .TakeWhile(ancestor => ancestor != scope && scope.Span.Contains(ancestor.Span)) .OfType() - .Any(inv => inv.Expression is MemberAccessExpressionSyntax ma - && NestedBuilderScopes.Contains(SimpleName(ma.Name)) - && inv.ArgumentList.Span.Contains(node.Span)); + .Any(inv => IsNestedBuilderFence(inv, node)); + } + + /// + /// Determines whether fences off from the outer + /// entity: an owned-type / join-entity builder call () whose ARGUMENT + /// LIST contains the node. The argument list — not the whole invocation — is tested because such a call + /// is itself chained onto the entity being configured, so a node on its receiver spine is not fenced by + /// it. The single home of the boundary rule shared by and + /// 's ancestor search. + /// + /// The candidate fence invocation. + /// The node being tested. + private static bool IsNestedBuilderFence(InvocationExpressionSyntax invocation, SyntaxNode node) + { + return invocation.Expression is MemberAccessExpressionSyntax ma + && NestedBuilderScopes.Contains(SimpleName(ma.Name)) + && invocation.ArgumentList.Span.Contains(node.Span); } /// @@ -121,23 +136,17 @@ public static bool IsInsideNestedBuilderScope(SyntaxNode node, SyntaxNode scope) InvocationExpressionSyntax? enclosingEntity = null; foreach (var ancestor in configInvocation.Ancestors().OfType()) { - if (ancestor.Expression is not MemberAccessExpressionSyntax ma) - { - continue; - } - - var callName = SimpleName(ma.Name); - - // Climbing past an owned-type / join-entity builder invocation (e.g. the OwnsOne call whose + // Climbing past an owned-type / join-entity builder fence (e.g. the OwnsOne call whose // argument list configInvocation lives inside) would reattribute this call to whatever // Entity() lies beyond it. Stop instead, so ambientEntity — the owned/join target the // caller scoped this search to — wins. - if (NestedBuilderScopes.Contains(callName)) + if (IsNestedBuilderFence(ancestor, configInvocation)) { break; } - if (callName == EfAnalysisConstants.EfMethods.Entity) + if (ancestor.Expression is MemberAccessExpressionSyntax ma + && SimpleName(ma.Name) == EfAnalysisConstants.EfMethods.Entity) { enclosingEntity = ancestor; break; diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/Golden/fixtures/OwnedFkConfigContext.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/Golden/fixtures/OwnedFkConfigContext.cs new file mode 100644 index 0000000..8f27e34 --- /dev/null +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/Golden/fixtures/OwnedFkConfigContext.cs @@ -0,0 +1,54 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Metadata.Builders; +namespace Fixtures; + +// Fixture for owned types whose IEntityTypeConfiguration builder declares the ownership keys +// explicitly (WithOwner().HasForeignKey / HasKey), the shape a hand-written config class shares with +// generated snapshots: +// Sender - table-split (no ToTable), string-form keys -> StripShadowKeys must drop the FK column +// and clear the PK marker, exactly as on the snapshot path +// Recipient - own table (ToTable), lambda-form HasForeignKey (the Expression overload on the generic +// OwnershipBuilder returned by WithOwner()) -> the FK is a +// real, separate column that must be kept and marked +public class OwnedFkConfigContext : DbContext +{ + public DbSet Parcels { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.ApplyConfiguration(new ParcelConfiguration()); +} + +public class ParcelConfiguration : IEntityTypeConfiguration +{ + public void Configure(EntityTypeBuilder builder) + { + builder.HasKey(p => p.Id); + + builder.OwnsOne(p => p.Sender, a => + { + a.WithOwner().HasForeignKey("ParcelId"); + a.HasKey("ParcelId"); + a.Property(x => x.Street).HasMaxLength(120); + }); + + builder.OwnsOne(p => p.Recipient, a => + { + a.ToTable("ParcelRecipients"); + a.WithOwner().HasForeignKey(x => x.ParcelId); + a.Property(x => x.Street).HasMaxLength(120); + }); + } +} + +public class Parcel +{ + public int Id { get; set; } + public ParcelEndpoint Sender { get; set; } = null!; + public ParcelEndpoint Recipient { get; set; } = null!; +} + +public class ParcelEndpoint +{ + public int ParcelId { get; set; } + public string Street { get; set; } = ""; +} diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/Infrastructure/FluentOwnedTypeWalkerTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/Infrastructure/FluentOwnedTypeWalkerTests.cs index 278587d..37ec04f 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/Infrastructure/FluentOwnedTypeWalkerTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/Infrastructure/FluentOwnedTypeWalkerTests.cs @@ -141,7 +141,8 @@ public void OwnsMany_CapturesCollectionOwnedTypeOnItsOwnTable() lines.IsCollection.Should().BeTrue(); lines.OwnerEntity.Should().Be("OwnedModesInvoice"); lines.TableName.Should().Be("OwnedModesInvoice_Lines", - "an owned collection never shares the owner's table; EF's default is {{OwnerTable}}_{{Nav}}"); + "an owned collection never shares the owner's table; EF defaults it to the owner's table " + + "name suffixed with the navigation name"); lines.Properties.Should().ContainSingle(p => p.Name == "Amount") .Which.Precision.Should().Be(18); } @@ -253,6 +254,42 @@ public void ConfigClass_OwnedTypes_DoNotLeakOntoOwner() "owned navigations are not columns"); } + [Fact] + public void ConfigClass_TableSplitOwnedTypeWithExplicitOwnershipKeys_StripsShadowKeyAndFkColumn() + { + // StripShadowKeys against a config-class owned type whose builder declares the ownership keys + // explicitly (WithOwner().HasForeignKey / HasKey) - the same shape a generated snapshot always + // emits, but reached through the IEntityTypeConfiguration path. + var model = Analyze("OwnedFkConfigContext.cs", "OwnedFkConfigContext"); + + var sender = model.Entities.Should().ContainSingle(e => e.Key == "Parcel.Sender").Subject; + sender.TableName.Should().Be("Parcel", "OwnsOne without ToTable table-splits onto the owner"); + sender.Properties.Should().NotContain(p => p.Name == "ParcelId", + "a table-split owned type's explicit WithOwner().HasForeignKey column is the owner's own PK " + + "re-projected, not an extra column"); + sender.Properties.Should().NotContain(p => p.IsPrimaryKey, + "the explicit HasKey on a table-split owned type declares EF's shadow key, an implementation " + + "detail that must not surface as a modelled PK"); + sender.Properties.Should().ContainSingle(p => p.Name == "Street") + .Which.MaxLength.Should().Be(120); + } + + [Fact] + public void ConfigClass_OwnTableOwnedType_LambdaFormHasForeignKey_KeepsRealFkColumn() + { + // Pins ForeignKeyPropertyNames' lambda branch as LIVE: the generic OwnershipBuilder returned by + // WithOwner() has had an Expression HasForeignKey overload since EF Core 3.0, so the branch is + // reachable from real user code and must keep extracting the FK property name. + var model = Analyze("OwnedFkConfigContext.cs", "OwnedFkConfigContext"); + + var recipient = model.Entities.Should().ContainSingle(e => e.Key == "Parcel.Recipient").Subject; + recipient.TableName.Should().Be("ParcelRecipients"); + recipient.Properties.Should().ContainSingle(p => p.Name == "ParcelId") + .Which.IsForeignKey.Should().BeTrue( + "an owned type on its own table has a real, separate FK column back to the owner, and " + + "the lambda form WithOwner().HasForeignKey(x => x.ParcelId) must mark it"); + } + [Fact] public void OwnsOne_OwnerToTableAppliedBySeparateConfigClass_ResolvesToConfigClassTable() { diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/Rendering/OwnedTypeRenderingTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/Rendering/OwnedTypeRenderingTests.cs index d5ba8f9..43015af 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/Rendering/OwnedTypeRenderingTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/Rendering/OwnedTypeRenderingTests.cs @@ -176,6 +176,8 @@ public void MirrorEf_NestedOwnedType_InlinesOntoTrueOwnerOnlyNotSameNamedSibling // Geo table-splits onto ShipTo, which itself table-splits onto Invoice, so the nested // Geo_Latitude column compounds onto Invoice's box under the ShipTo_Geo_ prefix. + // The Contain below is the discriminating assertion: pre-fix, Name-keyed matching dropped the + // column entirely rather than duplicating it, so the NotContain passes both pre- and post-fix. output.Should().Contain("ShipTo_Geo_Latitude", "Geo must be attributed to its true owner ShipTo, keyed Invoice.ShipTo"); output.Should().NotContain("BillTo_Geo_Latitude", @@ -228,6 +230,54 @@ public void MirrorEf_DisplayName_QualifiesCollidingBoxesAndRelationshipsReferenc output.Should().Contain("Order ||--|| Order_BillTo"); } + [Fact] + public void MirrorEf_InlinedSameNamedSibling_DoesNotForceQualifiedLabelOntoSoleRenderedBox() + { + // Order owns ShipTo (table-split onto Orders, so inlined and never drawn) and BillTo (its own + // table, so drawn). Both are CLR "Address". Only entities actually drawn can collide on the + // diagram, so the undrawn ShipTo must not force the sole rendered Address box to the qualified + // Order_BillTo label. + var order = new EfEntity { Name = "Order", TableName = "Orders" }; + order.Properties.Add(new EfProperty { Name = "Id", Type = "int", IsPrimaryKey = true, IsValueType = true }); + + var shipTo = new EfEntity + { + Name = "Address", + Key = "Order.ShipTo", + IsOwned = true, + OwnerEntity = "Order", + NavigationName = "ShipTo", + TableName = "Orders" + }; + shipTo.Properties.Add(new EfProperty { Name = "City", Type = "string" }); + + var billTo = new EfEntity + { + Name = "Address", + Key = "Order.BillTo", + IsOwned = true, + OwnerEntity = "Order", + NavigationName = "BillTo", + TableName = "BillToAddresses" + }; + billTo.Properties.Add(new EfProperty { Name = "City", Type = "string" }); + + var model = new EfModel { ContextName = "Ctx" }; + model.Entities.Add(order); + model.Entities.Add(shipTo); + model.Entities.Add(billTo); + + var output = Render(model); + + output.Should().Contain("ShipTo_City", "the table-split sibling inlines onto Order"); + output.Should().Contain("Address {", + "the only rendered box of this name must keep its plain label"); + output.Should().NotContain("Order_BillTo", + "an inlined, undrawn sibling cannot collide on the diagram, so it must not force " + + "qualification onto the sole rendered box of that name"); + output.Should().Contain("Order ||--|| Address", "the relationship must reference the plain label"); + } + [Fact] public void Classic_TableSplitOwnsOne_RendersBoxAndIdentifyingRelationship() { @@ -250,6 +300,10 @@ public void Classic_OwnerDoesNotGainNavigationColumn() var output = new MermaidErdRenderer().Render( model, new DiagramOptions(false, false, false, ErdOwnedMode.Classic)); + // Assert the box exists before splitting on it: if "Address {" were ever missing, [0] would be + // the WHOLE output and the NotContain below would test the wrong thing entirely. + output.Should().Contain("Address {", "classic mode always gives the owned type its own box"); + var orderBlock = output.Split("Address {")[0]; orderBlock.Should().NotContain("ShipToAddress", "the relationship line carries the navigation; the owner gets no reference column"); From 41b2a20f9f3613597567173ee81dc6920dd50684 Mon Sep 17 00:00:00 2001 From: = Date: Thu, 16 Jul 2026 21:25:27 +0200 Subject: [PATCH 3/5] fix(ef): close record-type gaps in owned-nav discovery, PK syntax scan, config-class scan Three ClassDeclarationSyntax/PropertyDeclarationSyntax-only sites kept record-declared types invisible to EF analysis: - Owned-nav pre-pass: a positional record's navigation is a primary-constructor parameter, not a property member, so its owned type's file was never discovered and the owned entity degraded to an empty box. The nav lookup now also reads RecordDeclarationSyntax.ParameterList. - EntityAnalyzer.ExtractPrimaryKeysFromSyntax: the [PrimaryKey]/[Key] syntax fallback skipped RecordDeclarationSyntax; widened to TypeDeclarationSyntax. - EntityConfigurationWalker.FindConfigClassesInRoot: a record-declared IEntityTypeConfiguration was silently skipped; widened to TypeDeclarationSyntax. Each gap is pinned by a regression test verified to fail pre-fix. Closes #163 Co-Authored-By: Claude Fable 5 --- .../Infrastructure/EfModelAnalyzer.cs | 27 ++++++++-- .../Infrastructure/EntityAnalyzer.cs | 23 +++++---- .../EntityConfigurationWalker.cs | 11 ++-- .../EntityAnalyzerTests.cs | 26 ++++++++++ .../EntityConfigurationWalkerTests.cs | 28 +++++++++++ .../OwnedRecordOwnerRegressionTests.cs | 50 +++++++++++++++++++ 6 files changed, 148 insertions(+), 17 deletions(-) diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs index cb45999..b124236 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs @@ -348,9 +348,8 @@ private async Task ResolveOwnedNavigationTypesAsync( classDeclsByName.TryGetValue(ownerClrType, out ownerDecl); } - var propertyDecl = ownerDecl?.Members.OfType() - .FirstOrDefault(p => p.Identifier.Text == navigation); - var ownedTypeName = propertyDecl is null ? null : OwnedClrTypeName(propertyDecl.Type, isCollection); + var navigationType = ownerDecl is null ? null : OwnedNavigationTypeSyntax(ownerDecl, navigation); + var ownedTypeName = navigationType is null ? null : OwnedClrTypeName(navigationType, isCollection); if (ownedTypeName is null) { continue; @@ -373,6 +372,28 @@ private async Task ResolveOwnedNavigationTypesAsync( } } + /// + /// Returns the type syntax of the owner's navigation named : a member + /// property declaration, or — for a positional record like record Product(int Id, Money Price) — + /// the primary-constructor parameter of that name, whose synthesized property is a real EF navigation + /// but never appears as a member. Returns + /// when the owner declares no such navigation. + /// + /// The owner's type declaration. + /// The navigation property name. + private static TypeSyntax? OwnedNavigationTypeSyntax(TypeDeclarationSyntax ownerDecl, string navigation) + { + var propertyDecl = ownerDecl.Members.OfType() + .FirstOrDefault(p => p.Identifier.Text == navigation); + if (propertyDecl is not null) + { + return propertyDecl.Type; + } + + return (ownerDecl as RecordDeclarationSyntax)?.ParameterList?.Parameters + .FirstOrDefault(p => p.Identifier.Text == navigation)?.Type; + } + /// /// Reduces a property's type syntax to the owned CLR type's simple name: unwraps a nullable /// annotation, and — for an OwnsMany navigation — the collection's element type diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs index eea1d95..6ed46d9 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs @@ -167,25 +167,28 @@ private static void ExtractPrimaryKeysFromSyntax(INamedTypeSymbol type, HashSet< { foreach (var syntaxRef in type.DeclaringSyntaxReferences) { - if (syntaxRef.GetSyntax() is not ClassDeclarationSyntax classSyntax) + // TypeDeclarationSyntax, not ClassDeclarationSyntax: a record-declared entity's [PrimaryKey] + // and [Key] attributes live on a RecordDeclarationSyntax, which is not a class declaration — + // the same hazard family that made record-declared owners invisible to owned-nav discovery. + if (syntaxRef.GetSyntax() is not TypeDeclarationSyntax typeSyntax) { continue; } - ExtractPrimaryKeysFromClassAttributes(classSyntax, primaryKeyNames); - ExtractPrimaryKeysFromPropertySyntax(classSyntax, primaryKeyNames); + ExtractPrimaryKeysFromClassAttributes(typeSyntax, primaryKeyNames); + ExtractPrimaryKeysFromPropertySyntax(typeSyntax, primaryKeyNames); } } /// - /// Extracts primary key names from class-level attribute syntax. + /// Extracts primary key names from type-level attribute syntax. /// - /// The class declaration syntax to analyze. + /// The type declaration syntax (class or record) to analyze. /// The set to populate with primary key names. - private static void ExtractPrimaryKeysFromClassAttributes(ClassDeclarationSyntax classSyntax, + private static void ExtractPrimaryKeysFromClassAttributes(TypeDeclarationSyntax typeSyntax, HashSet primaryKeyNames) { - foreach (var attr in classSyntax.AttributeLists.SelectMany(al => al.Attributes)) + foreach (var attr in typeSyntax.AttributeLists.SelectMany(al => al.Attributes)) { var name = attr.Name.ToString(); if (name is not (EfAnalysisConstants.EfAttributes.PrimaryKey @@ -210,12 +213,12 @@ private static void ExtractPrimaryKeysFromClassAttributes(ClassDeclarationSyntax /// /// Extracts primary key names from property syntax with Key attributes. /// - /// The class declaration syntax to analyze. + /// The type declaration syntax (class or record) to analyze. /// The set to populate with primary key names. - private static void ExtractPrimaryKeysFromPropertySyntax(ClassDeclarationSyntax classSyntax, + private static void ExtractPrimaryKeysFromPropertySyntax(TypeDeclarationSyntax typeSyntax, HashSet primaryKeyNames) { - foreach (var prop in classSyntax.Members.OfType() + foreach (var prop in typeSyntax.Members.OfType() .Where(p => p.AttributeLists.SelectMany(al => al.Attributes) .Any(a => a.Name.ToString() is EfAnalysisConstants.EfAttributes.Key or EfAnalysisConstants.EfAttributes.KeyAttribute))) diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs index e724d02..8b96664 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs @@ -143,7 +143,10 @@ private static IEnumerable FindConfigClasses(Compilation compilatio /// The syntax root to scan. internal static IEnumerable FindConfigClassesInRoot(SyntaxNode root) { - foreach (var declaration in root.DescendantNodes().OfType()) + // TypeDeclarationSyntax, not ClassDeclarationSyntax: a record (or struct) can implement + // IEntityTypeConfiguration too, and a class-only scan would silently skip its Configure body — + // the same hazard family that made record-declared owners invisible to owned-nav discovery. + foreach (var declaration in root.DescendantNodes().OfType()) { if (AsConfigClass(declaration) is { } configClass) { @@ -152,9 +155,9 @@ internal static IEnumerable FindConfigClassesInRoot(SyntaxNode root } } - /// Interprets a class declaration as a config class, or returns if it is not one. - /// The class declaration. - private static ConfigClass? AsConfigClass(ClassDeclarationSyntax declaration) + /// Interprets a type declaration as a config class, or returns if it is not one. + /// The type declaration. + private static ConfigClass? AsConfigClass(TypeDeclarationSyntax declaration) { var configInterface = declaration.BaseList?.Types .Select(baseType => baseType.Type) diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs index cb79d23..2b8dca8 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs @@ -211,6 +211,32 @@ public class Ticket entity.Properties.First(p => p.Name == "Title").IsPrimaryKey.Should().BeFalse(); } + [Fact] + public void AnalyzeEntity_RecordWithTypeLevelPrimaryKeyAttribute_SyntaxPathMarksPrimaryKey() + { + // The EF Core [PrimaryKey] attribute is not resolvable in these fixtures (no EF reference), so + // only the syntax fallback can read it - and that fallback filtered on ClassDeclarationSyntax, + // silently skipping record-declared entities. Regression for the widening to + // TypeDeclarationSyntax (issue #163, item 2). + var compilation = RoslynTestHelper.CreateCompilation(""" + using Microsoft.EntityFrameworkCore; + [PrimaryKey(nameof(Code))] + public record Ticket + { + public string Code { get; set; } + public string Title { get; set; } + } + """); + var type = RoslynTestHelper.GetTypeSymbol(compilation, "Ticket")!; + + var entity = EntityAnalyzer.AnalyzeEntity(type); + + entity.Properties.First(p => p.Name == "Code").IsPrimaryKey.Should().BeTrue( + "the type-level [PrimaryKey] attribute lives on a RecordDeclarationSyntax, which the " + + "syntax fallback must not skip"); + entity.Properties.First(p => p.Name == "Title").IsPrimaryKey.Should().BeFalse(); + } + [Fact] public void AnalyzeEntity_RequiredAttribute_ShouldMarkAsRequired() { diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs index 0cdcc52..c2f9d64 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs @@ -72,6 +72,34 @@ void OnModelCreating(dynamic modelBuilder) entities["Widget"].Properties.Single(p => p.Name == "Id").IsPrimaryKey.Should().BeTrue(); } + [Fact] + public void Apply_RecordDeclaredConfigClass_IsDiscoveredAndFolded() + { + // A record can implement IEntityTypeConfiguration too; the class-only scan silently skipped + // its Configure body. Regression for the widening to TypeDeclarationSyntax (issue #163, item 3). + const string source = """ + using Microsoft.EntityFrameworkCore; + using Microsoft.EntityFrameworkCore.Metadata.Builders; + public class Widget { public int Id { get; set; } public string Name { get; set; } = ""; } + public record WidgetConfiguration : IEntityTypeConfiguration + { + public void Configure(EntityTypeBuilder builder) + => builder.Property(w => w.Name).HasMaxLength(64); + } + public class Ctx + { + void OnModelCreating(dynamic modelBuilder) + => modelBuilder.ApplyConfiguration(new WidgetConfiguration()); + } + """; + var (method, compilation, entities, model) = Build(source, "Widget"); + + EntityConfigurationWalker.Apply(method, entities, model, compilation); + + entities["Widget"].Properties.Single(p => p.Name == "Name").MaxLength.Should().Be(64, + "a record-declared IEntityTypeConfiguration must be discovered and its Configure body folded in"); + } + [Fact] public void Apply_ApplyConfiguration_ConfiguresRelationship() { diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/OwnedRecordOwnerRegressionTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/OwnedRecordOwnerRegressionTests.cs index 371f3f3..f0f9ee3 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/OwnedRecordOwnerRegressionTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/OwnedRecordOwnerRegressionTests.cs @@ -98,4 +98,54 @@ public class Money rendered.Should().NotContain("Price {\n }", "a record-declared owner must not fall back to the zero-property empty-box safety net"); } + + [Fact] + public async Task AnalyzeContextAsync_PositionalRecordOwner_CrossFileOwnedRecordColumnsCapturedAndRendered() + { + using var temp = new TestDirectory(); + + // Product is a POSITIONAL record: its Price navigation is a primary-constructor parameter, not a + // PropertyDeclarationSyntax member, so the pre-pass's member-based nav lookup missed it entirely. + // Money.cs then stayed out of the compilation, the navigation resolved to an error type, and the + // owned entity degraded to the spec's empty-box floor. Regression for the parameter-list nav + // lookup (issue #163, item 1); Money is itself a positional record, per the issue's fixture shape. + const string contextContent = """ + using Microsoft.EntityFrameworkCore; + namespace Test; + + public class PositionalRecordContext : DbContext + { + public DbSet Products { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity().OwnsOne(p => p.Price); + } + } + + public record Product(int Id, Money Price); + """; + const string moneyContent = """ + namespace Test; + + public record Money(decimal Amount, string Currency); + """; + + var contextPath = temp.CreateFile("Context.cs", contextContent); + temp.CreateFile("Money.cs", moneyContent); + + var model = await CreateService().AnalyzeContextAsync(contextPath, "PositionalRecordContext"); + + var owned = model.Entities.SingleOrDefault(e => e.Key == "Product.Price"); + owned.Should().NotBeNull( + "OwnsOne(p => p.Price) must capture the owned entity even though Price is declared as a " + + "primary-constructor parameter rather than a property member"); + owned!.Properties.Select(p => p.Name).Should().BeEquivalentTo(["Amount", "Currency"], + "the pre-pass must find the Price parameter on the positional record to discover Money.cs; " + + "without that, Money resolves to an error type and the owned entity renders as an empty box"); + + var rendered = new MermaidErdRenderer().Render(model, new DiagramOptions(false, false)); + rendered.Should().Contain("Price_Amount", "the table-split owned columns must be inlined onto Product"); + rendered.Should().Contain("Price_Currency"); + } } From 934c7e823e53d6b65e4bb04725c946ef0f3805b4 Mon Sep 17 00:00:00 2001 From: = Date: Thu, 16 Jul 2026 21:36:09 +0200 Subject: [PATCH 4/5] fix(cli): strict parsing - unknown options now fail instead of being silently ignored Spectre.Console.Cli's default parser drops unrecognized long options on every command: 'projgraph erd --owned-mod classic' (typo, missing 'e') exited 0 and silently rendered in mirror mode. Enable StrictParsing in Program.cs so any unknown option produces 'Error: Unknown option ...' and a non-zero exit code; mirror the setting in the CLI test harness app. The Program.Main-based regression test must remain the only test that reaches Spectre's default error rendering: the library freezes AnsiConsole.Console in a process-lifetime Lazy on first render, so a second such test would write to the first test's disposed capture writer (documented on the test). Verified against the real binary: the typo'd option now exits 255 with a pointed error; valid invocations still exit 0. Closes #162 Co-Authored-By: Claude Fable 5 --- src/ProjGraph.Cli/Program.cs | 6 +++ .../Helpers/CliTestHelpers.cs | 3 ++ .../StrictParsingTests.cs | 51 +++++++++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 tests/ProjGraph.Tests.Integration.Cli/StrictParsingTests.cs diff --git a/src/ProjGraph.Cli/Program.cs b/src/ProjGraph.Cli/Program.cs index db31536..a60155d 100644 --- a/src/ProjGraph.Cli/Program.cs +++ b/src/ProjGraph.Cli/Program.cs @@ -29,6 +29,12 @@ public static int Main(string[] args) { config.SetApplicationName("projgraph"); + // Spectre's default parser silently ignores unrecognized long options: a typo like + // `--owned-mod classic` (missing 'e') exits 0 and renders with defaults, giving the user + // no signal their option was dropped. Strict parsing turns any unknown option into a + // parse error with a non-zero exit code instead. + config.Settings.StrictParsing = true; + config.AddCommand(packageDiagramCommandName) .WithDescription("Visualize the dependency graph of a solution or project") .WithExample(packageDiagramCommandName, "MySolution.sln") diff --git a/tests/ProjGraph.Tests.Integration.Cli/Helpers/CliTestHelpers.cs b/tests/ProjGraph.Tests.Integration.Cli/Helpers/CliTestHelpers.cs index 8bc67bd..f94ca4a 100644 --- a/tests/ProjGraph.Tests.Integration.Cli/Helpers/CliTestHelpers.cs +++ b/tests/ProjGraph.Tests.Integration.Cli/Helpers/CliTestHelpers.cs @@ -32,6 +32,9 @@ public static CommandApp CreateApp() app.Configure(config => { config.PropagateExceptions(); + // Mirror the production app's parser configuration (Program.cs) so tests exercise the + // same strict-parsing behaviour users get. + config.Settings.StrictParsing = true; config.AddCommand("visualize"); config.AddCommand("erd"); config.AddCommand("classdiagram"); diff --git a/tests/ProjGraph.Tests.Integration.Cli/StrictParsingTests.cs b/tests/ProjGraph.Tests.Integration.Cli/StrictParsingTests.cs new file mode 100644 index 0000000..ddcf0f8 --- /dev/null +++ b/tests/ProjGraph.Tests.Integration.Cli/StrictParsingTests.cs @@ -0,0 +1,51 @@ +using ProjGraph.Cli; +using ProjGraph.Tests.Integration.Cli.Helpers; +using Spectre.Console.Cli; + +namespace ProjGraph.Tests.Integration.Cli; + +/// +/// Regression tests for the silently-ignored-option bug (issue #162): without +/// StrictParsing, Spectre.Console.Cli dropped unrecognized long options on every command, +/// so a typo like --owned-mod classic exited 0 and rendered in the default mode with no +/// signal to the user. +/// +[Collection("CLI Tests")] +public sealed class StrictParsingTests +{ + /// + /// Runs the REAL app configuration via , not the test harness's own + /// CommandApp, so a regression in Program.cs itself is caught. This must remain the ONLY + /// test in the assembly that reaches Spectre's default (non-propagating) error rendering: + /// Spectre.Console.Cli caches AnsiConsole.Console in a process-lifetime Lazy on + /// first render (Internal/Extensions/AnsiConsoleExtensions.cs), so a second such test would + /// write to this test's already-disposed capture writer and crash with ObjectDisposedException. + /// + [Fact] + public void UnknownLongOption_FailsWithNonZeroExitCodeAndNamesTheOption() + { + var exitCode = 0; + var output = CliTestHelpers.CaptureConsoleOutput(() => + { + // The typo'd form of --owned-mode that motivated the issue. Parsing fails before the + // command executes, so the nonexistent input path is never touched. + exitCode = Program.Main(["erd", "DoesNotExist.cs", "--owned-mod", "classic"]); + }); + + exitCode.Should().NotBe(0, "an unknown option must fail the invocation, not be silently dropped"); + output.Should().Contain("owned-mod", "the error must tell the user which option was not recognized"); + } + + [Fact] + public void UnknownLongOption_OnAnotherCommand_ThrowsParseExceptionUnderStrictParsing() + { + // The pre-fix behaviour affected every command, not just erd. The harness app propagates + // exceptions, so strict parsing surfaces as a CommandParseException here. + var app = CliTestHelpers.CreateApp(); + + var act = () => app.Run(["stats", "DoesNotExist.slnx", "--tpo", "5"]); + + act.Should().Throw() + .WithMessage("*tpo*", "the parse error must name the unrecognized option"); + } +} From c4384633f931eb62d8bdef2d9897ae846cad2836 Mon Sep 17 00:00:00 2001 From: = Date: Thu, 16 Jul 2026 21:47:55 +0200 Subject: [PATCH 5/5] fix(ef): close review-found gaps in the record widening; correct relationship-walk comment Multi-angle review of the branch surfaced four follow-ups: - EntityFileDiscovery.ProcessConfigurationFileAsync was still ClassDeclarationSyntax-only, so a record config class in a SEPARATE file never reached the (widened) EntityConfigurationWalker - the cross-file case is the one that matters. Widened, interfaces excluded. - EntityFileDiscovery.ExtractBaseClassNamesFromSyntax was class-only, so a record entity's record base type in another file was never pulled into the compilation and its inherited columns vanished. - The TypeDeclarationSyntax widening in EntityConfigurationWalker admitted interfaces with default-implemented Configure bodies, which EF's ApplyConfigurationsFromAssembly never applies; now excluded. - ExtractPrimaryKeysFromPropertySyntax also scans a positional record's [property: Key] primary-constructor parameters. - The FluentRelationshipWalker precondition comment claimed relationships inside owned builders are never captured; they ARE (FindRelationshipRoots fences only UsingEntity) and get attributed to the enclosing entity - reworded to state the real behavior. All behavioral fixes pinned by regression tests verified to fail pre-fix. Refs #163 #165 Co-Authored-By: Claude Fable 5 --- .../Infrastructure/EntityAnalyzer.cs | 30 ++++- .../EntityConfigurationWalker.cs | 8 ++ .../Infrastructure/EntityFileDiscovery.cs | 24 +++- .../FluentRelationshipWalker.cs | 15 ++- .../EntityAnalyzerTests.cs | 19 +++ .../EntityConfigurationWalkerTests.cs | 29 +++++ .../RecordTypeDiscoveryRegressionTests.cs | 120 ++++++++++++++++++ 7 files changed, 228 insertions(+), 17 deletions(-) create mode 100644 tests/ProjGraph.Tests.Unit.EntityFramework/RecordTypeDiscoveryRegressionTests.cs diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs index 6ed46d9..450d404 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityAnalyzer.cs @@ -211,7 +211,10 @@ private static void ExtractPrimaryKeysFromClassAttributes(TypeDeclarationSyntax } /// - /// Extracts primary key names from property syntax with Key attributes. + /// Extracts primary key names from property syntax with Key attributes. For a positional record, + /// a key declared as [property: Key] lives on a primary-constructor parameter (whose + /// synthesized property carries the attribute), not on a + /// member, so the parameter list is scanned as well. /// /// The type declaration syntax (class or record) to analyze. /// The set to populate with primary key names. @@ -220,13 +223,34 @@ private static void ExtractPrimaryKeysFromPropertySyntax(TypeDeclarationSyntax t { foreach (var prop in typeSyntax.Members.OfType() .Where(p => p.AttributeLists.SelectMany(al => al.Attributes) - .Any(a => a.Name.ToString() is EfAnalysisConstants.EfAttributes.Key - or EfAnalysisConstants.EfAttributes.KeyAttribute))) + .Any(IsKeyAttribute))) { primaryKeyNames.Add(prop.Identifier.Text); } + + if (typeSyntax is not RecordDeclarationSyntax { ParameterList: not null } record) + { + return; + } + + // Only property-targeted attributes count: EF reads the attribute off the synthesized + // property, and a bare [Key] on a parameter targets the parameter itself, which EF ignores. + foreach (var parameter in record.ParameterList.Parameters + .Where(p => p.AttributeLists + .Where(al => al.Target?.Identifier.Text == "property") + .SelectMany(al => al.Attributes) + .Any(IsKeyAttribute))) + { + primaryKeyNames.Add(parameter.Identifier.Text); + } } + /// Determines whether an attribute syntax names the EF [Key] attribute. + /// The attribute syntax. + private static bool IsKeyAttribute(AttributeSyntax attribute) + => attribute.Name.ToString() is EfAnalysisConstants.EfAttributes.Key + or EfAnalysisConstants.EfAttributes.KeyAttribute; + private static void CollectNamesFromConstant(TypedConstant constant, HashSet names) { diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs index 8b96664..73f0c93 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityConfigurationWalker.cs @@ -159,6 +159,14 @@ internal static IEnumerable FindConfigClassesInRoot(SyntaxNode root /// The type declaration. private static ConfigClass? AsConfigClass(TypeDeclarationSyntax declaration) { + // EF's ApplyConfigurationsFromAssembly only instantiates concrete types (a record compiles to + // a class, so it qualifies); an interface extending IEntityTypeConfiguration with a + // default-implemented Configure is never applied at runtime and must not be folded in here. + if (declaration is InterfaceDeclarationSyntax) + { + return null; + } + var configInterface = declaration.BaseList?.Types .Select(baseType => baseType.Type) .OfType() diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityFileDiscovery.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityFileDiscovery.cs index 19ad39c..7d76a50 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityFileDiscovery.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityFileDiscovery.cs @@ -396,9 +396,18 @@ private async Task ProcessConfigurationFileAsync(string filePath, Dictionary()) + // TypeDeclarationSyntax (minus interfaces), not ClassDeclarationSyntax: a record can implement + // IEntityTypeConfiguration too, and this cross-file discovery feeds the same walker that was + // widened for exactly that reason (EntityConfigurationWalker.FindConfigClassesInRoot) — a + // class-only scan here would keep a separate-file record config invisible end-to-end. + foreach (var typeDecl in root.DescendantNodes().OfType()) { - var implementsInterface = classDecl.BaseList?.Types + if (typeDecl is InterfaceDeclarationSyntax) + { + continue; + } + + var implementsInterface = typeDecl.BaseList?.Types .Select(baseType => baseType.Type) .OfType() .Any(generic => @@ -407,7 +416,7 @@ private async Task ProcessConfigurationFileAsync(string filePath, Dictionary> ExtractBaseClassNamesAsync(DictionaryThe root of the syntax tree to analyze. /// A to store the extracted base class names. /// - /// This method traverses the syntax tree to find all class declarations with a base list. + /// This method traverses the syntax tree to find all class and record declarations with a base list + /// (interfaces are skipped: their base lists name other interfaces, never an entity base type). /// It then extracts the names of the base types using the method /// and filters them using the method before adding them to the set. /// public void ExtractBaseClassNamesFromSyntax(SyntaxNode root, HashSet baseClassNames) { foreach (var baseTypeName in root.DescendantNodes() - .OfType() - .Where(classDecl => classDecl.BaseList is not null) - .SelectMany(classDecl => classDecl.BaseList!.Types) + .OfType() + .Where(typeDecl => typeDecl is not InterfaceDeclarationSyntax && typeDecl.BaseList is not null) + .SelectMany(typeDecl => typeDecl.BaseList!.Types) .Select(ExtractBaseTypeName) .Where(IsValidBaseClassName)) { diff --git a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs index 3baa852..b178f92 100644 --- a/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs +++ b/src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentRelationshipWalker.cs @@ -109,13 +109,14 @@ private static bool IsInsideUsingEntity(SyntaxNode node) // Entity(e => e.HasMany(...)): the Has call is inside the Entity configuration lambda. // - // PRECONDITION: this ancestor walk is unbounded — it climbs past owned-type / join-entity - // builder fences without stopping, unlike FluentSyntax.ResolveOwningEntity. That is safe only - // because FindRelationshipRoots never yields a Has call from inside such a builder (UsingEntity - // is fenced there, and relationships inside OwnsOne/OwnsMany builders are not captured at all). - // If HasOne/HasMany capture inside owned builders is ever added, this walk must stop at - // nested-builder boundaries too, or it will reattribute the relationship to whatever - // Entity() lies beyond the fence. + // CAUTION: this ancestor walk is unbounded — it climbs past owned-type builder fences without + // stopping, unlike FluentSyntax.ResolveOwningEntity. FindRelationshipRoots fences only + // UsingEntity, so a HasOne/HasMany declared INSIDE an OwnsOne/OwnsMany builder lambda IS + // yielded, reaches this walk, and gets attributed to the Entity() enclosing the builder — + // the owner, not the owned type. That coarse attribution is a known limitation (this walker + // has no way to produce an owned {Owner}.{Nav} source key). If owned-builder relationships + // are ever modelled properly, this walk must stop at nested-builder fences the way + // FluentSyntax.ResolveOwningEntity does, or the relationship will silently leak to the owner. var enclosingEntity = chain.HasNode.Ancestors() .OfType() .FirstOrDefault(inv => inv.Expression is MemberAccessExpressionSyntax ma diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs index 2b8dca8..df4d9bb 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityAnalyzerTests.cs @@ -237,6 +237,25 @@ public record Ticket entity.Properties.First(p => p.Name == "Title").IsPrimaryKey.Should().BeFalse(); } + [Fact] + public void AnalyzeEntity_PositionalRecordWithPropertyTargetedKeyAttribute_MarksPrimaryKey() + { + // A positional record's [property: Key] lives on a primary-constructor parameter; the + // synthesized property carries the attribute at runtime, but the syntax fallback saw only + // PropertyDeclarationSyntax members and missed it (issue #163 hazard family). + var compilation = RoslynTestHelper.CreateCompilation(""" + using System.ComponentModel.DataAnnotations; + public record Voucher([property: Key] string Code, string Label); + """); + var type = RoslynTestHelper.GetTypeSymbol(compilation, "Voucher")!; + + var entity = EntityAnalyzer.AnalyzeEntity(type); + + entity.Properties.First(p => p.Name == "Code").IsPrimaryKey.Should().BeTrue( + "EF reads [property: Key] off the property synthesized from the record parameter"); + entity.Properties.First(p => p.Name == "Label").IsPrimaryKey.Should().BeFalse(); + } + [Fact] public void AnalyzeEntity_RequiredAttribute_ShouldMarkAsRequired() { diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs index c2f9d64..ce77069 100644 --- a/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/EntityConfigurationWalkerTests.cs @@ -100,6 +100,35 @@ void OnModelCreating(dynamic modelBuilder) "a record-declared IEntityTypeConfiguration must be discovered and its Configure body folded in"); } + [Fact] + public void Apply_InterfaceWithDefaultConfigureBody_IsNotTreatedAsConfigClass() + { + // Widening the scan to TypeDeclarationSyntax must not admit interfaces: EF's + // ApplyConfigurationsFromAssembly never instantiates an interface, so a default-implemented + // Configure body on one is dead code at runtime and folding it in would diverge from EF. + const string source = """ + using Microsoft.EntityFrameworkCore; + using Microsoft.EntityFrameworkCore.Metadata.Builders; + public class Widget { public int Id { get; set; } public string Name { get; set; } = ""; } + public interface IWidgetConfig : IEntityTypeConfiguration + { + void IEntityTypeConfiguration.Configure(EntityTypeBuilder builder) + => builder.Property(w => w.Name).HasMaxLength(10); + } + public class Ctx + { + void OnModelCreating(dynamic modelBuilder) + => modelBuilder.ApplyConfigurationsFromAssembly(typeof(Ctx).Assembly); + } + """; + var (method, compilation, entities, model) = Build(source, "Widget"); + + EntityConfigurationWalker.Apply(method, entities, model, compilation); + + entities["Widget"].Properties.Single(p => p.Name == "Name").MaxLength.Should().BeNull( + "an interface's default Configure body is never applied by EF and must not be folded in"); + } + [Fact] public void Apply_ApplyConfiguration_ConfiguresRelationship() { diff --git a/tests/ProjGraph.Tests.Unit.EntityFramework/RecordTypeDiscoveryRegressionTests.cs b/tests/ProjGraph.Tests.Unit.EntityFramework/RecordTypeDiscoveryRegressionTests.cs new file mode 100644 index 0000000..914b9f1 --- /dev/null +++ b/tests/ProjGraph.Tests.Unit.EntityFramework/RecordTypeDiscoveryRegressionTests.cs @@ -0,0 +1,120 @@ +using ProjGraph.Lib.Core.Infrastructure; +using ProjGraph.Lib.EntityFramework.Application; +using ProjGraph.Lib.EntityFramework.Application.UseCases; +using ProjGraph.Lib.EntityFramework.Infrastructure; +using ProjGraph.Tests.Shared.Helpers; + +namespace ProjGraph.Tests.Unit.EntityFramework; + +/// +/// End-to-end regressions for the file-discovery layer's record blind spots (issue #163's hazard +/// family): scanned for IEntityTypeConfiguration<T> +/// classes and for entity base types with ClassDeclarationSyntax only, so a record-declared +/// config class or base entity in a SEPARATE file was never pulled into the compilation — making the +/// record widenings in and +/// unreachable for exactly the cross-file case they exist for. Fixtures live in isolated +/// instances, matching OwnedRecordOwnerRegressionTests. +/// +[Trait("Category", "EntityFramework")] +public sealed class RecordTypeDiscoveryRegressionTests +{ + private static EfAnalysisService CreateService() + { + var fs = new PhysicalFileSystem(); + var analyzer = new EfModelAnalyzer(new CompilationFactory(), fs, new EntityFileDiscovery(fs)); + return new EfAnalysisService( + new AnalyzeContextUseCase(analyzer), + new DiscoverContextsUseCase(analyzer, fs), + new AnalyzeSnapshotUseCase(analyzer), + new DiscoverSnapshotsUseCase(analyzer, fs)); + } + + [Fact] + public async Task AnalyzeContextAsync_RecordConfigClassInSeparateFile_ConfigureBodyIsFolded() + { + using var temp = new TestDirectory(); + + const string contextContent = """ + using Microsoft.EntityFrameworkCore; + namespace Test; + + public class RecordConfigContext : DbContext + { + public DbSet Widgets { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.ApplyConfiguration(new WidgetConfiguration()); + } + + public class Widget + { + public int Id { get; set; } + public string Name { get; set; } = ""; + } + """; + const string configContent = """ + using Microsoft.EntityFrameworkCore; + using Microsoft.EntityFrameworkCore.Metadata.Builders; + namespace Test; + + public record WidgetConfiguration : IEntityTypeConfiguration + { + public void Configure(EntityTypeBuilder builder) + => builder.Property(w => w.Name).HasMaxLength(64); + } + """; + + var contextPath = temp.CreateFile("Context.cs", contextContent); + temp.CreateFile("WidgetConfiguration.cs", configContent); + + var model = await CreateService().AnalyzeContextAsync(contextPath, "RecordConfigContext"); + + var widget = model.Entities.Should().ContainSingle(e => e.Name == "Widget").Subject; + widget.Properties.Should().ContainSingle(p => p.Name == "Name") + .Which.MaxLength.Should().Be(64, + "the record-declared config class lives in a separate file, so it only reaches the " + + "walker if the cross-file config discovery scans record declarations too"); + } + + [Fact] + public async Task AnalyzeContextAsync_RecordEntityWithRecordBaseInSeparateFile_BaseColumnsCaptured() + { + using var temp = new TestDirectory(); + + const string contextContent = """ + using Microsoft.EntityFrameworkCore; + namespace Test; + + public class RecordBaseContext : DbContext + { + public DbSet Invoices { get; set; } = null!; + } + + public record AuditedInvoice : InvoiceBase + { + public decimal Total { get; set; } + } + """; + const string baseContent = """ + namespace Test; + + public record InvoiceBase + { + public int Id { get; set; } + public string CreatedBy { get; set; } = ""; + } + """; + + var contextPath = temp.CreateFile("Context.cs", contextContent); + temp.CreateFile("InvoiceBase.cs", baseContent); + + var model = await CreateService().AnalyzeContextAsync(contextPath, "RecordBaseContext"); + + var invoice = model.Entities.Should().ContainSingle(e => e.Name == "AuditedInvoice").Subject; + invoice.Properties.Select(p => p.Name).Should().Contain(["Total", "Id", "CreatedBy"], + "the record base type's file must be discovered through the record-aware base-name scan; " + + "without it, InvoiceBase never joins the compilation and its inherited columns vanish"); + invoice.Properties.Should().Contain(p => p.Name == "Id" && p.IsPrimaryKey, + "the inherited Id follows the EF primary-key convention"); + } +}