dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.6k stars 3.14k forks source link

Interceptors for SQL generation #19748

Open BalassaMarton opened 4 years ago

BalassaMarton commented 4 years ago

Note: #28505 to allow interception of the LINQ expression tree has been split off from this and implemented in EF7.

Use cases

The current query generation pipeline is hard to extend without relying on implementation details.

Proposal

Enable intercepting the translation steps in QueryCompilationContext.CreateQueryExecutor. API details are to be discussed here, I don't have a concrete proposal yet.

roji commented 4 years ago

@BalassaMarton the query pipeline is already extensible in many different ways, thanks to the DI infrastructure of EF Core:

Admittedly we currently lack documentation on how to do the above - we definitely plan on making all this more discoverable for 5.0 (see https://github.com/aspnet/EntityFramework.Docs/issues/681 for example, as well as other issues).

Are there any specific scenarios you have in mind, where you're unsure how to proceed? Is your interest more as a provider writer, or as an end user?

BalassaMarton commented 4 years ago

I'm only interested as a user, but I'll look deeper into the visitors and try to understand how it can be extended, thank you.

ajcvickers commented 4 years ago

Note from team discussion: we need to document this (see https://github.com/aspnet/EntityFramework.Docs/issues/951 and https://github.com/aspnet/EntityFramework.Docs/issues/500) but following that we should look at adding sugar for common patterns to avoid the need to understand expression visitors for common custom translations.

smitpatel commented 4 years ago

@BalassaMarton & @powermetal63 - Can you provide details of exactly what you want to change in expression tree for which you need extensibility? We can determine based on that what could be right hook for it and if it is missing, add it. Query pipeline is a complex system which has multiple parts end to end. As @roji mentioned above, there are hooks a long the way depending what kind of task need to be done.

powermetal63 commented 4 years ago

@smitpatel I would start with the ability to preprocess/transform the IQueryable.Expression before EF Core infrastructure. This will be very useful for lambda injection extensions like NeinLinq, LinqKit and similar which currently have to implement custom IQueryProvider and IQueryable just to do that. The end user is required to call a custom method like ToInjectable(), AsExpandanle(), which can easily be forgotten, and also custom query providers doesn't play well with EF Core queryable/async extensions.

Also it will be useful if we want to add "fixes" to some things you are not willing to (or have no time, or will do it in future version we cannot afford waiting for etc.), like the one here https://github.com/dotnet/efcore/issues/19930#issuecomment-587476008.

I guess all this falls into bullet #3 - Easy way to add query pipeline preprocessors. By easy I mean to be able to do that without implementing custom options in order to add services etc. Something as easy to use as the current ReplaceService.

smitpatel commented 4 years ago

Something as easy to use as the current ReplaceService.

That already works for everything you want to inject during compilation phase.

Anything before pre-funcletization can be done outside of EF before passing expression into EF.

powermetal63 commented 4 years ago

@smitpatel But we don't want it do be done externally. I already gave you several examples. We don't want to ask the user to call custom methods like ToInjectable, AsExpandable, ConvertGroupJoins etc. in order get the functionality. Because we are extending/fixing EF/C# query limitations, and want these pure query expression transformations to be executed automatically for each EF query "compilation".

So what we want is an easy way to plug something similar to your current QueryTranslationPreprocessor.Process - like GroupJoinConverter from https://github.com/dotnet/efcore/issues/19930#issuecomment-587476008, or LinqKit Expand functionality. Or in other words, something similar to AddInterceptors, but for query preprocessors which then run before EF.

smitpatel commented 4 years ago

@powermetal63 - You can already replace QueryTranslationPreprocessor by using ReplaceService API on IQueryTranslationPreprocessorFactory service. Did you look at that functionality?

powermetal63 commented 4 years ago

@powermetal63 - You can already replace QueryTranslationPreprocessor by using ReplaceService API on IQueryTranslationPreprocessorFactory service. Did you look at that functionality?

@smitpatel Sure I know it, but that's the problem - all I can do is to replace it. Let say I replace it and return custom QueryTranslationPreprocessor. Which one QueryTranslationPreprocessor should I inherit - QueryTranslationPreprocessor, RelationalQueryTranslationPreprocessor or something else - at that moment I have no idea which is the default (current) IQueryTranslationPreprocessorFactory, what dependencies it has and what type of QueryTranslationPreprocessor. it creates. And since EF relies on that query preprocessing, we can easily break it unintentionally.

Shortly, IMHO this factory service approach is quite limited - I'm not sure why you created it and what it will be used for internally by EF, but it definitely is not something I (or 3rd party extensions creators) want to dial with. We don't need to replace something existing, we need something additive like recently added interceptors.


P.S. Please don't get me wrong. I'm doing EF Core hacks/workarounds from the very beginning. Here we are discussing a good public way of extending EF Core. Cheers.

smitpatel commented 4 years ago

Here we are discussing a good public way of extending EF Core. Cheers.

Infrastructure to extend EF already exists as asked by OP. It is personal preference to use it or not. That has no affect on this issue whatsoever.

powermetal63 commented 4 years ago

Here we are discussing a good public way of extending EF Core. Cheers.

Infrastructure to extend EF already exists as asked by OP. It is personal preference to use it or not. That has no affect on this issue whatsoever.

Where is the existing infrastructure you are talking about and how we as end users can use it? For instance, where and how I as end user can safely plug my GroupJoinConverter ?

smitpatel commented 4 years ago

If you are using SqlServer or Sqlite provider, then derive from RelationalQueryTranslationPreprocessor add your custom processor. Create a new implementation of IQueryTranslationPreprocessorFactory which during create method creates the new derived class you generated and use Replace service method to register your custom factory in EF Core provider and it works.

powermetal63 commented 4 years ago

@smitpatel What if I'm using Npgsql? Oracle? What if someday SqlServer provider decides to use something like SqlServerQueryTranslationPreprocessor? Should I check every provider implementation and create derived factory/processor classes from theirs?

And doing all that in order to add a single line similar to

query = new GroupJoinFlatteningExpressionVisitor().Visit(query);

at the beginning? You have to be kidding.

Doing it publicly and easily is the essential part of/reason for OP request :

The current query generation pipeline is hard to extend without relying on implementation details.

smitpatel commented 4 years ago

It is public and as easy as calling replacing service with factory and adding your custom code. If you want to dislike it, that is your choice.

roji commented 4 years ago

@powermetal63 you may be lacking some information (which is understandable as we unfortunately don't have much docs on this). As @smitpatel wrote, RelationalQueryTranslationProcessor is a public service which can be replaced, adding another visitor in a way that isn't provider-specific. In other words, this would work regardless of which relational provider is being used. All you have to do is override NormalizeQueryableMethod and run you visitor before everything else.

Try giving it a try - and let us know if there are any blockers.

powermetal63 commented 4 years ago

@roji It might be public, but definitely NOT clean and NOT easy.

Again, all I need is to add a SINGLE line of code to let EF calls my single custom method before others.

Just compare the

optionsBuilder.AddInterceptors(new MyInterceptor());

to what you are suggesting here: create TWO additional custom classes (factory + processor), in order to call MyPreprocessor. The effort is pretty much the same as implementing custom query provider as many libraries currently do just to be able to preprocess the query expression before passing it to the original provider.

But even this is not the main problem. Apart from the fact that it's unclear which base class(es) to inherit with "replacements", the main problem is that this pattern is NOT additive. What if some library wants to add their preprocessor? If they take the path you are suggesting, then depending of the order of configuration calls either they will replace mine or vice versa. How about that?

Shortly, IQueryTranslationPreprocessorFactory / QueryTranslationPreprocessor can be used even currently for quick-and-dirty workarounds, but long term they do not provide the desired extensibility hook.

I'm ending the discussion here. I luckily don't use EF Core in my production software, so I have no "blockers" at all. All I wrote was as response to @smitpatel

Can you provide details of exactly what you want to change in expression tree for which you need extensibility?

I think I provided enough examples, including popular 3rd party packages showing the simplest extensibility hook needed to perform pure expression tree transformation. At this moment I have no opinion for other OP cases, but even if I do at some point, based on my experience with EF Core team discussions I don't think it worth my time arguing the obvious things.

roji commented 4 years ago

@powermetal63 yes, the existing solution does require you to write two additional trivial classes rather than a single line - for an advanced and niche scenario. As in #19930, we have almost no user requests for simplifying this further: people very rarely wish to integrate an expression tree visitor into the query pipeline. Although it may not fit with the specific idea you have of what you want, we consider the current solution clean and easy for the advanced scenario which it serves.

depending of the order of configuration calls either they will replace mine or vice versa. How about that?

~The same problem exists with your proposal of a simple AddInterceptors call.~ (NOTE: see below for clarification)

powermetal63 commented 4 years ago

The same problem exists with your proposal of a simple AddInterceptors call.

@roji AddInterceptors was given as an example. If you don't know (?!), is part of an existing new interception API in EF Core 3.0. Are you saying that if some plugin calls AddInterceptors with it's own interceptors, and then another one calls it with theirs, the second will replace the previous? I don't think so. It is an example of "proper" extension point for db operations.

We want SIMILAR for IQueryable.Expression, why it is so hard to understand?? Following your style, all you need is to define new trivial single method interface and add few trivial lines of code in a few places.

Your current "solution" does not serve any even simple practical purpose. And the usage scenarios I've shown you are NOT advanced, because they solve query expression tree limitations YOU are not willing to solve (or have no resources/time which is understandable). Votes here doesn't matter, all these libraries - NeinLinq, LinqKit, DelegateDecompiler, and many others suffer from the lack of such extension point and have to take the custom IQueryProvider / IQueryable path, requiring calling special methods for EACH query, and if you don't know (?), NOT playing well with EF Core queryable and async extensions. Their authors simply don't go here. Which is understandable seeing this style of communication (basically, EF Core team are telling us what is good for us) and is just waste of time and energy.

Anyway, if you don't understand why your current "solution" doesn't work or isn't proper, there is nothing we can do.

roji commented 4 years ago

FWIW my comment above was imprecise - I didn't mean to suggest that AddInterceptors doesn't allow for adding multiple interceptors.

But our current API also allows that. A user may simply replace IQueryTranslationPreprocessorFactory in their application, calling multiple visitors coming from multiple libraries in the order they see fit. As such, there's nothing functionally lacking in our current API - you're only asking for a terser way to do the same, i.e. syntactic sugar.

And once again, from our long experience maintaining this project, people have simply not been asking for what you're asking, so we haven't prioritized anything like this. I'd guess that people simply aren't bothered with adding ToInjectable and the like, and if they are, they still have the option of replacing IQueryTranslationPreprocessorFactory.

powermetal63 commented 4 years ago

people have simply not been asking for what you're asking

@roji If you look at the OP (which you even responded), you will see that I'm NOT the one who is asking for this. And your original reply to them was much more constructive than recently.

Anyway, I'm really taking off this and all EF Core discussions. As I said many times, I have absolutely no professional interest in using EF Core - I'm just your current FREE all time StackOverflow supporter, but that could easily be changed.

ajcvickers commented 4 years ago

I disagree with @roji and @smitpatel on this. As I said on Feb 3:

Note from team discussion: we need to document this (see dotnet/EntityFramework.Docs#951 and dotnet/EntityFramework.Docs#500) but following that we should look at adding sugar for common patterns to avoid the need to understand expression visitors for common custom translations.

I still believe that we should "look at adding sugar for common patterns to avoid the need to understand expression visitors for common custom translations" which was the conclusion we came to when discussing this with the team.

I'm not going to say any more about my thoughts now before discussing again with the whole team so that I fully understand the objections that are being raised by @smitpatel and @roji.

Amigolden commented 4 years ago

I agree with @powermetal63. He's provided multiple examples of why his approach would be better. It seems that @roji and @smitpatel's argument can be summed up as

  "We have and api that work and is easy"
  "people have simply not been asking for what you're asking"

However this is not the case.

When people have problems and Issues with the code there writing, They don't go and post a feature request on github. They ask a question on Stackoverflow. If they don't get an answer they want, they'll do what easier at the time, which often can lead to using different technologies.

.Net-core is in its early stages, and you've been given an opportunity, to make the publicly exposed interface extendible and easy to use. @powermetal63 has been leading the forefront in addressing issues relating to .net-core. He of all people has a way better understanding of what people have been asking for.

The current implementation is overly verbose, it forces users to remember call ToInjectable() which is conding based on convention and error prone.

Please do the right thing for once and listen to the community.

axelheer commented 3 years ago

I've implemented this "Query Translation Preprocessor" thing as an EF Core Extension for NeinLinq:

axelheer/nein-linq#24

Any feedback would be appreciated.

ajcvickers commented 2 years ago

Note: https://github.com/dotnet/efcore/issues/28505 to allow interception of the LINQ expression tree has been split off from this and implemented in EF7.

sensslen commented 1 year ago

I am currently trying to use the ReplaceService solution in order to register additional functions when evaluating expressions. This seems to work fine when using a real microsoft wql connection. However in our unit tests we use an in memory database. Whenever I try to resolve e.g. QueryTranslationPreprocessorDependencies this fails. Is there some sample code on how this would work?

MrZander commented 5 months ago

There is a lot of back-and-forth about the fact that this can be done, but not a whole lot of info on how to actually do it. Especially considering it is still undocumented. Can anyone shed some light on how I'm supposed to implement this?:

If you are using SqlServer or Sqlite provider, then derive from RelationalQueryTranslationPreprocessor add your custom processor. Create a new implementation of IQueryTranslationPreprocessorFactory which during create method creates the new derived class you generated and use Replace service method to register your custom factory in EF Core provider and it works.

I understand replacing the service with my own factory, but how do I actually derive from RelationalQueryTranslationPreprocessor?

    public class CustomQueryTranslationFactory : IQueryTranslationPreprocessorFactory
    {
        public QueryTranslationPreprocessor Create(QueryCompilationContext queryCompilationContext)
        {
            return new MyCustomPreprocessor(queryCompilationContext);
        }
    }

    public class MyCustomPreprocessor : RelationalQueryTranslationPreprocessor
    {
        public MyCustomPreprocessor(QueryCompilationContext queryCompilationContext)
            //What the heck are the first two arguments to this constructor supposed to be?
            : base(null, null, queryCompilationContext)
        {
        }

        public override Expression Process(Expression query)
        {
            return base.Process(query);
        }
    }

I could also be misunderstanding what this is for. We used IDbCommandTreeInterceptor extensively in EF6 and it was quite simple to transform the DbCommandTree before it was translated into SQL. Is this issue proposing this level of control or am I barking up the wrong tree?

roji commented 5 months ago

@MrZander if your goal is to perform some transformation on the incoming LINQ expression tree - that's the tree that represents your LINQ query before it's translated to SQL, then we've added an interceptor for that in EF Core 7.0 (docs). I'm mentioning that since that's what preprocessing means in this context. So it should no longer be necessary to extend RelationalQueryTranslationPreprocessor.

This issue is about intercepting at a much later point in query processing, after it has already been translated to SQL; so the interceptor here would accept an expression tree representing a (mostly) finalized representation of the SQL query, just before it gets serialized to a string.

MrZander commented 5 months ago

My goal is the latter.
Ideally, we would be able to intercept the SqlExpression as close to the end of the pipeline as possible.

I see a few services I could potentially use, like IMemberTranslator or ISqlExpressionFactory, but it isn't clear to me which will get me the "final" representation of the SqlExpression.

For some context, we are trying to swap out tables and add filtering at runtime in order to show entities at a certain point-in-time. The replacement tables are not part of the entity model, so I can't reference them in the LINQ query.

roji commented 5 months ago

Yeah, at this point you best bet is to use a command interceptor to do this at the SQL string level. It's definitely not great - but that's the recommended thing at the moment.

Otherwise, you would replace RelationalQueryTranslationPostprocessor (postprocessor, not preprocessor), and add an additional visitation step at the end that would go through the SQL tree and do its thing. This is not a trivial thing to do, but should be possible.