fix(ef): read entities across project boundaries and stop guessing Guid over a max length - #184
Merged
Merged
Conversation
…id over a max length Pre-release validation against eShopOnWeb and ardalis/CleanArchitecture surfaced two defects in the DbContext path (the ModelSnapshot path was already correct). Entity discovery searched only the context directory and its immediate parent, while base-class discovery already walked up to the workspace root. In the standard layered layout — entities in `src/Core`, DbContext in `src/Infrastructure/Data/` — no entity CLR file was ever read, so columns came only from explicit fluent `Property()` calls: eShopOnWeb lost `Order.OrderDate` and `CatalogItem.Description` plus two of its four relationships, and CleanArchitecture's `Contributor.PhoneNumber` rendered as an empty box. Discovery now walks outward to the enclosing solution root, one level at a time so the nearest declaration of a name still wins. Widening it exposed two collisions that had to be fixed with it: - Configuration classes are collected by shape, not by name, so the wider radius pulled in entities from unrelated solutions nested in the same repository — the six config classes of CleanArchitecture's MinimalClean tree added Cart, GuestUser, Order and friends to an ERD whose only DbSet is Contributor. Config discovery keeps the narrow radius. - `dotnet ef migrations add PhoneNumber` emits `class PhoneNumber : Migration` beside the DbContext, nearer than the value object it is named for, so the owned navigation resolved to the migration and bound as `Nullable`. Migration classes are now skipped, detected by base type or `[Migration]` rather than by folder name. Separately, a property whose name ends in `Id` and whose CLR type is unresolvable was guessed as a `Guid`, so eShopOnWeb's string `BuyerId` rendered as the self-contradictory `Guid BuyerId "required, max:256"`. A max length cannot apply to a value type, so it now corrects a guessed type to `string`. `EfProperty.IsTypeInferred` records whether a type was guessed, so only guesses are ever corrected — a CLR-declared type is left alone, and an explicit `HasColumnType` now also outranks a guessed `Guid` rather than only the `string` fallback. Also fixes the library README quickstarts, which resolved renderers registered only as `IDiagramRenderer<T>` and referenced five APIs that do not exist (`GetGraphAsync`, `AnalyzeAsync`, `ISolutionParser.ParseAsync`, two `AnalysisOptions` properties and three `SolutionStats` properties). Every snippet is now compiled and resolved against the real container. 9 new tests; 1131 green; all 13 real-world diagrams parse under Mermaid v11.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two EF DbContext-path analysis defects found during real-world validation (layered solutions and Fluent API type inference), and updates library READMEs so sample code matches the actual public APIs and DI registrations.
Changes:
- Expand EF entity CLR file discovery to walk outward up to the enclosing solution root (while keeping configuration discovery on a narrow radius) and skip EF migration classes during entity matching.
- Track whether an EF property type was inferred/guessed, and allow
HasMaxLength/HasColumnTypeto correct only guessed types (preventing “Guid + max length” contradictions). - Update NuGet/library README quickstarts to compile and to resolve renderers/services via the correct APIs and registrations.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ProjGraph.Tests.Unit.EntityFramework/CrossProjectEntityDiscoveryTests.cs | Adds regression coverage for cross-project entity discovery, config leakage prevention, and migration-class collision avoidance. |
| tests/ProjGraph.Tests.Unit.EntityFramework/ColumnTypeInferenceTests.cs | Adds regression coverage for correcting guessed Guid types when HasMaxLength / HasColumnType provide stronger evidence. |
| src/ProjGraph.Lib/README.md | Updates top-level library quickstarts to use current service APIs and renderer resolution patterns. |
| src/ProjGraph.Lib.EntityFramework/README.md | Updates EF library quickstarts to resolve renderers via IDiagramRenderer<EfModel> and fixes example usings. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentPropertyWalker.cs | Makes HasMaxLength and HasColumnType correctly override only inferred types and preserves authoritative column-type decisions. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/EntityFileDiscovery.cs | Widens entity discovery across solution roots, adds a narrow-radius helper for config scanning, and skips migration classes when matching entity names. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/EfPropertyOverrides.cs | Adds override support for the new IsTypeInferred flag. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/EfPropertyFactory.cs | Introduces and propagates IsTypeInferred for name-based type guessing and subsequent overrides. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs | Ensures configuration discovery remains narrow-radius to avoid leaking unrelated-solution entities. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/Constants/EfAnalysisConstants.cs | Adds migration-related constant names used by migration filtering. |
| src/ProjGraph.Lib.EntityFramework/Application/IEntityFileDiscovery.cs | Extends the interface with BuildLocalSearchDirectories for narrow-radius discovery cases. |
| src/ProjGraph.Lib.Dependencies/README.md | Updates stats quickstart output to match current SolutionStats surface. |
| src/ProjGraph.Lib.Core/README.md | Updates solution parsing quickstart to current parser abstractions (ISlnxParser + GetProjectPaths). |
| src/ProjGraph.Lib.Core/Infrastructure/WorkspaceRootResolver.cs | Adds FindEnclosingSolutionRoot to locate a solution boundary without stopping at .csproj markers. |
| src/ProjGraph.Lib.ClassDiagram/README.md | Updates class diagram quickstarts to directory-based analysis and correct renderer/service resolution. |
| src/ProjGraph.Core/Models/EfModel.cs | Adds EfProperty.IsTypeInferred to distinguish guessed vs. authoritative types in the EF model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+262
to
+269
| var derivesFromMigration = typeDecl.BaseList?.Types | ||
| .Any(baseType => baseType.Type is IdentifierNameSyntax { Identifier.Text: EfAnalysisConstants.CommonNames.Migration }) == true; | ||
|
|
||
| var hasMigrationAttribute = typeDecl.AttributeLists | ||
| .SelectMany(list => list.Attributes) | ||
| .Any(attribute => attribute.Name.ToString() is EfAnalysisConstants.CommonNames.Migration | ||
| or EfAnalysisConstants.CommonNames.MigrationAttribute); | ||
|
|
Owner
Author
There was a problem hiding this comment.
Confirmed and fixed in 1b81865. Both checks now reduce to the right-most identifier via the existing SimpleTypeName helper, which I extended to unwrap AliasQualifiedNameSyntax and to recurse through nested qualified names rather than reading only qualified.Right.
Reproduced the gap first: added a fully qualified designer half to the migration fixture ([Microsoft.EntityFrameworkCore.Migrations.Migration("…")] plus a qualified base type), which failed before the fix and passes after — so the collision guard is pinned against generated code that does not write Migration unqualified.
…self-contained Addresses review on #184. `IsMigrationClass` only recognised an unqualified `: Migration` base type and an exactly matching `[Migration]` attribute name, so a migration written with qualified names — which generated code gives no guarantee against — slipped through and the entity-name collision it guards could reappear. Both checks now reduce to the right-most identifier through the existing `SimpleTypeName` helper, extended to unwrap alias-qualified names and to recurse through nested qualified names. Covered by adding a fully qualified designer half to the migration-collision fixture, which reproduced the gap before the fix. The unrelated-solution fixture called `Assembly.GetExecutingAssembly()` without a `using System.Reflection;`, so the analysed source was not valid C# on its own; `typeof(ShopContext).Assembly` is self-contained. Configuration application is detected by method name, so the test still exercises the same path — re-verified by reverting the config-discovery fix and confirming the test fails. 1131 green, format clean.
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.
Pre-release validation for v1.1.0 against eShopOnWeb and ardalis/CleanArchitecture surfaced two defects in the DbContext path. The ModelSnapshot path was already correct throughout, and served as the ground truth for every comparison below.
1. Entity discovery never crossed a project boundary
EntityFileDiscovery.BuildSearchDirectoriessearched only the context directory and its immediate parent, while base-class discovery already walked up to the workspace root. In the standard layered layout — entities insrc/Core, DbContext insrc/Infrastructure/Data/— that meant no entity CLR file was ever read, so columns could only come from explicit fluentProperty()calls.CatalogContextOrder.OrderDate, noCatalogItem.Description,Guidkeysintkeys, both columns presentContributor.PhoneNumberPhoneNumber {}boxPhoneNumber_CountryCode/Number/Extension, matching the snapshotDiscovery now walks outward to the enclosing solution root, one level at a time so the nearest declaration of a name still wins — jumping straight to the root regressed the
complex-ecommercegolden by matching a same-namedMoneyelsewhere in the repo.Widening the radius exposed two collisions that had to be fixed with it, both found only on the real repositories:
MinimalCleantree added Cart, CartItem, GuestUser, Order, OrderItem and Product to an ERD whose only DbSet isContributor.dotnet ef migrations add PhoneNumberemitspublic partial class PhoneNumber : Migrationbeside the DbContext — nearer than the value object it is named for — so the owned navigation resolved to the migration and bound asNullable. Detected by base type or[Migration]rather than by folder name, which is configurable and often renamed.2. A max length could not overrule a guessed
GuidA property whose name ends in
Idand whose CLR type is unresolvable was guessed as aGuid, so eShopOnWeb's stringBuyerIdrendered as the self-contradictoryGuid BuyerId "required, max:256". A max length cannot apply to a value type, so it is proof the guess is wrong.EfProperty.IsTypeInferrednow records whether a type was guessed, so only guesses are corrected: a CLR-declaredGuidwithHasMaxLengthis left exactly as declared (pinned by its own test). The same flag fixes a related gap —HasColumnType's override guard only caught thestringfallback, so an explicit column type could not correct a guessedGuideither.Also: library README quickstarts
The shipped NuGet READMEs resolved renderers registered only as
IDiagramRenderer<T>(InvalidOperationExceptionfor anyone copying them) and referenced five APIs that do not exist:GetGraphAsync,AnalyzeAsync,ISolutionParser.ParseAsync, twoAnalysisOptionsproperties and threeSolutionStatsproperties. Every snippet was compiled against the real libraries and every documented service resolved from a live container.Verification
dotnet formatclean, Release build cleanResidual differences from the snapshot are genuinely unknowable from a context file: Vogen-converted
Contributor.Id/Status, andHasMaxLength(ContributorName.MaxLength)behind a const reference.Note
Validate outside the system temp directory:
EntityFileDiscoveryskips the parent search underPath.GetTempPath()to isolate parallel test runs, so a clone in/tmpexercises different behaviour than a real checkout.🤖 Generated with Claude Code