Narrow the DbSet roster to what the application actually registers - #4
Conversation
The roster landed in #3 as a set of bare simple names matched globally, which reports more than the application declares in three ways: every part of a partial class matches, so a scaffolded split becomes two entities each holding half its columns; a DTO sharing a simple name joins the business inventory; and a DbSet<T> written as a local counts the same as a registration. Registrations now carry the namespaces they could have been naming, are read only from the properties of a DbContext -- found through its base chain -- and the parts of a partial class are folded into the one entity they describe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| foreach (var directive in root.DescendantNodes().OfType<UsingDirectiveSyntax>()) | ||
| { | ||
| if (directive.Alias is null && directive.Name is not null) | ||
| scopes.Add(directive.Name.ToString()); | ||
| } |
| foreach (var property in context.Members.OfType<PropertyDeclarationSyntax>()) | ||
| { | ||
| if (property.Type is not GenericNameSyntax generic) continue; | ||
| if (generic.Identifier.Text != "DbSet") continue; | ||
| if (generic.TypeArgumentList.Arguments.Count != 1) continue; | ||
|
|
||
| foreach (var root in roots) | ||
| var argument = generic.TypeArgumentList.Arguments[0].ToString(); | ||
| if (argument.Length > 0) | ||
| roster._registrations.Add((argument, scopes)); | ||
| } |
| foreach (var candidate in declared) | ||
| { | ||
| if (contextNames.Contains(candidate.Identifier.Text)) continue; | ||
| if (!GetBaseTypeNames(candidate).Any(contextNames.Contains)) continue; | ||
|
|
||
| contextNames.Add(candidate.Identifier.Text); | ||
| grew = true; | ||
| } |
CI caught this on Linux while Windows passed: which half of a partial class becomes the primary depended on the file system doing the listing. NTFS compares names without case and ext4 by byte, so Shipment.Generated.cs sorts after Shipment.cs on one and before it on the other -- and with it went the entity's reported file and the order of its columns. Files are now read in ordinal path order, and the part declaring the base list is the primary regardless. A document that cannot be regenerated identically is one nobody can diff, which is most of what regenerating it is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| foreach (var key in order) | ||
| { | ||
| var group = parts[key]; | ||
|
|
||
| // The part declaring the base list is the hand-written one, and it is where the class | ||
| // attributes live. Taking whichever half came first instead would make the reported | ||
| // file, and the order of the columns, depend on the file system doing the listing. | ||
| var primary = group.Find(part => part.BaseTypes.Count > 0) ?? group[0]; | ||
| merged.Add(primary); | ||
|
|
||
| foreach (var entity in group) | ||
| { | ||
| if (ReferenceEquals(entity, primary)) continue; | ||
|
|
||
| primary.Description ??= entity.Description; | ||
| primary.NavigationGroup ??= entity.NavigationGroup; | ||
| primary.DefaultProperty ??= entity.DefaultProperty; | ||
| primary.ModelCaption ??= entity.ModelCaption; | ||
| primary.SourceProject ??= entity.SourceProject; | ||
| primary.IsDefaultClassOptions |= entity.IsDefaultClassOptions; | ||
| primary.IsCloneable |= entity.IsCloneable; | ||
|
|
||
| // Non-persistent anywhere means non-persistent: one part saying so is the class. | ||
| primary.IsPersistent &= entity.IsPersistent; | ||
|
|
||
| if (primary.BaseType is "object" or "" && entity.BaseType is not ("object" or "")) | ||
| primary.BaseType = entity.BaseType; | ||
|
|
||
| foreach (var baseType in entity.BaseTypes.Where(name => !primary.BaseTypes.Contains(name))) | ||
| primary.BaseTypes.Add(baseType); | ||
|
|
||
| var known = primary.Properties.Select(property => property.Name).ToHashSet(StringComparer.Ordinal); | ||
| primary.Properties.AddRange(entity.Properties.Where(property => known.Add(property.Name))); | ||
|
|
||
| primary.Relationships.AddRange(entity.Relationships); | ||
| primary.ValidationRules.AddRange(entity.ValidationRules); | ||
| primary.AppearanceRules.AddRange(entity.AppearanceRules); | ||
| primary.InferredBusinessRules.AddRange(entity.InferredBusinessRules); | ||
| primary.SourceComments.AddRange(entity.SourceComments); | ||
| } | ||
| } |
| foreach (var entity in group) | ||
| { | ||
| if (ReferenceEquals(entity, primary)) continue; | ||
|
|
||
| primary.Description ??= entity.Description; | ||
| primary.NavigationGroup ??= entity.NavigationGroup; | ||
| primary.DefaultProperty ??= entity.DefaultProperty; | ||
| primary.ModelCaption ??= entity.ModelCaption; | ||
| primary.SourceProject ??= entity.SourceProject; | ||
| primary.IsDefaultClassOptions |= entity.IsDefaultClassOptions; | ||
| primary.IsCloneable |= entity.IsCloneable; | ||
|
|
||
| // Non-persistent anywhere means non-persistent: one part saying so is the class. | ||
| primary.IsPersistent &= entity.IsPersistent; | ||
|
|
||
| if (primary.BaseType is "object" or "" && entity.BaseType is not ("object" or "")) | ||
| primary.BaseType = entity.BaseType; | ||
|
|
||
| foreach (var baseType in entity.BaseTypes.Where(name => !primary.BaseTypes.Contains(name))) | ||
| primary.BaseTypes.Add(baseType); | ||
|
|
||
| var known = primary.Properties.Select(property => property.Name).ToHashSet(StringComparer.Ordinal); | ||
| primary.Properties.AddRange(entity.Properties.Where(property => known.Add(property.Name))); | ||
|
|
||
| primary.Relationships.AddRange(entity.Relationships); | ||
| primary.ValidationRules.AddRange(entity.ValidationRules); | ||
| primary.AppearanceRules.AddRange(entity.AppearanceRules); | ||
| primary.InferredBusinessRules.AddRange(entity.InferredBusinessRules); | ||
| primary.SourceComments.AddRange(entity.SourceComments); | ||
| } |
|
Reviewed against the application that motivated #1 — 221 entities, legacy LIMS schema, You did not narrow it too far. Nothing was lost.
The set difference is empty in both directions. All 14 that disappeared were the second declaration of a The merge is also exact rather than lossy, which is the part I most wanted to check: Every property survived the fold; none was counted twice. Your three findings were all mine and all correct. The one I want to record for anyone reading later is why my guard test didn't catch them: One thing I noticed while reading, unrelated to this PR and possibly already known.
That is the same one-hop shape as the transitive base walk we talked about, so I will fold it into that issue rather than open a second one. |
|
This is the review I could not have done. I had 279 synthetic tests and one claim about a corpus I have never seen; you had the corpus. Running the merged change against 221 entities and reporting the set difference in both directions is the only thing that could have told me whether I narrowed it too far, and it is worth saying that a green suite on my side was never evidence of that. The property counts are the part I care about most: Exact, not lossy. That was the risk in folding the parts rather than just dropping the duplicate, and it is now measured rather than argued.
I am going to quote you on that, with attribution, in the test file's remark. On The CLI does use the five-name list. So do the sync service and the MCP context; all three write it out by hand, and the tests agree with them: It is the default that agrees with nobody. Which means the population that loses Which also means no test could have caught it, since the tests override it too. #7 asks for one that reads |
|
Correction to my comment above: I claimed |
3 participants
Follow-up to #3, which I merged before this was fixed. @MBrekhof — asking you to review this one, since it is your feature and you will spot faster than anyone whether I have narrowed it too far.
The roster idea is right and it stays. What it needed was an identity: it landed as a set of bare simple names matched globally, and a bare name is not an identity.
The three
1. Every part of a
partialclass matched. Matching by base class could only ever match once — one part declares the base list. Matching by name matches all of them, and the scaffolded split that produces two parts is exactly the population the roster is for. The class came out twice, each copy holding half its columns:Two incomplete truths, with nothing to tell a reader they are the same class.
2. A DTO sharing a simple name became a table.
BusinessObjects.Contracts.Warehouseis a wire shape;BusinessObjects.Warehouseis the entity. The roster could not tell them apart.3.
DbSet<T>as a local counted as a registration.CollectDbSetTypeNameswalked everyGenericNameSyntaxnamedDbSetanywhere in the source, soDbSet<AuditEntry> entries = database.Set<AuditEntry>()inside a method body registeredAuditEntry.What changed
Registrations now carry the namespaces they could have been naming — the registering file's usings, its own namespace, and the namespaces enclosing it. That is ordinary C# lookup, the part of it syntax can see. Aliases,
using staticand extern aliases are not modelled: a registration needing one of those finds no class and is dropped, which is the safe direction, and it is written down in the remark rather than left for someone to discover.They are read only from the properties a
DbContextdeclares. Contexts are found through their base chain too, because an application that puts auditing in a sharedAuditedDbContexthas no context that namesDbContextin its own declaration — reading only classes that say: DbContextwould have found nothing in one, which is the same silent-empty failure the roster exists to fix. That base walk is by simple name; the remark says so, and says why the cost is small here: a class only contributes if it also declaresDbSet<T>properties.The parts of a
partialclass are folded into one entity. That also recovers members XPO extraction has always dropped, where a hand-written part carries: BaseObjectand a generated part carries the mapping — a pre-existing gap, silent until now because nothing ever reported the second part at all.Verification
Three of the four new tests fail on
main:The fourth,
FindsEntitiesRegisteredThroughASharedContextBase, passes onmain— it has to, since the old code matched that name from anywhere. It is a regression guard on this change, not a demonstration of the bug, and I would rather say that than let a green tick imply otherwise.One thing worth knowing about the blast radius
My first draft of these fixtures put the DTO in
Module/Contracts/and the repository inModule/Services/, and two of the three tests passed without the fix. Entity extraction only reads**/BusinessObjects/**, so neither file was ever parsed.So (2) and (3) only bite when the colliding class or the stray
DbSet<T>sits inside a BusinessObjects folder — which happens, but is narrower than I first wrote in the review. The fixtures were moved there so the tests are load-bearing. Correcting my own overstatement, since the rule cuts both ways.Still open
#2 is yours if you want it. Everything in this PR is downstream of the same idea: where the tool cannot tell, it must not pick a side and state it.