diff --git a/Postgrest/Interfaces/IPostgrestTable.cs b/Postgrest/Interfaces/IPostgrestTable.cs index eb480ef..6849854 100644 --- a/Postgrest/Interfaces/IPostgrestTable.cs +++ b/Postgrest/Interfaces/IPostgrestTable.cs @@ -352,11 +352,12 @@ IPostgrestTable Order(string foreignTable, string column, Constants.Orde IPostgrestTable Select(Expression> predicate); /// - /// 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 calls will /// be parsed as an "AND" query. - /// + /// /// Examples: /// `Table<Movie>().Where(x => x.Name == "Top Gun").Get();` /// `Table<Movie>().Where(x => x.Name == "Top Gun" || x.Name == "Mad Max").Get();` @@ -364,8 +365,28 @@ IPostgrestTable Order(string foreignTable, string column, Constants.Orde /// `Table<Movie>().Where(x => x.CreatedAt <= new DateTime(2022, 08, 21)).Get();` /// `Table<Movie>().Where(x => x.Id > 5 && x.Name.Contains("Max")).Get();` /// + /// + /// Supported predicate shapes: + /// + /// 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(...)`). + /// Comparisons (`==`, `!=`, `>`, `>=`, `<`, `<=`) can be combined with `&&` and `||`, and `String`/collection `Contains` is supported. + /// Null checks translate to Postgrest `is null` filters: `x => x.Name == null` and `x => x.Name == null || x.Name == "Top Gun"` both work. + /// Conditions that never reference the Model (i.e. `x => localVariable == null`) are evaluated locally: an always-true predicate applies no filter, an always-false one throws. + /// + /// + /// Invoking a delegate (`Func<TModel, bool>`) inside the predicate is not supported: a compiled + /// delegate cannot be translated into a Postgrest filter and throws an . + /// To apply an optional filter, declare it as an `Expression<Func<TModel, bool>>` and pass it + /// conditionally: + /// + /// var query = client.Table<Movie>(); + /// if (optionalFilter != null) + /// query = query.Where(optionalFilter); + /// + /// /// /// + /// The predicate contains an expression that cannot be translated into a Postgrest filter, or never matches any row. IPostgrestTable Where(Expression> predicate); /// diff --git a/Postgrest/Linq/WhereExpressionVisitor.cs b/Postgrest/Linq/WhereExpressionVisitor.cs index 8b6b63c..1e2da08 100644 --- a/Postgrest/Linq/WhereExpressionVisitor.cs +++ b/Postgrest/Linq/WhereExpressionVisitor.cs @@ -19,15 +19,45 @@ namespace Supabase.Postgrest.Linq /// internal class WhereExpressionVisitor : ExpressionVisitor { + private ParameterExpression? parameter; + + public WhereExpressionVisitor() { } + + private WhereExpressionVisitor(ParameterExpression? parameter) => + this.parameter = parameter; + /// /// The filter resulting from this Visitor, capable of producing nested filters. /// public QueryFilter? Filter { get; private set; } + /// + /// Set instead of 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). + /// + public bool? ConstantValue { get; private set; } + + /// + protected override Expression VisitLambda(Expression 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); + } + /// /// An entry point that will be used to populate . - /// - /// Invoked like: + /// + /// Invoked like: /// `Table<Movies>().Where(x => x.Name == "Top Gun").Get();` /// /// @@ -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 { leftVisitor.Filter!, rightVisitor.Filter! }); + new List { 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,19 +129,19 @@ 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); } @@ -96,6 +149,31 @@ protected override Expression VisitBinary(BinaryExpression node) return node; } + /// + /// Visits one side of an `AND`/`OR` expression, producing either a or, + /// when the branch never references the model, its locally evaluated boolean value. + /// + /// + /// + /// + 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); + } + /// /// Called when evaluating a method /// @@ -143,15 +221,7 @@ protected override Expression VisitMethodCall(MethodCallExpression node) /// 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); } /// @@ -162,7 +232,29 @@ private void HandleConstantExpression(string column, Operator op, ConstantExpres /// private void HandleMemberExpression(string column, Operator op, MemberExpression memberExpression) { - Filter = new QueryFilter(column, op, GetMemberExpressionValue(memberExpression)); + Filter = BuildFilter(column, op, GetMemberExpressionValue(memberExpression)); + } + + /// + /// 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. + /// + /// + /// + /// + 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) + }; } /// @@ -298,6 +390,46 @@ private Operator GetMappedOperator(Expression node) }; } + /// + /// 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. + /// + /// + /// + private bool ContainsParameter(Expression expression) + { + var finder = new ParameterFinder(parameter); + finder.Visit(expression); + return finder.Found; + } + + /// + /// Evaluates an expression that doesn't reference the model locally. + /// + /// + /// + 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); + } + } + /// /// Gets arguments from a method call expression, (i.e. x => x.Name.Contains("Top")) <- where `Top` is the argument on the called method `Contains` /// diff --git a/Postgrest/Table.cs b/Postgrest/Table.cs index ed38f65..43b126a 100644 --- a/Postgrest/Table.cs +++ b/Postgrest/Table.cs @@ -355,17 +355,18 @@ public IPostgrestTable Where(Expression> 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 PrepareFilter(IPostgrestQueryFilter filter { var list = new List>(); 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},"); diff --git a/PostgrestTests/LinqTests.cs b/PostgrestTests/LinqTests.cs index 8b68b94..06f8a2c 100644 --- a/PostgrestTests/LinqTests.cs +++ b/PostgrestTests/LinqTests.cs @@ -43,101 +43,6 @@ public async Task TestLinqSelect() }); } - [TestMethod("Linq: Where")] - public async Task TestLinqWhere() - { - var client = new Client(BaseUrl); - - // Test boolean equality - var query1 = await client.Table() - .Where(x => x.Id == "ea07bd86-a507-4c68-9545-b848bfe74c90") - .Get(); - - Assert.IsTrue(query1.Models.Count == 1); - - // Test string.contains - var query2 = await client.Table() - .Where(x => x.Name!.Contains("Gun")) - .Get(); - - Assert.IsTrue(query2.Models.Count == 2); - - // Test multiple conditions - var query3 = await client.Table() - .Where(x => x.Name!.Contains("Gun") && x.CreatedAt <= new DateTimeOffset(new DateTime(2022, 8, 23))) - .Get(); - - Assert.IsTrue(query3.Models.Count == 1); - - // Test null value checking - var query4 = await client.Table() - .Where(x => x.StringValue != null) - .Get(); - - foreach (var q in query4.Models) - Assert.IsNotNull(q.StringValue); - - // Test Collection Contains - var query5 = await client.Table() - .Where(x => x.ListOfStrings!.Contains("set")) - .Get(); - - foreach (var q in query5.Models) - Assert.IsTrue(q.ListOfStrings!.Contains("set")); - - var query6 = await client.Table() - .Where(x => x.ListOfFloats!.Contains(10)) - .Get(); - - foreach (var q in query6.Models) - Assert.IsTrue(q.ListOfFloats!.Contains(10)); - - var query7 = await client.Table() - .Filter(x => x.DateTimeValue!, Operator.NotEqual, null) - .Get(); - - foreach (var q in query7.Models) - Assert.IsNotNull(q.DateTimeValue); - - //Testing where condition with Enum as constant - var query8 = await client.Table() - .Where(x => x.Status == MovieStatus.OnDisplay) - .Get(); - foreach (var q in query8.Models) - Assert.IsTrue(q.Status == MovieStatus.OnDisplay); - - //Test where condition with Enum as Memeber expression - var testMovie = new Movie { Status = MovieStatus.OnDisplay }; - var query9 = await client.Table() - .Where(x => x.Status == testMovie.Status) - .Get(); - foreach (var q in query9.Models) - Assert.IsTrue(q.Status == MovieStatus.OnDisplay); - - await client.Table() - .Where(x => x.DateTimeValue == DateTime.Now) - .Get(); - - await client.Table() - .Where(x => x.DateTimeValue == null) - .Get(); - - await client.Table() - .Where(x => x.DateTimeValue == null) - .Set(x => x.BooleanValue!, true) - .Update(); - - await client.Table() - .Where(x => x.DateTimeValue == null) - .Set(x => x.BooleanValue, true) - .Update(); - - await client.Table() - .Where(x => x.DateTimeValue == null) - .Set(x => x.StringValue!, null) - .Update(); - } - [TestMethod("Linq: OnConflict")] public async Task TestLinqOnConflict() { diff --git a/PostgrestTests/LinqWhereTests.cs b/PostgrestTests/LinqWhereTests.cs new file mode 100644 index 0000000..30008e7 --- /dev/null +++ b/PostgrestTests/LinqWhereTests.cs @@ -0,0 +1,151 @@ +using System; +using System.Threading.Tasks; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Supabase.Postgrest; +using PostgrestTests.Models; +using static Supabase.Postgrest.Constants; + +namespace PostgrestTests +{ + [TestClass] + public class LinqWhereTests + { + private const string BaseUrl = "http://localhost:54321/rest/v1"; + + [TestMethod("Linq: Where")] + public async Task TestLinqWhere() + { + var client = new Client(BaseUrl); + + // Test boolean equality + var query1 = await client.Table() + .Where(x => x.Id == "ea07bd86-a507-4c68-9545-b848bfe74c90") + .Get(); + + Assert.IsTrue(query1.Models.Count == 1); + + // Test string.contains + var query2 = await client.Table() + .Where(x => x.Name!.Contains("Gun")) + .Get(); + + Assert.IsTrue(query2.Models.Count == 2); + + // Test multiple conditions + var query3 = await client.Table() + .Where(x => x.Name!.Contains("Gun") && x.CreatedAt <= new DateTimeOffset(new DateTime(2022, 8, 23))) + .Get(); + + Assert.IsTrue(query3.Models.Count == 1); + + // Test null value checking + var query4 = await client.Table() + .Where(x => x.StringValue != null) + .Get(); + + foreach (var q in query4.Models) + Assert.IsNotNull(q.StringValue); + + // Test Collection Contains + var query5 = await client.Table() + .Where(x => x.ListOfStrings!.Contains("set")) + .Get(); + + foreach (var q in query5.Models) + Assert.IsTrue(q.ListOfStrings!.Contains("set")); + + var query6 = await client.Table() + .Where(x => x.ListOfFloats!.Contains(10)) + .Get(); + + foreach (var q in query6.Models) + Assert.IsTrue(q.ListOfFloats!.Contains(10)); + + var query7 = await client.Table() + .Filter(x => x.DateTimeValue!, Operator.NotEqual, null) + .Get(); + + foreach (var q in query7.Models) + Assert.IsNotNull(q.DateTimeValue); + + //Testing where condition with Enum as constant + var query8 = await client.Table() + .Where(x => x.Status == MovieStatus.OnDisplay) + .Get(); + foreach (var q in query8.Models) + Assert.IsTrue(q.Status == MovieStatus.OnDisplay); + + //Test where condition with Enum as Memeber expression + var testMovie = new Movie { Status = MovieStatus.OnDisplay }; + var query9 = await client.Table() + .Where(x => x.Status == testMovie.Status) + .Get(); + foreach (var q in query9.Models) + Assert.IsTrue(q.Status == MovieStatus.OnDisplay); + + await client.Table() + .Where(x => x.DateTimeValue == DateTime.Now) + .Get(); + + await client.Table() + .Where(x => x.DateTimeValue == null) + .Get(); + + await client.Table() + .Where(x => x.DateTimeValue == null) + .Set(x => x.BooleanValue!, true) + .Update(); + + await client.Table() + .Where(x => x.DateTimeValue == null) + .Set(x => x.BooleanValue, true) + .Update(); + + await client.Table() + .Where(x => x.DateTimeValue == null) + .Set(x => x.StringValue!, null) + .Update(); + } + + [TestMethod("Linq: Where with a null-check on an absent delegate applies no filter (supabase-csharp#192)")] + public void GivenNullDelegate_ShouldApplyNoFilter() + { + var client = new Client(BaseUrl); + var requestModel = new UserRequestModel(); + var table = client.Table().Where(x => requestModel.FilterPredicate == null || requestModel.FilterPredicate(x)); + Assert.AreEqual($"{BaseUrl}/users", table.GenerateUrl()); + } + + [TestMethod("Linq: Where with a null-check on a present delegate throws a descriptive exception (supabase-csharp#192)")] + public void GivenNonNullDelegate_ShouldThrowArgumentException() + { + var client = new Client(BaseUrl); + var requestModel = new UserRequestModel { FilterPredicate = u => u.Username == "supabot" }; + var exception = Assert.ThrowsException(() => client.Table().Where(x => requestModel.FilterPredicate == null || requestModel.FilterPredicate(x))); + StringAssert.Contains(exception.Message, "Unable to translate expression"); + } + + [TestMethod("Linq: Where with an always-false predicate throws a descriptive exception (supabase-csharp#192)")] + public void GivenAlwaysFalsePredicate_ShouldThrowArgumentException() + { + var client = new Client(BaseUrl); + var requestModel = new UserRequestModel(); + var exception = Assert.ThrowsException(() => client.Table().Where(x => requestModel.FilterPredicate != null && requestModel.FilterPredicate(x))); + StringAssert.Contains(exception.Message, "always evaluates to false"); + } + + [TestMethod("Linq: Where translates a nested null-check to `is.null` (supabase-csharp#192)")] + public void GivenNullCheckInsideOrPredicate_WhereTranslateToIsNullFilter() + { + var client = new Client(BaseUrl); + var table = client.Table().Where(x => x.Catchphrase == null || x.Catchphrase == "fat cat"); + var urlEncodedIsNullFilter = "or=(catchphrase.is.null%2ccatchphrase.eq.fat+cat)"; + Assert.AreEqual($"{BaseUrl}/users?{urlEncodedIsNullFilter}", table.GenerateUrl()); + } + + private class UserRequestModel + { + public Func? FilterPredicate { get; set; } + } + } +}