dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.6k stars 3.14k forks source link

EF Core 8 scaffold for boolean columns with default `true` doesn't work as expected #34223

Closed gao-artur closed 1 week ago

gao-artur commented 1 month ago

File a bug

It's actually the same complaint as in #33683.

After the recent changes in EF Core 8, the default value for a boolean column in DB became "useless". I mean, It allows the EF Core to decide whether to send the value to the DB, but you can't skip setting the C# property and rely on the default value being set by the DB if it's different from the CLR default value.

I was able to work it around by adding = true; in the EntityType.t4 when the ClrType is bool and default value is true:

+        var needsBoolInitializer = property.ClrType == typeof(bool) && property.GetDefaultValue() is true;
#>
-    public <#= code.Reference(property.ClrType) #><#= needsNullable ? "?" : "" #> <#= property.Name #> { get; set; }<#= needsInitializer ? " = null!;" : "" #>
+    public <#= code.Reference(property.ClrType) #><#= needsNullable ? "?" : "" #> <#= property.Name #> { get; set; }<#= needsInitializer ? " = null!;" : needsBoolInitializer ? " = true;" : "" #>
<#

I think without these changes, the new behavior is harmful, and the true assignment should be built into the t4 template.

Include provider and version information

EF Core version: 8.0.7 Database provider: Pomelo.EntityFrameworkCore.MySql Target framework: .NET 8 Operating system: Win10 IDE: Visual Studio 2022 17.10.4

ajcvickers commented 1 month ago

@gao-artur Can you explain in more detail what behavior you want? EF would previously scaffold a nullable bool property for this; is that what you would prefer? If so, can you explain why it is better? If not, what would you like instead?

gao-artur commented 1 month ago

Consider the following MySql table that defines not nullable boolean column is_active with default value b'1' (true):

CREATE TABLE `bool_test` (
  `id` int NOT NULL,
  `is_active` bit(1) NOT NULL DEFAULT b'1',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;

When inserting a new entry to the table using raw SQL (without ORM) like this

INSERT INTO `bool_test`.`bool_test` (`id`) VALUES ('1');

The is_active will be set to b'1' because it's defined as default value image

Now, when scaffolding this table (and assuming this bug is fixed), you will receive the following model

public partial class bool_test
{
    public int id { get; set; }

    public bool is_active { get; set; }
}

When inserting a new bool_test without explicitly setting the is_active property

dbContext.bool_test.Add(new bool_test { id = 1 });
await dbContext.SaveChangesAsync();

I expect to get the same behavior, but in reality, False is set into the is_active column:

Executed DbCommand (26ms) [Parameters=[@p0='1', @p1='False'], CommandType='Text', CommandTimeout='30']
SET AUTOCOMMIT = 1;
INSERT INTO `bool_test` (`id`, `is_active`)
VALUES (@p0, @p1);

But if I change the model to have a default value true

public partial class bool_test
{
    public int id { get; set; }

    public bool is_active { get; set; } = true;
}

I get exactly what I expect

Executed DbCommand (20ms) [Parameters=[@p0='1'], CommandType='Text', CommandTimeout='30']
INSERT INTO `bool_test` (`id`)
VALUES (@p0);
SELECT `is_active`
FROM `bool_test`
WHERE ROW_COUNT() = 1 AND `id` = @p0;

Nothing is sent for the is_active column, and the DB applies the default value.

gao-artur commented 1 month ago

I like the not nullable booleans in the EF Core 8 scaffold. But I expect to get the default values applied if I don't explicitly set the boolean value in the C# code (Db default, not CLR default).

cincuranet commented 1 month ago

If I understand this correctly, this is more of a task for provider. It should parse and provide the (CLR) default value for DatabaseColumn. Somewhere here and around. Then the HasDefaultValue should take care of it.

gao-artur commented 1 month ago

Are you talking about DatabaseColumn.DefaultValue? If so, parsing the DB default and setting the CLR default will allow scaffolding not nullable booleans as shown in this bug. But this is not enough, as the C# property will be false by default, and EF will send this false to the DB, overriding the DB default.

davisnw commented 1 month ago

It seems to me that this request boils down to two complementary changes to scaffolding when the database has a default constraint

  1. Apply the T4 template edits in the original post in to the core EF Scaffolding
  2. Apply HasDefaultValue (or the equivalent provider configuration) in the model configuration

That we we get full consistency on the C# and the mapping side for default values.

cincuranet commented 1 month ago

@gao-artur Ahh, right, now the property is bool not bool?.

ajcvickers commented 1 month ago

@davisnw @gao-artur You're both going to have to be much more explicit about this. There have been many threads about this behavior in the past. Make sure you have full read the discussion on #13613 and related issues, and then make a detailed suggestion that takes the issues discussed there into account. This is very far from a simple problem with a simple solution.

gao-artur commented 1 month ago

@ajcvickers I have read these issues but still don't understand why public bool is_active { get; set; } = true; is not a universally good solution. I don't know how to be more explicit than what I already described above. We are doing a database-first, and I have nearly zero experience in code-first and migrations, so I may be missing something. Our use case is making changes in the DB and then scaffolding it to get changes in code. Then, we use EnsureCreated for integration tests, so we need the DbContext to have enough information to re-create the schema.

ajcvickers commented 1 month ago

Then, we use EnsureCreated for integration tests, so we need the DbContext to have enough information to re-create the schema.

In general, this won't work. The scaffolding process is lossy, as is Migrations/EnsureCreated. This is because not everything in the schema can be represented in the EF model, and not everything in the EF model can be represented in the schema.

ajcvickers commented 1 week ago

EF Team Triage: Closing this issue as the requested additional details have not been provided and we have been unable to reproduce it.

BTW this is a canned response and may have info or details that do not directly apply to this particular issue. While we'd like to spend the time to uniquely address every incoming issue, we get a lot traffic on the EF projects and that is not practical. To ensure we maximize the time we have to work on fixing bugs, implementing new features, etc. we use canned responses for common triage decisions.

gao-artur commented 1 week ago

@ajcvickers I would like to provide more details, but I don't know what to add to what I already described. Can you please explain where public bool is_active { get; set; } = true; won't work or may cause issues?

ajcvickers commented 1 week ago

@gao-artur That would just be repeating the entire discussion from before. It boils down to when EF should send a value and when should it not send a value. At what point the value gets set into the CLR object is irrelevant for this discussion.

gao-artur commented 1 week ago

I expect the value to be sent only when the user explicitly sets it and the CLR value differs from the DB default value.

CLR value DB default Should send
false false no
true false yes
unset false no
false true yes
true true no
unset true no

Today, it sends the value if the current CLR value differs from the database default value. Since the default CLR value for the boolean is false, unset is the same as false. This means that all the combinations except the last one work as expected. But if the scaffold initiates the boolean field with true when the corresponding boolean DB column has the default true (essentially changing the unset default value), all the combinations will work as expected.

gao-artur commented 1 week ago

Without this change, the change made in #31148 just confusing and will lead to a lot of bugs. Before it, you could leave the boolean field unset and rely on DB to set its default value. Now it doesn't work anymore as you will get false sent when you expect true to be set by the DB.

ajcvickers commented 1 week ago

@gao-artur So you want false on the CLR type to become true in the database, while, of course, true values also become true. If you want all values to be true, then yes, the previous behavior was better. Most people want true to become true in the database and false to become false. Please note that in your table the CLR value of unset is the same as the CLR value of false. This is the whole point. There is no difference between unset and false.

gao-artur commented 1 week ago

So you want false on the CLR type to become true in the database

No, I want unset value to become true (only when the default DB value is true). When I say "unset", I mean the user hasn't explicitly set it as I explained in this comment.

public partial class bool_test
{
    public int id { get; set; }

    public bool is_active { get; set; } = true;
}

dbContext.bool_test.Add(new bool_test { id = 1 });
await dbContext.SaveChangesAsync();

This way, you get the best of both worlds: the boolean with the default DB value is not nullable, but you can still skip setting it in the code and get the correct value set by the DB.

ajcvickers commented 1 week ago

How do you determine if a bool value has been set or not?

gao-artur commented 1 week ago

I don't. But if the boolean has the same value as the DB default, the EF Core skips sending it. This is how it works today. This is the generated query from my previous code example

Executed DbCommand (20ms) [Parameters=[@p0='1'], CommandType='Text', CommandTimeout='30']
INSERT INTO `bool_test` (`id`)
VALUES (@p0);
SELECT `is_active`
FROM `bool_test`
WHERE ROW_COUNT() = 1 AND `id` = @p0;

So, aligning the boolean property default value with the DB default value sounds logical to me. And I really can't find any disadvantage for this approach.

ajcvickers commented 1 week ago

@gao-artur What logic should EF use to decide whether to send the value or not? Not sending the value if it has not been set is the same as not sending the value if the value has been set to false. In other words, there are not three states here, only two. True or false. For a non-nullable bool there is no third "unset" state.

gao-artur commented 1 week ago

Again, I don't ask to change the current logic. What I described above already happens. The EF Core doesn't send the value if its value equals to DB default. If the value is false and the DB default is false, nothing is sent. If the value is true and the DB default is true nothing is sent. What I ask for, is to change the default value for bool properties from false to true if the DB default is true.

ajcvickers commented 1 week ago

The EF Core doesn't send the value if its value equals to DB default.

This is not true. EF doesn't send the value if it matches the sentinel set on the property. By default, that sentinel is the CLR default. So, EF doesn't send the value if it equals the CLR default. However, the sentinel can be changed. So, for bools, you can have EF not send the value when it is true, or not send the value when it is false.

gao-artur commented 1 week ago

Ok, I read this and I understand that there must be two changes:

  1. Add .HasSentinel(true) to entity property configuration
  2. Add = true; to the entity property itself

Then why the scaffold can't do that for boolean columns when they have default DB value true?

And how can you explain this result? I didn't change the sentinel when preparing this example, only the property default value. But nothing has been sent for the is_active on SaveChangesAsync().

public partial class bool_test
{
    public int id { get; set; }

    public bool is_active { get; set; } = true;
}

dbContext.bool_test.Add(new bool_test { id = 1 });
await dbContext.SaveChangesAsync();
Executed DbCommand (20ms) [Parameters=[@p0='1'], CommandType='Text', CommandTimeout='30']
INSERT INTO `bool_test` (`id`)
VALUES (@p0);
SELECT `is_active`
FROM `bool_test`
WHERE ROW_COUNT() = 1 AND `id` = @p0;
gao-artur commented 1 week ago

Ok, the explanation is actualy in the next section

EF8 fixes this problem by setting the sentinel for bool properties to the same value as the database default value. Both cases above then result in the correct value being inserted, regardless of whether the database default is true or false.

When explaining sentinel in the previous section it says

It can often be useful to reflect this in the entity type as well as in the EF configuration. For example:

public class Person
{
    public int Id { get; set; }
    public int Credits { get; set; } = -1;
}

This means that the sentinel value of -1 gets set automatically when the instance is created, meaning that the property starts in its "not-set" state.

All I ask for is to apply this recommendation by the scaffold tool on boolean properties by default.

ajcvickers commented 1 week ago

All I ask for is to apply this recommendation by the scaffold tool on boolean properties by default.

We discussed doing this at the time, but decided against doing it by default. It is not necessarily the correct thing to do, although it can often be useful. Regardless, this is the kind of thing that the T4 template support is designed to address. If you want to scaffold = true;, then add that to the template.

gao-artur commented 1 week ago

then add that to the template.

I did, but I think this should be the default behavior. Unfortunately, I didn't get the answer to why "It is not necessarily the correct thing to do" not in this thread or in the referenced issues.

Anyway, I accept your refusal to do that.