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

A BUG in EntityFramewrokCore about ValueGenerateOnAdd() and ValueGenerateOnAddOrUpdate() #16434

Closed
ryanmajidi opened this issue Jul 3, 2019 · 6 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@ryanmajidi
Copy link

ryanmajidi commented Jul 3, 2019

Submitted to site feedback by @ym9288

There are my 2 Properties Sets of my Entity:

entity.Property(e => e.CreateTime)
  .HasColumnName("createTime")
  .HasColumnType("datetime")
  .IsRequired(true)
  .ValueGeneratedOnAdd()
  .HasDefaultValueSql("getdate()");

entity.Property(e => e.UpdateTime)
  .HasColumnName("updateTime")
  .HasColumnType("datetime")
  .IsRequired(true)
  .ValueGeneratedOnAddOrUpdate()
  .HasDefaultValueSql("(getdate())");

they "createTime" use ValueGeneratedOnAdd and "updateTime" use ValueGeneratedOnAddOrUpdate
It looks like that whenever my Update on my entity should make the "updateTime" be Updated!,and "createTime" should never be updated when I update my entity.

But,when I update my entity,I got the SQL generated by EFCore:

UPDATE [PatientInfo] SET [address] = @p0, [age] = @p1, [ageStr] = @p2, [patientName] = @p3, [phone] = @P4, [sex] = @p5, [createTime] = @p6
WHERE [patientId] = @P7;

EFCore updated the "createTime" column,otherwise,did not update "updateTime" column,

At this time,I changed the "createTime" column's Property Set to use ValueGeneratedOnAddOrUpdate(),
and changed the "updateTime" column's Property to use ValueGenerateOnAdd().
with the same opertion of just now(tips:update entity),
here is the SQL

UPDATE [PatientInfo] SET [address] = @p0, [age] = @p1, [ageStr] = @p2, [patientName] = @p3, [phone] = @P4, [sex] = @p5, [updateTime] = @p6
WHERE [patientId] = @P7;

You see,"updateTime" was seted but no "createTime".
over... here is my contact,you can ask me some questions.
QQ/Email:280823786@qq.com,

@ajcvickers
Copy link
Member

@ym9288 Please post a small, runnable project/solution or complete code listing that demonstrates the behavior you are seeing so that we can investigate.

@ym9288
Copy link

ym9288 commented Jul 6, 2019

@ajcvickers
Copy link
Member

@ym9288 The call to context.Update(p1) explicitly marks all properties as modified, which then causes the of the properties to be used. Based on your repro code, Update is not needed here--removing it fixes the issue.

Note fore triage: this is somewhat confusing behavior, but changing it risks not saving modified value if they need to be saved. We should see if we can make the experience better.

@ym9288
Copy link

ym9288 commented Jul 9, 2019

@ajcvickers My meaning :
First, I hava Configed the property "updateTime" with ValueGenerateOnAddOrUpdate() and WithDefaultValue("(getdate())") in my HISDbContext, so I thought this field in database would be reset by EF's generated SQL when I update this record.
In my update code , I didnot set a value on "updateTime" so that it would be keep a temp default value "0001-00-00...." like this . Then The EF would be reset a new value like DateTime.Now for this field base on the Configuration :ValueGenerateOnAddOrUpdate() and WithDefaultValue("(getdate())"). Or : EF do not reset the "updateTime" use the temp value to update to SQLServer,when SQLSERVER find the value is a default value("0001-00-00..."),this field will be reset by SQLSERVER 's 'getdate()' from the Configuration WithDefaultValue("getdate()").
Did I get a wrong understanding from your meaning?

@ajcvickers
Copy link
Member

@ym9288 Sorry, my previous analysis was not correct. I didn't that the configuration for UpdateTime is using HasDefaultColumnSql when it needs to be ComputedColumnSql to get SQL Server to generate a new value every time the entity is queried. For example:

entity.Property(e => e.UpdateTime)
    .HasColumnName("updateTime")
    .HasColumnType("datetime")
    .IsRequired(true)
    .ValueGeneratedOnAddOrUpdate()//Here is the point for the BUG
    .HasComputedColumnSql("(getdate())");

Note that ValueGeneratedOnAddOrUpdate is poorly named, since it is really about creating a computed column in this case, which means the value will be generated whenever the row is read from the database. There isn't a great way make a timestamp update when a row is updated other than using a trigger--see #10770. We will discuss as a team making this better.

After this change, running your code, inserting the entity uses the following SQL:

SET NOCOUNT ON;
INSERT INTO [PatientInfo] ([address], [age], [ageStr], [patientName], [phone], [sex])
VALUES (@p0, @p1, @p2, @p3, @p4, @p5);
SELECT [patientId], [createTime], [updateTime]
FROM [PatientInfo]
WHERE @@ROWCOUNT = 1 AND [patientId] = scope_identity();

As you cane see, the values for createTime and updateTime are obtained from the database. The values in the entity after calling SaveChanges are:
image

The generated SQL for the update is:

SET NOCOUNT ON;
UPDATE [PatientInfo] SET [address] = @p0, [age] = @p1, [patientName] = @p2, [phone] = @p3, [sex] = @p4
WHERE [patientId] = @p5;
SELECT [updateTime]
FROM [PatientInfo]
WHERE @@ROWCOUNT = 1 AND [patientId] = @p5;

and the values in the entity are:
image

@ajcvickers
Copy link
Member

Triage: added a note to #14532 to consider this scenario.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Jul 15, 2019
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants