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

calculated getter in entity's owned entity property cannot be added to DbContext #19791

Closed
daniel-jann opened this issue Feb 4, 2020 · 4 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@daniel-jann
Copy link

In an owned entity, when a property's getter returns a calculated value, as asked in #7946, adding the parent entity to the DbContext fails with exception

System.InvalidOperationException: 'Unable to track an entity of type 'MyOwnedEntityType' because primary key property 'MyEntityKeyField2' is null.'

I should also mention that the parent entity has a composite key.

You'll find a runnable project on https://github.com/daniel-jann/EFCorePropertyAccessMode

@ajcvickers
Copy link
Member

@daniel-jann

This is the problematic code:

 public MyOwnedEntityType MyOwnedEntity
 {
   get => new MyOwnedEntityType(1, "A");
   [UsedImplicitly] private set { }
 }

First, the getter will create a new instance every time that it is called. That is, every time EF looks at the property it will appear to have changed.

Second, an owned entity type is still an entity type. Initializing a new instance automatically in every object has ambiguous semantics. Specifically, does it mean never load the entity and always use the values set? Or does it mean this is a placeholder that should be filled by EF from values in the database.

Issue #7946 is about scalar values. I'm not sure having a "computed but persisted" entity instance is possible. We will discuss. It would help to know in more detail why you are trying to do this.

@daniel-jann
Copy link
Author

daniel-jann commented Feb 5, 2020

@ajcvickers I didn't want to go into the details of our own situation in order to keep it simple. But ok, if more details can help understand, here they are.

For context, we're creating a marketplace. So a user can buy from multiple selling enterprises.
And so we have a Cart where products aren't directly linked to the cart, but rather, the Cart owns many CartEnterprises, which in turn owns many CartProducts.
A CartProduct has a Quantity and owns one ProductUnitPrice of type Money, where Money has a Value and a Currency.

Now, we're doing CQRS so this is our command stack and the query stack uses a different DbContext, scaffolded from the database. And as we want the total prices to be persisted in the database so we don't need to compute them in the query stack, what we did was first to create in the CartEnterprise the following property:

public Money ProductPriceTotal => Quantity * GetUnitPriceForQuantity();

(not using UnitPrice directly as we can also have degressive prices based on quantity, and that depends on the selling enterprise, which is the reason why CartProducts are owned by CartEnterprises)
Then, the CartEnterprise has the property:

    public Money ProductsPrice
    {
      get => _cartProducts
        .Select(p => p.ProductPriceTotal)
        .Aggregate((p1, p2) => p1 + p2); // Money implements + operator

      [UsedImplicitly] private set { }
    }

And finally in the Cart:

    public Money TotalProductsPrice
    {
      get => _cartEnterprises
        .Select(e => e.ProductsPrice)
        .ZeroIfEmpty(CurrencyIsoCode) // Our own extension method
        .Sum(); // Our own extension method

      [UsedImplicitly] private set { };
    }

Maybe also an issue we're facing is that we want Money to be an immutable type. So every time a Money field changes, it'll be a new instance of Money, even if we change the implementation to something else than described here.

I didn't put all the details of our implementation, but hopefully that is helpful enough.

@ajcvickers
Copy link
Member

@daniel-jann Thanks for the additional info. This is a situation where a normal property with a value converter is probably more appropriate. This allows the type to be immutable (even a struct) and it will behave like any other property to EF.

The problem is we don't currently support mapping such a property to multiple columns--be sure to vote on #13947 which is tracking this.

I did some experimenting with your code using JSON to serialize the money values to and from a single column. The serialization is not ideal, especially when storing decimals, but it might be possible for you to use something like this as a workaround.

public class MyDbContext : DbContext
{
      private static readonly ILoggerFactory
          Logger = LoggerFactory.Create(x => x.AddConsole()); //.SetMinimumLevel(LogLevel.Debug));

  public DbSet<MyEntity> MyEntities { get; set; }

  protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) 
    => optionsBuilder
      .EnableSensitiveDataLogging()
      .UseLoggerFactory(Logger)
      .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");

  protected override void OnModelCreating(ModelBuilder modelBuilder)
  {
    base.OnModelCreating(modelBuilder);

    modelBuilder.Entity<MyEntity>(builder =>
    {
      builder.HasKey(myEntity => new {myEntity.KeyField1, myEntity.KeyField2});

      // Use a converted property instead of an owned type
      builder
        .Property(c => c.Money)
        .HasConversion(
          v => JsonConvert.SerializeObject(v),
          v => JsonConvert.DeserializeObject<Money>(v));
    });
  }
}

public class MyEntity
{
  public MyEntity(Guid keyField1, string keyField2)
  {
    KeyField1 = keyField1;
    KeyField2 = keyField2;
  }

  public Guid KeyField1 { get; [UsedImplicitly] private set; }
  public string KeyField2 { get; [UsedImplicitly] private set; }

  public Money Money
  {
    get => new Money(1.3342455m, "A");
    [UsedImplicitly] private set { }
  }
}

// Also made this a struct
public struct Money
{
  public Money(decimal amount, string currency)
  {
    Amount = amount;
    Currency = currency;
  }

  public decimal Amount { get; [UsedImplicitly] private set; }
  public string Currency { get; [UsedImplicitly] private set; }
}

public class Program
{
  private static void Main()
  {
    using (var dbContext = new MyDbContext())
    {
      dbContext.Database.EnsureDeleted();
      dbContext.Database.EnsureCreated();

      dbContext.Add(new MyEntity(Guid.NewGuid(), "keyField2"));

      dbContext.SaveChanges();
    }
    
    using (var dbContext = new MyDbContext())
    {
      Console.WriteLine(dbContext.MyEntities.Select(e => e.Money.Amount).Single());
      Console.WriteLine(dbContext.MyEntities.Single().Money.Amount);
    }
  }
}

@daniel-jann
Copy link
Author

@ajcvickers Didn't think about using a value converter. I think we would go that way if multiple columns were supported (upvoted that in the issue you mentioned), because doing a json mapping to a single column would probably prevent us from doing efficient queries in the query stack.

Anyways, for now we ended up with a compromise by making the Money type mutable and:

  • having a backing field for the property
  • having the setter mutate the backing field instead of creating a new instance
  • doing the calculations in the getter, setting the value through the setter
  • and having EF Core use the property instead of the backing field.
public class Money
{
  public Money(decimal value, string currencyIsoCode)
  {
    Value = decimal.Round(value, 2);
    CurrencyIsoCode = currencyIsoCode;
  }

  [Column(TypeName = "decimal(18,2)")]
  public decimal Value { get; [UsedImplicitly] private set; }

  [Column(TypeName = "char(3)")]
  public string CurrencyIsoCode { get; [UsedImplicitly] private set; }

  public Money UpdateFrom(Money money)
  {
    Value = money.Value;
    CurrencyIsoCode = money.CurrencyIsoCode;
    return this;
  }
}

public class Cart {
  public Money TotalProductsPrice
  {
    get
    {
      TotalProductsPrice = _cartEnterprises
        .Select(e => e.ProductsPrice)
        .ZeroIfEmpty(CurrencyIsoCode)
        .Sum();
      return _totalProductsPrice;
    }
    set => _totalProductsPrice = _totalProductsPrice != null ? _totalProductsPrice.UpdateFrom(value) : value;
  }
}

public class MarketplaceDbContext : DbContext {
    protected override void OnModelCreating(ModelBuilder modelBuilder) {
        modelBuilder.Entity<Cart>(builder => builder.UsePropertyAccessMode(PropertyAccessMode.Property));
    }
}

Thanks for your time !

@ajcvickers ajcvickers added closed-no-further-action The issue is closed and no further action is planned. and removed type-bug labels Feb 11, 2020
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

2 participants