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

Missing parentheses when using Expression.Or over bool (instead of OrElse) #30181

Closed
xhri opened this issue Jan 31, 2023 · 4 comments · Fixed by #30454
Closed

Missing parentheses when using Expression.Or over bool (instead of OrElse) #30181

xhri opened this issue Jan 31, 2023 · 4 comments · Fixed by #30454
Assignees
Labels
area-query 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

@xhri
Copy link

xhri commented Jan 31, 2023

Description

When EntityFrameworkCore generates SQL query it misses important brackets, when I use Expression.Lambda.
In my example it can cause situation, when all additional .Where() statements are being ignored.

Code

Expression<Func<TestEntity, bool>> expression1 = x => x.Name == "name" && x.Value == "value1";
Expression<Func<TestEntity, bool>> expression2 = x => x.Name == "name" && x.Value == "value2";
var param = Expression.Parameter(typeof(TestEntity), "x");
var body = Expression.Or(
        Expression.Invoke(expression1, param),
        Expression.Invoke(expression2, param)
    );
var logicSum = Expression.Lambda<Func<TestEntity, bool>>(body, param);

var res = context.TestEntities.Where(logicSum).Where(x => x.Id == 2).FirstOrDefault();

Generated SQL

SELECT t."Id", t."Name", t."Value"
FROM "TestEntities" AS t
WHERE (t."Name" = 'name' AND t."Value" = 'value1') OR (t."Name" = 'name' AND t."Value" = 'value2') AND t."Id" = 2
LIMIT 1

Issue with the SQL

As You can see, it's missing brackets, and now, when first of the two predicates is true, it will skip our second .Where clause at all.

Code description

Of course in this case use of this syntax is unnecessary, but it is just simplified example to illustrate the problem. My original issue was caused in code, when I needed to generate predicate based on list of pairs of values. That's the only way to create logic sum for generic Expression<Func<>> I could find. If there is a simpler way, I'm happy to hear it out(although it doesn't change the fact, that here's a bug).

On the contrary

If Expression.Lambda construction is not used, and describe predicates explicitly, this issue doesn't happen.

Code

var res2 = context.TestEntities.Where(x => (x.Name == "name" && x.Value == "value1") || (x.Name == "name" && x.Value == "value2")).Where(x => x.Id == 2).FirstOrDefault();

Generated SQL

SELECT t."Id", t."Name", t."Value"
FROM "TestEntities" AS t
WHERE ((t."Name" = 'name' AND t."Value" = 'value1') OR (t."Name" = 'name' AND t."Value" = 'value2')) AND t."Id" = 2
LIMIT 1

Conclusion

That's exactly how the original SQL should look like, when using Expression.Lambda.

Provider and version information

EF Core version: 7.0.2
Database provider: Microsoft.EntityFrameworkCore.PostgreSQL
Target framework: .NET 6.0
Operating system: Windows 10
IDE: Microsoft Visual Studio Professional 2022 Version 17.2.5

@roji
Copy link
Member

roji commented Jan 31, 2023

Thanks, I can indeed see this (repro for SQLite below, SQL Server hides this with CASE blocks).

Although your code should work (i.e. I agree there's an EF bug), you should be using Expression.OrElse rather than Expression.Or. When doing that, the parentheses are fine.

Minimal repro for SQLite
await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

Expression<Func<TestEntity, bool>> expression1 = x => x.Name == "name" && x.Value == "value1";
Expression<Func<TestEntity, bool>> expression2 = x => x.Name == "name" && x.Value == "value2";
var param = Expression.Parameter(typeof(TestEntity), "x");
var body = Expression.OrElse(
    Expression.Invoke(expression1, param),
    Expression.Invoke(expression2, param)
);
var logicSum = Expression.Lambda<Func<TestEntity, bool>>(body, param);

var res = context.TestEntities.Where(logicSum).Where(x => x.Id == 2).FirstOrDefault();

public class BlogContext : DbContext
{
    public DbSet<TestEntity> TestEntities { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlite("Data Source=/tmp/foo.sqlite")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
    }
}

public class TestEntity
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string Value { get; set; }
}

@ajcvickers ajcvickers added this to the Backlog milestone Feb 1, 2023
@roji roji changed the title Missing crucial brackets in SQL Query, when generating WHERE clause from Expression.Lambda Missing parentheses when using Expression.Or over bool (instead of OrElse) Mar 9, 2023
@roji
Copy link
Member

roji commented Mar 9, 2023

@maumar this would get fixed by the new parentheses work right? #27177

@maumar
Copy link
Contributor

maumar commented Mar 10, 2023

@roji this one is likely still broken - but will add this exact test to the regression suite to make sure

@roji roji removed this from the Backlog milestone Mar 10, 2023
@roji
Copy link
Member

roji commented Mar 10, 2023

OK, clearing out the milestone - if it's still broken let's fix it.

maumar added a commit that referenced this issue Mar 11, 2023
…n.Or over bool (instead of OrElse)

Just adding regression test, bug already fixed by roji in the parentheses work.

Resolves #30181
@maumar maumar added this to the 8.0.0 milestone Mar 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 Mar 11, 2023
@ghost ghost closed this as completed in #30454 Mar 11, 2023
ghost pushed a commit that referenced this issue Mar 11, 2023
…n.Or over bool (instead of OrElse) (#30454)

Just adding regression test, bug already fixed by roji in the parentheses work.

Resolves #30181
@ajcvickers ajcvickers modified the milestones: 8.0.0, 8.0.0-preview3 Mar 13, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview3, 8.0.0 Nov 14, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query 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
4 participants