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

Attribute (Data Annotations) mapping for composite primary keys #11003

Closed
Tracked by #24106 ...
ygoe opened this issue Feb 18, 2018 · 20 comments · Fixed by #27571
Closed
Tracked by #24106 ...

Attribute (Data Annotations) mapping for composite primary keys #11003

ygoe opened this issue Feb 18, 2018 · 20 comments · Fixed by #27571
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported ef6-parity type-enhancement
Milestone

Comments

@ygoe
Copy link

ygoe commented Feb 18, 2018

I'd like to declare a primary key that consists of multiple columns only with the Key attribute, just like for all other keys and as it was supported in EF6. This is the only situation when I have to go to the OnModelCreating method in another file to find out about the basic structure of the entity.

Currently I have to write this scattered code outside of where the entity is defined:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
	base.OnModelCreating(modelBuilder);
	modelBuilder.Entity<MyEntity>()
		.HasKey(e => new { e.KeyColumn1, e.KeyColumn2 });
}

I'd prefer to keep it simple and in a single place, the entity class:

class MyEntity
{
	[Key, Column(Order = 0)]
	public int KeyColumn1 { get; set; }

	[Key, Column(Order = 1)]
	public int KeyColumn2 { get; set; }
}

I could imagine it would be helpful to get rid of the slightly unintuitive Column attribute just for this and define the order where it belongs and is used, the Key attribute, like in [Key(Order = 1)].

I'm wondering why there isn't already an issue for this. I have found one or two issues that might be similar but I don't understand them well enough to consider them a match for this topic. Also I couldn't see this mentioned in the sparse information I could find about the upcoming EF Core 2.1 so I have to assume it's not included in the next version.

@ajcvickers
Copy link
Member

@ygoe We have been tracking the general issue of new attributes for things like this in issue #10111. However, I will leave this open as a specific case we should cover.

@cilerler
Copy link

cilerler commented May 9, 2019

@ajcvickers this issue becomes a big deal upon OData usage, is there any chance to bump the priority up.

@ajcvickers
Copy link
Member

@cilerler Can you provide some more details as to why OData changes things here?

@cilerler
Copy link

cilerler commented May 9, 2019

@ajcvickers in my case database may have new fields, and time to time I need to add those to code. The easy workaround for me is generating the new DbContext through scaffolding and compare it with the previously generated one.

Since scaffolding uses .HasKey instead of [Key], OData is not able to identify those and forcing me to use it in GetEdmModel() as below, which requires manual verification each time.

namespace MyProject.Models
{
    [Table("Items")]
    public partial class Item
    {
        [Column("ID")]
        public long Id { get; set; }
        public string Notes { get; set; }
    }

    [Table("ProcessorSettings")]
    public partial class ProcessorSettings
    {
        [StringLength(50)]
        public string Name { get; set; }  // <= IF it would set the [Key] for this property, I wouldn't need to define it later
        [StringLength(2)]
        public string Code { get; set; }
        [StringLength(500)]
        public string Description { get; set; }
    }
}

EF

// ...
public virtual DbSet<ProcessorSetting> ProcessorSettings { get; set; }
public virtual DbSet<Item> Items { get; set; }
// ...
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    // ...
	modelBuilder.Entity<ProcessorSettings>(entity =>
	{
		entity.HasKey(e => new { e.Name, e.Code });
		entity.Property(e => e.Name)
			.IsUnicode(false)
			.ValueGeneratedNever();
                entity.Property(e => e.Code).IsUnicode(false);
		entity.Property(e => e.Description).IsUnicode(false);
		entity.Property(e => e.Value).IsUnicode(false);
	});

	modelBuilder.Entity<Item>(entity =>
	{
		entity.Property(e => e.Notes).IsUnicode(false);
	});
    //...
}

OData

public static IEdmModel GetEdmModel()
{
    var builder = new ODataConventionModelBuilder();
    // ...
    builder.EntitySet<Item>("Item");  // Automatically locates the `Id` column as `Key`
    builder.EntitySet<ProcessorSetting>("ProcessorSettings").EntityType.HasKey(e => new { e.Name, e.Code });
    // ...
    return builder.GetEdmModel();
}

@ajcvickers
Copy link
Member

@cilerler I don't see any composite keys in that example.

@cilerler
Copy link

cilerler commented May 9, 2019

@ajcvickers updated as .HasKey(e => new { e.Name, e.Code });

@ajcvickers
Copy link
Member

@cilerler Can you show the attributes that would be needed by OData to configure this as a composite key, including how the order of the two parts of the composite key is defined?

@cilerler
Copy link

cilerler commented May 9, 2019

@ajcvickers Just having [Key] on properties would be fine, there is no order so far. You may see more details in here https://stackoverflow.com/a/27538581/439130

@ajcvickers
Copy link
Member

@cilerler That is something we can never support because the order of the properties in the composite key is critical for correctness. I'm actually very surprised that OData allows this given they they presumably have the same problem since the order is also critical to the OData model, but maybe it is constrained in some other way that isn't apparent here.

@smitpatel
Copy link
Member

maybe it is constrained in some other way that isn't apparent here.

Based on stackoverflow post, OData is actually not caring about order at all. It's all about which are the key properties which should be bound when calling Get method. Order of properties does not matter because ODataRoute attribute specify which parameter should be bound with which key property.

@cilerler - You are free to add KeyAttribute to your model to make OData work. The EF model will not complain since Fluent API is overriding data annotation based configuration.
We will never scaffold KeyAttribute when generating EntityType from database until the ordering of properties issue is resolved out.

The OData issue does not change anything in this issue.

@ajcvickers
Copy link
Member

Thanks @smitpatel for clarifying that.

@cilerler
Copy link

cilerler commented May 9, 2019

Thanks, @smitpatel.

We will never scaffold KeyAttribute when generating EntityType from database until the ordering of properties issue is resolved out.

I was under the impression that I was asking if you may make the issue priority, did I sound different?

I got into the details because of @ajcvickers comments. As I mentioned above, it already works the way it is, and I have to do a manual check about the keys each time I do scaffolding which is an error-prone step.

Just wanted to nudge to get attention on this issue. 😁

@smitpatel
Copy link
Member

@cilerler - I just wanted to clarify 2 different aspect. This issue particularly talks about support for KeyAttribute at runtime. i.e. not requiring Fluent API configuration. In your particular case, we can scaffold the code which would work with OData & it would still work with EF Core. It is just matter of scaffolding KeyAttribute which would be ignored by EF. But I was commenting that it would not be good idea to EF to write some code which can be misleading.

If you want to got bit crazy (warning: internal code so it can break between EF core releases):
You can copy this file and change below code and replace the service to get behavior you desire.
https://github.com/aspnet/EntityFrameworkCore/blob/97b7479503f1d187bab5bf1163ef420e92908286/src/EFCore.Design/Scaffolding/Internal/CSharpEntityTypeGenerator.cs#L231-L250

All you need is to remove the check for 1 property. Then your scaffolding code would KeyAttribute for composite key so OData is happy. And it will still generate Fluent API so EF Core is also happy.

@cilerler
Copy link

Thank you for clarifying that and even further providing me a workaround. 🤗

I'm sorry for hijacking this issue; it wasn't my intention.
I just created an issue for what I was asking #15677

@ajcvickers
Copy link
Member

See also the notes on #15677 (comment)

@cmorgan091
Copy link

I'm currently doing a migration from EF6 to EF Core 2.2 and where we use composite PK's extensively and do it all through decoration with the KeyAttribute and ColumnAttribute.

I couldn't bear to do literally hundreds of statements in fluent, so I think I have a workaround, but please note THIS HAS NOT BEEN PROPERLY TESTED YET. Add this to OnModelCreating

// get all composite keys (entity decorated by more than 1 [Key] attribute
foreach (var entity in modelBuilder.Model.GetEntityTypes()
    .Where(t => 
        t.ClrType.GetProperties()
            .Count(p => p.CustomAttributes.Any(a => a.AttributeType == typeof(KeyAttribute))) > 1))
{
    // get the keys in the appropriate order
    var orderedKeys = entity.ClrType
        .GetProperties()
        .Where(p => p.CustomAttributes.Any(a => a.AttributeType == typeof(KeyAttribute)))
        .OrderBy(p => 
            p.CustomAttributes.Single(x => x.AttributeType == typeof(ColumnAttribute))?
                .NamedArguments?.Single(y => y.MemberName == nameof(ColumnAttribute.Order))
                .TypedValue.Value ?? 0)
        .Select(x => x.Name)
        .ToArray();

    // apply the keys to the model builder
    modelBuilder.Entity(entity.ClrType).HasKey(orderedKeys);
}

Let me know if this helps anyone looking for a workaround, as the key attribute was a really useful feature of EF6 IMHO

@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@smitpatel smitpatel removed the good first issue This issue should be relatively straightforward to fix. label Sep 18, 2019
@jez9999
Copy link

jez9999 commented Oct 1, 2019

Another one I'd very much like to see implemented in EF Core. Let's hope this kind of thing gets done soon so EF Core has feature parity with EF classic.

@ajcvickers ajcvickers self-assigned this Nov 18, 2019
@richardlawley
Copy link

@cmorgan091 I am also doing a port from EF6 and found your workaround useful. I understand the reasons for the team not supporting KeyAttribute and ColumnAttribute but for those of us trying to get onto the current version, this makes life a bit eaiser!

@ashishtilak
Copy link

I'm currently doing a migration from EF6 to EF Core 2.2 and where we use composite PK's extensively and do it all through decoration with the KeyAttribute and ColumnAttribute.

I couldn't bear to do literally hundreds of statements in fluent, so I think I have a workaround, but please note THIS HAS NOT BEEN PROPERLY TESTED YET. Add this to OnModelCreating

// get all composite keys (entity decorated by more than 1 [Key] attribute
foreach (var entity in modelBuilder.Model.GetEntityTypes()
    .Where(t => 
        t.ClrType.GetProperties()
            .Count(p => p.CustomAttributes.Any(a => a.AttributeType == typeof(KeyAttribute))) > 1))
{
    // get the keys in the appropriate order
    var orderedKeys = entity.ClrType
        .GetProperties()
        .Where(p => p.CustomAttributes.Any(a => a.AttributeType == typeof(KeyAttribute)))
        .OrderBy(p => 
            p.CustomAttributes.Single(x => x.AttributeType == typeof(ColumnAttribute))?
                .NamedArguments?.Single(y => y.MemberName == nameof(ColumnAttribute.Order))
                .TypedValue.Value ?? 0)
        .Select(x => x.Name)
        .ToArray();

    // apply the keys to the model builder
    modelBuilder.Entity(entity.ClrType).HasKey(orderedKeys);
}

Let me know if this helps anyone looking for a workaround, as the key attribute was a really useful feature of EF6 IMHO

You are life saver @cmorgan091

@ghost
Copy link

ghost commented Aug 10, 2021

Closing the summer of 2021 and still no support for DataAnnotations for this (3 years old feature enhancement item)?

@ajcvickers ajcvickers modified the milestones: Backlog, 7.0.0 Oct 20, 2021
ajcvickers added a commit that referenced this issue Mar 4, 2022
Fixes #11003

This PR introduces a new `[PrimaryKey]` which follows the same pattern as `[Index]` in that it is applied to the entity type class and takes an ordered list of property names. It takes precedence over any `[Key]` attributes on properties, since these may still be needed for OData or other technologies. `PrimaryKey` and `Keyless` cannot be used on the same type.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 4, 2022
ajcvickers added a commit that referenced this issue Mar 9, 2022
Fixes #11003

This PR introduces a new `[PrimaryKey]` which follows the same pattern as `[Index]` in that it is applied to the entity type class and takes an ordered list of property names. It takes precedence over any `[Key]` attributes on properties, since these may still be needed for OData or other technologies. `PrimaryKey` and `Keyless` cannot be used on the same type.
ajcvickers added a commit that referenced this issue Mar 14, 2022
Fixes #11003

This PR introduces a new `[PrimaryKey]` which follows the same pattern as `[Index]` in that it is applied to the entity type class and takes an ordered list of property names. It takes precedence over any `[Key]` attributes on properties, since these may still be needed for OData or other technologies. `PrimaryKey` and `Keyless` cannot be used on the same type.
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview3 Mar 15, 2022
@ajcvickers ajcvickers changed the title KeyAttribute support for composite primary key Attribute (Data Annotations) mapping for composite primary keys Apr 18, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview3, 7.0.0 Nov 5, 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
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported ef6-parity type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants