efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Wrong SQL translating from LINQ to SQL looking for all users that have all the specified claims.

Open darcome opened this issue 2 years ago • 9 comments

EF Core version: 7 Database provider: Pomelo.EntityFrameworkCore.MySql 7.0.0-silver.1 with mariadb 10.10.2 Target framework: .NET 7.0 Operating system: Windows 11 IDE: Visual Studio 2022 17.4.3

I have a users table with a many to many relationship with user claims through a join table:

users -> users_userclaims -> user_claims

In order to get all the users that CanEditUsers (1) and CanDeleteUsers (2) I have the following LINQ query:

query = query.Where (u => u.Claims.All (c => filters.ClaimIds.Contains (c.Id)));

which returns the following SQL:

SELECT `u`.`Id`, `u`.`Created`, `u`.`Email`, `u`.`EmailVerificationSuccessful`, `u`.`Enabled`, `u`.`IsArchived`, `u`.`LastLoginAttempt`, `u`.`LastModified`, `u`.`LoginAttempts`, `u`.`PasswordHash`, `u`.`PasswordResetSuccessful`, `u`.`Username`
FROM `users` AS `u`
WHERE NOT EXISTS (
    SELECT 1
    FROM `users_userclaims` AS `u0`
    INNER JOIN `user_claims` AS `u1` ON `u0`.`ClaimsId` = `u1`.`Id`
    WHERE (`u`.`Id` = `u0`.`UserId`) AND `u1`.`Id` NOT IN (1, 2));

which returns also users that have only one claim of the ones selected.

therefore I have rewritten the LINQ query as:

query = query.Where (x => x.Claims	.Select (claim => claim.Id)
					.Where (userClaimId => filters.ClaimIds.Any (claimId => claimId == userClaimId))
					.Count () == filters.ClaimIds.Count);

which returns the following SQL:

SET @__filters_ClaimIds_Count_1 = 2;

SELECT `u`.`Id`, `u`.`Created`, `u`.`Email`, `u`.`EmailVerificationSuccessful`, `u`.`Enabled`, `u`.`IsArchived`, `u`.`LastLoginAttempt`, `u`.`LastModified`, `u`.`LoginAttempts`, `u`.`PasswordHash`, `u`.`PasswordResetSuccessful`, `u`.`Username`
FROM `users` AS `u`
WHERE (
    SELECT COUNT(*)
    FROM `users_userclaims` AS `u0`
    INNER JOIN `user_claims` AS `u1` ON `u0`.`ClaimsId` = `u1`.`Id`
    WHERE (`u`.`Id` = `u0`.`UserId`) AND `u1`.`Id` IN (1, 2)) = @__filters_ClaimIds_Count_1

which returns no users if I use both claims.

As far as I understand, the first LINQ query should work too or am I missing something?

Thanks in advance!

P.S. I am not sure if, if this is a bug, is a Pomelo or EF Core bug... It seems to me that this is a SQL problem and not a provider problem, but of course I not sure.

darcome avatar Jan 08 '23 11:01 darcome

Please submit a minimal, runnable code sample showing the above - it's not clear where the NOT IN comes from in the above SQL.

roji avatar Jan 08 '23 16:01 roji

I hope this is enough.

https://github.com/darcome/EFCoreDemo

I will also attach a screenshot:

image

darcome avatar Jan 08 '23 18:01 darcome

@roji as per your request during the last efcore community standup, I am pinging you on this issue.

Thanks in advance!

darcome avatar Jan 12 '23 10:01 darcome

Note for triage:

Query is:

List<long> ids = new() {1, 2};
var query = context.Users.Where(u => u.Claims.All(c => ids.Contains(c.Id)));

MySQL SQL is:

SELECT `u`.`Id`, `u`.`Created`, `u`.`Email`, `u`.`IsArchived`, `u`.`LastModified`, `u`.`PasswordHash`, `u`.`Username`
FROM `users` AS `u`
WHERE NOT EXISTS (
    SELECT 1
    FROM `users_userclaims` AS `u0`
    INNER JOIN `user_claims` AS `u1` ON `u0`.`ClaimsId` = `u1`.`Id`
    WHERE (`u`.`Id` = `u0`.`UserId`) AND `u1`.`Id` NOT IN (1, 2))

SQL Server SQL is:

SELECT [u].[Id], [u].[Created], [u].[Email], [u].[IsArchived], [u].[LastModified], [u].[PasswordHash], [u].[Username]
FROM [users] AS [u]
WHERE NOT EXISTS (
    SELECT 1
    FROM [users_userclaims] AS [u0]
    INNER JOIN [user_claims] AS [u1] ON [u0].[ClaimsId] = [u1].[Id]
    WHERE [u].[Id] = [u0].[UserId] AND [u1].[Id] NOT IN (CAST(1 AS bigint), CAST(2 AS bigint)))

ajcvickers avatar Jan 12 '23 22:01 ajcvickers

Another not for triage: At least on SQL Server, this query seems to be producing the correct results:

Both users have only claims in the list:
User1
User2
User1 only has one claim in the list, but User2 has both claims in the list:
User2
Both users have some claims not in the list:
Both users have no claim in the list:

And that output also matches what LINQ to Objects returns.

Test code:

RepositoryContext rc = new();

rc.Database.EnsureDeleted();
rc.Database.EnsureCreated();

var u1 = new User
{
    Id = 1,
    Username = "User1",
    Claims =
    {
        new UserClaim {Id = 11, Name = "C1"},
        new UserClaim {Id = 12, Name = "C2"}
    }
};

var u2 = new User
{
    Id = 2,
    Username = "User2",
    Claims =
    {
        new UserClaim {Id = 21, Name = "C2"},
        new UserClaim {Id = 22, Name = "C3"}
    }
};

rc.AddRange(u1, u2);
rc.SaveChanges();
rc.ChangeTracker.Clear();

Console.WriteLine("Both users have only claims in the list:");
RunQuery(new() {11, 12, 21, 22});

Console.WriteLine("User1 only has one claim in the list, but User2 has both claims in the list:");
RunQuery(new() {11, 21, 22});

Console.WriteLine("Both users have some claims not in the list:");
RunQuery(new() {11, 22});

Console.WriteLine("Both users have no claim in the list:");
RunQuery(new() {31, 32});

void RunQuery(List<long> ids)
{
    foreach (var v in rc.Users.Include(e => e.Claims).ToList().Where(u => u.Claims.All(c => ids.Contains(c.Id))))
    {
        Console.WriteLine(v.Username);
    }
}

ajcvickers avatar Jan 12 '23 22:01 ajcvickers

Hi @ajcvickers

I have updated the repo to have a more meaningful test:

Here is a screenshot of the result:

image

Can you please tell me if you have the same result?

My case is slightly different than yours, because I use the same claim for both users:

UserClaim c1 = new UserClaim ()
{
	Id = 1,
	Name = "Administrator"
};

UserClaim c2 = new UserClaim ()
{
	Id = 2,
	Name = "Moderator"
};

User u1 = new User ()
{
	Id = 1,
	Email = "[email protected]",
	Username = "aaa",
	Claims = new List<UserClaim> { c1 }
};

User u2 = new User ()
{
	Id = 2,
	Email = "[email protected]",
	Username = "bbb",
	Claims = new List<UserClaim> { c1 }
};

Thanks in advance

darcome avatar Jan 13 '23 07:01 darcome

@darcome The query is asking for every user where for all claims the given ids contains the claim ID.

So, for both users, there is one claim with ID 1, and ids contains { 1, 2 } so ids contains the given claim ID (1).

ajcvickers avatar Jan 17 '23 14:01 ajcvickers

Sorry @ajcvickers I thought this query

query = query.Where (u => u.Claims.All (c => filters.ClaimIds.Contains (c.Id)));

means that every claim id must be present in the user, therefore if I specify id 1 and 2, both must be present in the u.Claims list.

Can you please tell me what would be the correct LINQ query?

Thanks in advance!

darcome avatar Jan 19 '23 07:01 darcome

@darcome I'm not sure--@roji or @maumar may be able to write such a query.

My runnable test code:

using System.ComponentModel.DataAnnotations.Schema;
using System.Linq.Expressions;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Builders;

RepositoryContext rc = new();

rc.Database.EnsureDeleted();
rc.Database.EnsureCreated();

var u1 = new User
{
    Id = 1,
    Username = "User1",
    Claims =
    {
        new UserClaim {Id = 11, Name = "C1"},
        new UserClaim {Id = 12, Name = "C2"}
    }
};

var u2 = new User
{
    Id = 2,
    Username = "User2",
    Claims =
    {
        new UserClaim {Id = 21, Name = "C2"},
        new UserClaim {Id = 22, Name = "C3"}
    }
};

rc.AddRange(u1, u2);
rc.SaveChanges();
rc.ChangeTracker.Clear();

Console.WriteLine("Both users have only claims in the list:");
RunQuery(new() {11, 12, 21, 22});

Console.WriteLine("User1 only has one claim in the list, but User2 has both claims in the list:");
RunQuery(new() {11, 21, 22});

Console.WriteLine("Both users have some claims not in the list:");
RunQuery(new() {11, 22});

Console.WriteLine("Both users have no claim in the list:");
RunQuery(new() {31, 32});

void RunQuery(List<long> ids)
{
    foreach (var v in rc.Users.Include(e => e.Claims).Where(u => u.Claims.All(c => ids.Contains(c.Id))))
    {
        Console.WriteLine(v.Username);
    }
}

public class BaseEntity
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public long Id { get; set; }
    public DateTime Created { get; set; }
    public DateTime LastModified { get; set; }
    public bool IsArchived { get; set; } = false;
}

public sealed class UserClaim : BaseEntity
{
    public string Name { get; set; } = string.Empty;
}

public class User : BaseEntity
{
    public string Email { get; set; } = string.Empty;
    public string Username { get; set; } = string.Empty;
    public string PasswordHash { get; set; } = string.Empty;
    public virtual List<UserClaim> Claims { get; set; } = new();
}

public sealed class RepositoryContext : DbContext
{
    public DbSet<User>? Users { get; set; }
    public DbSet<UserClaim>? UserClaims { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder builder)
    {
        builder.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Database=AllTogetherNow");
    }
}

ajcvickers avatar Jan 26 '23 15:01 ajcvickers

I thought this query

query = query.Where (u => u.Claims.All (c => filters.ClaimIds.Contains (c.Id)));

means that every claim id must be present in the user, therefore if I specify id 1 and 2, both must be present in the u.Claims list.

No - this only checks whether all the user's claims are contained in the externally-provided ClaimIds filter.

Conceptually, you're looking for something like the following:

rc.Users.Where(u => filters.ClaimIds.All(i => u.Claims.Any(c => c.Id == i)))

In other words, get users which, for all externally-provided ClaimIds, checks that there's any Claim that matches it. Unfortnuately, this LINQ query cannot be translated to regular SQL.

If you know the number of claims in the filter, you can write it like this:

rc.Users.Where(u => u.Claims.Any(c => c.Id == ids[0] && u.Claims.Any(c => c.Id == ids[1]))).ToList();

i.e. gets users where there's a Claim that matches the first ID and a Claim that matches the second ID. Using dynamic expression tree generation, this can be generalized.

roji avatar Jan 27 '23 20:01 roji

ok, thank you for your answer.

darcome avatar Feb 05 '23 21:02 darcome