Efcore: DbFunctions: Consider allowing instance methods

[anpete]

Currently, only static methods are permitted. Allowing instance methods is interesting because it can allow the method to contain a working in-memory impl. instead of it being a stub or throwing.

[pmiddleton]

Currently if you provide a working implementation it will be used as a fallback when the method call can't be translated and EF tries to execute it client side.

[bricelam]

+1. I want to write this...

db.Posts.Where(p => db.PostReadCount(p.Id) > 5);

...not this.

db.Posts.Where(p => BloggingContext.PostReadCount(p.Id) > 5);

[pmiddleton]

This is the next thing on my list to look into.

[bricelam]

Sweet. No rush; I was just adding my feedback. I'm writing a blog post about you and used the feature for the first time. 😉

[pmiddleton]

Cool - where will the blog post be so I can check it out?

[bricelam]

It'll be on my blog sometime this week. Do you Twitter? If so, I'll mention you there when I post it.

[pmiddleton]

I don't Tweet.

[bricelam]

It's out: SQLite & EF Core: UDF all the things!

[pmiddleton]

Awesome post.

Is Sqllite sql case sensitive? If not then you don't need the name on your [DbFunction] for greet.

[bricelam]

It's not. I know. 😄 I just wanted to be explicit about how the two APIs pair up.

[divega]

+1: nice post @bricelam!

@anpete, after reading @pmiddleton's comment:

Currently if you provide a working implementation it will be used as a fallback when the method call can't be translated and EF tries to execute it client side.

I am wondering what this issues is exactly about.

Perhaps the point is that the function might always need to be computed in the database, so in order to use it in memory (in a query or even outside of a query) you need to "bootstrap" it with the DbContext?

Going back to @bricelam's example, we could make both of these things work (I know it seems a bit contrived):

// call outside of query
foreach(var p in posts)
{
  if (db.PostReadCount(p.Id) > 5)
  {
    // do something
  }
}

// call in query
foreach(var p in posts.Where(p => db.PostReadCount(p.Id) > 5))
{
  // do something
}

Since in the body of the method we have access to "this" DbContext, we can use it to execute a query in which the method is invoked:

public int PostReadCount(int postId)
{
  return Posts.Where(p => p.Id == postId).Select(p => db.PostReadCount(p.Id));  
}

Or perhaps something like this some day:

public int PostReadCount(int postId)
{
  return Execute(db => db.PostReadCount(postId));  
}

We actually had this kind of capability in EF6, although it wasn't at all easy to setup, and it was useful for mapping regular functions as well as TVFs.

Note that it was also safer in EF6 to invoke the method through the LINQ provider as part of the implementation of the method, because there was no automatic client evaluation. In EF Core we should probably have a way to protect against re-entrant calls.

If this is the reason it is more compelling to enable instance methods, I think we need to reflect it better in the description of the issue. If it isn't then I would like to understand better.

Otherwise, when I hear enabling mapping instance methods to functions, I think that what I would really want to have is support for this:

db.Posts.Where(p => p.ReadCount() > 5);

I tried to describe this in https://github.com/aspnet/EntityFrameworkCore/issues/9393.

[pmiddleton]

I think the biggest win for this would be the simpler syntax when writing your queries. Less typing is always better.

[pmiddleton]

@anpete - I just ran into something interesting while implementing this.

ParameterExtractingExpressionVisitor is extracting the DBContext 'this' parameter when it walks the query. That parameter is stored in the QueryContext and then promptly ignored by the backend sql generation because it isn't needed.

The only side effect I have found thus far is that the sql parameter names are one off of what you would expect them to be. For example @__startDate_1 instead of @__startDate_1.

Do you think we should update ParameterExtractingExpressionVisitor to avoid extracting the DBContext parameter, or should we let things run as they are now? I can't see any use for the DBContext parameter on the backend, but there might be one at some point? Also trying to change ParameterExtractingExpressionVisitor might be problematic. I'm not sure which way to go on this one.

[pmiddleton]

Should we support instance methods defined on types other than DbContext? See my comment in pr #9755

I don't see any advantage to adding this extra complexity. You are going to have a reference to a DbContext while writing your query so instance methods there make sense - you get to save some typing.

Why would you want to instantiate another object to make a function call in your query. Especially since you can't use any actual state from that other object.

I think the fluent api should just disallow instance methods on classes other than DbContext.

[ajcvickers]

Note from triage: Since using the context inside client eval of the function is blocked by #7375, and this is in turn blocked by #8864, @anpete is going to investigate #8864 again to see whether we can do something for 2.1. However, #8864 looks quite hard, so we will cycle back again after the investigation and decide how to proceed.

[smitpatel]

Partially fixed via 0edbe21da611c535eab455cbac16db107f2bfe9d in #9755

[anpete]

@smitpatel Why partially? What is remaining here?

[smitpatel]

Entity based instance methods are tracked at #9811