-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
EF Core 8 scaffold for boolean columns with default true
doesn't work as expected
#34223
Comments
@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? |
Consider the following MySql table that defines not nullable boolean column 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 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 dbContext.bool_test.Add(new bool_test { id = 1 });
await dbContext.SaveChangesAsync(); I expect to get the same behavior, but in reality, 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 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 |
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). |
If I understand this correctly, this is more of a task for provider. It should parse and provide the (CLR) default value for |
Are you talking about |
It seems to me that this request boils down to two complementary changes to scaffolding when the database has a default constraint
That we we get full consistency on the C# and the mapping side for default values. |
@gao-artur Ahh, right, now the property is |
@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. |
@ajcvickers I have read these issues but still don't understand why |
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. |
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. |
@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 |
@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. |
I expect the value to be sent only when the user explicitly sets it and the CLR value differs from the DB default value.
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 |
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 |
@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. |
No, I want unset value to become 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. |
How do you determine if a bool value has been set or not? |
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. |
@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. |
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 |
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. |
Ok, I read this and I understand that there must be two changes:
Then why the scaffold can't do that for boolean columns when they have default DB value 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 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; |
Ok, the explanation is actualy in the next section
When explaining sentinel in the previous section it says
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 |
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. |
Thank you @gao-artur for making this ticket. It might have saved us from some nasty bugs. |
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 theEntityType.t4
when the ClrType isbool
and default value istrue
: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
The text was updated successfully, but these errors were encountered: