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

Query: Consider removing client code for all EF.Functions #20294

Closed
smitpatel opened this issue Mar 15, 2020 · 5 comments
Closed

Query: Consider removing client code for all EF.Functions #20294

smitpatel opened this issue Mar 15, 2020 · 5 comments
Assignees
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@smitpatel
Copy link
Member

The use of EF.Functions is about using a function on provider. If for any reason client code gets executed, is this a good behavior, or should we throw to warn user that their function is not going to evaluated on server.

@ajcvickers
Copy link
Member

This will prevent these methods from being used in LINQ-to-Objects queries, but we don't believe that is common, and it's not something we want to support going forward.

Where the implementation was being used for in-memory functionality we should move this to the in-memory provider so that it is not a breaking change for people using in-memory queries as opposed to LINQ to Objects.

@mc0re
Copy link

mc0re commented Apr 2, 2021

In-memory database was always meant for testing. How am I supposed to test the repository functionality now?

Say, I have a repository that looks up a BINARY (essentially a byte array) by a substring. MySQL, which I am targeting, has a nifty HEX function, that converts a BINARY to a string (like [0x01, 0x23, 0xAB, 0xCD] to "0123ABCD"). I want to find this row by both 23AB and 3AB search. Hex() method, provided by Pomelo, does the job very well. But now it throws in every test case - of which I have many. How can I test that the repository code works without InMemory support?

@roji
Copy link
Member

roji commented Apr 6, 2021

@mc0re we explicitly discourage using in-memory for testing your applications (see the docs). While it's possible for some functions to have a .NET implementation, this isn't possible in the general case. Some database functions simply cannot be duplicated on the client side (think full-text search), in other cases getting the exact database behavior without discrepancies is very tricky. And that's before we get into stuff like transactions or raw SQL.

We recommend either using the repository pattern, and then mocking that (in which case EF Core is no longer involved at all in the test), or testing against the same database you use in production - the latter will give you full fidelity testing, without the possibility of your code working against InMemory and then failing after pushing to production.

@mc0re
Copy link

mc0re commented Apr 6, 2021

@roji Thanks for answering.

Let's say, we have a repository, with one of the methods finding a user whose name contains a certain substring. Mind that no one would want to fetch all users into memory prior to filtering, so repository-service pattern does not fit here. This needs to be a pure repository method. Which means that when testing that repository, only DbContext is beneath - i.e. a database.

To avoid the need for setting up a real database on all involved development and CI machines as well as the runtime penalty in the unit tests, it's best to implement an in-memory store, replicating the basic filtering functionality (Where), which will then be used by the method in question. So I can't see how we're not heading towards the in-memory database.

If the provider implementers (like Pomelo) are kind enough to replicate the database-specific functions for an in-memory provider (why wouldn't full-text search work in memory?), it's all nice and working fine. Now with the removal of that possibility, and wanting it anyway, I'm forced to somehow do it myself. Duplicated work; error-prone, as I might not know all the database details; etc. Integration tests against the real database is something else, we're talking about unit tests, supposedly executed by Live Unit Test (my favourite tool) - so must be really fast.

So from my perspective, you've removed a very essential tool, forcing me to create make-shift implementations of it for every project. Yes, InMemory provider has its perks (like not checking the uniqueness of PK), but having a tool that works even half the time is definitely better than nothing. You can ask me how do I go about testing the other half - well, I'd use integration tests with a real database. But those are more difficult to set up, so the fewer the better.

From another side, if I may ask - what is the projected use case for the in-memory provider, a database that can't store data?

@roji
Copy link
Member

roji commented Apr 7, 2021

Let's say, we have a repository, with one of the methods finding a user whose name contains a certain substring. Mind that no one would want to fetch all users into memory prior to filtering, so repository-service pattern does not fit here. This needs to be a pure repository method. Which means that when testing that repository, only DbContext is beneath - i.e. a database.

If your goal is to test the (business) code that's using the repository, then you should probably consider mocking it - this would mean that your tests run without any DbContext or EF Core code. On the other hand, if your goal is to test your repository, then using InMemory again makes no sense, because you'd be testing your repository while using a fake "database" that's different from your production system; such a test wouldn't provide a lot of confidence that your repository actually work in production.

In other words, I'd try to clarify exactly what it is that you're looking to test; InMemory generally does not work for testing user applications which use EF Core, as mentioned in our docs.

To avoid the need for setting up a real database on all involved development and CI machines as well as the runtime penalty in the unit tests, it's best to implement an in-memory store, replicating the basic filtering functionality (Where), which will then be used by the method in question. So I can't see how we're not heading towards the in-memory database.

We get that argument a lot, but we still believe that testing against your real database is the way to go. Assuming you're on SQL Server, LocalDB provides an effortless database "setup", and if the tests are done right, they can run very quickly; EF Core itself tests a huge number of query scenarios against real databases, and the tests execute efficiently. In our experience, many times it's a matter of the test infrastructure being done correctly.

If the provider implementers (like Pomelo) are kind enough to replicate the database-specific functions for an in-memory provider (why wouldn't full-text search work in memory?), it's all nice and working fine.

Full-text search (unlike simple string searching) usually means that the search is aware of language morphology (e.g. searching for singular returns results for plural), and sometimes involves a specialized query language (see the SQL Server docs for an example). What you're asking for basically amounts to reimplementing the entire functionality in .NET - that simply doesn't work.

Integration tests against the real database is something else, we're talking about unit tests, supposedly executed by Live Unit Test (my favourite tool) - so must be really fast.

Again, I'd clarify first what it is you want to test. If you're looking to unit-test your business logic, use a mocked repository which avoids using any database or EF Core.

So from my perspective, you've removed a very essential tool, forcing me to create make-shift implementations of it for every project.

EF.Functions never contained comprehensive client-side implementations. Yes, it did contain an implementation for Like specifically (and maybe a couple other trivial functions), but that's it - in many/most cases, a .NET client implementation is simply unfeasible, as I wrote above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants