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

TemporalAll for temporal owned entities mapped to parent table. #29156

Closed
fiseni opened this issue Sep 19, 2022 · 3 comments · Fixed by #32031
Closed

TemporalAll for temporal owned entities mapped to parent table. #29156

fiseni opened this issue Sep 19, 2022 · 3 comments · Fixed by #32031
Assignees
Labels
area-temporal-tables closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@fiseni
Copy link

fiseni commented Sep 19, 2022

Hi All,

There are several related issues, but I couldn't find any issue on this specific topic. I apologize if it's a duplicate.
I read the conversations. I do see the point regarding data inconsistencies in cases where we have to include the navigations, especially from non-temporal tables. But, how about this scenario (which is very common)

  • I have defined an owned entity mapped to the same parent table.
  • The parent entity is defined as temporal.
  • The owned entity is defined as temporal.
  • Both, the parent and owned entity have mapped the period properties to the same columns (it's an EF requirement actually).

Issues:

  • I can't use TemporalAll (with or without projection).
  • I can't get correct data even with a raw query.
  • I can't even exclude the navigation (since it's an owned entity).

It would be great if we can get any solution. I mean even some workaround. I'm open to any ideas from the team and community.

Environment

.NET SDK (reflecting any global.json):
Version: 6.0.400
Commit: 7771abd614

Microsoft.EntityFrameworkCore.SqlServer
Version: "7.0.0-rc.1.22426.7"

Full sample (ready to run). I listed 6 possible options.

using Microsoft.EntityFrameworkCore;
using System.Text.Json;

await SeedAsync();

//await Read_Version1(); // Throws
//await Read_Version2(); // Throws
//await Read_Version3(); // Throws
await Read_Version4();
await Read_Version5();
await Read_Version6();


/// Throws an exception.
/// Navigation expansion is only supported for 'AsOf' temporal operation. For other operations use join manually.
async Task Read_Version1()
{
    using var dbContext = new AppDbContext();

    var customers = await dbContext.Customers
        .TemporalAll()
        .ToListAsync();

    Print(customers);
}

/// Throws with projection too.
/// Navigation expansion is only supported for 'AsOf' temporal operation. For other operations use join manually.
async Task Read_Version2()
{
    using var dbContext = new AppDbContext();

    var customers = await dbContext.Customers
        .TemporalAll()
        .Select(x => new
        {
            Id = x.Id,
            Name = x.Name,
            Email = x.Contact.Email,
            PeriodStart = EF.Property<DateTime>(x, "PeriodStart"),
            PeriodEnd = EF.Property<DateTime>(x, "PeriodEnd")
        })
        .ToListAsync();

    Print(customers);
}

/// Throws an exception (since both the parent and the owned entity contain these columns).
/// The column 'PeriodEnd' was specified multiple times for 'c'.
async Task Read_Version3()
{
    var table = "Customers";
    using var dbContext = new AppDbContext();

    var queryString = dbContext.Customers.ToQueryString();

    var sql = queryString.Replace($"[{table}]", $"[{table}] FOR SYSTEM_TIME ALL");
    var data = await dbContext.Customers
        .FromSqlRaw(sql)
        .AsNoTracking()
        .ToListAsync();

    Print(data);
}

/// Returns incorrect data. It return 3 records, and all of them have "Email1_Updated" value for Email.
/// It takes only the recent values of the owned type and assigns to all records.
/// Also, PeriodStart and PeriodEnd are omitted from the results. 
/// I assume since FromSqlRaw operates on Set<Customer>, and those are shadow fields only?
async Task Read_Version4()
{
    var table = "Customers";
    using var dbContext = new AppDbContext();

    var queryString = dbContext.Customers
        .Select(x => new
        {
            Id = x.Id,
            Name = x.Name,
            Email = x.Contact.Email,
            PeriodStart = EF.Property<DateTime>(x, "PeriodStart"),
            PeriodEnd = EF.Property<DateTime>(x, "PeriodEnd")
        })
        .ToQueryString();

    var sql = queryString.Replace($"[{table}]", $"[{table}] FOR SYSTEM_TIME ALL");
    var data = await dbContext.Customers
        .FromSqlRaw(sql)
        .AsNoTracking()
        .ToListAsync();

    Print(data);
}

/// Same issues as version 4.
async Task Read_Version5()
{
    using var dbContext = new AppDbContext();

    var data = await dbContext.Customers
        .FromSqlRaw("SELECT [c].[Id], [c].[Name], [c].[Contact_Email], [c].[PeriodStart], [c].[PeriodEnd] FROM [Customers] FOR SYSTEM_TIME ALL AS [c]")
        .AsNoTracking()
        .ToListAsync();

    Print(data);
}

/// Finally PeriodStart and PeriodEnd are projected to the result.
/// The Email values are still incorrect.
async Task Read_Version6()
{
    using var dbContext = new AppDbContext();

    var data = await dbContext.Customers
        .FromSqlRaw("SELECT [c].[Id], [c].[Name], [c].[Contact_Email], [c].[PeriodStart], [c].[PeriodEnd] FROM [Customers] FOR SYSTEM_TIME ALL AS [c]")
        .Select(x => new
        {
            Id = x.Id,
            Name = x.Name,
            Email = x.Contact.Email,
            PeriodStart = EF.Property<DateTime>(x, "PeriodStart"),
            PeriodEnd = EF.Property<DateTime>(x, "PeriodEnd")
        })
        .ToListAsync();

    Print(data);
}


async Task SeedAsync()
{
    using var dbContext = new AppDbContext();
    await dbContext.Database.EnsureDeletedAsync();
    await dbContext.Database.EnsureCreatedAsync();
    var customer = new Customer
    {
        Name = "Customer1",
        Contact = new Contact { Email = "Email1" }
    };
    dbContext.Customers.Add(customer);
    await dbContext.SaveChangesAsync();

    customer.Name = "Customer1_Updated";
    await dbContext.SaveChangesAsync();

    customer.Contact.Email = "Email1_Updated";
    await dbContext.SaveChangesAsync();
}

static void Print<T>(T data)
{
    var result = JsonSerializer.Serialize(data, new JsonSerializerOptions
    {
        WriteIndented = true
    });

    Console.WriteLine(result);
}

class AppDbContext : DbContext
{
    public DbSet<Customer> Customers => Set<Customer>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer("Data Source=(localdb)\\MSSQLLocalDB;Initial Catalog=EFTemporalDemo;Integrated Security=SSPI;ConnectRetryCount=0;");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Customer>().ToTable(x => x.IsTemporal());
        modelBuilder.Entity<Customer>().Property<DateTime>("PeriodStart").HasColumnName("PeriodStart");
        modelBuilder.Entity<Customer>().Property<DateTime>("PeriodEnd").HasColumnName("PeriodEnd");

        modelBuilder.Entity<Customer>().OwnsOne(x => x.Contact, cb =>
        {
            cb.ToTable(x => x.IsTemporal());
            cb.Property<DateTime>("PeriodStart").HasColumnName("PeriodStart");
            cb.Property<DateTime>("PeriodEnd").HasColumnName("PeriodEnd");
        });
    }
}

class Customer
{
    public int Id { get; set; }
    public string? Name { get; set; }
    public Contact Contact { get; set; } = default!;
}

class Contact
{
    public string? Email { get; set; }
}
@ajcvickers
Copy link
Contributor

Note for triage: I would have expected this to work after #26451 and #26469.

@sawirth
Copy link

sawirth commented Sep 23, 2022

Hi, I am currently also exploring the temporal table features in the latest RC of ef core and I ran into the same issue. However if I skip the exception in SqlServerNavigationExpansionExtensibilityHelper.ValidateQueryRootCreation I get the correct results but only if I limit my query to owned entities in the same table (usually the ones with OwnsOne). Maybe this helps identifying the cause for this problem.
If I also include properties that are mapped with OwnsMany, I get wrong results. The owned entities get all attached to one single parent object. However the generated SQL looks to be correct so there might be a problem in creating the objects.

Do you know for which next release this is considered?

Query if I only include owned entities that are mapped to the same table as the parent (OwnsOne):

-- PurchasePriceAmount and Currency are mapped to an owned entity
-- Everything after ValidTo is mapped to an owned entity
SELECT [p].[Id]
     , [p].[PurchasePriceAmount]
     , [p].[PurchasePriceCurrencyIso4217Code]
     , [p].[ValidFrom]
     , [p].[ValidTo]
     , [p].[QuantityOnSupplierStock]
     , [p].[ImportedQuantityOnStock]
     , [p].[IsOnSupplierStock]
     , [p].[SupplierRestockDate]
FROM [xyz].[Offer] FOR SYSTEM_TIME ALL AS [p]
WHERE [p].[Id] = CAST(2 AS BIGINT)

Query if I also include owned entities that are mapped in their own table (OwnsMany):

SELECT [p].[Id]
     , [p].[PurchasePriceAmount]
     , [p].[PurchasePriceCurrencyIso4217Code]
     , [p].[ValidFrom]
     , [p].[ValidTo]
     , [p].[QuantityOnSupplierStock]
     , [p].[ImportedQuantityOnStock]
     , [p].[IsOnSupplierStock]
     , [p].[SupplierRestockDate]
     , [d].[Id]
     , [d].[CountryOfDeliveryIsoCode]
     , [d].[DeliveryDurationSourceTypeId]
     , [d].[OfferId]
     , [d].[ValidFrom]
     , [d].[ValidTo]
     , [d].[DurationHighPercentileLowerBound]
     , [d].[DurationHighPercentileUpperBound]
     , [d].[DurationMediumPercentile]
FROM [xyz].[Offer] FOR SYSTEM_TIME ALL AS [p]
      LEFT JOIN [xyz].[OfferDeliveryDuration] FOR SYSTEM_TIME ALL AS [d]
ON [p].[Id] = [d].[OfferId]
WHERE [p].[Id] = CAST(2 AS BIGINT)

@bystfpatTietoEvry
Copy link

Any new information about this issue? Could it at least be possible to allow OwnsOne since it gives the correct value (if you skip the ValidateQueryRootCreation check)?

maumar added a commit that referenced this issue Oct 11, 2023
…ent table.

Similar to JSON entities, owned entities that are mapped to the same table as their owner should be treated as scalars for the purpose of temporal query validation - they are always in sync with the parent entity, so all operations should be allowed for them, not only AsOf.

Fixes #29156
@maumar maumar modified the milestones: Backlog, 8.0.0 Oct 11, 2023
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 11, 2023
maumar added a commit that referenced this issue Oct 11, 2023
…ent table.

Similar to JSON entities, owned entities that are mapped to the same table as their owner should be treated as scalars for the purpose of temporal query validation - they are always in sync with the parent entity, so all operations should be allowed for them, not only AsOf.

Fixes #29156
maumar added a commit that referenced this issue Oct 12, 2023
…ent table. (#32031)

Similar to JSON entities, owned entities that are mapped to the same table as their owner should be treated as scalars for the purpose of temporal query validation - they are always in sync with the parent entity, so all operations should be allowed for them, not only AsOf.

Fixes #29156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-temporal-tables closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
6 participants