fix(search): use a translatable predicate in the three list searches … - #52
Open
wilderg88 wants to merge 1 commit into
Open
fix(search): use a translatable predicate in the three list searches …#52wilderg88 wants to merge 1 commit into
wilderg88 wants to merge 1 commit into
Conversation
…that throw
`string.Contains(value, StringComparison)` has no SQL translation in EF Core,
so any non-empty search term on these three lists throws
InvalidOperationException at materialisation rather than filtering:
- Modules/Financial/Expenses/Queries/GetExpenses/GetExpensesHandler.cs:53-54
(throws at the CountAsync on the next line)
- Modules/Platform/Portal/Queries/GetPortalLoads/GetPortalLoadsHandler.cs:25
- Modules/Platform/Portal/Queries/GetPortalInvoices/GetPortalInvoicesHandler.cs:26
All three already compute `var search = req.Search.ToLower()` and then compare
against a column that was never lowered, so the intent was plainly
case-insensitive matching - only the overload was wrong.
The fix is this codebase's own established idiom, not a new one. `ToLower()
.Contains(term)` is already used for exactly this purpose in eight places,
including two handlers with the identical "filter a list by a search box"
shape: GetAdminInvitationsHandler.cs:23 and GetInvitationsHandler.cs:37. These
three handlers were the outliers; this brings them back in line, and it
translates to `lower(col) LIKE '%term%'` on any provider rather than only on
one.
Behaviour is unchanged for every input that previously worked: the term was
already lowercased, so lowering the column produces the same case-insensitive
match - except that it now runs in SQL instead of throwing.
Adds SearchPredicateQueryTranslationTests, following the existing
TenantQuotaUsageQueryTranslationTests pattern in the same project (force
translation with ToQueryString, no database required). It covers all three
predicates, plus a control test asserting the OLD shape still fails to
translate - without that control the other three would pass just as happily if
ToQueryString quietly stopped forcing translation.
The control is also the direct evidence for this fix: run on the real
Npgsql-configured TenantDbContext, `Contains(value, StringComparison)` throws
"could not be translated", which is the whole bug.
Worth flagging separately: the existing handler tests for these queries build
data with MockQueryable, which is LINQ to Objects. The broken overload works
perfectly in memory and throws against PostgreSQL, so those tests cannot tell
a working search from a broken one by construction. That is how one shape
reached three handlers. The new tests are in the Persistence project precisely
so they run against the real provider's translation.
Full suite: 1,263 passed, 4 skipped, 0 failed across all 10 test projects.
|
All contributors have signed the CLA. |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
Author
|
recheck |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
string.Contains(value, StringComparison)has no SQL translation in EF Core, so a non-empty search term on three lists throwsInvalidOperationExceptionat materialisation instead of filtering:GetExpensesHandler.cs:53-54(throws at theCountAsyncon the next line)GetPortalLoadsHandler.cs:25GetPortalInvoicesHandler.cs:26All three already compute
var search = req.Search.ToLower()and then compare against a column that was never lowered, so the intent was case-insensitive matching and only the overload was wrong.The fix uses the idiom this repo already uses for the same job in eight places, including
GetAdminInvitationsHandler.cs:23andGetInvitationsHandler.cs:37, which have the identical shape. Behaviour is unchanged for every input that previously worked; it now runs in SQL instead of throwing, and translates on any provider rather than one.Adds
SearchPredicateQueryTranslationTestsfollowing the existingTenantQuotaUsageQueryTranslationTestspattern in the same project -ToQueryString(), no database needed - covering all three predicates plus a control asserting the old shape still fails to translate.One thing worth flagging beyond this fix: the existing handler tests for these queries use
MockQueryable, i.e. LINQ to Objects, where the broken overload works fine. They cannot detect an EF translation failure by construction, which is probably why one shape reached three handlers.Full suite: 1,263 passed, 4 skipped, 0 failed.