Find EF Core entities by their DbSet registration - #3
Conversation
An XAF application mapped onto an existing schema rarely derives from BaseObject: the tables bring their own keys, so the project writes its own base class or maps a plain POCO. IsXafBusinessObject matched only the class's direct base list against four XPO-era names, so none of those were seen, and the loss was silent -- an entity that is never extracted cannot be reported as missing, and AGENTS.md goes on to tell the agent its inventory is complete. Under EF Core the application already states what it persists, in the one place that must be right for it to run: the DbSet<T> properties of its DbContext. Classifying a class as an entity when it is registered there finds these applications without giving up the existing base-type rule. Only classes declared in the analyzed source are eligible, so the framework tables a DbContext also registers -- ModuleInfo, FileData, ModelDifference -- drop out on their own, with no list of names to maintain. On a real 221-entity XAF application over a legacy LIMS schema this moves extraction from 3 entities to 210, and its business rules from 469 bytes to 8.5 KB. Fixes peopleworks#1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
peopleworks
left a comment
There was a problem hiding this comment.
Thank you for this. Three things before anything else, because they are not the usual first contribution:
You read the code before filing. You separated #1 and #2 by cause rather than by symptom, even though one fixture produced both. And you found the argument this project is actually built on — "a class that is never seen cannot be reported as missing" — and applied it to a place I had not applied it myself. That last one is the part I care about most. The controller fix in 0.12.0 and this one are the same defect wearing different clothes, and I did not notice.
You are also right about the thing that makes this severe rather than annoying. A missing entity is not a hole in a document. AGENTS.md goes on to tell the agent that its inventories are complete and that anything absent does not exist in the application — so a silent gap does not make the agent uninformed, it makes it confidently wrong. Three entities out of 221, with a header promising the full set, is close to the worst output this tool can produce.
What I verified
Built and ran the branch locally on Windows, .NET 10, Release:
Passed! Failed: 0, Passed: 275, Skipped: 0 0 warnings, 0 errors
I also confirmed the four new tests fail on main with the message you quoted, and that DoesNotReportFrameworkTypesRegisteredInTheDbContext passes there vacuously — your read of that was exact, and saying so in the PR body instead of letting the green tick speak for itself is the reason this review took me an hour instead of a day.
Three things I need changed first
I built throwaway projects and put them through EntityAnalyzer rather than reasoning from the diff. All three come from one root: registeredTypes is a set of bare simple names, matched globally, so it matches more than the roster you documented.
1. A partial class split across files is reported twice — blocking
// Ctx.cs
public class LegacyDbContext : DbContext { public DbSet<Invoice> Invoices { get; set; } }
// Invoice.cs
public partial class Invoice { public virtual int Id { get; set; } }
// Invoice.Generated.cs
public partial class Invoice { public virtual string Number { get; set; } }Probe.Module.BusinessObjects.Invoice props=1 file=Invoice.cs
Probe.Module.BusinessObjects.Invoice props=1 file=Invoice.Generated.cs
Two entities, and neither one is complete — the properties are split between the copies, so the agent is told Invoice has Id and, separately, that Invoice has Number.
main does not do this: IsXafBusinessObject returns false on BaseList == null, so only the part carrying the base class ever matched. The roster matches every part by name, so this arrives with the fix. And it arrives precisely in the population the fix is for — a scaffolded legacy schema splits partials as a matter of routine. Your own fixture writes public partial class Invoice; it just happens to be one file, so the suite never asks the question.
2. An unrelated class sharing the simple name becomes an entity
Probe.Module.BusinessObjects.Invoice props=1 file=Invoice.cs
Probe.Module.Contracts.Invoice props=1 file=InvoiceDto.cs
A DTO that merely shares a name joins the business inventory.
3. The roster is wider than "the DbContext's registrations"
public void Work(DbContext db) { DbSet<AuditLog> local = db.Set<AuditLog>(); }AuditLog is registered as an entity. CollectDbSetTypeNames walks every GenericNameSyntax named DbSet anywhere in the source, so a local, a parameter or a return type counts the same as a property on a context.
Why I am asking rather than merging and patching
The argument you wrote for this change is the reason. Over-reporting is worse than under-reporting here, because a fabricated entity is indistinguishable from a real one to the agent reading the file, while a missing one at least stays missing. You knew that — it is why DoesNotReportFrameworkTypesRegisteredInTheDbContext is in the PR. That guard is name-based, and these three walk under it.
The direction is right and I want this in. What it needs is a narrower roster:
- read it only from property declarations whose declaring type derives from
DbContext, which is what the remark already claims and would resolve (3) on its own; - key entities on
(namespace, class name)and merge the parts of apartialrather than emitting one per declaration — that fixes (1), and (2) reduces to picking the right namespace, for which the context file'susingdirectives are a reasonable syntax-only signal. If that turns out to be ugly without a semantic model, say so and we will take same-namespace-as-the-context first with a documented fallback. I would rather have an honest heuristic with its limits written down than a clever one that cannot say when it is guessing.
A test for each, please — the split partial especially, since it is the one that would ship.
Two other things
#2 — yes, send it. Separately, as you suggested. And I agree with your instinct there over my own default: where no ORM signal is found at all, "unknown" is the right thing to render. That fallback is emitting a guess in the voice of a fact, which is the same failure as the empty inventory that promises completeness. If you would rather I take that one so you are not carrying two, say the word.
The transitive base walk on the XPO side — you were right to leave it out, and right that it exists. Open an issue for it when you have a moment and it is yours if you want it.
Last thing. I have been building this on my own, and you have improved it twice in a day, in a style that reads like you have maintained something before. If you would like to be more involved than one PR — commit rights, a say in where the extractor goes next, your name on the project — I am interested, and I would rather ask now than after you have done the work for free. Email is in my profile.
Either way: thank you. This is the contribution I hoped the extraction-gap template would bring in, and I did not expect it in the first week.
| var argument = generic.TypeArgumentList.Arguments[0].ToString(); | ||
| var simpleName = argument[(argument.LastIndexOf('.') + 1)..]; | ||
|
|
||
| if (simpleName.Length > 0) | ||
| names.Add(simpleName); | ||
| } | ||
| } | ||
|
|
||
| return names; | ||
| } | ||
|
|
What this changes
EntityAnalyzernow also classifies a class as an entity when the application registers it as aDbSet<T>, instead of only when its direct base list matches one of the four names inExtractionOptions.BaseTypeNames.Why
An XAF application mapped onto an existing schema rarely derives from
BaseObject: the tables bring their own primary keys, so the project writes its own base class or maps a plain POCO. None of those were being seen, and — as theSelectControllersremark already puts it for the controller case — a class that is never extracted cannot be reported as missing.AGENTS.mdthen goes on to tell the agent the inventory is complete and that anything absent does not exist.Under EF Core the application already declares what it persists, in the one place that has to be correct for it to run at all. Reading that roster needs no compilation and no new heuristic.
Only classes declared in the analyzed source are eligible, so the framework tables a DbContext also registers —
ModuleInfo,FileData,ModelDifference— drop out on their own rather than through a list of excluded names that would need maintaining. There's a test pinning that, since over-reporting here would be worse than the gap it fixes.Measured on a real 221-entity XAF application over a legacy LIMS schema (the one that motivated #1):
*_Entities.md*_BusinessRules.mdThe remaining 11 of 221 are the framework-declared types above, correctly excluded.
Closes #1
The pattern, if this is an extraction change
The fixture lives in its own solution folder as
LegacyEfSolution, rather than joiningEfCoreSolution, for two reasons:OrmDetectionTests.FindsEfCoreEntitiesDespiteTheSharedBaseClassNameasserts that fixture's exact entity set, and the two are genuinely different applications — one a greenfield model onBaseObject, one mapped onto tables that were already there.Deliberately not in this PR
SelectControllers. Real, but a separate pattern and a separate change.IXafEntityObject/[DefaultClassOptions]as independent signals. They would catch an entity that is somehow not in any DbContext; the roster covers every case I can currently demonstrate, so this keeps to the smaller rule.DetectOrmType. Happy to send that separately if you want it.Checklist
dotnet build XAFLogicExplainer.slnxis clean (CI treats warnings as errors) — 0 warnings, 0 errorsXafLogicExplainer.CoreDbSet<T>whose type is not declared in the source is ignored, not faultedCHANGELOG.mdupdated under[Unreleased]275 tests pass, 270 before. Four of the five new tests fail on
mainwithThe fixture has no entity 'Invoice'. Extracted:— the fifth isDoesNotReportFrameworkTypesRegisteredInTheDbContext, which passes there vacuously because nothing is extracted at all, and only becomes load-bearing once the roster is read. The README's tested test-count claim is updated to match.