fix(query): preserve discriminator predicates in type filters#263
Conversation
- Enhanced inheritance query tests with SQL assertion checks. - Verified generated SQL for TPH discriminator and derived type filtering scenarios. - Improved test reliability by validating expected SQL queries during execution. docs(query): clarify limitations in type filtering - Documented support for `GetType` with exact concrete entity types. - Updated guidance on using `is` and `OfType<T>` for derived type filtering. refactor(query): handle missing discriminator predicates gracefully - Added checks for missing discriminator predicates in `OfType<TDerived>` queries. - Returned unsupported operator messages when discriminator filtering is required but unavailable. - Ensured consistent behavior across all type hierarchy queries.
…icate validation - Added SQL assertion checks for `OfType<T>` inheritance query tests. - Improved handling of missing discriminator predicates for filtered type hierarchies. - Refactored `CreateFalsePredicate` for better code reusability in query translation. - Updated documentation to clarify concrete type filtering limits and guidance for derived types.
- Extended `NoSyncTest` to include an `expectSyncFailure` flag for more flexible test scenarios. - Updated tests to use the new parameter, allowing explicit control over sync failure expectations. - Revised `DynamoTestHelpers` to conditionally assert sync failures based on the new flag. - Improved code readability by consistently applying the optional parameter across test cases.
- Included `Microsoft.EntityFrameworkCore.Query.Inheritance` namespace conditionally for .NET 11+. - Ensured compatibility with new APIs introduced in recent .NET versions.
…c' into test/tests-inheritance-query-spec
…d 11 builds - Added support for named EF Core 10 (`Debug EF10`, `Release EF10`) and EF Core 11 (`Debug EF11`, `Release EF11`) build configurations. - Introduced specific commands for building, testing, and packing EF Core 10 and 11 using `task`. - Enhanced flexibility with options to install CI-matching .NET 11 SDK for consistent local builds. - Updated documentation with detailed commands and usage examples for managing EF Core versions.
…nces - Added `NU1510` to `NoWarn` in the project file to handle incorrect package pruning warnings. - Documented the need for retaining `.NET 11` package references to prevent compilation errors.
- Moved `NU1510` suppression to individual package references instead of a global `NoWarn` property. - Updated comments to explain package pruning behavior and necessity of retaining references. - Ensured clarity regarding required public API types for .NET 11 compatibility.
- Added detection for Enumerable/Queryable Any() calls in primitive collections. - Implemented SQL size-based translation for Any() without predicates. - Introduced method `TranslateCollectionAny` for handling Any() expressions. - Added test case for Any() on primitive collections in subtype queries.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 019f3f7809
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ncipollina
left a comment
ncipollina
left a comment
There was a problem hiding this comment.
Review: fix(query) — preserve discriminator predicates in type filters
This is a substantial, well-structured addition. The core translation work (OfType, is, GetType()) is solid and the spec test coverage is comprehensive. Several correctness and design points to address before merging.
Must Fix
1. Cross-root assignability guard is over-constrained (both visitors)
The double-sided GetRootType() check rejects valid same-hierarchy queries. If sourceEntityType is Animal (root) and targetEntityType is Kiwi (derived), targetEntityType.GetRootType() == Animal, which is assignable from sourceEntityType.GetRootType() == Animal. So the second clause always passes for valid same-hierarchy cases, meaning the only real guard is the first. But the first clause can reject when the source is a non-root base (e.g., Bird navigating to Kiwi) if the roots differ across mapped tables. The two-clause pattern should be sourceRoot == targetRoot; the current form is confusing and has a latent bug for multi-table mixed hierarchies.
2. VisitTypeBinary ignores TypeAs / other node types silently
ExpressionType.TypeAs is a valid TypeBinaryExpression node type (C# as operator). The current implementation returns NotTranslatedExpression for it, which is correct, but the node.NodeType == ExpressionType.TypeIs check burns through both cases in one ternary, making it harder to see intent. A named constant or early-return guard would be clearer.
3. TranslateCollectionAny — IsNotMissing is semantically wrong for empty collections
IsNotMissing checks attribute existence, but DynamoDB can store an empty list that is present but has zero elements. A stored [] passes IS NOT MISSING but .Any() should return false. The correct translation would be a size check (e.g., size(attr) > 0) if supported, or fall back to NotTranslatedExpression for now. This is a correctness bug.
4. NoSyncTest(expectSyncFailure: false) — silent success when exception is not thrown
The original NoSyncTest always called Assert.Fail when sync succeeded unexpectedly. The new expectSyncFailure = false path returns silently even when the sync call threw a different exception type. That swallows unexpected failures for the six callers that pass false. The catch clause should remain specific: only catch IsExpectedSyncQueryFailure exceptions, and re-throw anything else regardless of the flag.
Suggestions
5. CreateFalsePredicate() — 1 = 0 is fragile
A literal 1 = 0 may or may not be valid PartiQL. A dedicated SqlFalseExpression or a known-false constant expression would be safer. At minimum, add a unit test that verifies the false predicate serializes to valid PartiQL.
6. Redundant discriminator predicate in OfType + entity-type root filter
Can_use_of_type_bird_with_projection shows WHERE ("discriminator" = 'Eagle' OR "discriminator" = 'Kiwi') AND ("discriminator" = 'Eagle' OR "discriminator" = 'Kiwi'). The root entity materializer applies its own discriminator predicate, and TranslateOfType appends another. EF Cosmos deduplicates this. The query still executes correctly (it's AND of identical predicates), but it's wasteful and looks like a bug in generated output. Worth a follow-up issue.
7. #if NET11_0_OR_GREATER in ComplianceDynamoTest.cs — guarded using without guarded type reference
InheritanceQueryTestBase<> is yielded unconditionally on EF10 via the EF10 namespace import at the top of the file. The #if NET11_0_OR_GREATER guard on the using is inverted from what InheritanceQueryDynamoTest.cs does (which guards with #if NET10_0 for the old namespace). Double-check that EF10 can resolve InheritanceQueryTestBase<> from Microsoft.EntityFrameworkCore.Query as imported by ComplianceDynamoTest.cs.
Positives
✅ The TryUnwrapGetTypeComparison with bidirectional argument order check (|| TryUnwrap(right, left, ...)) correctly handles both GetType() == typeof(T) and typeof(T) == GetType() forms.
✅ Wrapping discriminator predicates in SqlDiscriminatorPredicateExpression before passing to Not() is clean — it preserves the predicate's identity for downstream inspection.
✅ The exact parameter that selects GetConcreteDerivedTypesInclusive vs. [targetEntityType] is a clear, non-duplicated design for is vs. GetType() semantics.
✅ The OfTypeRequiresDiscriminatorPredicate error string clearly distinguishes the no-discriminator-metadata failure from the general unsupported case.
✅ Promoting InheritanceQueryTestBase from the Future table to Implemented in docs/spec-test-coverage.md with accurate method counts is the right housekeeping.
✅ The expectSyncFailure parameter solves the real problem (tests that succeed on sync path but EF throws for unrelated reasons). The fix direction is right; only the exception re-throw gap needs closing.
|
Addressed review feedback in 85af743:\n\n- Replaced double-sided root assignability checks with explicit root equality in both visitors.\n- Made TypeBinary handling use an explicit non-TypeIs early return.\n- Changed primitive collection Any() translation from IS NOT MISSING to size(attr) > 0 and updated EF11 spec baseline.\n- Fixed NoSyncTest so expected sync-query failures are only swallowed when expectSyncFailure is true.\n- Simplified OfType identity/base helper.\n- Opened follow-up #264 for discriminator predicate deduplication.\n\nValidation:\n- task test:spec CONFIG="Debug EF10"\n- task test:spec CONFIG="Debug EF11" |
ncipollina
left a comment
ncipollina
left a comment
There was a problem hiding this comment.
All three must-fix items from the previous review have been correctly addressed:
- ✅
TranslateCollectionAnynow usessize(attr) > 0— semantically correct for primitive collection.Any() - ✅
NoSyncTestre-throws whenexpectSyncFailure == false— no more silent swallowing - ✅ Double-sided
IsAssignableFromreplaced with clean!=equality check
Happy to approve. 🎉
2 participants
Summary
Preserves discriminator predicate metadata when translating C# type filters so downstream query generation can handle them correctly. Documents that
OfType<T>(),is, andGetType()type filtering require active discriminator metadata when shared-table discrimination has been disabled.Changes
ispredicates andGetType()equality predicates inSqlDiscriminatorPredicateExpression.GetType() != typeof(...)through the wrapped discriminator predicate.docs/limitations.mdanddocs/modeling/single-table-design.md.Validation
dotnet buildRelease Notes
HasNoDiscriminator()should use key predicates or concreteDbSetqueries when keys already isolate types.