Support value generation with converters
ajcvickers opened this issue · 42 comments
Current behavior is to throw by default if a converter is associated with a property that will use value generation
However, a value generator to use can be specified:
- On the property
- On the type mapping
- On a converter
A client-side generator is added to the Guid converters so that Guid key scenarios still work. We should find similar "safe" cases and make those work as well.
Original PR: #11479
Consider the Oracle case in #11559 when looking into this.
Note: when working on this, also consider #11970
Is anyone already working on this and is there an ETA? If not, would you accept PR's for this?
@twsI It's in the 3.0 milestone, which means it is tentatively planned for 3.0. I don't believe anyone is working on it right now. We would consider PRs, but it would be helpful if you could provide some details of what you plan to do before submitting a PR, since it's not clear what the behavior here should be or how it should be implemented.
Any clue if this is going to be ready in version 3.0?
@gbrantzos No. The punted-for-3.0 label means that it had to be cut from the release. The current milestone is Backlog which also means that it is not going to happen for the 3.0 release. We will re-assess the backlog following the 3.0 release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.
Is there anything we can do to push this into a release asap?
@twsI The 3.0 release is essentially done. Beyond that, you can vote for this by a thumbs-up 👍 on the issue.
I was poking around with this one to support identity value generation using a strongly typed PK that simply wraps an int value, and I was able to get it working setting the underlying metadata (thus circumventing all the checks hat are done in the SqlServerPropertyAnnotations in my case.
I'm not sure why we're rejecting cases where we have a converter, is there any technical limitation somewhere or is just a not implemented and tested now?
Server side value generation strategy should utilize provider CLR type to identify value generation as mentioned in above comment.
@ilmax - There are some positive case where it works, and many negative cases where it does not so we have blocked almost everything except we are positive scenario works. You could say that not implemented and tested scenario.
@smipatel thanks for coming back to me on this one, I would love to see the scenario I've mentioned above supported, this is essentially to allow strongly typed primary keys, IMHO it's really nice to be able to avoid the "primitive obsession" anti pattern and so far we can only use Guid or generate values ourselves, but actually I was able to have it working perfectly fine just avoiding the checks as mentioned.
Is this something the team may consider to enable?
@ilmax - We can consider it for next major release. Since the issue is marked as needs-design, can you write some design, what would be architecture or logic to figure out value generations with converters. I can present it to team and get final decision. (Also since we are working on 3.1 right now, it would be few months before we may be able to get to it.)
@smitpatel it's not super clear to me what design/architecture are you asking here so I may need some help, essentially what am I proposing is just relaxing some checks because in the simple case everything works, so I think this boils down to identify cases that doesn't work and make sure we're blocking them, while allowing the cases that work "out of box".
@ilmax The fundamental problem is changes in valid key-space and available value generation. Consider these examples:
- A Guid property being converted to a string in the database
- Here it makes sense to use the property type (Guid) to generate a value; it's reasonable to assume that all strings created in this way will be valid in the database and won't collide
- This is easy because Guids can be client-generated
- An int property being converted to a string in the database
- Here it again makes sense to use an int type to generate a value, and assume that any integer generated can be converted to an appropriate string
- However, there is no way to do (purely) client-side generation of ints, so then we can't really do this.
- Also, the database column is string, which can't generate ints and so isn't very useful.
- So what should happen here?
- A string property being converted to a Guid in the database
- Here it makes sense to generate Guids, and then assume strings in the model are in a form that can be parsed as Guid.
- But now we're using to use the column type, rather than the property type, for the generation.
There are many other combinations of property/column types, some of which have reasonable value generation available based on either the column type or model type, as well as many that don't have any reasonable way to generate a value.
So, from a behavior perspective we need to decide:
- Given an arbitrary conversion, can we come up with an approach that is reasonably safe and not unexpected?
- Assuming the answer is no, then can we instead figure out some reasonable special cases where we can do this? Will these special cases be useful?
- For other cases, what API do we need to allow the value generation to be explicitly specified?
@ajcvickers thanks for the very detailed comment, it make sense to block all the cases you're mentioning, I was just thinking about opening up few straightforward ones, like have a guid wrapper class or an short/int/long wrapper class (i.e. the DDD way) so for example have an OrderId class that's wrapping let's say a long and be able to configure a value converter and use the identity generation strategy.
Since I was able to get this one working perfectly fine circumventing all the validation, I was thinking about making this a supported feature, what was required to get this working was configuring the property with identity generation via the metadata to circumvent the checks, implement a custom ValueConverterSelector and a delegating ValueGenerator that wraps a TemporaryLongValueGenerator.
Since I really don't like using int/long/guid as my PK instead I prefer to have dedicated classes, I'd love to have this one supported, so do you think we can make checks smart enough to only open up this case and block the others?
@ilmax Possibly; that's one of the questions that need answering.
@ajcvickers I agree with @ilmax, if I understand the discussion correctly. EF should allow conversions for keys, and should validate configuration for invalid combinations of conversion and value generation. There are many cases where key generation is not used and the key is more than just a number, has some structure and needs to be validated by business code (possible example is SSN). I'd be happy to encapsulate such an ID in a class/struct.
@andriysavin Not sure I follow the motivation for your comment. Are you just agreeing that you would like to see this implemented?
@ilmax I was thinking about this again a couple of nights ago. I think the following might be reasonable:
- If the store type has some kind of value generation available, then by default use it.
- If this results in temporary values, then use the converter to convert these temp values in the normal way. This may fail, in some cases, but I think it's okay to require value generation to be manually switched off in those cases. (I think this should already work, but it needs testing.)
- When the final value is available, make sure that it is converted as part of the normal propagation. (Likewise, this should work.)
- Otherwise, if the model type supports client-side value generation (i.e. the value generator does not return temporary values), then use it.
- Otherwise, value generation should not be configured by default.
Taking the cases from above:
- GUID to string
- String has no value generation, so store-type generation cannot be used.
- Client-side value generator is available for GUID, so it is used.
- Int to string
- String has no value generation, so store-type won't be used
- Int does not support client-side generation, so it won't be used
- No value generation for this case
- String to GUID
- Generation is available for GUID, so it is used.
This also covers the common cases where a store type is being wrapped in, for example, a struct. Since the store-type allows value generation, it will be used.
Removing this from the backlog to allow others on the team to review this proposal.
@ajcvickers Sorry, I possibly read the discussion not accurately enough and repeated what was already said.
@ilmax Team agrees that this is a reasonable starting point.
@ilmax Could you share some code with example where you configured your OrderId property with TemporaryLongValueGenerator to be able to UseHiLo / HasDefaultValueSql / UseIdentityColumn? I've tried multiple different configurations and currently I've a problem with
Unable to cast object of type 'System.Int64' to type 'Identifiers.OrderId'
in this method
@sygnowskip I will try to come push a sample here on github today or tomorrow CET time, essentially my use case was to wrap the underlying long value into it's own class (OrderId) and have EF treat it like it's underlying value to not use guids. I was able to accomplish this circumventing EF validation so I think this is an unsupported feature and I've not tested it enough.
@ajcvickers I can try to dedicate some time this month at enabling the very basic approach described above. Is the team interested in this or does the team want to broaden the scope of this to support more complicate conversions?
@ilmax Doing something basic is fine as long as it isn't a pervasive hack and as long as it doesn't introduce behaviors that would break later when a more complete implementation exists.
@sygnowskip here you can find a simple project using strongly typed id with a generated db value (Sql server identity)
@ajcvickers my plans are just make the some checks more permissive to allow using identity with strongly types id without circumventing EF like I did in the prototype posted above.
@ilmax I would start by writing a bunch of tests for the types of converters you want to cover with different kinds of value generation enabled.
Thanks @ilmax, your solution is not working on EF Core 3.0 (working fine on 2.2.6), but based on your solution I was able to produce working version for EF Core 3.0 😃 Thanks!
EDIT: If anyone is interested, code for natural identifiers with GUID (server side generation) / numeric (database side generation) could be found here https://github.com/sygnowskip/sygnowskip.github.io/tree/feature/support-for-numeric-natural-ids/sources/2019-11-03-natural-identifiers-with-entity-framework-core/NaturalIdentifiers
@ilmax did I understand correctly that you're working on adding support for object IDs to EF Core? I'm willing to help you with implementation, because it's quite important feature for me.
Also, I thought about moving those ValueConverters / ValueSelectors to some nuget package like EntityFrameworkCore.Identifiers and share it with people that are looking for this feature, but maybe it's a better solution to provide support for this directly in EF Core 🤔
@ajcvickers Can you help me get started with the test you're mentioning above? What do you think should I cover?
@ilmax I'm working on some tests in this area when I get the chance to do some coding. I'll ping you when they are in a PR.
@ilmax See KeysWithConvertersTestBase in #19642. This is a new test suite for testing scenarios that use value converters on keys--you could add more tests here. The entities, key types and configuration in those tests don't currently cover value generation, but you should be able to look at the structure of the code and add more types and tests in basically the same way. Make sure to cover:
- Different types of converted key properties (e.g. GUIDs, ints, strings, byte[])
- Client-generation and store-generation
- Generated temporary and permanent values
@ajcvickers I'm having some time lately so I came back to this and started adding some tests. Essentially my goal was to support using strongly typed entity ID with a value generation strategy so have a class (e.g. OrderId) that simply wraps a type for which EF already supports store-generation (e.g. short/int/long and so on).
Right now I'm adding tests with Identity,HighLow and Client-generation for classes that essentially just wraps a several different integral types, just to double check I fully understood your comment here does this part
- GUID to string
- String has no value generation, so store-type generation cannot be used
- Client-side value generator is available for GUID, so it is used.
- Int to string
- String has no value generation, so store-type won't be used
- Int does not support client-side generation, so it won't be used
- No value generation for this case
- string GUID
- Generation is available for GUID, so it is used
means you want test to ensure we don't allow such configurations?
@ilmax Yes. A major part of this issue is about making sure we have a solution that works generally across the different dimensions. I would expect to write much more test code than product code when implementing this--probably in the order of 100 lines of test code per line of product code.
I just want to join the thread and say that I too would love to have this implemented because I really want to use strongly-typed ids for a new project I'm working on.
We tried various workarounds mentioned, but these do not seem to work (anymore). We created our own EFIdentityColumnWithValueConverterWorkaround (for the Npgsql.EntityFrameworkCore.PostgreSQL provider), based on previous input.
Key essence of this workaround is to not only skip IdentityColumn validation by adding annotations directly instead of using UseIdentityColumn and friends but also replace the NpgsqlValueGenerationStrategyConvention, which would also trigger a validation via HasValueGenerationStrategy.
I just used code from @Dresel mentioned in #11597 (comment). Seems to work OK though my foreign keys went from .HasConstraintName("fk_movies_cinemas_cinema_id") to .HasConstraintName("fk_movies_cinemas_cinema_id1") in model snapshot and in migration. I can live with that for now.
Please consider fixing this for 6.0. NHibernate has this since ages AFAIK... And now with C#9 records easy value types make DDD so much fun.
Have anyone got this to work on a strongly typed Id using Guids? I can get it to work on Integer types, but not using Guids in the record struct.
public readonly record struct StronglyTypedGuidId
{
public StronglyTypedGuidId(Guid id)
{
Value = id;
}
public Guid Value { get; }
public static StronglyTypedGuidId Create(Guid entityId) => new(entityId);
}
public readonly record struct StronglyTypedIntId
{
public StronglyTypedIntId(intid)
{
Value = id;
}
public int Value { get; }
public static StronglyTypedIntId Create(int entityId) => new(entityId);
}
protected override void OnModelCreating(ModelBuilder builder)
{
base.OnModelCreating(builder);
builder.Entity<StronglyTypedIntEntity>().Property(entity=>entity.Id).HasConversion<StronglyTypedIntIdValueConverter>().UseIdentityColumn();
builder.Entity<StronglyTypedGuidEntity>().Property(entity => entity.Id).HasConversion<StronglyTypedGuidIdValueConverter>().UseIdentityColumn();
}
If I apply both the following errors is produced when building the migration:
If I apply both the following errors is produced when building the migration:
Unable to create a 'DbContext' of type ''. The exception 'Identity value generation cannot be used for the property 'Id' on entity type 'StronglyTypedGuidEntity' because the property type is 'StronglyTypedGuidId'. Identity value generation can only be used with signed integer properties.' was thrown while attempting to create an instance. For the different patterns supported at design time, see https://go.microsoft.com/fwlink/?linkid=851728
If I comment out the Guid version, the migration builds and the following is produced:
migrationBuilder.AlterColumn<int>(
name: "Id",
schema: "dbo",
table: "StronglyTypedIntIdEntities",
type: "int",
nullable: false,
oldClrType: typeof(int),
oldType: "int")
.Annotation("SqlServer:Identity", "1, 1");
I think you shouldn't use .Useidentity with a guid. Also where do you expect the guid to be generated? Db or from the application?
I can be I misunderstand the documentation, but it says Guids can be auto generated by the provider. If this happens on the client (by the provider) or in the Db, doesn't make a difference to me.
Primary keys
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. Your database provider typically takes care of the necessary configuration; for example, a numeric primary key in SQL Server is automatically set up to be an IDENTITY column.
@lars-lindstrom You need to use ValueGeneratedOnAdd to let EF know that value of this type should be generated. For example:
modelBuilder
.Entity<Blog>()
.Property(entity => entity.Id)
.ValueGeneratedOnAdd()
.HasConversion<StronglyTypedGuidIdValueConverter>();Runnable:
using (var context = new SomeDbContext())
{
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();
context.Add(new Blog { Name = "X" });
context.SaveChanges();
}
using (var context = new SomeDbContext())
{
var blogs = context.Blogs.ToList();
}
public class SomeDbContext : DbContext
{
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder
.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Database=AllTogetherNow")
.LogTo(Console.WriteLine, LogLevel.Information)
.EnableSensitiveDataLogging();
public DbSet<Blog> Blogs => Set<Blog>();
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder
.Entity<Blog>()
.Property(entity => entity.Id)
.ValueGeneratedOnAdd()
.HasConversion<StronglyTypedGuidIdValueConverter>();
}
}
public class StronglyTypedGuidIdValueConverter : ValueConverter<StronglyTypedGuidId, Guid>
{
public StronglyTypedGuidIdValueConverter()
: base(v => v.Value, v => new StronglyTypedGuidId(v))
{
}
}
public readonly record struct StronglyTypedGuidId
{
public StronglyTypedGuidId(Guid id)
{
Value = id;
}
public Guid Value { get; }
public static StronglyTypedGuidId Create(Guid entityId) => new(entityId);
}
public class Blog
{
public StronglyTypedGuidId Id { get; set; }
public string? Name { get; set; }
}@ajcvickers Thx.
I believe what confused me, was the fact of no ValueGeneratedOnAdd was pressent in the migration files. After your post I found it in the modelsnapshot. With the risk of being completely mistanken, I suspect autogenerated Guids are not created in the DB, but client side by EF Core. Correct, or am I completely wrong? :-)
Any way, huge thank you. And great job you and rest the team are doing on EF Core.
@lars-lindstrom By default they are generated on the client. This can be changed to server generating using .HasDefaultValueSql to specify the database function to use--usually newsequentialid.