-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Interceptors for SQL generation #19748
Comments
@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 dotnet/EntityFramework.Docs#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? |
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. |
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. |
@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. |
@smitpatel I would start with the ability to preprocess/transform the 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 #19930 (comment). 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 |
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. |
@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 So what we want is an easy way to plug something similar to your current |
@powermetal63 - You can already replace QueryTranslationPreprocessor by using ReplaceService API on |
@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 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. |
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 |
If you are using SqlServer or Sqlite provider, then derive from |
@smitpatel What if I'm using Npgsql? Oracle? What if someday SqlServer provider decides to use something like And doing all that in order to add a single line similar to
at the beginning? You have to be kidding. Doing it publicly and easily is the essential part of/reason for OP request :
|
It is public and as easy as calling replacing service with factory and adding your custom code. |
@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. |
@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
to what you are suggesting here: create TWO additional custom classes (factory + processor), in order to call 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, 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
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. |
@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.
|
@roji We want SIMILAR for 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 Anyway, if you don't understand why your current "solution" doesn't work or isn't proper, there is nothing we can do. |
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. |
@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. |
I disagree with @roji and @smitpatel on this. As I said on Feb 3:
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. |
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
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. .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 Please do the right thing for once and listen to the community. |
I've implemented this "Query Translation Preprocessor" thing as an EF Core Extension for NeinLinq: Any feedback would be appreciated. |
Note: #28505 to allow interception of the LINQ expression tree has been split off from this and implemented in EF7. |
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? |
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.
I understand replacing the service with my own factory, but how do I actually derive from
I could also be misunderstanding what this is for. We used |
@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. |
My goal is the latter. I see a few services I could potentially use, like 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. |
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. |
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.
The text was updated successfully, but these errors were encountered: