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

InMemory database does not throw client-side evaluation errors #20288

Closed
isen-ng opened this issue Mar 15, 2020 · 9 comments
Closed

InMemory database does not throw client-side evaluation errors #20288

isen-ng opened this issue Mar 15, 2020 · 9 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@isen-ng
Copy link

isen-ng commented Mar 15, 2020

When using InMemory databases, the exceptions that we see for client-side evaluations are not thrown in EFCore 3.1 (I did not test 3.0 so I'm not sure if this is present in 3.0).

Many developers use InMemory databases for their integration tests, and because these exceptions are not thrown, the developers may get a false sense of security that their code works. However, once we plug in an actual database, the code might fail with client-side evaluation errors.

See https://github.com/BlaiseD/LogicBuilder.DataComponents/blob/master/LogicBuilder.Kendo.ExpressionExtensions.IntegrationTests/RepositoryTests.cs
These tests runs fine against an InMemory database.

However, once I plug in an actual database, https://github.com/isen-ng/LogicBuilder.DataComponents/blob/failing-test/LogicBuilder.Kendo.ExpressionExtensions.IntegrationTests/RepositoryTests.cs#L89
the tests begin to fail.

(You will need docker to be installed to run the tests with an actual database)

Steps to reproduce

  1. Run the tests: https://github.com/BlaiseD/LogicBuilder.DataComponents/blob/master/LogicBuilder.Kendo.ExpressionExtensions.IntegrationTests/RepositoryTests.cs

  2. Run the tests:
    https://github.com/isen-ng/LogicBuilder.DataComponents/blob/failing-test/LogicBuilder.Kendo.ExpressionExtensions.IntegrationTests/RepositoryTests.cs#L89

Further technical details

EF Core version: 3.1
Database provider: Npgsql
Target framework: NetCore 3.1
Operating system: osx
IDE: Rider 2019

@smitpatel
Copy link
Contributor

Any database provider would throw client side evaluation error when they would encounter something which requires client side evaluation. InMemory database can evaluate almost everything on server side. So InMemory is not doing client side evaluation hence does not throw exception. This would be same for any pair of providers. e.g. Sqlite database cannot do many of computation on decimal. If you test your app which is using decimal comparison using SqlServer, it will work but if you run it against Sqlite, it will throw client side evaluation warning. If you want to have same client eval error in your test/prod then you need to use same database provider at both places.

@isen-ng
Copy link
Author

isen-ng commented Mar 15, 2020

What you're saying isn't technically wrong but is beside my point.

Despite all these technicalities, there are still going to be many libraries out there that will use InMemory database to perform their integration tests because it's the simplest way, and it's the way that is depicted in many MS blog posts or user created content on the internet.

Unfortunately, this will give them a false sense of security as InMemory do not even mimic such high level features like client-side evaluation errors that all actual databases should face. To me, the client-side evaluation error should be one that affects all databases, in memory or not, to promote best practises of not writing linq queries that will fetch the entire database, isn't it?

At the very least, InMemory databases should have options to allow it to be configured to mimic actual databases in terms of high level features of EFCore.

@smitpatel
Copy link
Contributor

You are requesting InMemory to mimic actual database but which database would you choose?

  • Should InMemory throw client eval errors when SqlServer database fails to evaluate something on server?
  • Should InMemory match with Sqlite database which cannot handle decimal column comparisons well?
  • Should it match with MySql or npgsql?

There is no single abstraction of what a provider cannot translate. It is intentionally left to individual provider. Just because something cannot be translated in Sqlite, does not meal SqlServer should also block it when it can just work. That is penalize users of SqlServer who has nothing to do with Sqlite with no reason other than testing with InMemory for some customers. What can be translated can be abstracted out. What cannot be is not an abstraction.

@AndriySvyryd
Copy link
Member

Related: #18457

@isen-ng
Copy link
Author

isen-ng commented Mar 16, 2020

I would definitely choose

  • Should InMemory throw client eval errors when SqlServer database fails to evaluate something on server?

This should be outright obvious. The reason is that, client eval errors are not database specific. It is an error that affects all databases except InMemory. Don't you find that gap weird and concerning?

Client eval errors are a feature of EFCore, not a specific database provider, whereas the other 2 points are database specific features that has nothing to do with EFCore's features.

Edit: See the first paragraph of the related issue #18457

While the in-memory provider is marketed as "for testing" I think that is somewhat misleading.

@smitpatel
Copy link
Contributor

The reason is that, client eval errors are not database specific.

That is wrong. Client eval is error is the error which is raised when particular C# code cannot be converted to equivalent to execute on server. I have given example of decimal columns already. Run them on Sqlite/SqlServer providers and see for yourself how they behave differently where SqlServer works fine but Sqlite throws. The premise of this issue itself is incorrect. If you want to take a step further then Cosmos provider does not support joins either which all relational providers do. So for a fact, client eval errors are database specific.

@isen-ng
Copy link
Author

isen-ng commented Mar 16, 2020

If that's the case, wouldn't providing libraries that extend/work on linq queries for databases be near impossible? There is no way for the library maintainers to test against all database providers, or do you think that they should convert their library to a database provider specific library?

Shouldn't there be a common set of high level client eval errors that all databases should throw? A simple example is if you have this where(column.ToString() == "Value"), this should have been an obvious throw for all actual/non-test database providers, isn't it?

@isen-ng
Copy link
Author

isen-ng commented Mar 16, 2020 via email

@isen-ng isen-ng closed this as completed Mar 16, 2020
@smitpatel
Copy link
Contributor

If that's the case, wouldn't providing libraries that extend/work on linq queries for databases be near impossible? There is no way for the library maintainers to test against all database providers, or do you think that they should convert their library to a database provider specific library?

Undoubtedly it is and it will remain that way no matter what behavior of InMemory and client eval is. Each database provider is distinct in features they support and as a library provider, if you want to be cross-database, you need test on each of them. That is the reason EF Core runs tests against each provider.

Query is just one facet. I suggest you read #7166 which is about different database capabilities in terms of modelling. Which ran into same issue and we decided to have each provider with own modeling features. Most of our customers uses only 1 database in production. And most of the library writers who extends EF has not argued otherwise. So with most value to our customers in mind, you can call it philosophical difference between us.

I think the inmemory database is inadequate to be used in integration tests

Exactly the reason I pushed hard for #18457 so that people stop using wrong tool. You can read whole thread there where even though there are differences people are acknowledging that and still want to use it. In this case, even though I agree with you, our customers are not.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Mar 31, 2020
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants