HFW1004: Analyzer použití FilteringCollection v dotazu do databáze

[vaclavek]

Proč

FilteringCollection<T> je in-memory wrapper nad namapovanou kolekcí ([NotMapped]), EF Core ji nepřeloží do SQL. Ověřeno empiricky na modelu 180.EXE (ExtranetDbContext, ToQueryString(), SqlServer provider):

tvar dotazu	chování EF Core
Where(x => x.UserCards.Any(…))	InvalidOperationException – could not be translated
Where(x => x.UserCards.Count > 0)	InvalidOperationException
OrderBy(x => x.UserCards.Count)	InvalidOperationException
SelectMany(x => x.UserCards)	InvalidOperationException
Include(x => x.UserCards)	InvalidOperationException – „is invalid inside an 'Include' operation"
Select(x => new { Ids = x.UserCards.Select(uc => uc.Id).ToList() })	InvalidOperationException
Select(x => new { Count = x.UserCards.Count })	projde bez chyby – SQL na navázanou tabulku vůbec nesáhne, klientské vyhodnocení běží nad nenaplněnou kolekcí a vrátí 0
Select(x => new { Cards = x.UserCards.ToList() })	projde bez chyby – prázdný seznam

Poslední dva případy jsou hlavní motivace: hlasitý pád si člověk najde při prvním spuštění, nulu v gridu ne. Lazy loading nikde zapnutý není (a properties nejsou virtual), takže se nic nedonačte – výsledek závisí jen na tom, co náhodou drží change tracker.

Co PR přidává

HFW1004 (FilteringCollectionInExpressionTreeAnalyzer, Warning, kategorie Usage) hlásí přístup ke členu typu FilteringCollection<T> uvnitř lambdy konvertované na Expression<…>.

Aby nepálil na legitimní použití, rozlišuje dva tvary:

• entity => entity.Collection (celé tělo lambdy) – hlásí jen tehdy, jde-li o metodu nad IQueryable (Include). Tenhle tvar totiž používá i IDataLoader.Load*, IFluentDataLoader.ThenLoad*, FluentValidation RuleFor nebo Moq Setup.
• entity.Collection jako součást větší expression (.Any(), .Count, projekce, subquery) – hlásí vždy, tedy i ve WhereIf, OrderByMultiple nebo v lambdě složené v pomocné metodě vracející Expression<Func<T, bool>>.

Navíc je natvrdo vyloučené volání data loaderu (podle typu IDataLoader / IFluentDataLoader / FluentDataLoaderExtensions) – data loader FilteringCollection podporuje, PropertyLoadSequenceResolverIncludingDeletedFilteringCollectionsSubstitution si sám přepíše property na XIncludingDeleted.

Hláška navrhne konkrétní jméno XIncludingDeleted, pokud ho entita deklaruje (stejná substituce, jakou dělá data loader), jinak obecný text:

warning HFW1004: 'UserCards' is a FilteringCollection which cannot be translated to SQL
- the query fails at runtime or silently returns no data.
Use 'UserCardsIncludingDeleted' with an explicit filter instead.

Druhý commit: úklid balíčku

Balíček …Patterns.Analyzers nesl závislosti, které do vývojové závislosti s analyzery nepatří – ProjectReference na Havit.Core (analyzer z něj nic nepoužívá, jen linkuje DiagnosticsIdentifiers.cs) a nepoužívaný Microsoft.CodeAnalysis.CSharp.Workspaces bez PrivateAssets. Doplněn Microsoft.CodeAnalysis.Analyzers (kontrola AnalyzerReleases.*.md) a README s popisem HFW1002–HFW1004. PackageVersion 2.10.1 → 2.10.2. Výsledný balíček má nulové závislosti a obsahuje jen analyzers/dotnet/cs/*.dll.

Ověření

• 13 nových testů (CSharpAnalyzerTest), celý projekt 30/30 zelený. Pokrývají oba hlásící tvary, tichou projekci, Include, IDataLoader.Load / LoadAllAsync / ThenLoad, in-memory LINQ (delegát, ne expression), RuleFor-like API mimo IQueryable, namapovanou kolekci a fallback hlášky bez XIncludingDeleted protějšku.
• End-to-end na reálném kódu 180.EXE: analyzer injektovaný do buildu Model + DataLayer + Services + Facades → 0 hlášení (včetně tří míst, kde se FilteringCollection legitimně načítá data loaderem). Po umělém záměně UserCardsIncludingDeleted → UserCards v UserDetailEmploymentQuery analyzer zahlásil HFW1004 se správným návrhem.

Poznámky k review

• Zvolený režim je vědomě široký: hlásí i v expression tree, které nekončí v databázi (např. RuleFor(x => x.Addresses.Count)). Mitigace je severita Warning + .editorconfig/#pragma; allowlist se dá rozšířit podle typu.
• Aby pravidlo doputovalo do 180.EXE, bude potřeba v něm zaregistrovat PackageReference na tento balíček – to je samostatná věc, tady není.

[vaclavek]

Code review

Celkově solidní, promyšlený analyzer s dobře zdokumentovanou motivací (zejména tichý případ v projekci) a rozumným kompromisem mezi falešnými pozitivy a negativy. Kritické problémy jsem nenašel — jde o dev-time nástroj bez bezpečnostní plochy. Našel jsem ale 1 HIGH (tichá díra na query syntax), 2 MEDIUM (výkon analyzeru, mezery v testech) a 4 LOW — detaily v inline komentářích.

General

Falešná pozitiva v „broad“ režimu (MEDIUM, vědomé rozhodnutí): PR popis přiznává, že se hlásí i expression trees, které do databáze nikdy nedoputují — RuleFor(x => x.Addresses.Count), Moq SetupGet(m => m.Children.Count), AutoMapper ForMember(...) apod. Celotělový tvar je vyjmutý, ale jakmile je přístup součástí větší expression, hlásí se vždy. Než se pravidlo v aplikacích zpřísní na error, doporučuji doplnit allowlist podle containing type konzumující metody (např. Moq.*, FluentValidation.*, AutoMapper.*) — jinak si projekty pravidlo potlačí v .editorconfig plošně a přijde se o celý přínos.

Chybí CodeFixProvider (recommendation): hláška už dnes zná přesné jméno XIncludingDeleted protějšku. Nabízet automatickou opravu (výměna identifikátoru + TODO komentář na doplnění filtru Deleted == null) by výrazně zvedlo adopci; klidně jako follow-up.

Pozitivní postřehy (stručně)

Empiricky ověřená motivace v PR popisu, end-to-end validace na 180.EXE, úklid závislostí balíčku (odstranění zbytečné ProjectReference na Havit.Core a Workspaces bez PrivateAssets) je správně.

Code review by @claude

[vaclavek]

HIGH — falešné negativum: query syntax zcela projde. Analyzer je registrovaný jen pro SimpleLambdaExpression/ParenthesizedLambdaExpression. LINQ dotaz zapsaný query syntaxí:

var result = from m in dbContext.Masters
             where m.Children.Any()
             select m;

neobsahuje žádný lambda syntax node (lambdy vznikají až při bindingu), takže projde bez hlášení — za běhu přitom spadne úplně stejně. U pravidla, jehož smyslem je bezpečnostní síť proti tichému chování, je tichá díra na celý dialekt zápisu dotazů podstatná: vyvolává falešný pocit „0 hlášení = čisto“.

Nabízí se přepis na IOperation API: context.RegisterOperationAction(..., OperationKind.PropertyReference) + kontrola, zda operace leží uvnitř expression tree (předek IAnonymousFunctionOperation v pozici Expression konverze; query syntax je v operation tree už přeložená na lambdy — viz ITranslatedQueryOperation). Jedna implementace pak pokryje obě syntaxe a odpadne ruční GetSymbolInfo na každý member access. Minimálně bych ale gap explicitně zdokumentoval v README a pokryl testem.

Code review by @claude

[vaclavek]

Přepsáno na IOperation API: RegisterOperationAction pro OperationKind.PropertyReference/FieldReference, expression tree se pozná podle nadřazené IConversionOperation na Expression<TDelegate>. Query syntax je v operation tree už přeložená na lambdy (ITranslatedQueryOperation), takže ji stejná implementace pokrývá — doplněn pozitivní test (query syntax nad IQueryable) i negativní (query syntax nad List). Odpadlo i ruční GetSymbolInfo na každý member access.

[vaclavek]

MEDIUM — výkon analyzeru. Dvě věci, které se sčítají v IDE (analyzer běží při každém stisku klávesy nad otevřeným souborem):

1. Pro každou expression-tree lambdu se prochází Ancestors() až ke kořeni syntax tree a na každou nadřazenou lambdu se volá GetTypeInfo. V souborech hustých na dotazy (typický DataLayer/Services) je to O(počet lambd × hloubka zanoření). Levnější je omezit průchod na AncestorsAndSelf().TakeWhile(...) po nejbližší statement, nebo problém obrátit — analyzovat jen top-level lambdy nalezené shora.
2. Chybí compilation-level brána. Doporučený vzor: context.RegisterCompilationStartAction, v ní jednou compilation.GetTypeByMetadataName("Havit.Model.Collections.Generic.FilteringCollection1")(a stejně takExpression, IQueryable, IDataLoader, …); při nullse node action vůbec neregistruje a jinak se symboly porovnávají přesSymbolEqualityComparermísto dvojicName+ContainingNamespace.ToDisplayString()` (to při shodě jména pokaždé alokuje string). Kompilace bez reference na Havit.Core pak analyzer nestojí vůbec nic a porovnání je přesnější (dnes by prošel i cizí typ se shodným jménem a namespace).

Code review by @claude

[vaclavek]

Doplněna compilation-start brána: RegisterCompilationStartAction jednou resolvne známé typy přes GetTypeByMetadataName; bez reference na FilteringCollection se žádná akce neregistruje. Symboly se porovnávají přes SymbolEqualityComparer místo dvojic Name + ContainingNamespace.ToDisplayString(). Průchod Ancestors() s GetTypeInfo na každou nadřazenou lambdu odpadl s přepisem na IOperation — každá member reference se navštíví právě jednou a nahoru se jde přes IOperation.Parent bez dotazů do semantic modelu.

[vaclavek]

LOW — příliš hrubá výjimka pro data loader. Vyloučení platí pro jakoukoli metodu, jejíž containing type implementuje IDataLoader/IFluentDataLoader — tedy i pro metody, které s načítáním nesouvisí. A obráceně: dekorátor/helper, který interface neimplementuje, ale expression do data loaderu jen přeposílá, vyloučený nebude a bude generovat falešná pozitiva. Navíc se přeskočí celá lambda bez ohledu na tvar, ne jen property-path. Zvážil bych zúžení na známá jména metod (Load, LoadAsync, LoadAll, LoadAllAsync, ThenLoad*) — konstanty pro ně už v FilteringCollectionConstants stejně dávají smysl.

Code review by @claude

[vaclavek]

Ponecháno podle typu, záměrně: IDataLoader, IFluentDataLoader i FluentDataLoaderExtensions obsahují výhradně Load*/ThenLoad* metody, takže vyloučení podle typu je dnes ekvivalentní jmennému allowlistu a na rozdíl od něj se nerozbije při přidání overloadu nebo přejmenování. Falešná pozitiva u helperů, které expression jen přeposílají, jmenný allowlist neřeší také. Chování „přeskočí se celá lambda bez ohledu na tvar" je zafixované novým testem (ne-celotělová lambda předaná data loaderu).

[vaclavek]

LOW — alokace při každém přístupu. SupportedDiagnostics volá Roslyn velmi často a collection expression tady pokaždé vytvoří nové ImmutableArray. Uložit do private static readonly ImmutableArray<DiagnosticDescriptor> a vracet pole. (Stejný nešvar mají i stávající analyzery v projektu — nový kód je příležitost to zavést správně.)

Code review by @claude

[vaclavek]

Opraveno — private static readonly ImmutableArray<DiagnosticDescriptor>, property vrací pole.

[vaclavek]

LOW — zduplikovaná konvence. Sufix IncludingDeleted je tady zduplikovaný vůči runtime substituci (PropertyLoadSequenceResolverIncludingDeletedFilteringCollectionsSubstitution). Analyzer projekt na Patterns referencovat nemůže, takže duplicita je pochopitelná — ale zaslouží si křížový komentář na obou místech. Když se konvence v resolveru změní (nebo aplikace používá vlastní resolver s jiným pojmenováním), hláška začne navrhovat neexistující/nesprávný member a nikoho nenapadne hledat druhé místo.

Code review by @claude

[vaclavek]

Doplněny křížové komentáře na obou místech — u FilteringCollectionConstants.IncludingDeletedSuffix i v PropertyLoadSequenceResolverIncludingDeletedFilteringCollectionsSubstitution.

[vaclavek]

MEDIUM — mezery v pokrytí testy. 13 testů pokrývá hlavní tvary, ale netestované zůstávají právě ty nejkřehčí větve analyzeru:

1. Deduplikace vnořených lambd (řádek 49 analyzeru) nemá žádný test — nikde není ověřeno, že se pro quoted vnořenou lambdu uvnitř outer expression tree diagnostika hlásí právě jednou, a ne dvakrát.
2. GetSuggestion přes dědičnost — průchod BaseType řetězcem (protějšek XIncludingDeleted deklarovaný na bázové entitě) není pokrytý.
3. Data loader s ne-celotělovou lambdou — chybí negativní/pozitivní test tvaru, kdy FilteringCollection není celým tělem lambdy předané data loaderu (dnes se přeskočí celá lambda — je to záměr? test by to zafixoval).
4. Query syntax — test by zdokumentoval známé falešné negativum (viz komentář u registrace).

Code review by @claude

[vaclavek]

Doplněno 6 testů (celkem 36, vše zelené): 1) vnořená quoted lambda — diagnostika právě jednou (po přepisu na IOperation je deduplikace inherentní, každá member reference se navštíví jednou); 2) XIncludingDeleted deklarovaný na bázové entitě — návrh přes dědičnost; 3) data loader s ne-celotělovou lambdou — zafixováno, že se přeskakuje celá lambda; 4) params overload data loaderu (lambda zabalená v poli); 5) query syntax nad IQueryable — po přepisu pozitivní test, ne dokumentace falešného negativa; 6) query syntax nad in-memory kolekcí — negativní.

[vaclavek]

LOW — verzování. Nové pravidlo s default severitou Warning je nová funkcionalita, a u konzumentů s TreatWarningsAsErrors (nebo dotnet_analyzer_diagnostic.category-Usage.severity = error) může po updatu balíčku rozbít build. Podle semver by to měl být minor bump (2.11.0), ne patch (2.10.2) — patch signalizuje „bezpečné přijmout automaticky", což tady neplatí.

Code review by @claude

[vaclavek]

Opraveno — PackageVersion 2.11.0.

[jirikanda]

Select(x => new { Count = x.UserCards.Count })	projde bez chyby – SQL na navázanou tabulku vůbec nesáhne, klientské vyhodnocení běží nad nenaplněnou kolekcí a vrátí 0
Select(x => new { Cards = x.UserCards.ToList() })	projde bez chyby – prázdný seznam

Není to spíš na nareportování do EF Core?

[jirikanda]

Nelíbí se mi hrozící false positives, to je vždycky peklo největší. Pojdmě na to sednout a vymyslet, jaké scénáře nás trápí. Třeba z toho vypadne, že je naučíme EF Core překládat 💪 a toto vůbec nebude potřeba.
Už k tomu nějaká rozvaha byla, i nějaké mantinely použitelnosti, viz https://dev.azure.com/havit/DEV/_workitems/edit/82961.