Added a new filter expression for multiple search on strings

[PhyberApex]

Added tests for the new expression

[joukevandermaas]

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

[joukevandermaas]

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 :smile:

[PhyberApex]

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]

@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]

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]

I realize this line "everything else can stay the same" is super confusing :smile: 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]

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

[PhyberApex]

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]

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

[PhyberApex]

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]

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]

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

[PhyberApex]

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]

@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.