Non-nullable bool column with default constraint #8777

lajones · 2017-06-08T00:09:24Z

Fix for #8400.

If Reverse Engineering would map a non-nullable column to a non-nullable bool property but the column has a default constraint then a) map it to a nullable bool property instead and b) issue a warning message.

This is to allow the user of the generated classes to distinguish between actively setting the property to 'false' and not setting the property and allowing the default constraint to generate the value in the database.

default constraint then a) map it to a nullable bool property and b) issue a warning message.

smitpatel · 2017-06-08T05:03:06Z

src/EFCore.Design/Diagnostics/DesignEventId.cs

+        ///     A column would be mapped to a bool type, is non-nullable and has a default constraint.
+        ///     This event is in the <see cref="DbLoggerCategory.Scaffolding" /> category.
+        /// </summary>
+        public static readonly EventId NonNullableBoooleanColumnHasDefaultConstraintWarning = MakeScaffoldingId(Id.NonNullableBoooleanColumnHasDefaultConstraintWarning);


If we are not logging DiagnosticSource event then where are these ids used?
cc: @ajcvickers

@ajcvickers for response to this.

This is still the event ID for the ILogger message.

smitpatel · 2017-06-08T05:08:52Z

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs

            {
+                if (forceNullable)


Move warning outside of this if block & right after checking conditions.

Could. Don't feel it makes make much difference.

smitpatel · 2017-06-08T05:09:15Z

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs

@@ -310,8 +310,14 @@ protected virtual PropertyBuilder VisitColumn([NotNull] EntityTypeBuilder builde
            }

            var clrType = typeScaffoldingInfo.ClrType;
-            if (column.IsNullable)
+            var forceNullable = typeof(bool) == clrType && column.DefaultValue != null;


Create variable nullable taking value from Column.IsNullable & update the variable if conditions match (in same block as log message)

It's a relatively minor point, but I think the way it is now is much clearer. I think it's good for the developer to know that this is happening because nullability is being forced by the scaffolding, rather than it being any intrinsic property of the underlying database model. By saying forceNullable you are explaining that.

Its minor point but no need to compute same thing multiple times, if we are having extra variable then why not use it in well way. We start with value in database, we update the variable with whatever conditions comes up and we set nullability based on that. Today its 2 conditions, it may expand more in future. Its better to right a condensed code. We don't need variables to explain what is happening. Logging message would make it very clear why we decided to introduce nullability. And if still doesn't look very clear, feel free to write a comment for explanation.

You still need to issue the warning message only if you are forcing nullability - and not just because something is nullable - so you will need the forceNullable variable (or at least an if statement with the same logic). I could create forceNullable and then immediately create nullable as well to do what you want. If the variable name was something like wasNullableInDatabaseOrNullabilityWasForced I'd be OK with it. But that's ridiculously long. Personally I find the other way much clearer so I'm going to stick with it for now.

smitpatel · 2017-06-08T05:09:24Z

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs

@@ -361,7 +367,7 @@ protected virtual PropertyBuilder VisitColumn([NotNull] EntityTypeBuilder builde

            if (!column.PrimaryKeyOrdinal.HasValue)
            {
-                property.IsRequired(!column.IsNullable);
+                property.IsRequired(!column.IsNullable && !forceNullable);


This would be just !nullable.

See comment above - I think this is better.

smitpatel · 2017-06-08T05:10:10Z

...FCore.SqlServer.Design.FunctionalTests/ReverseEngineering/Expected/NonNullBoolWithDefault.cs

+    public partial class NonNullBoolWithDefault
+    {
+        public int Id { get; set; }
+        public bool? TestWithDefault { get; set; }


BooleanWithDefaultValueSql and likewise for other property & classname

OK. Don't think it's important as the test already explains that but OK. Why the Sql addition?

Just because it is Bool property with DefaultValueSql set. Becomes more explicit and easy to evaluate the expected output file as in fluent API, you can check if the property has HasDefaultValueSql generated. It's not important but being more explicit in test not gonna hurt and helps person to read the tests more easily.

I thought bool was obvious a) from the name of the test and b) from the produced type. And actually HasDefaultValueSql() is the choice of the scaffolder - it could (and did in the past) do something different if it wanted. The fact that the underlying database model has ColumnModel.DefaultValue set is what's being tested. But OK - like I say I don't think it really matters.

ajcvickers · 2017-06-08T17:20:00Z

src/EFCore.Design/Properties/DesignStrings.Designer.cs

+        /// </summary>
+        public static readonly EventDefinition<string> LogNonNullableBoooleanColumnHasDefaultConstraint
+            = new EventDefinition<string>(
+                DesignEventId.NonNullableBoooleanColumnHasDefaultConstraintWarning,


ajcvickers · 2017-06-08T17:20:48Z

src/EFCore.Design/Properties/DesignStrings.Designer.cs

@@ -584,6 +584,18 @@ public static string ExistingFiles([CanBeNull] object outputDirectoryName, [CanB
                    DesignEventId.ForeignKeyPrincipalEndContainsNullableColumnsWarning,
                    _resourceManager.GetString("LogForeignKeyPrincipalEndContainsNullableColumns")));

+        /// <summary>
+        ///     The column '{columnName}' would normally be mapped to a non-nullable bool property, but it has a default constraint. Such a column is mapped to a nullable bool property to allow a difference between setting the property to false and invoking the default constraint.


@divega fwlink this?

@lajones - wait for this

@ajcvickers @smitpatel @divega are we doing a fwlink within the warning message? I didn't see a response from @divega

@lajones I will have it in a couple of minutes. Trying to pick a temporary landing page for it until we have the actual topic.

@lajones https://go.microsoft.com/fwlink/?linkid=851278, for now it points to https://docs.microsoft.com/en-us/ef/core/modeling/generated-properties#value-generated-on-add.

Thanks, @divega. I'll add this to the message and check in.

lajones · 2017-06-09T17:30:04Z

@smitpatel @ajcvickers I think I've addressed everything. So I'm going to go ahead and check in soon. Let me know if I shouldn't.

smitpatel · 2017-06-09T17:34:06Z

Also can you rebase on latest dev, both CI services are red.

lajones · 2017-06-09T17:35:51Z

@smitpatel Sure. Np.

lajones · 2017-06-09T20:29:25Z

Checked in with commit 61650f8.

ryanelian · 2017-09-01T08:40:44Z

Excuse me, can I do something to disable this behavior?

I'm trying to re-scaffold a database context for an existing project from an existing database (schema manually managed / updated by SQL) after upgrading to ASP.NET Core 2.0 but suddenly EF Core puts ? to many columns within the entity classes, causing over 122 compile errors...

EDIT: oh lmao #9627

😕

If a non-nullable column would be mapped to a bool property but has a

cd00a9c

default constraint then a) map it to a nullable bool property and b) issue a warning message.

dnfclas added the cla-already-signed label Jun 8, 2017

smitpatel reviewed Jun 8, 2017

View reviewed changes

ajcvickers reviewed Jun 8, 2017

View reviewed changes

Updating with review comments.

c8724f3

smitpatel approved these changes Jun 9, 2017

View reviewed changes

lajones closed this Jun 9, 2017

lajones added this to the 2.0.0 milestone Jun 9, 2017

lajones self-assigned this Jun 9, 2017

lajones mentioned this pull request Jun 9, 2017

Reverse engineer bool column with default constraint to nullable bool property #8400

Closed

lajones deleted the 170607_8004_BoolDefConstraint_03 branch June 9, 2017 20:30

lajones added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-nullable bool column with default constraint #8777

Non-nullable bool column with default constraint #8777

lajones commented Jun 8, 2017 •

edited

Loading

smitpatel Jun 8, 2017

lajones Jun 8, 2017

ajcvickers Jun 8, 2017

smitpatel Jun 8, 2017

lajones Jun 8, 2017

smitpatel Jun 8, 2017

lajones Jun 8, 2017

smitpatel Jun 8, 2017

lajones Jun 8, 2017

smitpatel Jun 8, 2017

lajones Jun 8, 2017

smitpatel Jun 8, 2017 •

edited

Loading

lajones Jun 8, 2017

smitpatel Jun 8, 2017

lajones Jun 8, 2017 •

edited

Loading

ajcvickers Jun 8, 2017

ajcvickers Jun 8, 2017

smitpatel Jun 9, 2017

lajones Jun 9, 2017

divega Jun 9, 2017

divega Jun 9, 2017

lajones Jun 9, 2017

lajones commented Jun 9, 2017

smitpatel commented Jun 9, 2017

lajones commented Jun 9, 2017

lajones commented Jun 9, 2017

ryanelian commented Sep 1, 2017 •

edited

Loading

Non-nullable bool column with default constraint #8777

Non-nullable bool column with default constraint #8777

Conversation

lajones commented Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lajones Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lajones commented Jun 9, 2017

smitpatel commented Jun 9, 2017

lajones commented Jun 9, 2017

lajones commented Jun 9, 2017

ryanelian commented Sep 1, 2017 • edited Loading

lajones commented Jun 8, 2017 •

edited

Loading

smitpatel Jun 8, 2017 •

edited

Loading

lajones Jun 8, 2017 •

edited

Loading

ryanelian commented Sep 1, 2017 •

edited

Loading