fix: null-reference crash when a Where predicate null-checks a captured value#122
Merged
Tr00d merged 5 commits intoJul 9, 2026
Conversation
…ed value A LINQ predicate like `x => filterPredicate == null || filterPredicate(x)` crashed with a NullReferenceException on `Get`: branches the visitor could not translate (i.e. a delegate invocation) were combined with the null-forgiving operator, putting a literal null into the nested filter criteria that `PrepareFilter` later dereferenced. The visitor now tracks the lambda parameter and evaluates branches that never reference the model locally, folding them through `||`/`&&` with C#'s short-circuit semantics. The reported idiom therefore works: when the captured delegate is null the predicate is always true and `Where` applies no filter. Branches that do reference the model but cannot be translated (a compiled delegate is opaque) now throw a descriptive ArgumentException pointing at the conditional-query workaround, as does an always-false predicate (silently matching every row would be dangerous for `Update`/`Delete`). Also moves the `== null` -> `is.null` / `!= null` -> `not.is.null` conversion from `Table.Where` (top level only) into the visitor so nested null checks like `x => x.Name == null || x.Name == "foo"` no longer emit an invalid `eq.` filter, and guards `PrepareFilter` against null nested filters supplied through the public QueryFilter constructor. Fixes supabase-community/supabase-csharp#192 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
spydon
approved these changes
Jul 8, 2026
Comment on lines
+26
to
+27
| private WhereExpressionVisitor(ParameterExpression? parameter) => | ||
| this.parameter = parameter; |
There was a problem hiding this comment.
Can't you use a primary constructor here?
Contributor
Author
There was a problem hiding this comment.
@spydon A couple of reasons:
- The SDK isn't on the latest C# version yet, and the current version doesn't support primary constructors.
- Encapsulation: with a primary constructor, any external component could set the ParameterExpression. Currently, only this Visitor can do so (internally).
- It would be bit ugly with nullability and a default value:
internal class WhereExpressionVisitor(ParameterExpression? parameter = null)🤮 There's a bit of semantics/intent too as it would implyParameterExpressionis something expected
I'll have a fun time getting rid of nullability 😅
EDIT: I could upgrade the langversion or go through a deeper rework of the LINQ visitor, but I want this PR to address the bug first, and not refactor the whole thing. These will come later (soon enough); the multi-repo approach is just a pain to deal with atm
QuintinWillison
approved these changes
Jul 8, 2026
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
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.
Fixes supabase-community/supabase-csharp#192
Problem
A common C# idiom for an optional filter crashes on
Get:Three defects combine in the
Wheretranslation:||/&&,WhereExpressionVisitorcombined sub-visitor results with the null-forgiving operator. A branch it has no handler for — here the delegate invocationFilterPredicate(x)— left its sub-filternull, which was stuffed into the nested criteria list and dereferenced later byTable.PrepareFilterduringGenerateUrl(), producing the exactNullReferenceExceptionstack trace in the issue, far from the user's code.requestModel.FilterPredicate == nulldoesn't reference the model at all, yet it was translated as ifFilterPredicatewere a column, producing a bogusFilterPredicate=eq.filter.== null→is.nullconversion lived inTable.Whereand only applied at the top level; inside||/&&(e.g.x => x.Name == null || x.Name == "foo") it emitted an invalideq.filter.Fix
||/&&with C#'s short-circuit semantics. The reported idiom works: when the delegate is null,FilterPredicate == nullfolds totrue, the right side is never touched, andWhereapplies no filter — exactly what the reporter expected.ArgumentExceptionpointing at the conditional-query workaround, instead of an NRE atGet.Update/Delete.is.null/not.is.nullis emitted at any nesting depth.PrepareFilterguards against null nested filters reachable via the publicQueryFilter(Operator.And/Or, …)constructor with a clear exception.Deliberately out of scope: the other known LINQ-provider gaps (closure-list
Contains, bool member columns, column-vs-column comparisons) — same family, separate issues.Tests (written first, red → green)
New tests in
LinqTestsassert onGenerateUrl()(no live instance needed):NullReferenceException, identical stack trace to the issueArgumentExceptionArgumentExceptionname.eq.filteror=(name.is.null,name.eq.…)Full suite against a fresh
supabase startstack: 96/96 pass (rebased on #121).