fix: null-reference crash when a Where predicate null-checks a captured value (#122)

* fix: null-reference crash when a Where predicate null-checks a captured 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

* refactor: remove comments

* refactor: extract LINQ Where tests

* docs: update Where predicate documentation

* refactor: code cleanup

Author: Tr00d

Postgrest/Interfaces/IPostgrestTable.cs

@@ -352,20 +352,41 @@ IPostgrestTable<TModel> Order(string foreignTable, string column, Constants.Orde
         IPostgrestTable<TModel> Select(Expression<Func<TModel, object[]>> predicate);
 
         /// <summary>
-        /// Filter a query based on a predicate function. 
-        /// 
+        /// Filter a query based on a predicate expression. The expression is translated into a
+        /// Postgrest filter and evaluated server-side - it is never executed as C# code.
+        ///
         /// Note: Chaining multiple <see cref="Where(Expression{Func{TModel, bool}})"/> calls will
         /// be parsed as an "AND" query.
-        /// 
+        ///
         /// Examples:
         ///		`Table&lt;Movie&gt;().Where(x =&gt; x.Name == "Top Gun").Get();`
         ///		`Table&lt;Movie&gt;().Where(x =&gt; x.Name == "Top Gun" || x.Name == "Mad Max").Get();`
         ///		`Table&lt;Movie&gt;().Where(x =&gt; x.Name.Contains("Gun")).Get();`
         ///		`Table&lt;Movie&gt;().Where(x =&gt; x.CreatedAt &lt;= new DateTime(2022, 08, 21)).Get();`
         ///		`Table&lt;Movie&gt;().Where(x =&gt; x.Id > 5 &amp;&amp; x.Name.Contains("Max")).Get();`
         /// </summary>
+        /// <remarks>
+        /// Supported predicate shapes:
+        /// <list type="bullet">
+        /// <item><description>The left side of a comparison must be a Model property (with a `Column` or `PrimaryKey` attribute); the right side a constant, captured variable, or instantiation (`new DateTime(...)`).</description></item>
+        /// <item><description>Comparisons (`==`, `!=`, `&gt;`, `&gt;=`, `&lt;`, `&lt;=`) can be combined with `&amp;&amp;` and `||`, and `String`/collection `Contains` is supported.</description></item>
+        /// <item><description>Null checks translate to Postgrest `is null` filters: `x =&gt; x.Name == null` and `x =&gt; x.Name == null || x.Name == "Top Gun"` both work.</description></item>
+        /// <item><description>Conditions that never reference the Model (i.e. `x =&gt; localVariable == null`) are evaluated locally: an always-true predicate applies no filter, an always-false one throws.</description></item>
+        /// </list>
+        ///
+        /// Invoking a delegate (`Func&lt;TModel, bool&gt;`) inside the predicate is not supported: a compiled
+        /// delegate cannot be translated into a Postgrest filter and throws an <see cref="ArgumentException"/>.
+        /// To apply an optional filter, declare it as an `Expression&lt;Func&lt;TModel, bool&gt;&gt;` and pass it
+        /// conditionally:
+        /// <code>
+        ///	var query = client.Table&lt;Movie&gt;();
+        ///	if (optionalFilter != null)
+        ///		query = query.Where(optionalFilter);
+        /// </code>
+        /// </remarks>
         /// <param name="predicate"></param>
         /// <returns></returns>
+        /// <exception cref="ArgumentException">The predicate contains an expression that cannot be translated into a Postgrest filter, or never matches any row.</exception>
         IPostgrestTable<TModel> Where(Expression<Func<TModel, bool>> predicate);
 
         /// <summary>

Postgrest/Linq/WhereExpressionVisitor.cs

@@ -19,15 +19,45 @@ namespace Supabase.Postgrest.Linq
     /// </summary>
     internal class WhereExpressionVisitor : ExpressionVisitor
     {
+        private ParameterExpression? parameter;
+
+        public WhereExpressionVisitor() { }
+
+        private WhereExpressionVisitor(ParameterExpression? parameter) =>
+            this.parameter = parameter;
+
         /// <summary>
         /// The filter resulting from this Visitor, capable of producing nested filters.
         /// </summary>
         public QueryFilter? Filter { get; private set; }
 
+        /// <summary>
+        /// Set instead of <see cref="Filter"/> when the predicate (or the visited branch of it) never
+        /// references the model and was instead evaluated locally to a boolean
+        /// (i.e. `x => filterPredicate == null || filterPredicate(x)` where `filterPredicate` is null).
+        /// </summary>
+        public bool? ConstantValue { get; private set; }
+
+        /// <inheritdoc />
+        protected override Expression VisitLambda<T>(Expression<T> node)
+        {
+            parameter ??= node.Parameters.FirstOrDefault();
+
+            // A predicate that never references the model (i.e. `x => localVariable == null`) can't be
+            // translated into a filter - it is evaluated locally instead.
+            if (node.Body.Type == typeof(bool) && !ContainsParameter(node.Body))
+            {
+                ConstantValue = (bool)EvaluateExpression(node.Body)!;
+                return node;
+            }
+
+            return base.VisitLambda(node);
+        }
+
         /// <summary>
         /// An entry point that will be used to populate <see cref="Filter"/>.
-        /// 
-        /// Invoked like: 
+        ///
+        /// Invoked like:
         ///		`Table&lt;Movies&gt;().Where(x => x.Name == "Top Gun").Get();`
         /// </summary>
         /// <param name="node"></param>
@@ -44,29 +74,52 @@ protected override Expression VisitBinary(BinaryExpression node)
                 case ExpressionType.Or:
                 case ExpressionType.AndAlso:
                 case ExpressionType.OrElse:
-                    var leftVisitor = new WhereExpressionVisitor();
-                    leftVisitor.Visit(node.Left);
+                    var shortCircuitValue = node.NodeType is ExpressionType.Or or ExpressionType.OrElse;
 
-                    var rightVisitor = new WhereExpressionVisitor();
-                    rightVisitor.Visit(node.Right);
+                    var (leftConstant, leftFilter) = VisitBranch(node.Left);
+
+                    // Follow C#'s short-circuit semantics: `true || anything` and `false && anything`
+                    // never evaluate their right side (i.e. `filterPredicate == null || filterPredicate(x)`).
+                    if (leftConstant == shortCircuitValue)
+                    {
+                        ConstantValue = leftConstant;
+                        return node;
+                    }
+
+                    var (rightConstant, rightFilter) = VisitBranch(node.Right);
+
+                    // A non-short-circuiting constant (`false ||` / `true &&`) reduces to the other side.
+                    if (leftConstant != null)
+                    {
+                        ConstantValue = rightConstant;
+                        Filter = rightFilter;
+                        return node;
+                    }
+
+                    if (rightConstant != null)
+                    {
+                        if (rightConstant == shortCircuitValue)
+                            ConstantValue = rightConstant;
+                        else
+                            Filter = leftFilter;
+
+                        return node;
+                    }
 
                     Filter = new QueryFilter(op,
-                        new List<IPostgrestQueryFilter> { leftVisitor.Filter!, rightVisitor.Filter! });
+                        new List<IPostgrestQueryFilter> { leftFilter!, rightFilter! });
 
                     return node;
             }
 
             // Otherwise, the base case.
 
-            var left = Visit(node.Left);
-            var right = Visit(node.Right);
-
             string? column = null;
-            if (left is MemberExpression leftMember)
+            if (node.Left is MemberExpression leftMember)
             {
                 column = GetColumnFromMemberExpression(leftMember);
             } //To handle properly if it's a Convert ExpressionType generally with nullable properties
-            else if (left is UnaryExpression leftUnary && leftUnary.NodeType == ExpressionType.Convert &&
+            else if (node.Left is UnaryExpression leftUnary && leftUnary.NodeType == ExpressionType.Convert &&
                      leftUnary.Operand is MemberExpression leftOperandMember)
             {
                 column = GetColumnFromMemberExpression(leftOperandMember);
@@ -76,26 +129,51 @@ protected override Expression VisitBinary(BinaryExpression node)
                 throw new ArgumentException(
                     $"Left side of expression: '{node}' is expected to be property with a ColumnAttribute or PrimaryKeyAttribute");
 
-            if (right is ConstantExpression rightConstant)
+            if (node.Right is ConstantExpression rightConstantExpression)
             {
-                HandleConstantExpression(column, op, rightConstant);
+                HandleConstantExpression(column, op, rightConstantExpression);
             }
-            else if (right is MemberExpression memberExpression)
+            else if (node.Right is MemberExpression memberExpression)
             {
                 HandleMemberExpression(column, op, memberExpression);
             }
-            else if (right is NewExpression newExpression)
+            else if (node.Right is NewExpression newExpression)
             {
                 HandleNewExpression(column, op, newExpression);
             }
-            else if (right is UnaryExpression unaryExpression)
+            else if (node.Right is UnaryExpression unaryExpression)
             {
                 HandleUnaryExpression(column, op, unaryExpression);
             }
 
             return node;
         }
 
+        /// <summary>
+        /// Visits one side of an `AND`/`OR` expression, producing either a <see cref="QueryFilter"/> or,
+        /// when the branch never references the model, its locally evaluated boolean value.
+        /// </summary>
+        /// <param name="expression"></param>
+        /// <returns></returns>
+        /// <exception cref="ArgumentException"></exception>
+        private (bool? Constant, QueryFilter? Filter) VisitBranch(Expression expression)
+        {
+            if (expression.Type == typeof(bool) && !ContainsParameter(expression))
+                return ((bool)EvaluateExpression(expression)!, null);
+
+            var visitor = new WhereExpressionVisitor(parameter);
+            visitor.Visit(expression);
+
+            if (visitor.ConstantValue != null)
+                return (visitor.ConstantValue, null);
+
+            if (visitor.Filter == null)
+                throw new ArgumentException(
+                    $"Unable to translate expression '{expression}' into a Postgrest filter. If the condition depends on values that are not model columns (i.e. invoking a delegate), evaluate it outside of `Where` and build the query conditionally instead.");
+
+            return (null, visitor.Filter);
+        }
+
         /// <summary>
         /// Called when evaluating a method 
         /// </summary>
@@ -143,15 +221,7 @@ protected override Expression VisitMethodCall(MethodCallExpression node)
         /// <param name="constantExpression"></param>
         private void HandleConstantExpression(string column, Operator op, ConstantExpression constantExpression)
         {
-            if (constantExpression.Type.IsEnum)
-            {
-                var enumValue = constantExpression.Value;
-                Filter = new QueryFilter(column, op, enumValue);
-            }
-            else
-            {
-                Filter = new QueryFilter(column, op, constantExpression.Value);
-            }
+            Filter = BuildFilter(column, op, constantExpression.Value);
         }
 
         /// <summary>
@@ -162,7 +232,29 @@ private void HandleConstantExpression(string column, Operator op, ConstantExpres
         /// <param name="memberExpression"></param>
         private void HandleMemberExpression(string column, Operator op, MemberExpression memberExpression)
         {
-            Filter = new QueryFilter(column, op, GetMemberExpressionValue(memberExpression));
+            Filter = BuildFilter(column, op, GetMemberExpressionValue(memberExpression));
+        }
+
+        /// <summary>
+        /// Builds a filter from a column, operator and (possibly null) criterion, translating null
+        /// equality checks (i.e. `x => x.Name == null`) into the `IS NULL`/`IS NOT NULL` filters
+        /// Postgrest expects — at any nesting depth.
+        /// </summary>
+        /// <param name="column"></param>
+        /// <param name="op"></param>
+        /// <param name="value"></param>
+        private static QueryFilter BuildFilter(string column, Operator op, object? value)
+        {
+            if (value != null)
+                return new QueryFilter(column, op, value);
+
+            return op switch
+            {
+                Operator.Equals => new QueryFilter(column, Operator.Is, QueryFilter.NullVal),
+                Operator.NotEqual => new QueryFilter(column, Operator.Not,
+                    new QueryFilter(column, Operator.Is, QueryFilter.NullVal)),
+                _ => new QueryFilter(column, op, value)
+            };
         }
 
         /// <summary>
@@ -298,6 +390,46 @@ private Operator GetMappedOperator(Expression node)
             };
         }
 
+        /// <summary>
+        /// Checks if an expression references the model parameter of the `Where` predicate.
+        /// Expressions that don't (i.e. `filterPredicate == null`) can be evaluated locally
+        /// instead of being translated into a filter.
+        /// </summary>
+        /// <param name="expression"></param>
+        /// <returns></returns>
+        private bool ContainsParameter(Expression expression)
+        {
+            var finder = new ParameterFinder(parameter);
+            finder.Visit(expression);
+            return finder.Found;
+        }
+
+        /// <summary>
+        /// Evaluates an expression that doesn't reference the model locally.
+        /// </summary>
+        /// <param name="expression"></param>
+        /// <returns></returns>
+        private static object? EvaluateExpression(Expression expression) =>
+            Expression.Lambda(expression).Compile().DynamicInvoke();
+
+        private class ParameterFinder : ExpressionVisitor
+        {
+            private readonly ParameterExpression? _target;
+
+            public ParameterFinder(ParameterExpression? target) =>
+                _target = target;
+
+            public bool Found { get; private set; }
+
+            protected override Expression VisitParameter(ParameterExpression node)
+            {
+                if (_target == null || node == _target)
+                    Found = true;
+
+                return base.VisitParameter(node);
+            }
+        }
+
         /// <summary>
         /// Gets arguments from a method call expression, (i.e. x => x.Name.Contains("Top")) &lt;- where `Top` is the argument on the called method `Contains`
         /// </summary>

Postgrest/Table.cs

@@ -355,17 +355,18 @@ public IPostgrestTable<TModel> Where(Expression<Func<TModel, bool>> predicate)
             var visitor = new WhereExpressionVisitor();
             visitor.Visit(predicate);
 
+            if (visitor.ConstantValue == true)
+                return this;
+
+            if (visitor.ConstantValue == false)
+                throw new ArgumentException(
+                    "The supplied predicate always evaluates to false, so no row would ever match. Evaluate the condition outside of `Where` and build the query conditionally instead.");
+
             if (visitor.Filter == null)
                 throw new ArgumentException(
                     "Unable to parse the supplied predicate, did you return a predicate where each left hand of the condition is a Model property?");
 
-            if (visitor.Filter.Op == Operator.Equals && visitor.Filter.Criteria == null)
-                _filters.Add(new QueryFilter(visitor.Filter.Property!, Operator.Is, QueryFilter.NullVal));
-            else if (visitor.Filter.Op == Operator.NotEqual && visitor.Filter.Criteria == null)
-                _filters.Add(new QueryFilter(visitor.Filter.Property!, Operator.Not,
-                    new QueryFilter(visitor.Filter.Property!, Operator.Is, QueryFilter.NullVal)));
-            else
-                _filters.Add(visitor.Filter);
+            _filters.Add(visitor.Filter);
 
             return this;
         }
@@ -800,7 +801,13 @@ internal KeyValuePair<string, string> PrepareFilter(IPostgrestQueryFilter filter
                     {
                         var list = new List<KeyValuePair<string, string>>();
                         foreach (var subFilter in subFilters)
+                        {
+                            if (subFilter == null)
+                                throw new ArgumentException(
+                                    $"Expected all filters supplied to a `{filter.Op}` filter to be non-null.");
+
                             list.Add(PrepareFilter(subFilter));
+                        }
 
                         foreach (var preppedFilter in list)
                             strBuilder.Append($"{preppedFilter.Key}.{preppedFilter.Value},");