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

The order of ExecuteUpdate setters is not preserved #35361

Open
roji opened this issue Dec 20, 2024 · 2 comments
Open

The order of ExecuteUpdate setters is not preserved #35361

roji opened this issue Dec 20, 2024 · 2 comments
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

@roji
Copy link
Member

roji commented Dec 20, 2024

For the following ExecuteUpdate:

await context.Posts
    .ExecuteUpdateAsync(s => s
        .SetProperty(b => b.Content, b => b.Content + " ( Title was " + b.Title  + ")")
        .SetProperty(b => b.Title, b => "New Title"));

... we generate the following SQL:

UPDATE [p]
SET [p].[Title] = N'New Title',
        [p].[Content] = [p].[Content] + N' ( Title was ' + [p].[Title] + N')'
FROM [Posts] AS [p]

Note that the SQL order of the setters is reversed; this is important, as there can be a dependency between the setters. We should preserve the original LINQ ordering.

Originally flagged in PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1776

Full repro
await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

await context.Posts
    .ExecuteUpdateAsync(s => s
        .SetProperty(b => b.Content, b => b.Content + " ( Title was " + b.Title  + ")")
        .SetProperty(b => b.Title, b => "New Title"));

public class BlogContext : DbContext
{
    public DbSet<Post> Posts { 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();
}

public class Post
{
    public int Id { get; set; }
    public string Content { get; set; }
    public string Title { get; set; }
}
@roji
Copy link
Member Author

roji commented Dec 20, 2024

The plot thickens... It turns out that in most databases, the setters always see the row as it was before any updates were made to it; this means that the order of setters is irrelevant:

DROP TABLE IF EXISTS foo;
CREATE TABLE foo (a int, b int);
INSERT INTO foo (a, b) VALUES (0, 0);

UPDATE foo SET a = 10, b = a + 1;
SELECT b FROM foo;

The above returns 1 rather than 11 for most databases (SQL Server, PostgreSQL, SQLite). However, MySQL and MariaDB return 11, meaning that the previous setter is taken into account when evaluating the later setter; ordering matters there. Note that current EF simply inverses the setters, so as a workaround users can simply inverse themselves, starting with the last.

I've taken a look, and this is fixed in #35257, which should get merged shortly. @lauxjpn it would be great if you could add a test on your side to confirm this, something like:

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Update_Where_set_ordering_is_preserved(bool async)
    => TestHelpers.ExecuteWithStrategyInTransactionAsync(
        Fixture.CreateContext, Fixture.UseTransaction,
        async context =>
        {
            var updated = await context.Set<Customer>().ExecuteUpdateAsync(
                setters => setters
                    .SetProperty(c => c.ContactName, "X")
                    .SetProperty(c => c.ContactTitle, c => c.ContactName + "Y"));
            Assert.True(updated > 0);
            Assert.Equal(updated, await context.Set<Customer>().CountAsync(c => c.ContactTitle == "XY"));

            updated = await context.Set<Customer>().ExecuteUpdateAsync(
                setters => setters
                    .SetProperty(c => c.City, "Y")
                    .SetProperty(c => c.ContactName, c => c.City + "X"));
            Assert.True(updated > 0);
            Assert.Equal(updated, await context.Set<Customer>().CountAsync(c => c.ContactName == "YX"));
        });

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2024
@roji roji added closed-no-further-action The issue is closed and no further action is planned. type-bug and removed type-bug closed-no-further-action The issue is closed and no further action is planned. labels Dec 20, 2024
@roji roji added this to the 10.0.0 milestone Dec 20, 2024
@roji roji reopened this Dec 20, 2024
roji added a commit to roji/efcore that referenced this issue Dec 20, 2024
@ChrisJollyAU
Copy link
Contributor

@roji MS Access returns 1 so no issue there either

@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 Dec 21, 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

No branches or pull requests

2 participants