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

EFCore 7 ValueGeneratedOnUpdate not working on Guid that is not a key #29752

Closed
nicolaegeorgescu opened this issue Dec 3, 2022 · 6 comments
Closed

Comments

@nicolaegeorgescu
Copy link

nicolaegeorgescu commented Dec 3, 2022

We use a long Id as primary key and added a Guid so we can find related data cross databases/microservices.

public class User
{
public long Id { get; set; }
public Guid Username { get; set; }
public string PhoneNumber { get; set; }
....
}

And in the DBContext OnModelCreating we have
modelBuilder.Entity()
.Property(e => e.Username)
.ValueGeneratedOnAdd();

The problem is initializing the db fails with SqlException: Cannot insert the value NULL into column 'Username'

Basically the Username is not autogenerated on add.

Adding .HasDefaultValueSql("NEWID()"); in OnModelCreating is a fake solve, it basically works without ValueGeneratedOnAdd but the id's are random, they have no smart order.

The only way to make it work was to add .HasAlternateKey(e => e.Username) in OnModelCreating.
With this we get nice generated guid's that are in some order so it's probably faster to search them.

Note: we are not using Username as an actual key for a relation inside the table, we use it to find the related entity in a different microservice&database. So we just needed a unique id generated on add, ideally optimized for searching on it.

Expected Behavior

Expected behaviour imo would be to have everything working with just ValueGeneratedOnAdd and without HasAlternateKey. There is no such mention (that i understand) in the documentation

.NET Version

7.0.100

@davidfowl davidfowl transferred this issue from dotnet/aspnetcore Dec 4, 2022
@roji
Copy link
Member

roji commented Dec 4, 2022

Expected behaviour imo would be to have everything working with just ValueGeneratedOnAdd and without HasAlternateKey. There is no such mention (that i understand) in the documentation

This is the expected behavior, see #28024 (comment). Note also the following sentence in our docs:

By convention, non-composite primary keys of type short, int, long, or Guid are set up to have values generated for inserted entities if a value isn't provided by the application.

But I've found several places in the docs which are wrong, will fix.

Adding .HasDefaultValueSql("NEWID()"); in OnModelCreating is a fake solve, it basically works without ValueGeneratedOnAdd but the id's are random, they have no smart order.

If by "smart order" you mean SQL Server sequential IDs (for optimizing indexes over them), then you can use SQL Server's NEWSEQUENTIALID.

Alternatively, you can tell EF to generate the sequential GUIDs on the client side, which is what it does for GUID keys. This is more efficient when the generated GUID is a key (since it may be referenced by a dependent, affecting batching in insertions), but should be the same for non-keys:

modelBuilder.Entity<Blog>().Property(b => b.Guid).HasValueGenerator(typeof(SequentialGuidValueGenerator));

@roji
Copy link
Member

roji commented Dec 4, 2022

Duplicate of #28024

@roji roji marked this as a duplicate of #28024 Dec 4, 2022
@roji
Copy link
Member

roji commented Dec 4, 2022

Docs PR: dotnet/EntityFramework.Docs#4172

@nicolaegeorgescu
Copy link
Author

Thank you for the answer roji!
And yes, by "smart order" i mean SQL Server sequential IDs (for optimizing indexes over them).

I confirm the fix, i commented
//modelBuilder.Entity().HasAlternateKey(e => e.Username);
//modelBuilder.Entity().Property(e => e.Username).ValueGeneratedOnAdd();

And added this:
modelBuilder.Entity().Property(e => e.Username).HasValueGenerator(typeof(SequentialGuidValueGenerator));
modelBuilder.Entity().HasIndex(e => e.Username);

The guid's are created on add and they look sequential!

For the sake of improving documentation, what confused me from this documentation link is:

For keys it specifically sais EF takes care of things.

By convention, non-composite primary keys of type short, int, long, or Guid are set up to have values generated for inserted entities if a value isn't provided by the application.

Here it says that we can also do it for non-key, which is what we want:

We saw above that EF Core automatically sets up value generation for primary keys - but we may want to do the same for non-key properties. You can configure any property to have its value generated for inserted entities as follows:

Database providers may automatically set up value generation for some property types, but others may require you to manually set up how the value is generated.
For example, on SQL Server, when a GUID property is configured as value generated on add, the provider automatically performs value generation client-side, using an algorithm to generate optimal sequential GUID values. However, specifying ValueGeneratedOnAdd on a DateTime property will have no effect

So above as far as i understand it sais that for GUID it works automatically, but for DateTime something else needs to be done.

My 2 cents on Arthur Vickers response, though to be clear i don't claim to have experience with this:

Closed as by-design. Value generators are only selected by convention for key properties, where the semantics of the generated value (i.e. unique) are more fully understood than for an arbitrary property. Even for the GUID case, just because a value is marked as ValueGeneratedOnAdd does not mean that a new GUID should be used in every case--for example, we have seen sceanrios where people have a database default on an FK that gets a well-known GUID value if none is set when a new entity is inserted into the database.

If i add the option ValueGeneratedOnAdd, talking semantically, i expect a value to be generated on add :)
I understand that "what value" can be unclear, but generating no value and leaving it Guid.Empty imo doesn't make it clearer. It's like setting "TurnTheLightOn" and nothing happens because i didn't say which K value. It makes more sense to either force adding a K value in the interface or turning it on with whatever default value for K you want, but some light must be on :) Makes sense? Or perhaps i didnt understand the documentation/response.

Thanks for the answer!

@roji
Copy link
Member

roji commented Dec 4, 2022

Here it says that we can also do it for non-key, which is what we want:

Yes, that's the incorrect part of the docs which I'm fixing in #4172.

Thanks also for your feedback - we'll discuss this.

@roji
Copy link
Member

roji commented Dec 9, 2022

Design decision: we're not going to make any change here. The main reason is that starting to do client-side generation of GUIDs would be a breaking change; if anyone is using ValueGeneratedOnAdd to indicate that a value is being generated by the database (e.g. via a default value), EF will start overriding that by sending client-generated values.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2022
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

3 participants