efcore

efcore copied to clipboard

Similar to PR #19617, we can enable more decimal operations by leveraging UDFs. Unlike TimeSpan, however, there isn't an acceptable type we can convert to to perform the operations, so it will require roughly one UDF per operation. For example:

Done	.NET	SQL
✔	m1 + m2	ef_add($m1, $m2)
✔	m1 / m2	ef_divide($m1, $m2)
✔	m1 > m2	ef_compare($m1, $m2) > 0
✔	m1 >= m2	ef_compare($m1, $m2) >= 0
✔	m1 < m2	ef_compare($m1, $m2) < 0
✔	m1 <= m2	ef_compare($m1, $m2) <= 0
✔	m1 % m2	ef_mod($m1, $m2)
✔	m1 * m2	ef_multiply($m1, $m2)
✔	m1 - m2	ef_add($m1, ef_negate($m2))
✔	-m	ef_negate($m)
	Average(t => t.Decimal)	ef_avg(t.Decimal)
	Max(t => t.Decimal)	ef_max(t.Decimal)
	Min(t => t.Decimal)	ef_min(t.Decimal)
	Sum(t => t.Decimal)	ef_sum(t.Decimal)
	OrderBy(t => t.Decimal)	ORDER BY t.Decimal COLLATE EF_DECIMAL
	OrderByDescending(t => t.Decimal)	ORDER BY t.Decimal COLLATE EF_DECIMAL DESC

~~I don’t think there’s much we can do for OrderBy...~~

Hi, first time here looking to contribute to OSS projects. Being tagged as a good first issue, maybe I can give it a shot?

@lokalmatador Sure! You might want to start small on this one (e.g. just do ef_compare and it’s operators) in your first PR.

Great! I guess I start out by looking at your changes in the above mentioned PR (PR#19617) to get an idea on how to possibly achieve this. Also, I may ask one or another question the next days :)

Feel free to start a draft PR to ask questions about the implementation

(Added modulo as part of PR #20024)

First of, thanks for helping me in getting my first contribution done. I hope I wasn't too much of a burden. Would it be OK to continue on this task? I'd be happy to.

No burden at all! Please feel free to continue

Well, then I'd continue with ef_{add, divide, multiply, subtract} and ef_negate.

Draft PR is here

So, @JonPSmith got me thinking again about ORDER BY and I think we could actually do it via collation.

connection.CreateCollation(
    "EF_DECIMAL",
    (x, y) => decimal.Compare(decimal.Parse(x), decimal.Parse(y));

SELECT *
FROM MyTable
ORDER BY DecimalColumn COLLATE EF_DECIMAL

Wow, I wouldn't have thought of that!

I did DM @ErikEJ about this and it put me on to #18593 and I saw the Sqlite limitations page.

I use Sqlite in-memory as much as possible for unit testing. That's because a lot of people use 'standard' EF Core, i.e. don't use any of the SQL-specific parts, which means Sqlite in-memory works really well. I used Sqlite in-memory in my last client's project and they had an Azure CI/CD pipeline that includes unit tests and Sqlite in-memory works great!

So, of course, I am interested in differences between Sqlite and say SQL Server (a typical database people use). Other than string case sensitivity and filter/order on decimal, then Sqlite in-memory is a good fit for unit testing. I suspect I will go with a ValueConvertion to double if the database provider is Sqlite to overcome the decimal limitation.

PS. If you guys have any clever way to manage a real database in a CI/CD pipeline then I would love to know. Clearly I could call EnsureDeleted and then EnsureCreated in every unit test, but that is so slow!

@JonPSmith In my experience, it's only the creation and dropping of databases that is really slow. (Also, dropping a database destabilizes SQL Server, so we avoid ever doing it when running tests on the C.I.) We have a database cleaner that uses our reverse engineering code to clear the database instead of dropping and re-creating it. We haven't productized it, but we're considering it. Also, it looks like @jbogard's Respawn does something similar.

Hi @ajcvickers,

Yes, I have a cleaner too but I think yours might be better, but the real problem is migrations, i.e. the Model changes during development, which is going to happen say 2% (??) of the push to CI/CD pipeline.

I have a fix for handling changes to the Model that works well locally (and used it in real apps), i.e.

1. Create unique database names for unit tests, but with a given prefix.
2. If the Model changes the developer has to manually run a method that deletes all the databases with names with that prefix.

That works locally, but not for CI/CD. Here are two ideas I am thinking about.

1. At the start of each CI/CD I use my method to delete all the unit test databases before running any unit tests in the pipeline. That simple(ish), but it will be slow for the 98% times there is no Model change. Maybe speed in CI/CD isn't so important(?)
2. Add a test ahead of the 'delete all unit test databases' that checks if migrations are needed to a 'canary database'. If a migration was applied then it delete all the unit test databases. Quicker in 98%, but feels fragile.

Sorry, I'm unloading my ideas but I want to come up with something for an "article". If its off-topic then please ignore.

@JonPSmith The key thing about the cleaner is that it can't be based on the EF model otherwise, as you point out, when the model changes it may no longer match the database structure. That's why the EF one uses the reverse engineering code to figure out what is in the database and then drop it all. The process used in the EF C.I. is that every test database is created if it doesn't already exist, and then cleaned. The database is then never dropped.

@ajcvickers Wow - that is so cool! I want it :smile:

Obviously I hadn't looked at your cleaner properly before, but I have now. Its pretty complex so I want to reflect back what I think your cleaner does so you can correct me. I think it does the following:

1. If there is an existing database it
   1. Uses the reverse engineering code to read the schema
   2. The sql code in the cleaner then deletes all the indexes, foreign key (constraints?), tables, and sequences
   3. Then it runs some code given by the caller - for SQL Server this has code to delete other things like views, functions etc.
2. Then it creates the tables etc. defined by the current Model. I presume its uses the code in context.Database.EnsureCreated

Is that correct? If so that solves the C.I. problem, but actually makes the local unit testing much better too (no need to run a method when the Model changes). And I tried a few of your unit tests and the initial test is pretty quick too.

The next question is, can the SqlServerDatabaseCleaner be made available easily? The RelationalDatabaseCleaner doesn't access anything internal, but SqlServerDatabaseCleaner does.

Any comments?

This is exactly what my Respawn tool does, ha! https://github.com/jbogard/Respawn

On Thu, Apr 9, 2020 at 5:38 AM Jon P Smith [email protected] wrote:

@ajcvickers https://github.com/ajcvickers Wow - that is so cool! I want it 😄

Obviously I hadn't looked at your cleaner properly before, but I have now. Its pretty complex so I want to reflect back what I think your cleaner does so you can correct me. I think it does the following:

1. If there is an existing database it
   1. Uses the reverse engineering code to read the schema
   2. The sql code in the cleaner then deletes all the indexes, foreign key (constraints?), tables, and sequences
   3. Then it runs some code given by the caller - for SQL Server this has code to delete other things like views, functions etc.
2. Then it creates the tables etc. defined by the current Model. I presume its uses the code in context.Database.EnsureCreated

Is that correct? If so that solves the C.I. problem, but actually makes the local unit testing much better too (no need to run a method when the Model changes). And I tried a few of your unit tests and the initial test is pretty quick too.

The next question is, can the SqlServerDatabaseCleaner be made available easily? The RelationalDatabaseCleaner doesn't access anything internal, but SqlServerDatabaseCleaner does.

Any comments?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-611458660, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMW5R5UOHMQOZFEHPILRLWQR5ANCNFSM4KIVZ3HA .

Hi @jbogard,

I did look at Respawn. The EF Core SQL Server cleaner also deletes views, functions, procs, aggregates, types, schema, but I could copy the EF Core code for that. I notice Respawn is async, which means it can't be used in the unit test constructor, which is a pity. I have a look/think about this.

No for xUnit you need to use its async initialization stuff.

On Thu, Apr 9, 2020 at 8:33 AM Jon P Smith [email protected] wrote:

Hi @jbogard https://github.com/jbogard,

I did look at Respawn. The EF Core SQL Server cleaner also deletes views, functions, procs, aggregates, types, schema, but I could copy the EF Core code for that. I notice Respawn is async, which means it can't be used in the unit test constructor, which is a pity. I have a look/think about this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-611530272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMQV25XNH4VHORNBU4LRLXFB5ANCNFSM4KIVZ3HA .

OK, looking at that now

Hi @jbogard,

I made sure I looked at Respawn and it does a great job of removing all the data, but the EF Core cleaner deletes the actual tables in the database, i.e. Drop(MyTable). Dropping all the database tables, indexes, foreign keys , etc. is what I want, so that unit tests can handle changes in the EF Core Model when running unit tests.

Also @ajcvickers, looking at Respawn made me think - does your RelationalDatabaseCleaner handle circular table references? I assume that deleting the foreign keys first makes that work - correct?

Right, we don’t want that for our integration tests. That’s taken care of via migrations.

On Fri, Apr 10, 2020 at 9:01 AM Jon P Smith [email protected] wrote:

Hi @jbogard https://github.com/jbogard,

I made sure I looked at Respawn and it does a great job of removing all the data, but the EF Core cleaner deletes the actual tables in the database, i.e. Drop(MyTable). Dropping all the database tables, indexes, foreign keys , etc. the what I want, so that unit tests can handle changes in the EF Core Model when running unit tests.

Also @ajcvickers https://github.com/ajcvickers, looking at Respawn made me think - does your RelationalDatabaseCleaner handle circular table references? I assume that deleting the foreign keys first makes that work - correct?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-612041023, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMR4HKOXJ74NUIK7T7TRL4RCDANCNFSM4KIVZ3HA .

Catching up on this. We generally don't use migrations for our functional tests--remember our tests are for testing EF, not an application that uses EF, and hence we have models that change rapidly as we add new features/regression cases to the tests. So what we want is for the database to match the current model, which may be different from any previous models, and hence we re-create the database structure each time. So yes, @JonPSmith, your description seems correct to me.

Dropping database objects is something that has to be done in a specific order to avoid violating constraints--for example, dropping FK constraints before attempting to delete tables that reference the constraint.

The cleaner was written to handle all of the cases that the EF tests generate. It was not generalized beyond this. This means it hasn't been tested against any arbitrary database structure. This is why we haven't productized it--we would need to have confidence (tests) that it would work for at least a large percentage of customer databases. We're considering doing this, or finding some other way of making it usable by others.

Thanks @ajcvickers for the update. You said

... hence we have models that change rapidly as we add new features/regression cases to the tests.

This is exactly the situation I, and others, find when running unit tests on an evolving/agile application. In particular I always run unit tests before I build a migration, so I have to handle Model changes before there is a migration. This means your cleaner design is just what I was looking for unit testing, both locally and in a C.I. pipeline.

I also understand making your current cleaner both work in all states and easy for people to use adds another item to your very full list of changes. Therefore I will try building my own cleaner, maybe only for SQL Server, and put it into my EfCore.TestSupport library. I haven't got time straight straight away, but hopefully I can do something before/when EF Core 5 comes out.

@JonPSmith Sounds good. Let us know how it goes.

Basic arithmetics (#20212) (add, sub, multiply, divide, negate) are done. I would just continue working down the list. I assume for the remaining operations t denotes an IEnumerable? Is there anything else to be aware of?

Yes, t is an IEnumerable<TEntity>. I don't have any advice, but @smitpatel might. Aggregates over decimal columns are currently blocked in SqliteSqlTranslatingExpressionVisitor. OrderBy is blocked in SqliteQueryableMethodTranslatingExpressionVisitor.

For aggregate, register the functions an in SqliteSqlTranslatingExpressionVisitor generate translation to that custom SqlFunction.

For order by, now that we have CollateExpression, you can just wrapp the ordering expression in CollateExpression with EF_DECIMAL collation (for both TranslateOrderBy/TranslateThenBy).

Thanks for the tips, will give it a shot during this rainy and snowy weekend!

Hi @ajcvickers,

I have been using a version of the EnsureClean found in the EF Core test code for some time now with excellent results. I'm about to embark on writing the last chapter (hurrah!) of my book which is about unit testing and plan to update my EfCore.TestSupport open-source library to EF Core 5 and include an EnsureClean method which is based on a single class adapted from your Microsoft.EntityFrameworkCore.TestUtilities.RelationalDatabaseCleaner class.

My question is what do I need to do to recognize that this code came from the EF Core team? I have currently kept your header with your copyright and added a third line saying I have altered it. Is that enough?

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
// Altered by Jon P Smith, GitHub @JonPSmith, https://www.thereformedprogrammer.net/

Here is a link to the altered class.

PS. My library is under an MIT licence.