fix(query): fall back to full scan for computed properties in predicates - #144
Conversation
BsonExpressionEvaluator translated any bare bool member access (and other member-based patterns: NOT, Equals, string methods, IN, binary comparisons, CompareTo) into a BSON-level field lookup by property name, with no check that the property is actually persisted. A get-only computed property (e.g. `public bool IsOpen => State != Closed`) has no backing BSON field, so the generated predicate scanned every field in the document, never found one named "isopen", and silently returned false for every document - regardless of the real value. `.Where(x => x.IsOpen)` / `.FindAsync(x => x.IsOpen)` therefore always returned empty, while a plain `FindByIdAsync` (no predicate) returned the correct document intact. Added IsPersistedMember (a property is only pushed down if it has a setter) and gated every member-name extraction point in BsonExpressionEvaluator on it. When the check fails, TryCompileBody returns null and the caller falls through to the existing full-scan + in-memory-filter strategy, which evaluates the real getter correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Compound predicates can still partially compile (dropping the computed-property side) and be executed as Strategy 2 without an in-memory re-filter, risking incorrect query results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes incorrect query results when predicates reference get-only computed properties by preventing those members from being pushed down as BSON field lookups, so queries can fall back to the existing full-scan + in-memory evaluation path.
Changes:
- Introduces
IsPersistedMemberand gates all member-name-based predicate compilation paths on it. - Causes
TryCompileBodyto stop compiling member-based predicates for non-persisted members, intended to trigger Strategy 3 fallback inDocumentCollection.FetchAsync.
File summaries
| File | Description |
|---|---|
| src/BLite.Core/Query/BsonExpressionEvaluator.cs | Adds persisted-member gating across predicate compilation paths to avoid incorrect BSON pushdown for computed properties. |
Review details
Suppressed comments (1)
src/BLite.Core/Query/BsonExpressionEvaluator.cs:711
- There are existing unit tests for BsonExpressionEvaluator, but none covering the regression scenario described in #143 (computed/get-only property used in a predicate should force a Strategy 3 fallback and still return correct results). Adding a focused test would help prevent this silently returning wrong results again (including for compound predicates like
x => x.ComputedProp && x.Age > 10).
private static bool IsPersistedMember(MemberInfo member)
=> member is not PropertyInfo { CanWrite: false };
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // ── Bare bool member: e => e.IsActive → IsActive == true ────────────── | ||
| if (body is MemberExpression bareM && | ||
| bareM.Expression == parameter && | ||
| bareM.Type == typeof(bool)) | ||
| bareM.Type == typeof(bool) && | ||
| IsPersistedMember(bareM.Member)) |
There was a problem hiding this comment.
Fixed in 76d8218. AndAlso/OrElse now return null when both sides touch the parameter but either side cannot be compiled, so Strategy 2 won't run with a partially compiled predicate.
| /// <summary> | ||
| /// True for a field, or a property with a setter - the shapes BLite's document mapper actually | ||
| /// persists as a BSON field. A get-only property (<c>public bool IsOpen => State != Closed</c>) has | ||
| /// no backing BSON field at all, so pushing it down into <see cref="CreatePredicate"/> would scan | ||
| /// every document for a field name that can never exist and silently return <c>false</c> for | ||
| /// everyone - wrong, instead of falling back to a real in-memory evaluation of the getter. | ||
| /// </summary> | ||
| private static bool IsPersistedMember(MemberInfo member) | ||
| => member is not PropertyInfo { CanWrite: false }; | ||
|
|
There was a problem hiding this comment.
Fixed in 76d8218. IsPersistedMember now allows fields, settable properties, and getter-only properties when a compiler-generated (<Name>k__BackingField) or conventional (_name) backing field exists.
…ly members Co-authored-by: mrdevrobot <12503462+mrdevrobot@users.noreply.github.com>
Summary
IsOpen => State != Closed) were pushed down into a BSON field lookup by property name, silently returningfalsefor every document since no such field is ever stored.IsPersistedMember(a member is only pushed down if it's a field or a property with a setter) and gated every member-name extraction point inBsonExpressionEvaluatoron it: bare bool member,Nullable.HasValue, logical NOT,.Equals(), string instance methods, static string helpers, the IN operator (bothlist.Contains(x.Prop)andEnumerable.Contains(list, x.Prop)), the general binary comparison path, andCompareTo.TryCompileBodyreturnsnull, soDocumentCollection.FetchAsyncfalls through to its existing full-scan + in-memory-filter strategy, which compiles the real expression tree and evaluates the actual getter correctly - matching plain LINQ-to-Objects semantics instead of silently returning wrong results.Test plan
true, assertFindAsync(x => x.ComputedProp)returns it (currently returns empty without the fix)!x.Prop, string methods, IN, binary comparisons,CompareTo) continue to push down correctlydotnet build/dotnet testonBLite.Core🤖 Generated with Claude Code