Linq Expression Feature #27

alexis- · 2016-09-14T14:37:14Z

Added Expression<Func<T, bool>> ToLinqExpression() Method which converts a FilterScheme instance to a LINQ Expression.
Added a simple test to compare ToLinqExpression with ComputeResult results. More tests should be added to make sure all operators are operational.
FilterScheme.Apply uses ToLinqExpression.
TODO: Add Regex and Enum (StringExpression) handling in ToLinqExpression

…hod which converts a FilterScheme instance to a LINQ Expression. * Added a simple test to compare ToLinqExpression with ComputeResult results. More tests should be added to make sure all operators are operational. * **FilterScheme.Apply** uses **ToLinqExpression**. * TODO: Add Regex and Enum (**StringExpression**) handling in **ToLinqExpression**

alexis- · 2016-09-14T14:42:04Z

I went with reverting develop to upstream commit, creating new branches, and adding changes step-by-step.
I kept a local-develop branch of my previous version, and a hard backup.

Regarding the PR, does this work for you ?
Other PR will follow soon.

Regarding the UI, I appreciate your concern. It probably won't come as a surprise to you, but it was built for use in my own application. No work is wasted, apart from the reverting-branching overhead.

GeertvanHorrik · 2016-09-14T17:48:53Z

@michu could you please give a quick review here?

GeertvanHorrik · 2016-09-15T15:23:38Z

@mkhomutov , please also do a review of this.

mkhomutov · 2016-09-15T18:24:54Z

OK will do it tomorrow

mkhomutov

code looks really good. I've added few comments which would improve.

mkhomutov · 2016-09-16T18:55:06Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

+                case Condition.NotIsEmpty:
+                    return Expression.NotEqual(propertyExpr, Expression.Constant(string.Empty));
+                case Condition.IsNull:
+                    return Expression.Equal(propertyExpr, Expression.Constant(null));


please, replace to Expression.ReferenceEqual()

this is only for null checking

mkhomutov · 2016-09-16T18:55:53Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

+                case Condition.IsNull:
+                    return Expression.Equal(propertyExpr, Expression.Constant(null));
+                case Condition.NotIsNull:
+                    return Expression.NotEqual(propertyExpr, Expression.Constant(null));


please, replace to Expression.ReferenceNotEqual()

only for null checking

mkhomutov · 2016-09-16T18:57:26Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

+                    return Expression.NotEqual(propertyExpr, valueExpr);
+
+                case Condition.Contains:
+                    operatorExpr = Expression.Call(propertyExpr, typeof(string).GetMethod("Contains", new[] { typeof(string) }), valueExpr);


Instead of GetMethod() better to use GetMethodEx() extension method everywhere.
it uses cache under the hood, so works a bit faster

mkhomutov · 2016-09-16T19:00:29Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

+                    break;
+
+                case Condition.DoesNotStartWith:
+                    operatorExpr = Expression.IsFalse(Expression.Call(propertyExpr, typeof(string).GetMethod("StartsWith", new[] { typeof(string), typeof(StringComparison) }), valueExpr, Expression.Constant(StringComparison.CurrentCultureIgnoreCase)));


use constants instead of such strings as "StartsWith" or "Contains"

mkhomutov · 2016-09-16T19:01:17Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Extensions/FilterSchemeExtensions.cs

                filteredCollection.Add(item);
-            }
+
+#if false


not sure why is it for

mkhomutov · 2016-09-16T19:07:53Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Conditions/ConditionTreeItem.cs

+        /// </summary>
+        /// <param name="parameterExpr">LINQ ParameterExpression.</param>
+        /// <returns>LINQ Expression.</returns>
+        public abstract Expression ToLinqExpression(Expression parameterExpr);


I would probably combine all ToLinqExpresison() methods and move it into the Extensions class or some "LinqExpressionConverter"
not sure, but it looks and sounds like extension and looks like it is not really needed to put all this logic in separate classes

mkhomutov · 2016-09-16T19:13:26Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

+        /// </summary>
+        /// <param name="propertyExpr">LINQ <see cref="MemberExpression"/>.</param>
+        /// <returns>LINQ Expression.</returns>
+        public override Expression ToLinqExpression(Expression propertyExpr)


please use Argument.IsNotNull(() => propertyExpr)
everywhere for null checking the arguments

alexis- · 2016-09-17T09:27:44Z

Excellent, I appreciate the explanation bits you joined in your code review.
I will look into this matter soon.

alexis- · 2016-09-26T08:42:19Z

All done -- at the exception of the extension class.
I agree it would be best to move the logic in their own file.
Perhaps I will do it at some point if required by future changes, but I have to dedicate my time to other tasks for the moment.

Let me know if this works for you.
Cheers,
Alex.

GeertvanHorrik · 2016-09-26T08:41:55Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

@@ -134,9 +141,9 @@ public override Expression ToLinqExpression(Expression propertyExpr)
                case Condition.NotIsEmpty:
                    return Expression.NotEqual(propertyExpr, Expression.Constant(string.Empty));
                case Condition.IsNull:
-                    return Expression.Equal(propertyExpr, Expression.Constant(null));
+                    return Expression.ReferenceEqual(propertyExpr, Expression.Constant(null));


Have you tested this code? Not sure if calling Expression.Constant(null) returns the same actual reference every time. This is just an observation, I am not entirely sure about this one.

I've added a test to make sure. This is the way Microsoft suggest using null constants (see https://msdn.microsoft.com/en-us/library/bb349084(v=vs.110).aspx )

GeertvanHorrik · 2016-09-26T08:42:31Z

src/Orc.FilterBuilder/Orc.FilterBuilder.Shared/Expressions/StringExpression.cs

@@ -198,16 +205,16 @@ public override Expression ToLinqExpression(Expression propertyExpr)
                                   .IsMatch(entityValue);
                                   */
                case Condition.GreaterThan:
-                    return Expression.GreaterThan(Expression.Call(Expression.Constant(null, typeof(string)), typeof(string).GetMethod("Compare", comparisonMethodParams), propertyExpr, valueExpr, Expression.Constant(StringComparison.CurrentCultureIgnoreCase)), Expression.Constant(0, typeof(int)));
+                    return Expression.GreaterThan(Expression.Call(Expression.Constant(null, typeof(string)), typeof(string).GetMethodEx("Compare", comparisonMethodParams), propertyExpr, valueExpr, Expression.Constant(StringComparison.CurrentCultureIgnoreCase)), Expression.Constant(0, typeof(int)));


Also move these "Compare" to CompareMethodName

alexis- · 2016-09-26T10:09:08Z

A quick reminder, RegEx and Enum aren't yet supported in this PR.

GeertvanHorrik · 2016-10-03T14:52:20Z

@mkhomutov if you agree with this PR, please merge & release.

lock · 2019-08-27T01:08:45Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mkhomutov reviewed Sep 16, 2016

View reviewed changes

* PR refactoring

7944d26

GeertvanHorrik requested changes Sep 26, 2016

View reviewed changes

Alexis added 2 commits September 26, 2016 12:01

* PR fixes

3f89218

* Fixed commented null string test

24f6fbb

GeertvanHorrik approved these changes Oct 3, 2016

View reviewed changes

mkhomutov merged commit e68f832 into WildGums:develop Oct 3, 2016

GeertvanHorrik mentioned this pull request Oct 4, 2016

Latest Orc.FilterBuilder does not respect runtime metadata #32

Closed

mkhomutov mentioned this pull request Oct 4, 2016

Revert "Linq Expression Feature" #33

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linq Expression Feature #27

Linq Expression Feature #27

alexis- commented Sep 14, 2016

alexis- commented Sep 14, 2016 •

edited

Loading

GeertvanHorrik commented Sep 14, 2016

GeertvanHorrik commented Sep 15, 2016

mkhomutov commented Sep 15, 2016

mkhomutov left a comment

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016

alexis- commented Sep 17, 2016

alexis- commented Sep 26, 2016

GeertvanHorrik Sep 26, 2016

alexis- Sep 26, 2016

GeertvanHorrik Sep 26, 2016

alexis- Sep 26, 2016

alexis- commented Sep 26, 2016

GeertvanHorrik commented Oct 3, 2016

lock bot commented Aug 27, 2019

Linq Expression Feature #27

Linq Expression Feature #27

Conversation

alexis- commented Sep 14, 2016

alexis- commented Sep 14, 2016 • edited Loading

GeertvanHorrik commented Sep 14, 2016

GeertvanHorrik commented Sep 15, 2016

mkhomutov commented Sep 15, 2016

mkhomutov left a comment

Choose a reason for hiding this comment

mkhomutov Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhomutov Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhomutov Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

mkhomutov Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhomutov Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexis- commented Sep 17, 2016

alexis- commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexis- commented Sep 26, 2016

GeertvanHorrik commented Oct 3, 2016

lock bot commented Aug 27, 2019

alexis- commented Sep 14, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading

mkhomutov Sep 16, 2016 •

edited

Loading