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

ExecuteUpdate with OwnsOne property in target table fails in efcore 7.0.12 #32053

Closed
anoordover opened this issue Oct 14, 2023 · 5 comments · Fixed by #32773
Closed

ExecuteUpdate with OwnsOne property in target table fails in efcore 7.0.12 #32053

anoordover opened this issue Oct 14, 2023 · 5 comments · Fixed by #32773
Assignees
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@anoordover
Copy link

See my repository https://github.com/anoordover/OracleDbDemo

The methods ExecuteUpdateWithSelectNew1 en ExecuteUpdateWithSelectNew3 fail

The method ExecuteUpdateWithSelectNew2 succeeds

@ajcvickers
Copy link
Contributor

@anoordover ExecuteUpdate can only be used to update columns in a single table. So something like this cannot be expected to work:

var r = db.Credits.Where(c => c.Id == 1)
    .Select(c => new
    {
        credit = c,
        declaration = db.Declarations
            .First(d => d.Reference == c.DeclarationReference)
    })
    .ExecuteUpdate(calls => calls.SetProperty(
        c => c.credit.DeclarationId,
        c => c.declaration.Id));

Because declaration comes from the Declarations table, while credit comes from the Credits table.

@anoordover
Copy link
Author

You can use ExecuteUpdate to update columns in one table with a column fetch from another table.
See the update on the Declaration table where the update succeeds because there is no OwnsOne property in de target table.
@roji adviced me to try using 8.0 rc2 of ef core. Because I was using Oracle which doesn't have a 8.0 implementation available I created two branches doing the same tests using NpgSql on 8.0 rc2.

When you have a work-station that has docker available you can run the tests yourself. I use a testcontainer (https://dotnet.testcontainers.org/modules/postgres/) to run the code (the main branch uses an Oracle testcontainer).

@ajcvickers
Copy link
Contributor

@roji I think you're going to need to investigate here eventualy. There are five different scenarios. There are different failures for some on SQL Server and Oracle, and the same failures for others. Also, some scenarios start failing on SQL Server after #30528 was merged, which implies that #30528 has regressed something, but I'm not sure if those queries were supposed to work in the first place.

@roji
Copy link
Member

roji commented Nov 2, 2023

Sure thing. I'm adding this to my investigation list for when I have a bit more time - but realistically only after 8.0 is released I think...

@roji roji changed the title ExecuteUpdate with OnwsOne property in target table fails in efcore 7.0.12 ExecuteUpdate with OwnsOne property in target table fails in efcore 7.0.12 Nov 8, 2023
@roji
Copy link
Member

roji commented Jan 10, 2024

@anoordover thanks for your patience and sorry it took so long to get around to this.

There's a lot going on above - it's always a good idea to submit a short, minimal console program that demonstrates a single failing scenario (and ideally on a provider we maintain, as you've indeed done for PostgreSQL above). That can help cut down the time needed to investigate etc.

In any case, I've confirmed a failure that's reproducible with the following minimal console program:

ctx.Credits.Where(c => c.Id == 1)
    .Join(ctx.Declarations,
        c => c.DeclarationReference,
        d => d.Reference,
        (credit, declaration) => new {credit, declaration})
    .ExecuteUpdate(calls => calls.SetProperty(
        c => c.credit.DeclarationId,
        c => c.declaration.Id));
Full minimal repro
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

ctx.Credits.Where(c => c.Id == 1)
    .Join(ctx.Declarations,
        c => c.DeclarationReference,
        d => d.Reference,
        (credit, declaration) => new {credit, declaration})
    .ExecuteUpdate(calls => calls.SetProperty(
        c => c.credit.DeclarationId,
        c => c.declaration.Id));

public class BlogContext : DbContext
{
    public virtual DbSet<Credit> Credits { get; set; }
    public virtual DbSet<Declaration> Declarations { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
        => modelBuilder.Entity<Credit>().OwnsOne(c => c.Period);
}

public class Credit
{
    public long Id { get; set; }
    public string Reference { get; set; }
    public string DeclarationReference { get; set; }
    public Period Period { get; set; }
    public long? DeclarationId { get; set; }
    public Declaration? Declaration { get; set; }
}

public class Period
{
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }
}

public class Declaration
{
    public long Id { get; set; }
    public string Reference { get; set; }
}

The cause is essentially the same as #30528 - the fact that entity has an owned entity causes nav expansion to inject an IncludeExpression into the query tree, which causes ExecuteUpdate to fail (and also is meaningless, since ExecuteUpdate doesn't query for anything). The fix in #30571 was to prune out the IncludeExpression, but this was done only when its found at the top of the shaper tree; but because of the join in the query above, the shaper contains an anonymous object with the IncludeExpression nested inside. The fix here is to visit recursively, pruning IncludeExpressions everywhere, and not only at the top.

Note that complex properties don't cause IncludeExpressions to get injected, which is why there's no bug there.

@anoordover if there are other problems you wanted to raise here, please open separate issues with minimal repros for those - I'll use this issue to track the above only.

roji added a commit to roji/efcore that referenced this issue Jan 10, 2024
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 10, 2024
@roji roji added this to the 9.0.0 milestone Jan 10, 2024
roji added a commit to roji/efcore that referenced this issue Jan 11, 2024
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
@roji roji modified the milestones: 9.0.0-preview1, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants