Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IQueryable.OfType support to ExpressionParser. #248

Closed
dsidedp opened this issue Feb 17, 2019 · 17 comments
Closed

Add IQueryable.OfType support to ExpressionParser. #248

dsidedp opened this issue Feb 17, 2019 · 17 comments

Comments

@dsidedp
Copy link

dsidedp commented Feb 17, 2019

EF Core allows to use OfType to filter derived entities from base collections

It would be great to have this supported as part of Where clause, e.g. :
ctx.Set<Base>.Where("OfType("\Derived\").Any(DerivedProperty> 10)")

@StefH
Copy link
Collaborator

StefH commented Feb 19, 2019

Can't you just use:

var oftype = context.Base.OfType<Derived>().Where("DerivedProperty > 10");

@dsidedp
Copy link
Author

dsidedp commented Feb 19, 2019

I can do that. But i cant do this:
context.Where("BaseTypeCollectionProperty.OfType<DerivedType>().Any(DerivedProperty==10)")

@StefH
Copy link
Collaborator

StefH commented Feb 19, 2019

I see, I'll take a look to implement this.

@StefH
Copy link
Collaborator

StefH commented Feb 19, 2019

Try latest version from MyGet 1.0.10-ci-1952
https://www.myget.org/feed/system-linq-dynamic-core/package/nuget/System.Linq.Dynamic.Core/1.0.10-ci-1952

Use it like:

var config = new ParsingConfig
{
    AllowNewToEvaluateAnyType = true
};

context.ComplexDtos
    .Select(config, "ListOfBaseDtos.OfType(\"ConsoleAppEF2.Database.TestDto\").Where(Name == \"t\")");

@StefH
Copy link
Collaborator

StefH commented Feb 21, 2019

@dsidedp Can you please do some tests?

@dsidedp
Copy link
Author

dsidedp commented Feb 21, 2019

I did some with 1.0.11-ci-1955 on production model I have.

From all tests i ran, only one failed - when I tried to use OfType with context keywords.
Example: ctx.Base.Where("OfType("Derived").Any()")

Besides of single issue, I can think about couple of other things related to this:

  1. In EF models with inheritance, alongside OfType support for collection navigations, it will be great to have support for reference navigation as well.
    Example: ctx.Set.Where("BaseClassNavigation.Cast(DerivedClass).DerivedProperty == 1")

  2. [nice to have] Allow use type name instead of full name in OfType/Cast operators, and try to infer it. Even simple attempt to find a derived type in base type namespace might be useful.

And one more - Thanks a lot for fast response to feature request!

@StefH
Copy link
Collaborator

StefH commented Feb 21, 2019

  1. I'll check if I understand and can build this ...

  2. I understand your idea, I had the same issue when testing my code. However this functionality should be provided with a new config settings like "ResolveTypesBySimpleName" / "ResolveTypesByShortName"

  3. ctx.Base.Where("OfType("Derived").Any()") --> The normal linq would be : context.BaseDtos.Where(x => x is Derived) ???

@dsidedp
Copy link
Author

dsidedp commented Feb 21, 2019

Reference Navigaion

As an example of valid linq query with reference navigation:
ctx.Base.Where(x=>((Derived)x).DerivedProperty==1)

Basically, by doing this linq is adding ConvertExpression(x, DerivedType) as input for PropertyExpression.
Also, if you can test x for implementation of IEnumerable, it can be possible to reuse same operators OfType/Cast.

ResolveTypesBySimpleName

Having the ability to opt out from internal type resolver using config switch looks good to me. As another option (or extension), you can do something similar to what you have with CustomTypeProvider.
Using something like ICustomTypeNameResolver with default implementation will add additional flexibility.

OfType

Normal linq for ctx.Base.Where("OfType("Derived").Any()") would be ctx.Base.OfType<Derived>().Any().
ctx.Base.Where(x=>x is Derived).Any() will work a bit differently but will produce same result.

Another challenge here would be queries with like
ctx.Base.Where(OfType(Derived).Where(BaseProperty==2).Any() && Base.BaseProperty == 1)
Considering possibility of virtual properties, this expression is not convertible to linq (with single call to ctx.Base).

So it seems to me that allow mixing like this should be denied, and if OfType allied to "it" - there should be no "and"s and "or"s on the same level.

Here are couple of examples:
allowed Where(OfType().Where(condition).FirstOrDefault(condition) =! null)
not allowed: Where(OfType().Where(condition).FirstOrDefault(condition) =! null && SomeProperty == 1)

@StefH
Copy link
Collaborator

StefH commented Feb 22, 2019

ResolveTypesBySimpleName : I've created a new issue for this #252 and will create a new PR for this.

OfType : to get ctx.Base.Where("OfType("Derived").Any()") working I think I need to add an OfType function. I'll investigate and if it's easy to build, I'll do it. Else I create a new issue/PR.

@StefH
Copy link
Collaborator

StefH commented Feb 22, 2019

OfType

I tried to implement this:

bool isOfType = context.BaseDtos.Any(b => b is TestDto);
bool isOfTypeDynamic = context.BaseDtos.Any(config, "OfType(\"ConsoleAppEF2.Database.TestDto\")");

However the dynamic code does not work because I don't know the real type from the BaseDto, at the moment when the dynamic expression is evaluated, the Type is BaseDto.

See also this code:
https://github.com/StefH/System.Linq.Dynamic.Core/pull/253/files

@dsidedp
Copy link
Author

dsidedp commented Feb 22, 2019

Ok, I see.

How about "Is" operator?

In this case "it.OfType" can be replaced with:
ctx.BaseType.Where(Is(DerivedType)).Any() (as you already mentioned)

And with support of reference cast (cast object to derived type):
ctx.BaseType.Where(Is(DerivedType) && Cast(Derived).DerivedProp == 1).Any()

Considering what i was saying earlier regarding restrictions of OfType applied to "it", having "Is" operator seems to be good replacement.

@StefH
Copy link
Collaborator

StefH commented Feb 22, 2019

I did send you invite for gitter chat; it's easier to talk.

@dsidedp
Copy link
Author

dsidedp commented Feb 22, 2019

I didin't use gitter before, so just got registered there. You will probably need to send invite again.

Also, I looked at commented out code again, and it seems to be OK.
I will probably will need to run locally to get better understanding about the problem, but not sure how soon I can find time to do that.

@StefH
Copy link
Collaborator

StefH commented Feb 27, 2019

@dsidedp I think all is resolved now? Can I merge to master?

@dsidedp
Copy link
Author

dsidedp commented Feb 27, 2019

Looks good to me.

@StefH
Copy link
Collaborator

StefH commented Feb 28, 2019

OK, I'll merge to master and create a new NuGet

new version will be communicated here in this issue...

@StefH StefH closed this as completed Feb 28, 2019
@StefH
Copy link
Collaborator

StefH commented Feb 28, 2019

Official NuGet 1.0.11 is uploaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants