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

EF Core 1.0 not including columns with default values in insert into query #7089

Closed
monicadelprado opened this issue Nov 21, 2016 · 69 comments
Closed

Comments

@monicadelprado
Copy link

monicadelprado commented Nov 21, 2016

Steps to reproduce

I'm using EF Core 1.0 and I have a model where I design some properties with default values similar to
modelBuilder.Entity<Listings>(entity => { entity.Property(e => e.IsActive).HasDefaultValueSql("1");}
The table in the database is created correctly defining the columns with the corresponding default value
[IsActive] bit NOT NULL DEFAULT 1

The issue

When I add a new entity and save changes in my context, the insert into statement generated does not contain the properties that have default values even though I assign specific values for those properties. The result in the database is a record where the columns with default values doesn't contain the values I passed in my model.

More info on this post
http://stackoverflow.com/questions/40619319/entity-framework-not-including-columns-with-default-value-in-insert-into-query/

Further technical details

EF Core version: 1.0
Visual Studio version: VS 2015

@rowanmiller
Copy link
Contributor

I think we made some bug fixes around this in 1.1. Could you upgrade to 1.1 and see if the issue is fixed? https://blogs.msdn.microsoft.com/dotnet/2016/11/16/announcing-entity-framework-core-1-1/

@rowanmiller rowanmiller added this to the 1.2.0 milestone Nov 22, 2016
@monicadelprado
Copy link
Author

I have upgraded to EF Core 1.1.0 and still have the issue

@ajcvickers
Copy link
Contributor

@mdelprado-emphasys This looks like a dupe of #6054. If you need a bool with a store-generated default, then use a nullable bool. This gives EF a way to distinguish between not set (null) set to false and set to true.

@ajcvickers ajcvickers removed this from the 1.2.0 milestone Nov 23, 2016
@divega
Copy link
Contributor

divega commented Nov 23, 2016

@ajcvickers do you think there is an opportunity here for a model validation error or warning?

@ajcvickers
Copy link
Contributor

@divega Yes, I think that would be a good idea.

@gdoron
Copy link

gdoron commented Nov 23, 2016

@divega what kind of error or warning were you talking about?
And what about DateTime InsertDate with default value of GetUtcDate()? I really don't want to make InsertDate nullable.

BTW, I'm not sure what exactly did people expect to happen with boolean with default value of true... Did they expect EF to change the C# / CLR spec?

@divega
Copy link
Contributor

divega commented Nov 23, 2016

@gdoron we need to do more analysis but I think the warning would only apply to Boolean properties.

When you specify that a property has a default value in the database, EF Core will use the default value of the CLR type as an indication that it should let the database set the default value, hence it won't include the column in the INSERT operation.

But Boolean properties only have two states, so it is unlikely that you would intend any of these two values to mean "leave the column alone":

  • If you configure the default to be "1" it becomes impossible to insert a "0"
  • Configuring the default to be "0" is pointless

@gdoron
Copy link

gdoron commented Nov 23, 2016

@divega

But Boolean properties only have two states ...

This is why I don't get what did they expect to happen, and they may expect a similar thing with numeric properties too, I assigned my property 0 but it's being ignored.
(I'm assuming default(DateTime) wouldn't bother them as its value isn't considered to be "valid")

@monicadelprado
Copy link
Author

@gdoron , @divega It makes sense, now that you mention, that if I don't have a nullable type, the EF just adds the default value, but I was actually expecting the same behavior than when I do an INSERT INTO directly in the database. The INSERT INTO just adds the record with the values I send in the query and ignores the default values.

@gdoron
Copy link

gdoron commented Nov 23, 2016

@mdelprado-emphasys That's because SQL doesn't have the limitation there actually is a property that being sent.

@GertArnold
Copy link

IMO, when a property is not nullable, the HasDefaultValue(Sql) instruction should be ignored when it comes to setting property values, not for migrations. This gives users the opportunity to create the database schema as they like (i.e. with defaults), but they won't be surprised by this rather unexpected behavior. It's easier to understand that a non-nullable property always has a value (so setting a default by EF isn't necessary) than that a value is overwritten when it happens to have been set to the CLR default.

@gdoron
Copy link

gdoron commented Nov 23, 2016

@GertArnold It would be very annoying having DateTime? InsertDate all over the place just because people make mistakes.
But regardless, I believe it's too late for changing as it's a huge breaking change.
Maybe for EF Core Core. 😸

@GertArnold
Copy link

@gdoron I see your point. But people are going to tack HasDefaultValue to required properties other than InsertDate and such. That's common practice, for example to be able to add required columns to existing tables. For any required property of which the type's default value makes sense (i.e. not DateTime, usually) this causes the insane behavior that an entity with (CLR) default values can't be stored correctly if the defined default differs from the CLR default. Unless ValueGeneratedNever is applied. Do we expect people to understand (or even know) all that? For one, it's not in in the docs on HasDefaultValue.

@divega
Copy link
Contributor

divega commented Nov 23, 2016

There seems to be more useful information in this issue now than in the original #6054, so closing that one as a duplicate of this one.

@divega
Copy link
Contributor

divega commented Nov 23, 2016

@GertArnold that is an interesting insight. I do agree that currently you need to understand a lot before you can make some of these scenarios work correctly.

However as @AndriySvyryd pointed out in a recent conversation, ValueGeneratedNever is not even the right solution in this case: if you were to set that, the value you configured in HasDefaultValue() or HasDefaultValueSql() would never be used.

Instead what we are looking at is to introduce a warning (on second thought, besides bool it should also probably apply to properties of enum types) which suggests making the property of a nullable type. If the property is nullable then the default value of the CLR type will be null, which is hopefully outside of the domain of values you intend to store in the database and hence a better choice of a "sentinel value" to indicate that the value will be generated in the store.

In fact in most cases if you have ValueGeneratedOnAdd and a nullable type it would probably make sense to make the property IsRequired(true) at the same time. We could even consider introducing this as a convention or otherwise include it in the warning mentioned above.

We have considered other options, e.g.:

  • Having a way to configure a different "sentinel" value from the default value for the CLR type to decide whether the value should be sent to the database on INSERTs (I remember vaguely but I think we either had this and we removed it at some point or we were very close to implementing it and then we decided not to do so).
  • Having a way to configure that the default CLR type should not be used as a sentinel, and that the decision to send the value to the database should be made merely on whether the property value is marked as temporary.

But these options seem to lead to a more complicated experience or mental model and not necessarily help as much as making the property of a nullable type.

cc @ajcvickers

@rowanmiller rowanmiller added this to the 1.2.0 milestone Nov 28, 2016
@rowanmiller
Copy link
Contributor

@ajcvickers to split this out into some separate actionable items

@ajcvickers
Copy link
Contributor

Created #7163, #7165, and #7199.

@gdoron
Copy link

gdoron commented May 15, 2017

You are using .NET ORM and as such there are CLR limitations like this one.
EF has no way of knowing if Foo equals false because you set it to false or because you left it with the CLR default value.

I honestly not sure what's so complicated about this concept.

@iJungleboy
Copy link

@gdoron the concept isn't complicated - it just causes very unexpected behavior which hurts programmers. Of course if people would read all the documentation and already know about this, it's logical why it happens, but that still doesn't remove the problem.

Fact is, that people will not plan for this or assume it will happen, until in hits them unexpectedly. And that's bad. Basically part of the problem is simply that the insert wants to optimize for default values, resulting in an insert which is inconsistent with what the developer coded. That's very bad, and EF shouldn't do that.

@gdoron
Copy link

gdoron commented May 15, 2017

Suggest alternative.

@iJungleboy
Copy link

Here's a trivial alternative:
Don't try to optimize the insert-statement - I'm pretty sure we're not saving any performance by optimizing it at all - the work to evaluate if it's the default, + the work the SQL must do to retrieve the default, will probably never be smaller than just sending the value along.

@gdoron
Copy link

gdoron commented May 15, 2017

I don't think you understood what I wrote about the CLR.
It has NOTHING to do with optimization ​but to (as mentioned by me and others many times already) limitations of the CLR.

This is becoming a lengthy and repetitive thread.

@iJungleboy
Copy link

@gdoron I understand, and I must admit I haven't read every single post here - because it is too long. Sorry about that. I often think git is missing a summary-feature at the beginning of the thread, where the admin could summarize the situation and everything.

@popcatalin81
Copy link

popcatalin81 commented May 15, 2017

@Allann I'm saying you shouldn't use sentinel values to insert database defaults. I'm saying if you want (for legit reasons) to insert database defaults then either you do it by not mapping the columns in the model or with an explicit API.

@ajcvickers
Copy link
Contributor

ajcvickers commented May 15, 2017

@Allann Your statement at the end, especially this part "using nullable properties (that remain null), the database takes over and supplies the default values" describes the behavior in EF Core.

The EF6 code doesn't really work like this. EF6 either always uses the default constraint (with linitations) or never uses the default constraint. There is no way to have some inserts for that property use it, while others do not. (Note that EF Core can be configured to do the same thing using StoreGeneratedAlways.)

From what I can tell, the nHibernate code works the same was as EF6--that is, it will always use the database constraint. If it doesn't, then it looks like it would work the same way as EF Core since there is no other information provided that could make it work otherwise.

@popcatalin81 I don't think that is an unreasonable interpretation, and there is a way to set this up with EF Core using StoreGeneratedAlways. However, we have had a lot of feedback over the years indicating that many developers want the ability to insert only when an explicit value has not been set.

@gdoron
Copy link

gdoron commented May 15, 2017

@ajcvickers since NHibernate uses proxies it actually can know if Foo equals false because it was never set or because it was set to false.
I have no idea if it does that or not (I haven't used it for more than 5 years now).

Maybe you can also define a new EF Boolean type (with implicit casting to Boolean), though I don't think it would be any easier and user friendly than what we have now.

@ajcvickers
Copy link
Contributor

@gdoron I don't think the nHibernate code is using a proxy, or, if it is, it is created later and so wouldn't help here, because the code explicitly says new Blog....

I agree that the new Boolean type wouldn't be better than using a nullable bool.

@popcatalin81
Copy link

popcatalin81 commented May 15, 2017

@ajcvickers

I think there are two different problems and mindsets at conflict here.
Example 1:

new Product
{
           Name = "Petunias",
           Price = 2.5,
           LastEditDate = null //Default getdate()
}

This is typical case where you'd want to use the database generate value.

Example 2:

new Agreement
{
           Type = "Care rental",
           ExpirationDate = null // Default getdate() + 30
}

You'd really like to be able to insert null here as it is significant for the logic.

Example 3:

new Message
{
           Content = "Hi there",
           IsVisible = "false" // default true
} 

You'd really want to be able to insert false here even if for some other reasons you had to choose the default to be true.

What I'm trying to say, database defaults diffent from NULL and the need to insert NULL are common but EF core makes this scenario (and others) somewhat difficult.

@ajcvickers
Copy link
Contributor

@popcatalin81 Agreed that being able to sometimes insert null for a given property, but sometimes have the value be store-generated for the same property is something EF Core can't do. We explicitly scoped that out some time ago. This is the first time I have heard someone ask for it. 😸

The other cases are all doable in various ways--using nullable properties or StoreGeneratedAlways.

@popcatalin81
Copy link

popcatalin81 commented May 16, 2017

This is the first time I have heard someone ask for it.

I always was an early adopter, and used newest EF versions even in brown fields (dating back to EF 1 Beta 1 days ....) Just wait for it ... 😄

@John0King
Copy link

I totally don't get why this could be an issue. let .HasDefaultValue() only work for migration, and let the real model default value (eg. CLR default) use Property initializer 。
Database's defalut value is about add a default value when u missing the column . and when u specific that column ,then you can event add null to it.
with ORM this can happen too, when you add a column to the database first , and forget update the model .
Why EF Core want to make a column we already know(in our model) treat as a missing column ?

@hisuwh
Copy link

hisuwh commented Nov 2, 2018

Just want to weigh in as this issue has just stung us. Unsure why it has been closed.
We have the same scenario - added a default value of true to a boolean property so that when the migration was applied existing records would get the value of true.

Completely unexpected behaviour that this would make it impossible to set false when inserting a record.

@wozzo
Copy link

wozzo commented Aug 9, 2021

Using V5.0.5 and this is still an issue.
If it's not possible to set a default value that is not the same as the C# default for that type then why is the method offered?
We had an int property where our DB default was set to -1.
With this bug it meant that any time we tried to set it to 0, it was stored in the DB as -1.

@JanKotschenreuther
Copy link

We also ran into this issue.
A BIGINT NOT NULL column with a Defaul-Constraint 1 and an explicit request to insert that column with 0.

After finding this issue here, knowing that this is not a bug but a feature, i checked for all our default constraints.
With the result that we got hundreds more columns with a default constraint that does not correspond to a CLR-Type default.

I am wondering about a few things:

  1. Why only map booleans with a default constraint to nullable boolens but not any other types? I think this is kind of incosistent.
  2. Why is no API offered that helps setting a CLR-Default explicitly? May that API be complex, but let us have it.
  3. Why is no configuration on DbContextOptions or Scaffolding offered, like IncludeAllColumnsOnInsert or something like that?

We make use of database first and rescaffold our database on every database update.
We got about 500 tables and 300 views, modifying models or configuration by hand on every rescaffold is no option.

If 2. is to complex, how about wrapping some of the complexity.

How about a type ChangeTracker<T> which can help to set an entity explicitly with any value and also CLR-defaults and keep track of that. The type could hold a simple Dictionary<string,object> which holds property names of changed properties and the values that have been set explicitly or just call the to complex APIs which inform the EF Core ChangeTracker. A method Set() could be used to tell the tracker that a property has been changed and maybe there could even be a method UnSet() to tell the tracker that the property is not explicitly set anymore (but I cannot think of any useful case for Unset() at the moment).
I am sure you can come up with something even nicer, but here is some example of how the code could look like:

//lets assume Article.SalesPrice and Article.IsInStock got a default constraint that differs from CLR-default.

//example 1
var tracker = dbContext.ChangeTracker.New<Article>();
tracker.Set(nameof(Article.Name), "Shirt");
tracker.Set(nameof(Article.SalesPrice), 0.0m);
tracker.Set(nameof(Article.IsInStock), false);

dbContext.SaveChanges();
var entity = tracker.GetEntity(); //or .GetInstance()
//do some other work with entity.

//example 2
var entity2 = new Article() { Name = "Hat" };
var tracker2 = dbContext.ChangeTracker.Track(entity2);
tracker2.Set(nameof(Article.SalesPrice), 0.0m);
tracker2.Set(nameof(Article.IsInStock), false);

dbContext.SaveChanges();
//do some other work with entity2

I derived this idea from OData, which is offering a Delta<T> Delta.
The Delta<T> is used to deserialize objects from JSON.
JSON properties can contain a value, null or undefined, and of course CLR-Types cannot be undefined.
Delta<T> handles undefined properties as not explicitly passed and therefor those properties are not marked as changed properties.
Its methods Patch() and Put() can be used to update an object of the same type.
undefined (unchanged) properties are not used to update the given object.
Of course Delta<T> solves another problem but maybe it helps understand where I am coming from with my idea.

@Timka654
Copy link

Version 6.0.9 + Sql Server 15.0.2000.5 = error too
Scenario:

  • Create table
  • Append data
  • Append not null bit collumn and set default value to 1 for update exists data - if not set = error migration
  • Try add new row - new collumn always have default value(1)

How about check collunm setted and != SQL DefaultValue ? Now i'm need check all default values in my project and check all functions(400+) have insert new rows correct to db

Sorry but - its fail!!

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests