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

Count() does not equal number of results if included navigation property does not match query filter #19649

Closed
tmthill opened this issue Jan 20, 2020 · 19 comments
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

@tmthill
Copy link

tmthill commented Jan 20, 2020

If a query selects a property from a related entity reached by a required navigation property, but the related entity is excluded by a query filter, query.Count() will not match query.ToList().Count. Actualizing the results with ToList() will exclude the row from its results, as the related entity is filtered out, but Count() does not exclude the result even though the query is selecting a member of the navigation property explicitly.

We use this functionality to build search results one page at a time: build a filtered and projected IQueryable<TEntity> query, use query.Count() to get the total number of results, then query.OrderBy(orderBy).Skip(skip).Take(take).ToList() to resolve a subset of the results. Our actual implementation has a much more complicated projection thanks to AutoMapper, but as recently as EF Core 2.2.6, calling Count() on the queryable returned the same cardinality as the result set (indeed, the code generated for SQL Server has the same FROM and JOIN clauses and differs only in the SELECT lists).

Steps to reproduce

using Microsoft.EntityFrameworkCore;
using System.Linq;
using Xunit;

namespace EntityFrameworkCoreTests
{
    class Entity
    {
        public int EntityId { get; set; }
        public int RelatedEntityId { get; set; }
        public virtual RelatedEntity RelatedEntity { get; set; }
    }

    class RelatedEntity
    {
        public int RelatedEntityId { get; set; }
        public bool IsDeleted { get; set; }
        public string SomeProperty { get; set; }
    }

    class TestContext : DbContext
    {
        public TestContext(DbContextOptions<TestContext> options) : base(options) { }

        public DbSet<Entity> Entities { get; set; }
        public DbSet<RelatedEntity> RelatedEntities { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<RelatedEntity>()
                .HasQueryFilter(relatedEntity => !relatedEntity.IsDeleted);
        }
    }

    public class CountDoesNotMatchTests
    {
        [Fact]
        public void Queryable_Count_should_match_ToList_Count()
        {
            using var context = new TestContext(
                new DbContextOptionsBuilder<TestContext>()
                    .UseInMemoryDatabase(GetType().Name)
                    .Options);

            var entity = new Entity
            {
                RelatedEntity = new RelatedEntity
                {
                    IsDeleted = true
                }
            };

            context.Add(entity);
            context.SaveChanges();

            var query = context.Entities
                .AsNoTracking()
                .Select(e => e.RelatedEntity.SomeProperty);

            Assert.Equal(query.ToList().Count, query.Count());
            // query.ToList().Count is zero; query.Count() is one
            // As of EF Core 2.2.6, they were both zero
        }
    }
}

The code above uses InMemory for brevity, but Sqlite exhibits the same behavior and generates the following query for query.ToList(), returning no rows:

SELECT "t"."SomeProperty"
FROM "Entities" AS "e"
INNER JOIN (
    SELECT "r"."RelatedEntityId", "r"."IsDeleted", "r"."SomeProperty"
    FROM "RelatedEntities" AS "r"
    WHERE NOT ("r"."IsDeleted")
) AS "t" ON "e"."RelatedEntityId" = "t"."RelatedEntityId"

And for query.Count(), returning a count of 1:

SELECT COUNT(*)
FROM "Entities" AS "e"

I also noticed that, if I used .Include(e => e.RelatedEntity) instead of .Select(e => e.RelatedEntity.SomeProperty) in the test above, EF Core 2.2.6 and 3.1.1 both return 1 for query.Count() and 0 for query.ToList().Count. This is not currently causing us issues, but it looks similar yet behaves consistently across versions, so I thought it would be worth mentioning.

Further technical details

EF Core version: 3.1.1
Database provider: Microsoft.EntityFrameworkCore.InMemory 3.1.1, Microsoft.EntityFrameworkCore.Sqlite 3.1.1, Microsoft.EntityFrameworkCore.SqlServer 3.1.1
Target framework: .NET Core 3.1
Operating system: Windows 10
IDE: Visual Studio 2019 16.4.2

@ajcvickers
Copy link
Member

ajcvickers commented Jan 21, 2020

Note for team: repros on master.

/cc @maumar @roji

@maumar maumar self-assigned this Jan 22, 2020
@maumar
Copy link
Contributor

maumar commented Jan 22, 2020

We should be using left join here instead

@alirezanet
Copy link

alirezanet commented Jan 31, 2020

I think I have the same problem #19764
so +1

@maumar
Copy link
Contributor

maumar commented Feb 4, 2020

discrepancy comes from the fact that when navigation rewrite processes Count, we have optimization in place which just applies count on the source directly, completely ignoring pending selector. Assumption is that the count should be the same.

However, if the projection contains required navigation, and the entity that is on the other side of that navigation is filtered out by query filter, we remove the source to maintain referential integrity.

@maumar
Copy link
Contributor

maumar commented Feb 4, 2020

Filed #19801 to introduce model validation for this case as it seems counter-intuitive and can lead to issues.

@smitpatel smitpatel removed this from the 5.0.0 milestone Feb 5, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Feb 7, 2020
@maumar
Copy link
Contributor

maumar commented May 22, 2020

per design discussion, added a model validation phase that warns against using required navigations where required side has the query filter and the optional side doesn't (tracked by #19801). Also improved docs to illustrate the issue, closing.

@maumar maumar closed this as completed May 22, 2020
@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 May 22, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview6 Jun 1, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview6, 5.0.0 Nov 7, 2020
@hakanaltindis
Copy link

hakanaltindis commented Nov 17, 2022

I have same problem too. I get this problem with these versions:
Target Framework: Net6.0
OS: Windows 10
Microsoft.EntityFrameworkCore.Relational: 6.0.8
Npgsql.EntityFrameworkCore.PostgreSQL: 6.0.6

Is there any roadmap to solve this issue?

-- Update --
I think this issue still out there. This topic should not be closed.

@roji
Copy link
Member

roji commented Nov 17, 2022

@hakanaltindis please open a new issue with a minimal, runnable code sample that shows the problem occurring.

@hakanaltindis
Copy link

@hakanaltindis please open a new issue with a minimal, runnable code sample that shows the problem occurring.

I opened new issue

@roji
Copy link
Member

roji commented Nov 17, 2022

@hakanaltindis sorry, I should have read this issue more thoroughly before answering you.

When running your code repro in #29599, I'm seeing the proper warning emitted:

Entity 'Customer' has a global query filter defined and is the required end of a relationship with the entity 'Activity'. This may lead to unexpected results when the required entity is filtered out. Either configure the navigation as optional, or define matching query filters for both entities in the navigation. See https://go.microsoft.com/fwlink/
?linkid=2131316 for more information.

This points to this doc section, which explains what's going on and provides pretty much the same code sample as your own. So everything seems to be working as it was designed to - or are you seeing something different?

@hakanaltindis
Copy link

Hi @roji ,

I read this topic before. And it says you may encounter unexpected results because global query filter is included in automatically. But I am aware of it and my all results come my expected.

The point what I do not get that Why does EF core not add the global query filters to get number of records.

@roji
Copy link
Member

roji commented Nov 17, 2022

The point what I do not get that Why does EF core not add the global query filters to get number of records

I'm not following... Your posted SQL in #29599 (comment) clearly shows Where c.IsDeleted = false, which is your query filter, no? Please write clearly which LINQ query you're running, which SQL you're expecting to see, and which SQL you're actually seeing.

@hakanaltindis
Copy link

hakanaltindis commented Nov 17, 2022

When I call this LINQ _dbContext.Activities.Include(a => a.Customer).Count();, it generates this sql query Select Count(*) From Activity.

But when I call this LINQ _dbContext.Activities.Include(a => a.Customer).ToList();, please careful, it ends with ToList(). After that it generates this query

Select a.id, a.Name, c.Name
From Activity as a
	Inner Join Customer As c On (a.CustomerId = c.Id)
Where c.IsDeleted = false

What I try to say that If I want to get records/rows, everything works perfect. But If I just want to get number of records, it is not working correctly.

@roji
Copy link
Member

roji commented Nov 17, 2022

@hakanaltindis that behavior - and the discrepancy between Count() and ToList() - is explained above. I agree it's somewhat odd, but that's why we have the warning I referred to above.

@hakanaltindis
Copy link

hakanaltindis commented Nov 18, 2022

@roji yes, it is odd. So do you have any plan to fix it? Because this odd prevents consistency.

What is your recommendation? I understand we can not use EF Core for Enterprise applications from what you say.

@roji
Copy link
Member

roji commented Nov 18, 2022

I understand we can not use EF Core for Enterprise applications from what you say.

That is definitely not what I'm saying.

  • When EF sees _dbContext.Activities.Include(a => a.Customer).Count(), it ignores (removes) the Include, since including the related entities generally isn't necessary for the counting. The general meaning of Include is to "bring back the related entities", which makes no sense in the context of a Count operation.
  • However, if Customer is a required navigation, this has the side-effect of not applying any query filter on Customer, and so you get all Activities. Again, remember that Include is about loading related entities, not filtering.
  • If we stopped ignoring the Include above, other users may start complaining since they're relying on the Count not filtering (and consider EF "not usable for Enterprise applications", as you wrote).

In short, although I agree the behavior is odd, there doesn't seem to be an obvious correct behavior here, and some users would complain no matter what we do. This is why the warning (and documentation) exists, to alert people to this behavior.

Note that you can always explicitly ask to filter as you wish by adding a Where clause before the filter (instead of the Include) - that should give you the results you want.

@hakanaltindis
Copy link

I am really trying to understand you. Sometimes your thoughts about include() comes to me true. I indicated before. I am really confused on the below example.

// Numbers are related with my example.
_dbContext.Activities.Include(a => a.Customer).Count(); // returns 11
_dbContext.Activities.Include(a => a.Customer).ToList().Count(); // returns 9

But you specified how odd this behavior.

And I don't want to prolong this topic. So be it. :)

I try to find a workaround for my case.

Thank you for your responses.

@roji
Copy link
Member

roji commented Nov 21, 2022

@hakanaltindis we had a discussion around this, I've opened #29645 to track possibly changing our behavior.

@hakanaltindis
Copy link

Thank you, @roji . I will follow this topic very closely. 👍 🙂

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
Development

No branches or pull requests

7 participants