dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.68k stars 3.17k forks source link

Add foreign key to existing property with converter and explicit column name #29050

Closed MithrilMan closed 2 years ago

MithrilMan commented 2 years ago

I'm using DDD patterns while using ef core and I've a problem to configure an entity to generate a foreign key in this particular scenario where I have a property with a custom converter and an explicit column name mapping.

Suppose a class like this

public class GasStation : Entity, IAggregateRoot, IPlace
{
   public ImportFeed ImportFeed { get; private set; }
   ...

where ImportFeed is a special class similar to an Enum

public class ImportFeed : Enumeration
{
   public static ImportFeed None = new(1, nameof(None), "No feed.");
   public static ImportFeed ItalianMiseGasStations = new(2, nameof(ItalianMiseGasStations), "Italian Gas Stations");
   public static ImportFeed ItalianMiseGasStationPrices = new(3, nameof(ItalianMiseGasStationPrices), "Italian Gas Stations Prices");

   /// <summary>
   /// The description of the import feed.
   /// </summary>
   public string? Description
   {
      get;
      private set;
   }

   public ImportFeed(int id, string name, string description) : base(id, name)
   {
      Description = description;
   }
}

Enumeration class is similar to enum as concept, basically I never load such entities from DB but define them as static properties but they are anyway stored on Db to maintain relationships between tables.

public abstract class Enumeration : IComparable
{
   public int Id { get; private set; }

   public string Name { get; private set; }

   protected Enumeration(int id, string name)
   {
      Id = id > 0 ? id : throw new DomainException(nameof(id));
      Name = string.IsNullOrWhiteSpace(name) ? throw new DomainException(nameof(name)) : name;
   }

   public override string ToString() => Name;

   public static IEnumerable<T> GetAll<T>() where T : Enumeration
   {
      var fields = typeof(T).GetFields(BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly);

      return fields.Select(f => f.GetValue(null)).Cast<T>();
   }

   public override bool Equals(object? obj)
   {
      if (obj is not Enumeration otherValue) return false;

      var typeMatches = GetType().Equals(obj.GetType());
      var valueMatches = Id.Equals(otherValue.Id);

      return typeMatches && valueMatches;
   }

   public override int GetHashCode() => Id.GetHashCode();

   public static int AbsoluteDifference(Enumeration firstValue, Enumeration secondValue)
   {
      var absoluteDifference = Math.Abs(firstValue.Id - secondValue.Id);
      return absoluteDifference;
   }

   public static bool IsValid<T>(int value) where T : Enumeration
      => Parse<T, int>(value, "value", item => item.Id == value, false) is not null;

   public static T FromValue<T>(int value) where T : Enumeration
      => Parse<T, int>(value, "value", item => item.Id == value)!;

   public static T FromDisplayName<T>(string displayName) where T : Enumeration
      => Parse<T, string>(displayName, "display name", item => item.Name == displayName)!;

   private static T? Parse<T, K>(K value, string description, Func<T, bool> predicate, bool raiseError = true) where T : Enumeration
   {
      var matchingItem = GetAll<T>().FirstOrDefault(predicate);

      if (raiseError && matchingItem == null)
         throw new InvalidOperationException($"'{value}' is not a valid {description} in {typeof(T)}");

      return matchingItem;
   }

   public int CompareTo(object? other) => Id.CompareTo(((Enumeration?)other)?.Id);
}

My entity configuration is something like this:

class GasStationEntityTypeConfiguration : BaseEntityTypeConfiguration<GasStation>
{
   public override string TableName => "GasStations";

   public override void ConfigureColumns(EntityTypeBuilder<GasStation> config)
   {
      config.HasIndex(b => new { b.ImportFeed, b.ImportFeedEntryId });

      config.Property(b => b.Name)
          .IsRequired();

      config.Property(b => b.Location)
         .IsRequired()
         .HasColumnName("Location")
         .HasConversion(new GeoPointConverter());

      config.Property(b => b.ImportFeed)
         .IsRequired()
         .HasColumnName("ImportFeedId")
         .HasConversion(b => b.Id, b => Enumeration.FromValue<ImportFeed>(b));

      config.HasOne(b => b.ImportFeed)
          .WithMany()
          .HasForeignKey(b => b.ImportFeed)
          .IsRequired();
   }
}

base class just define the Id mapping and hide some properties that doesn't have to be mapped.

Now as you see I have a custom converter for the ImportFeed property because I don't want to load it from db and instead I just store the "enum id" and doing so I specify that the ColumnName for the ImportFeed property is ImportFeedId.

What I want to do is to create a foreing key to the ImportFeeds table but with the code above, it generates the error

The property or navigation 'ImportFeed' cannot be added to the entity type 'GasStation' because a property or navigation with the same name already exists on entity type 'GasStation'.

I tried different combinations but it keeps generating this problem, the only way to avoid that is to change

config.HasOne(b => b.ImportFeed)
   .WithMany()
   .HasForeignKey(b => b.ImportFeed)
   .IsRequired();

to

config.HasOne<ImportFeed>()
   .WithMany()
   .HasForeignKey("ImportFeedId")
   .IsRequired();

like I tried to do as first because I don't need a navigation property for that foreign key since I've already one. The problem is that this snippets generates a new column ImportFeedId1 and create a foreign key on that...

image

How am I supposed to create a foreign key using code-first (without attributes) in this scenario? Is this something not supported?

I know I can manually add it in a custom migration but of course I don't want to do that Thanks.

ajcvickers commented 2 years ago

@MithrilMan ImportFeed is being configured as both a related entity type with config.HasOne(b => b.ImportFeed) and as a property of the entity type with config.Property(b => b.ImportFeed). It's not clear to me which of these is intended from your description.

MithrilMan commented 2 years ago

@ajcvickers I want to keep my property definition

config.Property(b => b.ImportFeed)
         .IsRequired()
         .HasColumnName("ImportFeedId")
         .HasConversion(b => b.Id, b => Enumeration.FromValue<ImportFeed>(b));

while at the same time having a foreign key between the ImportFeedId colum mapped to that property, with the external ImportFeeds table (by it's key Id property)

ajcvickers commented 2 years ago

@MithrilMan If there is an "ImportFeeds" table, then there will need to have an entity type for it. That entity type can abstract what is in the table as you see fit. Putting a value converter on a navigation property is not supported.

MithrilMan commented 2 years ago

@ajcvickers

Putting a value converter on a navigation property is not supported.

That's unfortunate, I won't change my design for that, thanks

MithrilMan commented 2 years ago

Is there any reason why it's not allowed to support foreign keys like Indexes? An API like

config.HasForeignKey<GasStation, ImportFeed>(gasStation => gasStation.ImportFeed, feed => feed.Id);

or

config.HasForeignKey<GasStation, ImportFeed>("ImportFeedId", "Id");
ajcvickers commented 2 years ago

@MithrilMan I don't follow what that API is supposed to do. Can you explain in more detail?

MithrilMan commented 2 years ago

@MithrilMan I don't follow what that API is supposed to do. Can you explain in more detail?

Since my property definition is defining a converter and specify a column name, the API I'm talking about should behave this way: 1st API: config.HasForeignKey<GasStation, ImportFeed>(gasStation => gasStation.ImportFeed, feed => feed.Id);

Create a foreign key on the GasStation table, from the source property represented by my ImportFeed property (that has the name ImportFeedId given my property configuration that used .HasColumnName("ImportFeedId") ) to the destination table ImportFeed on it's column pointed by property Id (so Id by default)

2nd API: config.HasForeignKey<GasStation, ImportFeed>("ImportFeedId", "Id"); basically it's the same but instead having efcore to obtain the column names by analyzing the model property configurations, I explicitly state them as literals

Both should generaet an SQL similar to

ALTER TABLE [dbo].[GasStations] ADD FOREIGN KEY ([ImportFeedId]) REFERENCES [dbo].[ImportFeeds] ([Id])
ajcvickers commented 2 years ago

It's problematic to have something reference another table without having configuration for that table. I don't think we will add any API that involves implicitly doing something in another table.

MithrilMan commented 2 years ago

@ajcvickers The table itself in my scenario is configured (with an Id property as key, int, not autonumeric) and I populate it on database seeding. I just don't access it explicitly in my code because it's handled as an enum loaded by code and not from database access and so it doesn't have any lazy/early access through other entities.

The foreign key I want to create is to enforce data consistency at database level and maybe prevent accidental deletions

About the pattern itself, it's very close to what Microsoft already explain in its doc here: https://docs.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/enumeration-classes-over-enum-types

ajcvickers commented 2 years ago

@MithrilMan In my opinion, the patterns in those docs aren't that great. In particular, I don't think the enum object provides real value, and when taken in conjunction with the complexity it adds to the codebase, I think the value is negative. In other words, at least as implemented there, it feels like an anti-pattern to me. See https://github.com/dotnet/efcore/issues/29008 for some additional discussion on this.

MithrilMan commented 2 years ago

@ajcvickers I somewhat agree with you about the quality fo these docs, for this one explicitly, our implementation is similar to what's stated in the issue you refer to and it works well in every scenario we had tested and doesn't involve change detector at all

Enumeration classes don't add any negative values to us and I don't see how it can be considered an anti-pattern (using Enum types would be a problem instead), it's super useful because the written business rules that make use of such enumerations makes the code clear (using Enum type is instead a bad choice) and if foreign key were possible, would allow a database to enforce relational constraints between tables and have cascade behaviors

Beside the different opinion we could have about the Enumeration classes, what are the cons of having an API like I explained above? Is it complicate to implement it? Does the technical limitations comes actually from the value converter?

ajcvickers commented 2 years ago

The limitation is the implicit mapping and the implicit table. These tend to be pits of failure since they either can't be customized, or eventually require explicit mapping. What you are trying to do is also very unusual and not something I have ever seen requested before. It would take a lot of design and analysis to figure out all the consequences, and given that I don't think it even adds significant value, this is something we're not going to do.

MithrilMan commented 2 years ago

I leave a stackoverflow discussion about this issue I opened few days ago, an user gave a workaround I've still to look into https://stackoverflow.com/questions/73683157/how-to-explicitly-add-a-foreign-key-to-a-property-with-a-converter/73710263?noredirect=1#comment130185844_73710263

I don't want a tool limitation to drive software architecture/design, actually EF in my project is an infrastructure detail hidden behind a repository pattern so it's just a tool I could change if it doesn't fit the need.

At the moment I'm fine with this limitation so I could either ignore fk or give a look at that work around or I'll create a manual MigrationOperation to create "manual fk" like I had to do to create spatial indexes on custom Location type (something else that I suspect isn't requested that much)

Just to know, in case I wanted to implement a package to implement this kind of "manual fk", does ef core offers an extension point to create a foreign key in any supported db ?

If no and we don't have anything to add to the discussion you may close the issue