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

Include functionality improvements #27

Closed
fiseni opened this issue Jun 30, 2020 · 3 comments
Closed

Include functionality improvements #27

fiseni opened this issue Jun 30, 2020 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@fiseni
Copy link
Collaborator

fiseni commented Jun 30, 2020

One of contributors did great job with "ThenInclude". We might improve this a bit. In the current implementation, ultimately everything is still converted to include strings. Let's keep the additional expressions and evaluate properly in the spec evaluators, utilize "ThenBy" of EF.
I still didn't think through of the implementation, but since the user already is typing the expression within "ThenInclude", then we might keep the same to utilize it in the evaluator.

@fiseni
Copy link
Collaborator Author

fiseni commented Jul 2, 2020

Let's first examine what are our current limitations, and what we trying to achieve here.
This is how we use the Include for now.

    public class BlogWithPostsAndAuthorSpec : BaseSpecification<Blog>
    {
        public BlogWithPostsAndAuthorSpec(int id) : base(b => b.Id == id)
        {
            AddIncludes(x => x.Include(b => b.Posts)
                              .ThenInclude(p => p.Author)
            );
        }
    }

This is great, but we have the following issues:

  • We can't use ThenInclude more than once. It's not chainable, so we can include 2nd level only.
  • The implementation ultimately converts the expression into string. We're passing the string to EF Include methods, and not the expressions.
  • From the usage perspective, the expressions are set within the AddIncludes method, which is a bit confusing.

The improved version looks as following:

    public class BlogWithPostsAndAuthorSpec : BaseSpecification<Blog>
    {
        public BlogWithPostsAndAuthorSpec(int id) : base(b => b.Id == id)
        {
            // We can chain as much as we want
            Query.Include(b => b.Posts)
                 .ThenInclude(p => p.Author)
                 .ThenInclude(a => a.Posts)
                 .ThenInclude(p => p.Author);

            // We can start over with Include for another navigation
            Query.Include(a => a.Posts)
                 .ThenInclude(p => p.Author);

            // We can chain it with other methods
            Query.Where(b => b.Name == "Something")
                 .Include(b => b.Posts)
                 .ThenInclude(p => p.Author);
        }
    }

The existing Include infrastructure won't be used, we won't need the aggregator and the Visitor pattern at all.

@ardalis ardalis added this to the 4.0 milestone Jul 13, 2020
@ardalis ardalis added enhancement New feature or request help wanted Extra attention is needed labels Jul 13, 2020
@fiseni
Copy link
Collaborator Author

fiseni commented Jul 17, 2020

The usage now is as following (we can chain several levels if we want):

    public class CompanyWithStoresThenIncludeProductsSpec : Specification<Company>
    {
        public CompanyWithStoresThenIncludeProductsSpec(int id)
        {
            Query.Where(x => x.Id == id)
                 .Include(x => x.Stores)
                     .ThenInclude(x => x.Products);
        }
    }

Notice
The usage will be as we planned, and it has same syntax as the EF. But, I decided all those expressions to be parsed to string and stored as string, then in evaluator utilize navigationPropertyPath of EF include. This is done based on the following reasons:

  • I was doing some tests, and was surprised to find that passing string (navigation path) to EF Include, actually works faster. (probably while evaluating the expression there are a lot of checks etc).
  • If we want to keep the inputs as expressions in the specifications, the actual problem is storing them. For example, we can't use Expression<Func<object, object>>, the delegate argument shouldn't be object. Thus, we have to store Types too, and then in the evaluator we should reconstruct the whole expressions, and call the EF Include through reflection. I did this, and honestly it creates unnecessary complexity in the solution. It defies the purpose.

@fiseni
Copy link
Collaborator Author

fiseni commented Aug 9, 2020

This is implemented in version 4. I'm closing the issue. If anyone has additional comments or suggestions, please feel free to reply or re-open the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants