Added a new filter expression for multiple search on strings #205

PhyberApex · 2018-10-08T11:10:17Z

Added tests for the new expression

joukevandermaas · 2018-10-09T12:25:56Z

Thanks! I will review and hopefully give some pointers/ideas tomorrow morning, but it looks quite good at first glance.

joukevandermaas · 2018-10-10T06:21:18Z

After looking through the code again, I think the changes need to be one level lower for this to work for all types. Already when parsing the query parameters we assume there is one value. This will fail in the multi-filter scenario for everything except strings (because most things cannot contain a comma).

Even at that spot (or for clarity, this spot), we should assume there can be more than one filter value. So the values are always a List or IEnumerable or something.

Then in the filter interpreter instead of effectively creating an expression like:

collection.Where(item => item == value);

we should create one like:

collection.Where(item => item == value1 || item == value2 ... || item == valueN);

Note that since we don't know the types at compile time, we use reflection to build these expressions. Also note that the == in the examples above is not actually there in the code, instead that's whatever expression is being returned by the FilterExpression registered. The default is just == but people can create their own.

I realize some of this is quite complex (especially building the expressions), so please let me know if you get stuck on anything. I'll be happy to help out with specific parts, either by writing code or by helping you through it. In any case thank you for your work on this so far 😄

PhyberApex · 2018-10-12T13:32:40Z

So I did some tinkering around and am now at a point where I don't really know how to go from here. I think my experience with C# starts to 'shine'. I never worked with reflection. Any chance you could give me some hint how to fix this one? I am stuck here

~Cheers

joukevandermaas · 2018-10-16T07:48:46Z

@PhyberApex sorry for the late reply. The ultimate goal of this piece of code is to generate an Expression that looks like this: thing.filterProp == filterValue. The exact expression comes from the user though, they can register them on the QueryFilterExpressionCollection (as you know). So we need to somehow make it easy to write these things.

The secondary goal then is to create expressions for thing.filterProp and filterValue so we can pass them to the filter expression lambda registered by the user. As you know, they can create a lambda like (prop, filter) => prop == filter. However, when we actually run the Where query, that thing takes a lambda like (val) => val == constant. Basically, we need to convert the former to the latter. This is done by the method called Convert on the Lambda class.

As you can see, that method is generic, because all the Expression apis are generic. This kind of sucks for us, because we don't know the types at compile time, only at runtime. This is why we need to use reflection.

The line you linked essentially does the following:

Lambda.Convert<type>(expression, parsedValues, propertyExpression, param);

However since type in this case is an instance of Type and not an actual generic type parameter, you cannot just 'call' that line, but have to create it.

First we get the reflection Type instance of the class we're in (Lambda) using typeof. Then we look through all methods on the class (again, this is the class we're actually in) to find our static, private method called Convert. Now this MethodInfo represents the method, but like we said it's a generic method. This means we need to provide the type parameter before we can call it (MakeGenericMethod).

After doing all that we can call Invoke on the method info, setting the instance to null (because Lambda is a static class).

Hopefully (if the above was clear), you can see that the expression variable in the method you linked, only filters on one value. It needs to be changed to do the || chaining, for each filter. The rest of the code can probably stay the same.

PhyberApex · 2018-10-16T16:49:42Z

Just to make sure here. With

The rest of the code can probably stay the same.

You mean like it was before I started or how it is right now? Because currently I can't build the solution and I can't really figure out what to do to fix that. So I am not sure if you said that what I did so far is in the wrong direction or if I am going the right way. Because the splitting of the actual query parameter has to happen somewhere.

~Cheers
Edit:
Also don't worry about the late reply we all got stuff to do :)

joukevandermaas · 2018-10-17T05:57:40Z

I realize this line "everything else can stay the same" is super confusing 😄 sorry about that. What I meant was the code as it is in that specific method can stay the same, as you have it. The build fails because it says new[] { ... } but the types of the objects within { } are different. So changing that to new object[] { ... } will fix the build*. Basically those array elements map to the parameters of the Convert method below.

Down lower at Convert, the second parameter is now of type TProperty but this must become List<TProperty> to match what it is passed. The curriedBody here is the actual expression prop == constant, where constant is the value from the filter. So we must create a curriedBody for each of the values instead of just the one, then chain them together using Expression.OrElse. The result is a single expression, which we must pass where curriedBody went before.

I definitely think you're on the right track and I think it's all very close to working. This part of the code is certainly not the most straightforward. Thanks for doing this work!

* (There's also some build errors in the test project because you changed Value to Values. I guess if possible we should have both properties, so we don't break backwards compatibility. But this is really not that urgent and I wouldn't worry about it too much for the moment)

PhyberApex · 2018-10-17T06:15:56Z

Thank you for helping me understand what I am actually doing here and always being so responsive!

I will take a look at this soon!

~Cheers

Also added expression chaining

PhyberApex · 2018-10-21T22:22:16Z

Sorry for not trying again sooner, but I finally got around to give this another try this evening.

With your help I got somewhat further with this. I changed the parameter to object[] and added the expression chaining. It does compile now but I get a runtime error on the reflection part while running the tests that a list of object could not be cast to int/string. My guess would be that as the return value of TryConvert is now a list of objects instead of a single one and a conversion is not as easy now. I tried to get it done with some the .ofType<T> method on the object array but I guess that's not working as we do not know the type at compile timeso my guess would be this would probably require reflection again? I'm not too sure tbh. I hope you can shed some light into this for me.

PS:
I did take a look at the backwards compatibility tho and fixed that because I got frustrated with the first part :)

~Cheers

PhyberApex · 2018-10-22T10:47:45Z

So apparently it was just way to late yesterday for me and I got this working now. There are other errors now in the chaining of the expression. Will take a look at this soonish.

~Cheers

joukevandermaas · 2018-10-22T11:41:07Z

Saule/Queries/Lambda.cs

            MemberExpression propertyExpression,
            ParameterExpression parameter)
        {
-            var curriedBody = new FilterLambdaVisitor<TProperty>(propertyExpression, constant).Visit(expression.Body);
+            // initialize the expression with a always true one to make chaining possible
+            Expression curriedBody = Expression.Lambda(Expression.Constant(true));


should be false? (true || a == b || a == c || a == d is always true)

You are right my bad!

joukevandermaas · 2018-10-22T11:44:18Z

Saule/Queries/Lambda.cs

@@ -50,15 +51,27 @@ internal static List<object> TryConvert(List<string> values, Type type)
            return parsedList;
        }

+        internal static object TryConvert(string value, Type type)


I think we need to call this for each filter, so instead of

var parsedValues = TryConvert(values, valueType);

(line 17)

var parsedValues = values.Select(v => TryConvert(v, valueType)).ToList();

I had two TryConvert methods. One which took an array and one that only took a single one. I have those reduced to one now and added the Select function at the appropriate places.

… into feature/csvfilter

PhyberApex · 2018-10-22T12:41:15Z

The github outtage is given me quite a hard time atm. Sorry for comment spamming!

I did add the changes like you suggested. Will take a look at the other stuff soon.

~Cheers

PhyberApex · 2018-10-24T09:56:51Z

I think this is it. Please let me know if you think there need to be any additional tests or changes. Also if you could let me know how to run that code style check within Visual Studio that would help me out not having to commit to wait for AppVeyor to tell me whats wrong :)

~Cheers

PhyberApex · 2018-10-24T13:39:48Z

As I tried to get the nested filter working for the other PR I discovered an error in my CSV regex if the value had whitespace. I added a unit test for this as well.

~Cheers

joukevandermaas

Looks really great! Thanks very much for putting so much time and effort into this.

joukevandermaas · 2018-10-25T06:29:31Z

Saule/Queries/Filtering/FilterProperty.cs

+            // Spliting the string into multiple values with csv notation
+            Values = new List<string>();
+
+            var match = new Regex("(?:,\"|^\")(\"\"|[\\w\\W]*?)(?=\",|\"$)|(?:,(?!\")|^(?!\"))([^,]*?)(?=$|,)").Matches(values);


Can you maybe put a comment that explains this regex? Some time in the future we may want to remember how it works.

I tried my best to explain this one. I hope that it's enough.

joukevandermaas · 2018-10-25T06:33:25Z

Tests/Queries/FilterInterpreterTests.cs

+            var people = Get.People(100).ToList().AsQueryable();
+            var expected = people.Where(c => c.LastName == "Russel" || c.LastName == "Comma,Test").ToList();
+
+            var query = GetQuery(new[] { "LastName" }, new[] { "Russel,\"Comma,Test\"" });


Maybe we can also see what happens on some edge cases/malformed strings such as "a"b (no comma), "ab,c (no end quote), etc.

FWIW i'm not sure what should happen in this case.. I would be fine with a 400 although technically that would be a breaking change.

Right now this is failing silently as anything not captured will be ignored. It's certainly possible to add a 400. I am not sure if this would be the best way tho.

I would vote that either

We throw an error for this (400 bad request)

We identify the filter as "malformed" and interpret it as a single filter value

I don't think we should let parts drop, throw 500, etc.

I think you're right that option 2 is probably better.

I added a few checks and one more unit test for this.

Added explanation to the regex Added checks to ensure well formed csv

joukevandermaas

Great!! I'm really happy with this result. Thanks again for all the effort. I know this was a complicated one! 💪

PhyberApex · 2018-10-25T11:42:49Z

Btw. do you want me to help you with the documentation of the PRs I did as well? I discovered that you are using the github pages to host the official website for saule and the source files for that are in the doc folder. So if you are okay with that I'd more than happy to do so.

~Cheers

joukevandermaas · 2018-10-25T12:14:56Z

@PhyberApex yes, if you have time that would be great. For this PR I don't know if we really need much documentation, since this is basically what the spec describes.

Added a new filter expression

69cb72f

Added tests for the new expression

joukevandermaas self-requested a review October 10, 2018 05:46

PhyberApex and others added 2 commits October 12, 2018 15:25

Switched from Expression to a more generic approach

0f66882

Merge branch 'master' into feature/csvfilter

e087116

Added backwards campatibility

79f90f0

Also added expression chaining

Janis Walliser and others added 2 commits October 22, 2018 09:50

Added generating of array via reflection

dad182f

Uncommented test

218b159

joukevandermaas reviewed Oct 22, 2018

View reviewed changes

Janis Walliser added 2 commits October 22, 2018 14:11

Added suggested changes

45cb954

Merge branch 'feature/csvfilter' of https://github.com/PhyberApex/saule…

e41cbbe

… into feature/csvfilter

PhyberApex added 2 commits October 24, 2018 11:52

Fixed expression chaining and multiple int test

f743a13

Fixed code style

cbbadca

Fixed regex for CSV matching

58d57d0

joukevandermaas reviewed Oct 25, 2018

View reviewed changes

Added unit test for quotes in text

66dbc4c

Added explanation to the regex Added checks to ensure well formed csv

joukevandermaas approved these changes Oct 25, 2018

View reviewed changes

joukevandermaas merged commit a6564bf into joukevandermaas:master Oct 25, 2018

PhyberApex deleted the feature/csvfilter branch October 25, 2018 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a new filter expression for multiple search on strings #205

Added a new filter expression for multiple search on strings #205

PhyberApex commented Oct 8, 2018

joukevandermaas commented Oct 9, 2018

joukevandermaas commented Oct 10, 2018

PhyberApex commented Oct 12, 2018

joukevandermaas commented Oct 16, 2018

PhyberApex commented Oct 16, 2018 •

edited

Loading

joukevandermaas commented Oct 17, 2018

PhyberApex commented Oct 17, 2018

PhyberApex commented Oct 21, 2018 •

edited

Loading

PhyberApex commented Oct 22, 2018

joukevandermaas Oct 22, 2018

PhyberApex Oct 22, 2018

joukevandermaas Oct 22, 2018

PhyberApex Oct 22, 2018

PhyberApex commented Oct 22, 2018

PhyberApex commented Oct 24, 2018 •

edited

Loading

PhyberApex commented Oct 24, 2018

joukevandermaas left a comment

joukevandermaas Oct 25, 2018

PhyberApex Oct 25, 2018

joukevandermaas Oct 25, 2018

PhyberApex Oct 25, 2018

joukevandermaas Oct 25, 2018

PhyberApex Oct 25, 2018

joukevandermaas left a comment

PhyberApex commented Oct 25, 2018

joukevandermaas commented Oct 25, 2018

Added a new filter expression for multiple search on strings #205

Added a new filter expression for multiple search on strings #205

Conversation

PhyberApex commented Oct 8, 2018

joukevandermaas commented Oct 9, 2018

joukevandermaas commented Oct 10, 2018

PhyberApex commented Oct 12, 2018

joukevandermaas commented Oct 16, 2018

PhyberApex commented Oct 16, 2018 • edited Loading

joukevandermaas commented Oct 17, 2018

PhyberApex commented Oct 17, 2018

PhyberApex commented Oct 21, 2018 • edited Loading

PhyberApex commented Oct 22, 2018

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

PhyberApex commented Oct 22, 2018

PhyberApex commented Oct 24, 2018 • edited Loading

PhyberApex commented Oct 24, 2018

joukevandermaas left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joukevandermaas left a comment

Choose a reason for hiding this comment

PhyberApex commented Oct 25, 2018

joukevandermaas commented Oct 25, 2018

PhyberApex commented Oct 16, 2018 •

edited

Loading

PhyberApex commented Oct 21, 2018 •

edited

Loading

PhyberApex commented Oct 24, 2018 •

edited

Loading