Disable automatic query client evaluation

[ajcvickers]

This is planned for 3.0 anyway--see #12795. However, by disabling now for the next preview, we can start getting feedback on the break to prepare for when the 3.0 query changes are in a state to be shipped.

[darl0026]

Why was the decision made to not allow us to turn this off with the below? I agree that it should be on by default to prevent poor performance, but now this means I have to extensively test my application to make sure some moron didnt write bad code somewhere. This means we will never upgrade to EF Core 3.0. Too much time involved to test. optionsBuilder .UseSqlServer("") .ConfigureWarnings(w => w.Ignore(RelationalEventId.QueryClientEvaluationWarning));

[smitpatel]

Allowing to turn it off, means EF Core would perform automatic client eval. That is additional code to write and maintain.

[darl0026]

There is no way that it is the much code to write and maintain. No way i believe that! Do you not understand the impact, we have 100s of web api endpoints that return data. Unfortunately many are missing tests, so there is no way we can go and test them. I cant possibly be the first person who pushed back and said we need a way to turn this off.

[smitpatel]

@darl0026 - You are the first one to ask a way to turn this off as evident by comments on this issue. We would consider a well-written PR with tests to add a way to client eval for all query operators in EF Core codebase if you would like to contribute.

[ajcvickers]

@smitpatel I may be missing something, but I don't think we would accept such a PR. It would be allowing people to opt-in to high risk of regressions in patches, which is one of main the reasons for making this change in the first place.

[darl0026]

We literally have 100s of linq queries, how are we supposed to mitigate the regression risk? In 2.2 for any greenfield projects, i explicitly configured this option to throw to prevent writing bad queries... but there is too much old code to test. Even if we didnt have 100s of queries, there is still significant regression risk. Like i said, i like the default on... but we have to be able to turn it off. You have provided ways to go back to old behavior for non-high risk breaking changes, why would you not do it for the high-risk breaking changes?

[roji]

@darl0026 on more point in addition to the above: we really do believe that client evaluation is dangerous - both because of potentially critical issues discovered only in production, and because of behavior changes as evaluation switches from client to server between versions. As such, providing an opt in for this problematic behavior would be doing a disservice to our users who would continue using it. And as @smitpatel wrote, supporting this feature and the users running into the issues it generates would cost us a lot of time and effort which are better focused elsewhere.

[darl0026]

I absolutely 100% agree that it needs to be in place, this is why I opted into this behavior in earlier versions any time I could, to prevent performance issues. But you have to acknowledge there is a HUGE risk for regression issues. You say that you are concerned about doing a disservice to your customers, by not giving them a way to opt out then it is a disservice. Can you please give me guidance on how to make sure the 100s of linq queries that I have wont throw an exception. You say that you are worried about time and effort, well so am I. I cant possibly go back and test 100s of endpoints that dont have tests (but should have). I am making a prediction, people are going to upgrade to 3.0 (and not read the breaking changes doc, I PROMISE this happens) and get burned and it will come back to bite you. When i upgraded to asp.net core 3.0 at least there were compile time changes that prevented me from going forward and forcing me to think. This is strictly a run-time thing and will never get caught until an exception occurs. I understand that a major version release indicates that there will most likely be breaking changes and you need to test, but this is a big one, it requires full regression testing and I dont have time for it.

[smitpatel]

@jjxtra - Please stop spamming unrelated issues with your group by query is not working.

[KieranDevvs]

@darl0026 Not to sound blunt but then, don't upgrade to 3.0 if you're not ready / don't have the time to do so. Its a major version upgrade so breaking changes are to be expected. We're still on 2.2 for this exact reason and I don't think we will be upgrading for a while yet.

Also consider automatic integration testing / end to end testing, that way, if this happens again, you don't have to spend lot of man hours testing for breaking changes, you'll already have a compiled list of all the functionality that is broken and that you need to fix.

To be honest, Id rather it happen now in 3.0 rather than in 9.0 and the code base is bigger, its just something you'll have to deal with, or use something that gives you more control over your queries i.e Dapper.

I advocate for the usage of CQRS to split out the database queries to use Dapper where I get fine control on the query, and EF Core on the commands which gives me benefits such as the change tracker and unit of work pattern.

[dougiejay]

As a dev manager with over 40 developers working for me, I have to agree with @darl0026 This is a MAJOR breaking change that could prevent our company from ever upgrading to core 3.0+ And now MSFT have designated that 2.2 is out of life and we have to upgrade to 3.1, but we cant due to EF upgrade! This is kind of insane guys.

At the very least can you provide a tool that could search our codebase in VS.NET as an addin or something to help us find the breaking code up front and re-write.

Or if you cant do that, how about providing some code sample to allow us to turn on logging in production so we log the call stack of failing methods so at least we can build up a picture of our code that needs changing up front? We could dump that into a table or azure table storage and then try to collate all the code that needs fixing up front.

Otherwise what you are telling us to do is to set 2.2 to throw exceptions not warnings, run some code, let a method fail, fix it, compile it, run it, go to next method to test? That could literally take weeks/months in a large code base, and unless all code is covered by automated tests (not unit tests due to mocking of course, as that wont help here). I estimate the cost to my company to do that would be between £20,000-£100,000 which means an impossibility for us financially.

So you guys REALLY have to do something to help us out. If we had known about the client side execution when we started on EF 2, of course we would have turned it on. But it is way too late for that now.

[ErikEJ]

@dougiejay assume you mean "turned it off" ? And how did you miss it in the first place? We're you coming from EF6?

[dougiejay]

@ErikEJ Not sure what you mean. We left EF 2 running in its default settings, we didnt make any changes, it ran out of the box in that mode of allowing client side evaluation from my understanding.

"How did we miss it in the first place?" - As in how did we let something run in it's default mode? Not sure I follow. Surely you put the more dangerous setting (client eval) as the non-default setting that requires a customer to actively opt-in to the situation, not the other way around?

And really, voting down a comment from a corporate client of Microsoft seems pretty petty and shows a disdain for users of the product that you promote and work on.

[roji]

@dougiejay EF Core versions prior to 3.0 emitted warnings to the log whenever client evaluation occurred; you could tell EF to throw instead - see the documentation section on client evaluation in previous versions. To configure logging, see this page.

However, note that in EF Core 3.0 the query pipeline was also rewritten, so what involved client evaluation in 2.2 may not necessarily throw an exception in 3.0. The best way forward here would be to have a test/preproduction environment running on EF Core 3.1 and to use that to identify problematic queries. Hopefully you also have a test infrastructure to make this process easier.

[dougiejay]

Again you guys are missing the point. Everyone is happy you have stopped client side evaluation. It was something that we would have avoided if we knew about this feature in 2.0 a year and a half ago (instead of being simply logged to VS. Net console). My point is that you have not provided any upgrade path or tools to assist migration. Yes we have over 600 automated api tests which will help with the migration process, however one api might have multiple db calls, and maybe several will fail due to the client eval, meaning to get one api call passing might take several builds taking significant time. When professional development is conducted by corporate clients like us, we expect a bit more than a serious breaking change and no assistance whatsoever from Microsoft to minimise cost of upgrading. If you don't understand my viewpoint then not much more to say and I will cease posting on this thread.

[dougiejay]

I absolutely 100% agree that it needs to be in place, this is why I opted into this behavior in earlier versions any time I could, to prevent performance issues. But you have to acknowledge there is a HUGE risk for regression issues. You say that you are concerned about doing a disservice to your customers, by not giving them a way to opt out then it is a disservice. Can you please give me guidance on how to make sure the 100s of linq queries that I have wont throw an exception. You say that you are worried about time and effort, well so am I. I cant possibly go back and test 100s of endpoints that dont have tests (but should have). I am making a prediction, people are going to upgrade to 3.0 (and not read the breaking changes doc, I PROMISE this happens) and get burned and it will come back to bite you. When i upgraded to asp.net core 3.0 at least there were compile time changes that prevented me from going forward and forcing me to think. This is strictly a run-time thing and will never get caught until an exception occurs. I understand that a major version release indicates that there will most likely be breaking changes and you need to test, but this is a big one, it requires full regression testing and I dont have time for it.

Spot on agree with @darl0026

[roji]

Again you guys are missing the point. [,,,] My point is that you have not provided any upgrade path or tools to assist migration.

I do understand your point, and also the frustration with the situation. This is just my opinion/view, but had it been feasible/possible to provide some sort of automated tool to assist migration, we'd definitely have done it (for example, an analyzer to flag queries which would fail). However, doing this kind of analysis at compile time is an enormous investment (it really is), and time was already very tight to the 3.0 release (which there was no option to postpone). At the end of the day, we could have either made the change without a migration tool, or not have made the change at all - that's just the reality we had to face when making that decision.

And really, voting down a comment from a corporate client of Microsoft seems pretty petty and shows a disdain for users of the product that you promote and work on.

FWIW the only downvote I'm seeing is from @ErikEJ who isn't a Microsoft employee (although he's an important member of the community). I don't think anyone in the EF team takes the client evaluation change lightly, or imagines that the transition would be trivial for customers; it's just that everyone agrees that dropping client evaluation was the right thing to do, and there was no feasible way to provide automated upgrade analysis etc.

[ajcvickers]

@darl0026 I'm very sorry to hear you are having trouble updating to EF Core 3.1. We certainly didn't take the decision lightly to overhaul the query code in the way we did for 3.0. However, after much discussion, we decided it was necessary for several reasons, some of them around robustness and user experience going forward, some of them internal relating to dependencies.

With a change of this size it was clear regressions in existing functionality would occur, even outside the intended breaking changes. We have a small team (five engineers and a manager) so mitigations for this needed to be realistic. We focused on:

• Getting all the 50,000+ tests in this repo working against the new code
• Clear documentation of breaking changes
• Weekly status updates indicating progress
• Many previews and daily build releases for people to test with and provide feedback

It was not realistic for us to create any kind of static analysis tools for this--we simply don't have the resources for that. It's also not clear to me how many cases could be reasonably picked up by static analysis alone.

In addition, the problem space for LINQ queries is extremely large, which means that it is very easy for patterns to be missed even with tests we run. This means, realistically, using a new release of EF Core (whether it is a breaking release or not) requires that the application test with that new release to identify any issues. This is part of the reason for making frequent preview releases and daily builds available--we hope to encourage and foster relationships with customers so as to tighten the feedback on finding and fixing issues.

This was an extreme case of a breaking release--we don't plan to make this kind of break routinely. However, we are committed to moving the product and experience forward in ways that may be breaking. We are certainly open if you have ideas for how we can further improve in these areas, within the constraints of the resources we have.

[jjxtra]

Thanks for the heads up @ajcvickers, appreciate you being transparent.

[IanKemp]

1. Breaking changes are bad when their resolution cannot be quantified.

As a developer, I understand that it's sometimes necessary to make breaking changes, and the LINQ execution change is one I heartily endorse for its potential for improved performance. As someone who has to communicate with stakeholders, I shudder at the thought of trying to sell an EF Core 3.0 upgrade to them. "First we have to end-to-end test the entire application to determine what breaks in EF Core 3.0, then we need to spend an unknown amount of time fixing and testing the broken stuff." How many stakeholders are going to give the go-ahead for that, especially when the upside is "better performance... maybe"?

IMO, the execution engine rewrite is a large enough change that it warranted a special release plan. Personally, I would have released EF Core 2.2.1 that had no change apart from upgrading the client-side execution warning to an error, which would have given developers adequate notice that their current code was problematic and should be changed. Then I would have released EF Core 3.0 with the new engine, and nobody would've had a reason to complain.

2. Breaking changes are bad when they aren't adequately communicated.

I believe that the big issue that @darl0026 and others, including myself, have is that the breaking nature of EF Core 3 vs older versions WRT query evaluation was not sufficiently communicated. The fact of the matter is that for any EF-using app of decent size, upgrading to EF Core 3 is probably going to cause breakage somewhere due to the new evaluation model, and that isn't called out particularly well.

The announcement for EF Core 3.0 is peppered with weasel words like "can" and "may" and "could", when realistically any reasonably-sized app is going to be affected by these changes. What was, and arguably still is, needed on that blog post is a preface consisting of a big block of text on a red background saying something like:

"Breaking changes: Because EF Core 3.0's internal expression compilation engine has been completely rewritten, existing queries may require modification to work with this version. We strongly recommend that you thoroughly test your application with EF Core 3.0 in order to identify affected queries, and use that as a guideline to determine if updating to this new version at this time is optimal for your software. Read on for details of the changes and potential modifications."

Yes, that blog post did mention and link to the breaking changes, but its emphasis on the "breaking" part was not there, nor was it front and foremost as it should have been.

I can understand why Microsoft and the EF team would be reluctant to potentially scare people away from their software, but the fact of the matter is that EF Core 3.0 is not just a drop-in upgrade, and any issues it potentially causes might only come to light days or months or years later. That's a massive business risk, and Microsoft needs to be responsible and disclose that up-front.

Finally, the fact that there are so many questions on Stack Overflow regarding this exact topic (https://stackoverflow.com/q/59677609/70345 being only the most recent) is a strong indication that the existing language around migrating to EF Core 3.0 is not strong or clear enough.

[ajcvickers]

@IanKemp Thanks for the feedback; we appreciate your thoughts and as with all feedback we will consider it as we strive to continually improve.

To address some of your specific points/suggestions:

First we have to end-to-end test the entire application to determine what breaks in EF Core 3.0

In my opinion, this is something that needs to be done when updating any dependency that contains as much complexity as an OR/M. At one extreme, we try to be conservative with patch releases to reduce the chance of regressions. However, patch releases do sometimes still contain regressions, so its always important to test when updating a dependency, even for just a patch. This becomes even more important when taking a new minor version that has a lot of bug fixes and new features, but no planned breaking changes. And then more so for new major versions. EF Core 3.0 was at the extreme of this continuum, but it's still true that I have worked with customers who were able to update without anything breaking.

Personally, I would have released EF Core 2.2.1 that had no change apart from upgrading the client-side execution warning to an error

This would have been a major breaking change in a patch release, which isn't really on the table as an option. Also, the query code was fundamentally changed in 3.0. Just because something was being client evaluated in 2.2 doesn't mean it is client evaluated in 3.0. The reverse is also true to a lesser extent. So it would have given a false sense of security for the 3.0 update.

The announcement for EF Core 3.0 is peppered with weasel words like "can" and "may" and "could", when realistically any reasonably-sized app is going to be affected by these changes.

It was certainly not our intention to try to hide the breaking nature of this release. In fact, we have been striving for the opposite--to make sure as many people as possible are aware. This was front of mind for us in every preview, announcement, and blog post throughout the 3.0 cycle. Your feedback that this was not clear is certainly something we will consider when we make breaking changes in the future.

but the fact of the matter is that EF Core 3.0 is not just a drop-in upgrade,

I would like to emphasize again that, in my opinion, no new release (other than possibly patches of simple code) should be considered a drop-in upgrade, regardless if that is an EF release or a release of something else containing considerable complexity.

[peter-bozovic]

Got into issues with this breaking change while upgrading our project to 3.1

Was able to fix most of the simpler queries with AsEnumerable(), but we have few places where the query is built dynamically.

For example the user can choose many search parameters and then we build the query with AsQueryable() and depending on choices Where() and GroupBy() are selectivelly added to the query before we finally call ToList() to fetch the data:

In this scenario we can't insert AsQueryable() in the middle of the query build chain as it's not IQueryable anymore ...

I was happy with EF that was able to determine the best way to manage those requests and return me results. At the moment I can't imagine how I can do this manually and will probably end up in even worse performances because of the complexity and number of tables my dynamic query needs to handle ...

Si I guess i'll have to stick with 2.2 until I find some solution or hopefuly client evaluations might come back as optional query paramater in some future EF version ...

[roji]

@peter-bozovic note that there aren't any plans to bring back client evaluation at the moment. While the upgrade from 2.x to 3.x is indeed painful because of this, we believe that EF Core shouldn't be transparently pulling back an entire table because something couldn't be translated in a Where clause.

If you want to dynamically add search parameters which would get evaluated on the client side, you can still do that by composing an IEnumerable expression tree and compiling that. But once again, you're the one who knows whether it makes sense for your users that you perform arbitrary queries in a potentially extremely-inefficient way (which is what 2.2 did for you).

[jjxtra]

Agreed, simply use ToArray/ToList and finish out the rest of your query after that.

[jgehlbach]

We were also stuck in upgrading to EF Core 3 due to this issue.

Is there any chance to get the QueryClientEvaluationWarnings out of a production system running 2.2 with a hint of where the problem occurs (e.g call stack)

We could then run 2 weeks in production, fix all problematic code and then upgrade to 3.x with minimized chance of breaking.

Maybe something supporting this kind of tracing can be implemented in a 2.2 patch if it's not possible out of the box. That would be great help for anyone migrating to 3.x

[MarkMcTernan]

@darl0026 - You are the first one to ask a way to turn this off as evident by comments on this issue. We would consider a well-written PR with tests to add a way to client eval for all query operators in EF Core codebase if you would like to contribute.

You must be kidding.

This has got to be the worst example of premature optimization I've ever encountered in my professional career.

You have broken thousands of advanced queries written by people that depended upon IQueryable's graceful degradation to IEnumerable up until 3.x. You can no longer use ternary operators on select projections, string concatenation, or complex select projections without intermediate stupidity like forcing evaluation & execution in the middle of the code. Hell, right now you can't even use functions like Contains or pass a predicate lambda via a method call to a where clause, so a client that expects to work on an IEnumerable (even if EF has already cached the result for Linq to Object manipulation) breaks when it gets an IQueryable expression that can no longer gracefully degrade to IEnumerable.

Another example: Did you create a code first model with unmapped, calculated values to make queries simpler and less mistake prone, knowing that such things would be evaluated on the client after the full set (that matched a server side predicate) was pulled down? No more, now you must either explicitly force an evaluation via AsEnumerable() (or otherwise separate those two calls), or push the calculation into the server where clause. Elegant, code first calculated properties are now less valuable in 3.x than they were in 2.2. Why? to prevent a "mistake" that the developer fully intended? This was a major advantage of EF, now gone.

So, let me summarize the upshot here: "We don't want junior developers to inadvertently write inefficient queries that could make EF look bad in some relatively rare circumstances which could readily be detected and improved once the performance degradation was noticed... so we are going to break backwards compatibility with the entire EF installed base and CORE PHILOSOPHY of IQueryable and break every application under the sun that uses advanced techniques that made EF worth adopting in the first place."

This sucks. It was EF's flexibility and forgiving nature of accepting any valid LINQ expression for both data retrieval & client side data manipulation as it seamlessly toggled between LINQ to EF and LINQ to Objects that made it the best ORM available on ANY platform. EF's capabilities were the sort of thing you'd dangle in front of Java developers who didn't even know what they were missing over beers. Now we get to return to the world where 3NF querying concerns must be thought of as separate and distinct from OOP client manipulations.

"We would consider a well-written PR with tests to add a way to client eval for all query operators in EF Core codebase if you would like to contribute."

This is nearly the same tone and attitude expressed when the dotnet core System.Data team decided to remove the interfaces for IDbConnection/IDBTransaction, in their initial pass because the EF Core team thought abstract classes would be good enough. (thereby violating a core principle of SOLID). Of course MS eventually reversed themselves as the comments by reputable ORM developers shredding that decision piled up, but not before making all sorts of similar unhelpful statements.

I know you're volunteers, but EF Core is an important project and platform for developers (especially commercial developers) everywhere, so let me put it this way, hopefully without being similarly dismissive: I get paid to code for a living and literally am pretty busy in life, so I don't have time to undo a devastating decision to break compatibility with every client written in EF Core up to this point and time."

Nah. I'll just be sure to drop EF from the stack next time and go back to Dapper. Dapper's reliable. I thought EF was too, but evidently that is not the case anymore.

[smitpatel]

Core philosophy of IQueryable is not to evaluate anything rather build an expression tree to allow the query provider to process it whatever way they want. So we haven't broken core philosophy of IQueryable in any form. You may want to reconsider your understanding of IQueryable vs IEnumerable.

[MarkMcTernan]

@darl0026 Not to sound blunt but then, don't upgrade to 3.0 if you're not ready / don't have the time to do so. Its a major version upgrade so breaking changes are to be expected. We're still on 2.2 for this exact reason and I don't think we will be upgrading for a while yet.

EOL for 2.2 was 1 month after your comment. I've just spent the past week (while supposedly on vacation) porting 2.2 to 3.1 because Azure removed 2.2 from their stacks without warning around last weekend and the first warning I got (other than vague... "Hey, we're not going to support 2.2 any longer after December" was 500 errors from the App Service that then forced us to move to 3.0 only to discover the joys of EF Core's inexplicable abandonment of graceful degradation to LINQ to objects.

[MarkMcTernan]

Core philosophy of IQueryable is not to evaluate anything rather build an expression tree to allow the query provider to process it whatever way they want. So we haven't broken core philosophy of IQueryable in any form. You may want to reconsider your understanding of IQueryable vs IEnumerable.

1) That's a quite limited interpretation. Did someone in the dotnet core project empower the EF Core team to redefine IQueryable to NOT support graceful IEnumerable fallback because you guys thought it was problematic for EF? Because let me tell you, this decision, which flies in the face of EF from the very beginning, is very much problematic for EF. How does this breaking change not also violate the Liskoff substitution principle?

2) I thought the purpose of EF Core was to build a superior ORM that provided efficiency via IQueryProvider based translation to any given ADO.Net backstore via LINQ, while simultaneously offering the performance and network efficiency of client side cached objects that could be manipulated via LINQ to Objects before being round tripped back to the datastore with minimal fuss. While there are some use cases (very large data sets with well distributed results), that were less optimal for EF, this usage pattern was highly optimal for the typical LOB app, since most apps aren't FB or Stack Overflow. You guys have chosen to optimize for an edge case that most people would have reached for another tool for, such as Dapper.

3) You don't work for me nor do I pay you, but I am most definitely a customer of MS's offerings and platform. You are losing when you are arguing with a customer and tell them that they don't understand a product they have relied upon for years just because you have decided to make that product less useful than it was.

[dougiejay]

@MarkMcTernan I 100% agree with you. see my comments from Dec 2019. we STILL havent had capacity to move to 3.1x, over 8 months later. That is the reality of the breaking change in the real world for a large enterprise organisation.

[dougiejay]

@MarkMcTernan where have you had Azure removing 2.2? that terrifies me as we are still in production on 2.2.

[MarkMcTernan]

If you look on the documentation for Azure's AppService, you'll see that 2.2 is not among the advertised frameworks.

On June 19th, AzureDevOps removed 2.2 from their docker build agents on Mac, Linux, and Windows. This was announced here on June 1st: https://github.com/actions/virtual-environments/issues/975 [It strikers me as profoundly inadequate that AzureDevOps and Azure announce critical stuff with a Github post it note like this when I get missile launch code level communications every time the office 365 team wants to toggle a hosting option.]

For me, the DevOps build agents broke first (I run xunit as part of the CD pipeline), but after fixing that I discovered it also applied to the App Service docker images themselves, because a successful deploy started throwing 500 errors for the same 'missing 2.2 framework' issue. The MS support rep confirmed for me that build agents and app services apparently rely upon the same cached Docker images.

That was only a few days ago. I assumed it would be ok so long as I didn't run a deployment to my prod environment while I worked on the upgrade. That lasted a few days before my prod* App Service, which I never deployed to, started throwing 500 errors for the same reason, so apparently Azure attempted to spool up a running instance on a new container even though I had the "always on" feature set to true.

*A caveat: My customer's "prod" is a dev/test B2 instance (B2 supports SSL, always on, and other 'prod' features). Maybe they won't touch the prod tier App Services, but I wouldn't bet on that after my experience.

[jjxtra]

I understand the frustration with the breaking changes here. I would have very much preferred a configuration property on the context options to opt into this new behavior, with the default being 2.2 behavior. I hope this is an option for .net 5. I also understand the maintenance burden of having two code paths or implementations so I don’t take this kind of request lightly.