Find entities through a base class the project wrote itself - #9
Conversation
peopleworks
left a comment
There was a problem hiding this comment.
Approving. This is a strict improvement with no regression, and it does the thing I asked for in the way I hoped rather than the way that would have been quicker.
I warned you in #6 that SelectControllers grows by simple name, and that lifting it as it stands would carry "a name is not an identity" into the entity side. You did not lift it as it stands. DerivesFromAccepted resolves the base through the deriving file's own scope, and Contracts.NamedBaseObject / Contracts.Order are in the fixture to prove it — two namespaces each declaring a NamedBaseObject, one of them persistent, and the test pins which Order comes out. That is the harder version and it is the right one.
This is also good:
Every entry is tried rather than the first, because syntax cannot tell a base class from an interface. An interface name only matches if a class of that name was itself accepted, which an interface never is.
Correct, and the reason it is safe is written down instead of left for someone to rediscover.
Verified in a clean worktree, Windows, .NET 10, Release:
Passed! Failed: 0, Passed: 283, Skipped: 0 0 warnings, 0 errors
Two findings, and I want to be clear up front that neither is a defect in this change — both are true on main today and this PR does not make either worse. They matter because it changes which of them a reader will actually meet.
1. The abstract base is still reported as a business entity
I ran the fixture through extraction rather than reading the tests:
SampleDeep.Module.BusinessObjects.NamedBaseObject persistent=True props=1
SampleDeep.Module.BusinessObjects.Order persistent=True props=1 nav=Sales
SampleDeep.Module.BusinessObjects.PriorityOrder persistent=True props=1
NamedBaseObject is abstract and exists only to be inherited from, and it sits in the inventory indistinguishable from a table. #6 asked for both halves — "Order as an entity, and AuditedObject either folded away or marked as the abstract base it is" — and this delivers the first. Before the change the inventory had the base and not the entities; now it has all three. Better, but the false one is still there, and Closes #6 would retire the issue with the second half unrecorded.
My preference is marking over dropping. An abstract persistent class is not nothing — it is where the shared properties are declared, which is finding 2.
2. Inherited properties are not folded, and that now bites
NamedBaseObject declares Name
Order declares Number inherits Name
PriorityOrder declares Rank inherits Name, Number
Order is reported with one property. PriorityOrder is reported with one of its three.
That was already true and nobody could see it, because these classes were not extracted at all. Making them visible turns a silent omission into a stated one: PriorityOrder now appears in a section headed as this application's entities, with two thirds of its columns missing, and an agent told the lists are complete has no way to know. Under the project's own rule that is not "found the entity" — it is a heading promising completeness over something that is not complete.
It is also not hypothetical at your scale. A 221-entity application on a shared base loses whatever that base holds from every entity, and a shared base almost always holds the audit fields.
I would take this one as its own issue rather than growing this PR, and I am happy to file it if you would rather stay on #11 and #7. Say which.
One practical thing
You and #10 both edit README.md, CHANGELOG.md and SampleProjects.cs, and you claim different test counts — 283 here, 280 there. Whichever merges second will conflict and then fail CountsTheTestsThatActuallyExist, which reads the README claim against the real suite. Not a problem with either change, just an order-of-merge tax: rebase the second and fix the number.
The XPO hierarchy is PersistentBase -> XPBaseObject -> XPCustomObject -> XPObject, with XPLiteObject also under XPBaseObject. BaseTypeNames held the three leaves and neither of the classes above them, so the hole sat in the middle of an API that is used rather than at its edge: DevExpress documents all five as bases a persistent class may derive from, and recommends PersistentBase. Deriving from the higher bases is what you do when the table already brings its own key -- the same population of legacy schemas the DbSet roster was added for, which is why this turned up beside it. FeatureCenter.NET.XPO gains OidGenerator, NoKeyPropertyNamedBaseObject and LayoutDemoObject, all of them ": XPBaseObject". Nothing changes for MainDemo in either ORM, for OutlookInspired, or for a private 221-entity EF Core application. No demo derives from PersistentBase directly, so that half is covered by the fixture rather than by a corpus. The list also stops being duplicated. The CLI, the MCP server and the test harness each passed their own copy, so the default in Core was four names while every caller passed five -- four copies and three chances for them to disagree about what an entity is. They now use the default. Fixes peopleworks#6 in part; the transitive walk in peopleworks#9 is the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Classification matched a class's own base list against the root names and stopped there. An application with a shared base -- auditing, a key convention, a display-name property -- lost every business object below it. The inversion is what makes it severe rather than incomplete. The abstract base does match, so the inventory reports the one class that is not a table and omits the ones that are, while AGENTS.md goes on to say the inventory is complete and that anything absent does not exist. Selection now repeats until a round changes nothing, which is what SelectControllers already does on the controller side and for the same reasons: a base may be read after the class deriving from it, and a chain can be any depth. A base name is resolved through the deriving file's own scope rather than by simple name. A name is not an identity -- the reason the DbSet roster carries scopes -- so a Contracts.Order beside a BusinessObjects.Order still resolves to the base it actually named, not to any accepted class wearing that name. Acceptance is keyed on (namespace, name), so every part of a partial class is selected once any part of it is. On the demos shipped with 26.1: FeatureCenter.NET.XPO 43 -> 140 MainDemo.NET.XPO 14 -> 17 OutlookInspiredDemo.NET.EFCore 23 -> 24 MainDemo.NET.EFCore 14 -> 14 (unchanged) The OutlookInspired one is an EF Core application: TaxRate derives through a shared base and is not registered as a DbSet, so neither the roster nor the base match saw it. Fixes peopleworks#6 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c8e4643 to
fe5c5c3
Compare
|
Rebased onto The one worth checking is var roster = DbSetRoster.Read(parsedFiles.Select(parsed => parsed.Root));
var ormType = options.Orm == OrmType.Auto
? DetectOrmType(parsedFiles.Select(parsed => parsed.Root), roster)
: options.Orm;
options.ResolvedOrm = ormType;
var persistent = SelectPersistentClasses(parsedFiles.Select(parsed => parsed.Root), roster, options);If you meant
That 289 is the real check on the resolution: every test from #8, #12 and this one passes together. A wrong merge in that method would not have been quiet. Two practical notes:
|
| foreach (var root in trees) | ||
| { | ||
| var fileScopes = new HashSet<string>(globalUsings, StringComparer.Ordinal); | ||
|
|
||
| foreach (var directive in root.DescendantNodes().OfType<UsingDirectiveSyntax>()) | ||
| { | ||
| if (directive.Alias is null && directive.Name is not null) | ||
| fileScopes.Add(directive.Name.ToString()); | ||
| } | ||
|
|
||
| foreach (var classDecl in root.DescendantNodes().OfType<ClassDeclarationSyntax>()) | ||
| { | ||
| var @namespace = GetNamespace(classDecl); | ||
| var scopes = new HashSet<string>(fileScopes, StringComparer.Ordinal); | ||
|
|
||
| // Its own namespace and every one enclosing it: C# resolves an unqualified name |
| } | ||
|
|
||
| // Each round can accept a class whose base was accepted in the previous one, so it repeats | ||
| // until a round changes nothing. Bounded by the number of classes. | ||
| bool changed; | ||
|
|
| if (!DerivesFromAccepted(candidate.Declaration, candidate.Scopes, accepted)) | ||
| continue; | ||
|
|
||
| accepted.Add((candidate.Namespace, candidate.Name)); | ||
| changed = true; | ||
| } | ||
| } | ||
| while (changed); | ||
|
|
||
| return accepted; | ||
| } |
| if (generic > 0) written = written[..generic]; | ||
|
|
||
| var dot = written.LastIndexOf('.'); | ||
| var simpleName = dot < 0 ? written : written[(dot + 1)..]; | ||
|
|
||
| foreach (var (@namespace, name) in accepted) | ||
| { | ||
| if (!string.Equals(name, simpleName, StringComparison.Ordinal)) continue; | ||
|
|
||
| if (dot < 0) | ||
| { | ||
| // Unqualified: it named something this file can actually see. | ||
| if (scopes.Contains(@namespace)) return true; | ||
| continue; | ||
| } | ||
|
|
||
| // Qualified: the tail it wrote is more specific than any using directive. | ||
| var qualifier = written[..dot]; | ||
| if (@namespace.Equals(qualifier, StringComparison.Ordinal) | ||
| || @namespace.EndsWith("." + qualifier, StringComparison.Ordinal)) | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /// <summary> |
The XPO hierarchy is PersistentBase -> XPBaseObject -> XPCustomObject -> XPObject, with XPLiteObject also under XPBaseObject. BaseTypeNames held the three leaves and neither of the classes above them, so the hole sat in the middle of an API that is used rather than at its edge: DevExpress documents all five as bases a persistent class may derive from, and recommends PersistentBase. Deriving from the higher bases is what you do when the table already brings its own key -- the same population of legacy schemas the DbSet roster was added for, which is why this turned up beside it. FeatureCenter.NET.XPO gains OidGenerator, NoKeyPropertyNamedBaseObject and LayoutDemoObject, all of them ": XPBaseObject". Nothing changes for MainDemo in either ORM, for OutlookInspired, or for a private 221-entity EF Core application. No demo derives from PersistentBase directly, so that half is covered by the fixture rather than by a corpus. The list also stops being duplicated. The CLI, the MCP server and the test harness each passed their own copy, so the default in Core was four names while every caller passed five -- four copies and three chances for them to disagree about what an entity is. They now use the default. Fixes peopleworks#6 in part; the transitive walk in peopleworks#9 is the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What this changes
Entity selection follows base classes to a fixed point instead of matching only a class's own base list, and resolves each base name through the deriving file's scope rather than by simple name.
Why
An application with a shared base class — auditing, a key convention, a display-name property — lost every business object below it. The inversion is what makes it severe: the abstract base does match the root list, so the inventory reports the one class that is not a table and omits the ones that are, while ground rule 2 goes on saying the inventory is complete.
FeatureCenter.NET.XPOreported 43 entities of 136. Whole feature areas were absent — everyRule*Objectthe validation section is built on, everyGridListEditor*DemoObject, everyConditionalAppearance*Object,StringProperties,NumericProperties,DateTimeProperties— nearly all of them two hops below a singleNamedBaseObject : BaseObject.This is not only an XPO problem.
OutlookInspiredDemo.NET.EFCorewas missingTaxRate, which derives through a shared base and is the one entity in that application not registered as aDbSet<T>. Twenty of its twenty-three entities are two or more hops from a root and survive only because they are registered — the roster from #3/#4 is carrying that application, and where a class is not registered there was no floor under it. The walk is what makes EF Core extraction robust rather than lucky.How it decides
Same shape as
SelectControllers: seed with what the current rules accept (base-list match, or aDbSetregistration), then repeat until a round changes nothing, because a base may be read after the class deriving from it and a chain can be any depth.Resolution deliberately does not use bare names, per your remark on #4 — a name is not an identity. A base name resolves through the deriving file's own scope: its
usingdirectives,global usings, its own namespace and the namespaces enclosing it, which is ordinary C# lookup as far as syntax can see it. Qualified names match on their tail, as the roster does.The fixture pins that directly:
Contracts.Order : NamedBaseObjectsits besideBusinessObjects.Order : NamedBaseObject, each namespace declaring its ownNamedBaseObject, and only one of them persistent. Resolving by bare name turns the DTO into a table; resolving by scope does not.Acceptance is keyed on
(namespace, name), so every part of apartialclass is selected once any part is — which also means the XPO case you described in 0.12.1, where a hand-written part carries: BaseObjectand a generated part carries the mapping, now selects both parts rather than only the first.Verification
Demos shipped with 26.1, plus a private 221-entity legacy LIMS application:
FeatureCenter.NET.XPOModule.Win— zero missed, zero extraMainDemo.NET.XPOPerson,Employee,DemoTaskOutlookInspiredDemo.NET.EFCoreTaxRateMainDemo.NET.EFCoreGround truth is computed by walking each declared class's base chain using only bases declared in the analyzed source, against the five root names the CLI and MCP server pass. The FeatureCenter "+4" are
ImageSourceBrowserBaseand its three subclasses, which live in theFeatureCenter.Module.Winsibling project — sibling scanning working, not over-reporting.WLNCentral staying at exactly 196 is the result I most wanted: ~200 entities on a legacy schema with several hand-written bases, and the walk adds nothing false.
283 passed, 0 failed,dotnet build XAFLogicExplainer.slnxclean at 0 warnings. Four new tests, all failing onmain— the third one for the right reason, withExtracted: NamedBaseObjectnaming the inversion.Closes #6
Deliberately not in this PR
XPBaseObjectis still missing fromBaseTypeNames, which is whyOidGeneratorandNoKeyPropertyNamedBaseObjectin FeatureCenter are still absent.XPCustomObjectis in the list andXPBaseObjectis its ancestor, so the list has a hole one level up. One word plus a test, and a different cause — say the word and it follows.PortfolioFileData : FileAttachmentBasein MainDemo.XPO, still missing and still the third cause from [gap] XPO entities two hops from BaseObject are dropped, and their abstract base is reported instead #6: the chain leaves the analyzed source on its first hop, so syntax terminates without deciding. That wants the catalog or a documented limit, not a guess.Checklist
dotnet build XAFLogicExplainer.slnxis clean (CI treats warnings as errors)XafLogicExplainer.CoreCHANGELOG.mdupdated under[Unreleased]Independent of #8 — branched from
main, touching a different method.