ValueGeneratedOnUpdate() and ValueGeneratedOnAddOrUpdate() not working for DateTimeOffset property

itsthekeming commented 4 years ago

I'm having trouble getting ValueGeneratedOnUpdate and ValueGeneratedOnAddOrUpdate() to work for a DateTimeOffset property in my model. When I call AddAsync() or Update(), the entity local reflects the newly generated values. However, calling SaveChangesAsync() results in either an exception or no change to the values in question. ValueGeneratedOnAdd() appears to be working properly.

Steps to reproduce

My model has two DateTimeOffset properties.

public class Account
{
    public int Id { get; set; }
    public Guid Guid { get; set; }
    public DateTimeOffset CreatedDateTime { get; set; }
    public DateTimeOffset LastModifiedDateTime { get; set; }
}

I'm using the fluent API to configure my context.

builder.HasKey(account => account.Id);
builder.Property(a => a.Guid)
    .ValueGeneratedOnAdd()
    .HasValueGenerator(typeof(GuidValueGenerator))
    .IsRequired();
builder.Property(a => a.CreatedDateTime)
    .ValueGeneratedOnAdd()
    .HasValueGenerator(typeof(DateTimeOffsetValueGenerator))
    .IsRequired();
builder.Property(a => a.LastModifiedDateTime)
    .ValueGeneratedOnAddOrUpdate()
    .HasValueGenerator(typeof(DateTimeOffsetValueGenerator))

And I have a value generator (the GuidValueGenerator is similar, but returns Guid.NewGuid() instead).

internal class DateTimeOffsetValueGenerator : ValueGenerator<DateTimeOffset>
{
    public override bool GeneratesTemporaryValues => false;

    public override DateTimeOffset Next(EntityEntry entry)
    {
        if (entry is null)
        {
            throw new ArgumentNullException(nameof(entry));
        }

        return DateTimeOffset.UtcNow;
    }
}

Finally, I have my controller actions.

public async Task<IActionResult> Post([FromBody] Account account)
{
    ...

    await dbContext.Accounts.AddAsync(account);
    await dbContext.SaveChangesAsync();

    ...
}

public async Task<IActionResult> Put([FromRoute] int id, [FromBody] Account account)
{
    //retrieving accountToUpdate, error handling, assigning changed properties, etc.

    dbContext.Accounts.Update(accountToUpdate);
    await dbContext.SaveChangesAsync();

    ...
}

If:

CreatedDateTime uses ValueGeneratedOnAdd()
LastModifiedDateTime uses ValueGeneratedOnAddOrUpdate()
Both use HasValueGenerator(typeof(DateTimeOffsetValueGenerator))

Then the following occurs: After dbContext.AddAsync(account) completes, I can see that both CreatedDateTime and LastModifiedDateTime have the correct values in my locals. LastModifiedDateTime is not null. However, dbContext.SaveChangesAsync() throws a DbUpdateException, claiming it is null. Here is the stack trace (I've changed some names/paths to protect info):

Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while updating the entries. See the inner exception for details.
 ---> Microsoft.Data.SqlClient.SqlException (0x80131904): Cannot insert the value NULL into column 'LastModifiedDateTime', table 'DatabaseName.dbo.Accounts'; column does not allow nulls. INSERT fails.
The statement has been terminated.
   at Microsoft.Data.SqlClient.SqlCommand.<>c.<ExecuteDbDataReaderAsync>b__164_0(Task`1 result)
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
ClientConnectionId:1c5e1618-9998-46c8-967d-6b0ed12f44f5
Error Number:515,State:2,Class:16
   --- End of inner exception stack trace ---
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IList`1 entriesToSave, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(DbContext _, Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Application.Api.Controllers.AccountsController.Post(Account account) in PathToProject\Controllers\AccountsController.cs:line 95
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Okay, so ValueGeneratedOnAddOrUpdate() throws an exception. I tried changing the configuration to use ValueGeneratedOnUpdate() for LastModifiedDateTime instead. This results in the correct value being generated and saved when I create the entity, but when I try to update the entity, LastModifiedDateTime still has the old value. No new value is generated and saved. Any properties I manually update do successfully save.

I did try calling both ValueGeneratedOnAdd() and ValueGeneratedOnUpdate() on the configuration of LastModifiedDateTime and it behaved the same as if only ValueGeneratedOnUpdate() was present.

I did find issue #3955, but I'd prefer to generate the value in code instead of passing in a SQL function.

The CreatedDateTime property has no trouble generating the value, so I believe the value generation is being implemented properly. Am I missing something?

Further technical details

EF Core version: 3.1.1 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET Core 3.1 Operating system: Windows 10 Version 1903 Build 18362.592) IDE: Visual Studio 2019 16.4.3

obohaciak commented 4 years ago

Exactly the same problem here. Seems like EF Core treats the property that I configured with ValueGeneratedOnAddOrUpdate as calculated column and tries to read the column value from the DB after insert as if it was configured with HasDefaultValue/HasComputedColumnSql.

Just like @ebsndrs, this happens with DateTimeOffset properties on EF Core 3.1.1 with MSSQL and even the same Windows & VS versions. DateTimeOffset properties configured with ValueGeneratedOnAdd behave as expected.

ajcvickers commented 4 years ago

Looks like a duplicate of #6999. Value generators are not called for updates.

itsthekeming commented 4 years ago

Thank you for pointing to those other issues @ajcvickers. Are there still no plans to support value generators for updates in the future?

itsthekeming commented 4 years ago

I ended up using the ChangeTracker.Tracked and ChangeTracker.StateChanged in my DbContext to get this functionality. I hope the EF Core team considers allowing value generators on update operations in the future; it would be very useful for this sort of thing.

obohaciak commented 4 years ago

It would be great if this was supported. At the very least, the docs for Generated Values > Value generated on add or update in the current state are somewhat misleading as they claim that:

How the value is generated for added and updated entities will depend on the database provider being used. Database providers may automatically setup value generation for some property types, while others will require you to manually setup how the value is generated.

However reading through it, I believe that's because ValueGeneratedOnAdd' = '[DatabaseGenerated(DatabaseGeneratedOption.Identity)] ValueGeneratedOnUpdate' = '[DatabaseGenerated(DatabaseGeneratedOption.Computed)] (this seems to be also basically what DatabaseGeneratedAttributeConvention does)

ajcvickers commented 4 years ago

Better docs issue: https://github.com/aspnet/EntityFramework.Docs/issues/1398

Yes, we do plan to make improvements in this area, but closing this issue as duplicate since the functionality is primarily tracked by #6999.

mnsrulz commented 4 years ago

Is there a way we can extend the functionality until it's available natively? I think it's a good feature to add and valid use case.

eegeeZA commented 4 years ago

Here is my workaround

Continue to configure the generator with your Fluent API:

builder.Property(x => x.LastModified)
    .HasValueGenerator<UtcDateTimeGenerator>()
    .ValueGeneratedOnAddOrUpdate();

Update your DbContext to add the following methods:

public override int SaveChanges(bool acceptAllChangesOnSuccess)
{
    GenerateOnUpdate();
    return base.SaveChanges(acceptAllChangesOnSuccess);
}

public override Task<int> SaveChangesAsync(
    bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default)
{
    GenerateOnUpdate();
    return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
}

private void GenerateOnUpdate()
{
    foreach (EntityEntry entityEntry in ChangeTracker.Entries())
    {
        foreach (PropertyEntry propertyEntry in entityEntry.Properties)
        {
            IProperty property = propertyEntry.Metadata;
            Func<IProperty, IEntityType, ValueGenerator> valueGeneratorFactory =
                property.GetValueGeneratorFactory();
            bool generatedOnUpdate = (property.ValueGenerated & ValueGenerated.OnUpdate)
                == ValueGenerated.OnUpdate;
            if (!generatedOnUpdate || valueGeneratorFactory == null)
            {
                continue;
            }

            ValueGenerator valueGenerator = valueGeneratorFactory.Invoke(
                        property,
                        entityEntry.Metadata);
            propertyEntry.CurrentValue = valueGenerator.Next(entityEntry);
        }
    }
}

alexandis commented 4 years ago

I have the problem on Oracle 18C - I need to fill PK in the table, PK is a NUMBER, not IDENTITY (this is obviously a defect and will be changed later on, but now I have to deal with that since I don't have rights to change DB structure, however I need to prepare CRUD demo). I don't want to use some C# value generator, but instead - DB remedy. So I tried to use the following (but it did not work - the expression is ignored, "ORA-01400: cannot insert NULL"): b.HasKey(x => x.Id); b.Property(x => x.Id).HasColumnName("C_LICENCE").IsRequired().ValueGeneratedOnAdd().HasDefaultValueSql("select round(dbms_random.value(100000, 999999)) from dual"); I suspect it's probably because int primary column is never null :) But anyway, I need to somehow force it to be generated via SQL always.

roji commented 4 years ago

@alexandis it seems like you want your database to generate the values, but can't change your database schema to do that? If you can't make your database generate values, you must do that on the client. Either you manually do this in application code, or you can set up a value generator in EF Core - which you also seem to not want... Can you be more specific on exactly what you're looking for?

alexandis commented 4 years ago

@roji I think I don't have rights to change the table schema, however I'm free to insert values into DB in a way I want. So what I would like to achieve is - having 'incorrect' oracle table structure, where PK ID is defined, but it's a NUMBER instead of IDENTITY - to insert a new record like: INSERT INTO MYTABLE (ID) VALUES ((select round(dbms_random.value(100000,999999)) from dual). How am I supposed to do this?

roji commented 4 years ago

@alexandis if you can't change your database column definition - to define a default expression or configure it as identity - then it's up to your program to provide those values. The easiest is probably to set a value generator with EF Core, is there any reason you're trying to avoid that?

PS using random number generation like the above is a bad idea - you'll have collisions and I'm assuming ID needs to be unique.

alexandis commented 4 years ago

@roji I do not worry about duplicates, since for demo purposes I will need to insert probably just dozens of entries (the client will change the architecture later on). On a second thought - you are right, why not to use .NET "randomize" mechanism, it will be more or less the same. The only reason I might try to avoid that - is to make DB check if the next generated ID is unique for the table. But for this, I would need to create a SEQUENCE in the DB (and again - I have no right for that and all in all, it makes no sense either) :) So I think I am giving up with HasDefaultValueSql now - the only question remains, why it did not work in my case anyway? :)

roji commented 4 years ago

@alexandis in order to understand why HasDefaultValueSql didn't work for you, we'd need to see a runnable code sample. If you're interested in pursuing that, please open a new issue issue with the proper code.

hades200082 commented 3 years ago

I'm having this issue too. The documentation and IntelliSense on the ValueGeneratedOnAddOrUpdate() method is very misleading. At the very least I would expect it to use ValueGenerators on update the same way that ValueGeneratedOnAdd does.

Having ValueGenerators work on update would also allow solutions built with EF Core be much more DBMS agnostic since using C# ValueGenerators means you're not relying on DBMS specific features/SQL snippets and/or triggers to achieve the result. This would allow developers of applications to support multiple DBMSs more easily.

@eegeeZA I've added your workaround to my solution and, although I can step through and see it setting the "LastModified" DateTime property on my entity, the SQL generated by ef core omits the column in question resulting in a SQL Exception cannot insert nulls error.

In my case, the "LastModified" property is inherited from an abstract base class (an AuditedAggregateRoot class that just adds Created and LastModified properties) - if that has any bearing - and I'm running in .Net 5 with EF Core 5.0.2.

roji commented 3 years ago

@hades200082 there's some relevant information on this in this page of the EF docs.

tl;dr different database support very different mechanisms for value generation, both for add and for update. In some restricted cases, EF providers will indeed set up value generation automatically (e.g. integer key properties are automatically set up as identity). But for anything beyond that minimum, you need to specifically set up value generation yourself - e.g. a trigger - since there's no way for EF Core to know what you want.

hades200082 commented 3 years ago

@roji There is a way for EF Core to know what I want... Me telling it what I want through the type configuration.

I've told it when I want it to generate a value by using .ValueGeneratedOnAddOrUpdate().

I've told it how to generate that new value by using .HasValueGenerator<UtcDateTimeGenerator>()

So, I've told it both when I want it to generate a value for the property and exactly how to generate that value in C# code. No special database functionality required.

Why doesn't this work for updates as the docs and IntelliSense suggests it should?

From PropertyBuilder.cs:

/// <summary>
/// Configures a property to have a value generated when saving a new or existing entity.
/// </summary>
/// <returns> The same builder instance so that multiple configuration calls can be chained. </returns>
public new virtual PropertyBuilder<TProperty> ValueGeneratedOnAddOrUpdate()
    => (PropertyBuilder<TProperty>)base.ValueGeneratedOnAddOrUpdate();

/// <summary>
///     Configures a property to have a value generated when saving an existing entity.
/// </summary>
/// <returns> The same builder instance so that multiple configuration calls can be chained. </returns>
public new virtual PropertyBuilder<TProperty> ValueGeneratedOnUpdate()
    => (PropertyBuilder<TProperty>)base.ValueGeneratedOnUpdate();

I understand that it's not always possible to have the database do the generation automatically, but there should be no reason that EF can't generate the value based on providing the when and the how in the type configuration.

In addition, using the workaround posted above by @eegeeZA I can see that the ValueGenerator is being called prior to saving changes and yet it's still not including the property in the SQL that's generated for an insert and still doesn't update the column for updates when properties are configured with .ValueGeneratedOnAddOrUpdate().

roji commented 3 years ago

As written above in https://github.com/dotnet/efcore/issues/19765#issuecomment-581526253, Value generators are not called for updates, only for inserts (#6999 tracks that).

You're right that our docs aren't sufficient for client-side value generation, I've opened https://github.com/dotnet/EntityFramework.Docs/issues/3057 to track that.

hades200082 commented 3 years ago

You are correct at the moment, but surely given the fact that we can specify that we want values generated on update, and there is already a mechanism in place that could do this, it would make sense to do it?

It also doesn't explain why setting a value in overridden SaveChanges still throws a SQL null exception.

roji commented 3 years ago

I'm not saying it doesn't make sense - there's an issue tracking that in the backlog. We haven't done it because there are many other high-priority work items, and that issue has hardly received any user interest/votes.

For the null exception, can you please open a new issue with a runnable code sample so we can investigate?

eegeeZA commented 3 years ago

For clarification, it is important to set GeneratesTemporaryValues to false if the DB does not replace the null with a value. @hades200082

/// <summary>
///     Generates <see cref="DateTime" /> values using <see cref="DateTime.UtcNow" />.
///     The generated values are non-temporary, meaning they will be saved to the database.
/// </summary>
public class UtcDateTimeGenerator : ValueGenerator<DateTime>
{
    /// <summary>
    ///     Gets a value indicating whether the values generated are temporary or permanent. This implementation
    ///     always returns false, meaning the generated values will be saved to the database.
    /// </summary>
    public override bool GeneratesTemporaryValues => false;

    /// <summary>
    ///     Gets a value to be assigned to a property.
    /// </summary>
    /// <param name="entry">
    ///     The change tracking entry of the entity for which the value is being generated.
    /// </param>
    /// <returns>
    ///     The value to be assigned to a property.
    /// </returns>
    public override DateTime Next(EntityEntry entry) => DateTime.UtcNow;
}

hades200082 commented 3 years ago

Yep @eegeeZA.

Here's mine -

public class UtcDateTimeValueGenerator : ValueGenerator<DateTime>
{
    public override DateTime Next(EntityEntry entry) => DateTime.UtcNow;

    public override ValueTask<DateTime> NextAsync(EntityEntry entry,
        CancellationToken cancellationToken = new CancellationToken()) =>
        ValueTask.FromResult<DateTime>(DateTime.UtcNow);

    public override bool GeneratesTemporaryValues => false;
}

In your GenerateOnUpdate if I comment out your code and just do...

if (entityEntry.Properties.Any(x => x.Metadata.Name == "LastModified"))
{
     entityEntry.Properties.Single(x => x.Metadata.Name == "LastModified").CurrentValue = DateTime.UtcNow;
}

... everything works, but I'd rather have it driven by the ValueGenerator.

eegeeZA commented 3 years ago

Now that is weird. The code is doing the same kind of action, but through a more direct-hardcoded way. Try adding the inside of the if to the last line of the workaround. Then check in the debugger if

propertyEntry == entityEntry.Properties.Single(x => x.Metadata.Name == "LastModified")
valueGenerator.Next(entityEntry) == DateTime.UtcNow (approximiately) and let it run through. Based on what you said, it should do the correct action after the ValueGenerator. Hopefully, the debugger reveals why.

hades200082 commented 3 years ago

This doesn't work (note the last line):

private void GenerateOnUpdate()
{
    foreach (var entityEntry in ChangeTracker.Entries())
    {
        foreach (var propertyEntry in entityEntry.Properties)
        {
            var property = propertyEntry.Metadata;
            var valueGeneratorFactory =
                property.GetValueGeneratorFactory();
            var generatedOnUpdate = (property.ValueGenerated & ValueGenerated.OnUpdate)
                                    == ValueGenerated.OnUpdate;
            if (!generatedOnUpdate || valueGeneratorFactory == null)
            {
                continue;
            }

            var valueGenerator = valueGeneratorFactory.Invoke(
                property,
                entityEntry.Metadata);
            propertyEntry.CurrentValue = valueGenerator.Next(entityEntry);
            entityEntry.Properties.Single(x => x.Metadata.Name == "LastModified").CurrentValue = DateTime.UtcNow;
        }
    }
}

I verified that the last line of code inside the loops is executed but it still throws a SQL Exception because it's trying to insert a NULL in the LastModified column.

The following code works fine, without throwing the exception, though:

private void GenerateOnUpdate()
{
    foreach (var entityEntry in ChangeTracker.Entries())
    {
        if (entityEntry.Properties.Any(x => x.Metadata.Name == "LastModified"))
        {
            entityEntry.Properties.Single(x => x.Metadata.Name == "LastModified").CurrentValue = DateTime.UtcNow;
        }
    }
}

eegeeZA commented 3 years ago

Something is not adding up. I would have expected the code with the extra (temporarily hard-coded) line would have worked. One of the statements leading up to the extra line might have a side effect that is interfering. The code was working at the time of writing. A suggestion is to try commenting out lines from the bottom up to find the culprit.

hades200082 commented 3 years ago

You're right - something isn't adding up here. I now have this and am still getting the exception:

private void GenerateOnUpdate()
{
    foreach (var entityEntry in ChangeTracker.Entries())
    {
        foreach (var propertyEntry in entityEntry.Properties)
        {
            if (propertyEntry.Metadata.Name == "LastModified")
            {
                propertyEntry.CurrentValue = DateTime.UtcNow;
            }
        }
    }
}

This also fails:

private void GenerateOnUpdate()
{
    foreach (var entityEntry in ChangeTracker.Entries())
    {
        if (entityEntry.Properties.Any(x => x.Metadata.Name == "LastModified"))
        {
            entityEntry.Property("LastModified").CurrentValue = DateTime.UtcNow;
        }
    }
}

And now, even the previously working examples don't work. I'm so confused.

In addition, when I put a break-point on the start of the GenerateOnUpdate() method and inspect the entity properties for each entity in the change tracker (only one at this point) the LastModified property has a legitimate value in both CurrentValue and OriginalValue - both the same... yet I've not set them yet and it still throws the null SQL exception.

I feel like I'm going insane.

eegeeZA commented 3 years ago

You will need to start searching in other places. Verify the mapping configuration/annotations, check the value of PropertyEntry.IsTemporary, check the result of DbEntityValidationException.EntityValidationErrors, and possibly add EF Core logging.

olugt commented 3 years ago

Here is my workaround

Continue to configure the generator with your Fluent API:

builder.Property(x => x.LastModified)
    .HasValueGenerator<UtcDateTimeGenerator>()
    .ValueGeneratedOnAddOrUpdate();

Update your DbContext to add the following methods:

public override int SaveChanges(bool acceptAllChangesOnSuccess)
{
    GenerateOnUpdate();
    return base.SaveChanges(acceptAllChangesOnSuccess);
}

public override Task<int> SaveChangesAsync(
    bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default)
{
    GenerateOnUpdate();
    return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
}

private void GenerateOnUpdate()
{
    foreach (EntityEntry entityEntry in ChangeTracker.Entries())
    {
        foreach (PropertyEntry propertyEntry in entityEntry.Properties)
        {
            IProperty property = propertyEntry.Metadata;
            Func<IProperty, IEntityType, ValueGenerator> valueGeneratorFactory =
                property.GetValueGeneratorFactory();
            bool generatedOnUpdate = (property.ValueGenerated & ValueGenerated.OnUpdate)
                == ValueGenerated.OnUpdate;
            if (!generatedOnUpdate || valueGeneratorFactory == null)
            {
                continue;
            }

            ValueGenerator valueGenerator = valueGeneratorFactory.Invoke(
                        property,
                        entityEntry.Metadata);
            propertyEntry.CurrentValue = valueGenerator.Next(entityEntry);
        }
    }
}

@eegeeZA your workaround is really helpful but it doesn't work. Somewhere, I have this update code...

public async Task<int> UpdateAsync(TEntity entity, CancellationToken cancellationToken)
{
    _dbContext.Set<TEntity>().Attach(entity);
    _dbContext.Entry(entity).State = EntityState.Modified;
    return await _dbContext.SaveChangesAsync(cancellationToken);
}

But column like rowguid that is not present as a property in my entity/model class but present in the entity's configuration as...

entity.Property(e => e.Rowguid).HasColumnName("rowguid")
    .HasValueGenerator<RowGuidValueGenerator>().ValueGeneratedOnAddOrUpdate();

...and uses a value generator as seen above to generate GUID does not get updated with newly generated GUID even though your code at...

propertyEntry.CurrentValue = valueGenerator.Next(entityEntry);

...actually generates a new GUID. It didn't get saved to the database.

Kindly help.

olugt commented 3 years ago

@eegeeZA BTW, the code above is about scaffolded DbContext, just in case it has anything to do with it.

eegeeZA commented 3 years ago

The code for GenerateOnUpdate() will be called before saving, but still goes through the normal EF save method. Try setting the value manually without using a value generator and check if it saves. If the column's value is generated on the DB, setting it on the C# side will not have an effect.

ashlex commented 3 years ago

Hi, if you still haven't found a solution, you can try adding .Metadata.SetAfterSaveBehavior(PropertySaveBehavior.Save) in the property config along with the suggested workaround. overriding-value-generation

thereelaristotle commented 1 year ago

ou are correct at the moment, but surely given the fact that we can specify that we want values generated on update, and there is already a mechanism in place that could do this, it would make sense to do it?

It also doesn't explain why setting a value in overridden SaveChanges still throws a SQL null exception.

@roji There is a way for EF Core to know what I want... Me telling it what I want through the type configuration.

I've told it when I want it to generate a value by using .ValueGeneratedOnAddOrUpdate().

I've told it how to generate that new value by using .HasValueGenerator<UtcDateTimeGenerator>()

So, I've told it both when I want it to generate a value for the property and exactly how to generate that value in C# code. No special database functionality required.

Why doesn't this work for updates as the docs and IntelliSense suggests it should?

From PropertyBuilder.cs:
/// <summary>
/// Configures a property to have a value generated when saving a new or existing entity.
/// </summary>
/// <returns> The same builder instance so that multiple configuration calls can be chained. </returns>
public new virtual PropertyBuilder<TProperty> ValueGeneratedOnAddOrUpdate()
    => (PropertyBuilder<TProperty>)base.ValueGeneratedOnAddOrUpdate();

/// <summary>
///     Configures a property to have a value generated when saving an existing entity.
/// </summary>
/// <returns> The same builder instance so that multiple configuration calls can be chained. </returns>
public new virtual PropertyBuilder<TProperty> ValueGeneratedOnUpdate()
    => (PropertyBuilder<TProperty>)base.ValueGeneratedOnUpdate();
I understand that it's not always possible to have the database do the generation automatically, but there should be no reason that EF can't generate the value based on providing the when and the how in the type configuration.

In addition, using the workaround posted above by @eegeeZA I can see that the ValueGenerator is being called prior to saving changes and yet it's still not including the property in the SQL that's generated for an insert and still doesn't update the column for updates when properties are configured with .ValueGeneratedOnAddOrUpdate().

Amen, this is totally idiotic.

thereelaristotle commented 1 year ago

Seems to me that having the .HasValueGenerator() set on the property at all will cause it to not update the value. So regardless of what code you add to update the value in an overridden Save method it will not send it to the database.

Because I like the cleanliness of FluentAPI or Attributes....I threw this together, which allows you to decorate your properties and have the update happen. I haven't tested the performance but I tried to limit the reflection.

Attribute which has three flags, update on insert, update on update, and which timestamp to use.

    [AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = false)]
    sealed class UpdateTimestampAttribute : Attribute
    {
        private readonly bool _onInsert;
        private readonly bool _onUpdate;
        private readonly bool _useUtc;

        public UpdateTimestampAttribute(bool onInsert, bool onUpdate, bool useUtc)
        {
            _onInsert = onInsert;  
            _onUpdate = onUpdate;
            _useUtc = useUtc;
        }

        public bool OnInsert
        {
            get { return _onInsert; }
        }

        public bool OnUpdate
        {
            get { return _onUpdate; }
        }

        public bool UseUTC
        {
            get { return _useUtc; }
        }

    }

Decoration Example

    [Column("DATE_ADDED", TypeName = "datetime")]
    [UpdateTimestamp(true,false,false)]
    public DateTime DateAdded { get; set; }

    [Column("DATE_MODIFIED", TypeName = "datetime")]
    [UpdateTimestamp(true, true, false)]
    public DateTime DateModified { get; set; }

Override your saves with:

        public override int SaveChanges(bool acceptAllChangesOnSuccess)
        {
            OnSave();
            return base.SaveChanges(acceptAllChangesOnSuccess);
        }

        public override Task<int> SaveChangesAsync(
            bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default)
        {
            OnSave();
            return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
        }

        private void OnSave()
        {
            foreach (EntityEntry entityEntry in ChangeTracker.Entries())
            {
                if (entityEntry.State == EntityState.Modified || entityEntry.State == EntityState.Added)
                {
                    foreach (PropertyEntry propertyEntry in entityEntry.Properties)
                    {
                        UpdateTimestampAttribute? uts = propertyEntry.Metadata.PropertyInfo?.GetCustomAttribute<UpdateTimestampAttribute>();
                        if (uts != null)
                        {
                            if ((uts.OnUpdate == true && entityEntry.State == EntityState.Modified) ||
                                (uts.OnInsert == true && entityEntry.State == EntityState.Added))
                            {
                                try
                                {
                                    if (uts.UseUTC == true)
                                        propertyEntry.CurrentValue = DateTime.UtcNow;
                                    else
                                        propertyEntry.CurrentValue = DateTime.Now;
                                }
                                catch (Exception ex)
                                {
                                    throw new Exception("You can only decorate DateTime objects with UpdateTimestampAttribute", ex);
                                }

                            }
                        }
                    }
                }
            }
        }

samyonr commented 1 month ago

This workaround seems to work well:

public override int SaveChanges(bool acceptAllChangesOnSuccess)
{
    GenerateOnUpdate();
    return base.SaveChanges(acceptAllChangesOnSuccess);
}

public override Task<int> SaveChangesAsync(
    bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default)
{
    GenerateOnUpdate();
    return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
}

private void GenerateOnUpdate()
{
    // Update LastUpdatedAt only if there are changes
    foreach (var entityEntry in ChangeTracker.Entries<IEntity>().Where(e => e.State == EntityState.Modified))
    {
        entityEntry.Property(nameof(IEntity.LastUpdatedAt)).CurrentValue = DateTimeOffset.UtcNow;
        entityEntry.Property(nameof(IEntity.LastUpdatedAt)).IsModified = true;
    }
}

In my case, I know that IEntity has LastUpdatedAt, but this works as well:

public override int SaveChanges(bool acceptAllChangesOnSuccess)
{
    GenerateOnUpdate();
    return base.SaveChanges(acceptAllChangesOnSuccess);
}

public override Task<int> SaveChangesAsync(
    bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default)
{
    GenerateOnUpdate();
    return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
}

private void GenerateOnUpdate()
{
    foreach (var entityEntry in ChangeTracker.Entries().Where(e => e.State == EntityState.Modified))
    {
        if (entityEntry.Properties.Any(x => x.Metadata.Name == "LastUpdatedAt"))
        {
            entityEntry.Property("LastUpdatedAt").CurrentValue = DateTimeOffset.UtcNow;
            entityEntry.Property("LastUpdatedAt").IsModified = true;
        }
    }
}

Note that I'm using DateTimeOffset. If LastUpdatedAt is DateTime, convert to DateTime.UtcNow.

This is combined with the following in protected override void OnModelCreating(ModelBuilder modelBuilder):

entity.Property<DateTimeOffset>(nameof(IEntity.LastUpdatedAt))
    .HasDefaultValueSql("now()")
    .ValueGeneratedOnAdd()
    .Metadata.SetBeforeSaveBehavior(PropertySaveBehavior.Save);

now() is for Postgres. My guess is that I can omit it from here, unless I'm communicating with the DB also not via EF Core, and in that case having a trigger is a better option.

dotnet / efcore