dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.73k stars 3.18k forks source link

Add support for ignoring query filter on Include #21093

Closed jonesty closed 2 years ago

jonesty commented 4 years ago

Currently, query filters can only be ignored for an entire query. It would be great to be able to ignore a particular entity type's query filter when "including" a navigation property of that type. For example, assuming all entity types have a soft-delete query filter defined, this would be ideal:

db.Set<Car>()
  .Include(c => c.Manufacturer, ignoreQueryFilter: true)
  .Include(c => c.Features)
  ...

The query should yield:

In a relational context, the join onto Manufacturer would not contain the query filter condition.

Currently (without this mechanism) any car whose manufacturer is soft-deleted would not be returned, which is problematic.

maumar commented 4 years ago

related issue: #19801

maumar commented 4 years ago

@jonesty in order to get cars for whose manufacturers have been soft-deleted you can change the relationship between Car and Manufacturer to optional. The behavior you see is because for required navigations we assume INNER JOIN is correct, since there should be an entity on the other side of the navigation. However query filter may break that assumption.

Improvements to soft delete experience are tracked by #19695

jonesty commented 4 years ago

@maumar Thanks for your response. I did see that suggestion on another issue but unfortunately it doesn't help with this particular problem. It's not the inner join that's the issue here, it's the query filter being applied on the join. The relationship between Car and Manufactuer (in this example) should be required - a car must have a manufacturer, otherwise referential integrity is out the window. However, if that manufacturer is removed from the system for whatever reason, the car shouldn't just vanish. We have many relationships like this in our model for lookup data types, which currently cause chaos when a user deletes one of them.

The reason I suggested this filter control be implemented on "Include" is because we still want to use a query filter on things like Manufacturer but we want the option of ignoring it in some cases (typically when including a navigation property that represents a single entity).

Some examples for clarity:

db.Set<Manufacturer>().ToList()

Should return non-deleted manufacturers.

db.Set<Factory>()
  .Include(factory => Manufacturers)
  .ToList();

Should return non-deleted factories, including related non-deleted manufacturers.

db.Set<Factory>
  .Include(factory => factory.Manufacturers, ignoreQueryFilter: true)
  .ToList()

Should return non-deleted factories, including ALL manufacturers.

And, of course, the original example where the navigation property represents a single entity:

db.Set<Car>
  .Include(car => car.Manufacturer, ignoreQueryFilter: true)
  .ToList();

Should return non-deleted cars, including their manufacturers, whether deleted or not. This scenario is the real kicker for us.

jonesty commented 4 years ago

@maumar I hope my comments above make sense. Are we able to re-open this issue or at least discuss further, please? Moving forward, I'm not sure we'll be able to continue using EF Core without having a way to deal with this. That would really suck because it otherwise works beautifully for us and I don't particularly want to re-write our entire data services layer. Your help on this would be greatly appreciated. Thanks mate.

smitpatel commented 4 years ago

Should return non-deleted cars, including their manufacturers, whether deleted or not. This scenario is the real kicker for us.

From the perspective of referential integrity constraint, "a car must have a manufacturer". So if you are querying for non-deleted cars, their respective manufacturer must be non-deleted. If a manufacturer gets deleted (marked as soft-delete), all cars depending on it must be marked as deleted for referential integrity. For a perfect graph, you should not need to ignore query filter on Manufacturer table. Can you explain what I am missing here?

jonesty commented 4 years ago

@smitpatel In our application nothing is ever hard-deleted - every entity is set up to be soft-deleteable. However, in some scenarios soft-deletes do not cascade as real deletes typically would. This means you can "delete" a manufacturer but keep its cars, and because the foreign key on Car is still set, the car's manufacturer can technically still be retrieved by ignoring the fact that its IsDeleted flag is set (i.e. ignoring the query filter).

This behaviour currently isn't achievable because we can't selectively ignore the query filter for Manufacturer in this scenario. Please note the "car" example I've used here is contrived but illustrates the issue nonetheless. Our actual application is an aerial LiDAR processing platform that has a myriad of queries where we need to include potentially soft-deleted entities like this.

smitpatel commented 4 years ago

However, in some scenarios soft-deletes do not cascade as real deletes typically would.

This breaks referential integrity constraint. You may need to use manual joins rather than EF Core navigations.

jonesty commented 4 years ago

I understand where you're coming from, although RI is still intact at the database level. Is it even possible to join two sets, ignoring the query filter for one but not the other? I assume that's what you meant.

smitpatel commented 4 years ago

The RI is intact in database level because database has ignored query filter on both the tables. The same is true in EF Core also.

jonesty commented 4 years ago

Ok mate...so let's forget that idea. How would I achieve this using joins as you suggested?

maumar commented 4 years ago

@jonesty for tracking queries you can do something like this:

                var query1 = (from c in ctx.Cars
                             join m in ctx.Manufacturers on c.ManufacturerId equals m.Id into grouping
                             from m in grouping.DefaultIfEmpty()
                             select new { c, m }).ToList().Select(x => x.c);

for non-tracking you need to perform fixup yourself, like so:

                var query2 = (from c in ctx.Cars.AsNoTracking()
                             join m in ctx.Manufacturers on c.ManufacturerId equals m.Id into grouping
                             from m in grouping.DefaultIfEmpty()
                             select new { c, m }).ToList().Select(x => Fixup(x.c, x.m));

(...)
        private Car Fixup(Car c, Manufacturer m)
        {
            c.Manufacturer = m;
            c.ManufacturerId = m?.Id ?? 0;

            return c;
        }

or inline the fixup directly:

                var query3 = (from c in ctx.Cars.AsNoTracking()
                             join m in ctx.Manufacturers on c.ManufacturerId equals m.Id into grouping
                             from m in grouping.DefaultIfEmpty()
                             select new { c, m }).ToList().Select(x => { x.c.Manufacturer = x.m; x.c.ManufacturerId = x.m?.Id ?? 0; return x.c; });
ajcvickers commented 4 years ago

To me, this isn't really soft delete, because it doesn't have the same semantics as real deletes. That is, if a principal is really deleted, then the model integrity requires that any dependents with required relationships cannot exist and must also be deleted. This is what having a required relationship means. For an optional relationship, the dependents can continue to exist, but can't point to something that doesn't exist--again the model doesn't allow this. (But see #13146.)

I think this is instead this is a case of a principal (manufacturer) that does still exist but has been flagged in the domain model in some way to indicate that is no longer a current manufacturer. It would be fine to filter out non-current manufacturers for some business logic/reports, but that doesn't make them deleted in the domain model.

jonesty commented 4 years ago

To me, this isn't really soft delete, because it doesn't have the same semantics as real deletes.

I totally agree, but this issue isn't specific to soft-deleting so let me present it differently.

I have a Manufacturer entity that has an IsActive flag. I also have a query filter on this because most of the time I don't care about inactive manufacturers.

builder.HasQueryFilter(manufacturer => manufacturer.IsActive);

This is perfect until I include a Manufacturer navigation property in a query, in which case anything associated with an inactive one doesn't come back. I accept this is a default behaviour and that's absolutely fine.

However, it would be great to be able to include inactive manufacturers this way by simply telling EF not to apply its query filter on the generated inner join.

db.Set<Car>
  .Include(car => car.Manufacturer, ignoreQueryFilter: true)
  .ToList();

To me this seemed like a reasonable approach to the problem. It would also be handy to have when including collection navigation properties where it could be combined with a filter expression (i.e. filtered include), effectively providing a facility to override the global query filter for that type.

ajcvickers commented 4 years ago

@jonesty I'm not sure I would implement this with a global filter, as opposed to just creating the queries as appropriate for the domain with local filtering as needed. If it is implemented as a global query filter, then I agree that being able to switch off that filter is useful. Better support for ignoring filters is tracked by #17347. However, I don't think this should be tied to a specific Include, since a global query filter is, by its nature, global and should be either on or off for any given query, and not modified only for part of that query.

jonesty commented 4 years ago

@ajcvickers I can understand that mate and I appreciate the time you've all taken to entertain this idea.

To me the proposed solution seemed like a logical progression from the existing IgnoreQueryFilters method. Include seemed like the appropriate context since it's the Include that generates the join condition that causes the problem. Having this small toggle available would add a lot of flexibility in my opinion but at the end of the day it's not up to me.

jonesty commented 4 years ago

I've been looking at the effort required for us to move away from global query filters and have noticed we also rely on them for projected values. For example:

db.Set<Factory>()
  .Select(factory => new
  {
    // We don't want these sub-queries to consider inactive manufacturers.
    Foo = factory.Manufacturers.Count(m => m.Revenue > 1000000),
    Bar = factory.Manufacturers.Any(m => m.SafetyRating < 3)
  })
  .ToList();

This scenario currently works perfectly as the query filter is automatically applied to the sub-queries. I know these queries could be executed separately and manually filtered but it's certainly much more convenient to write as above and just leverage some sort of opt-out mechanism when required. Food for thought.

jonesty commented 4 years ago

Related issues: #11691, #14151, #20771

smitpatel commented 4 years ago

@jonesty - All the related issues you pointed ends in #19801, if user has required navigation pointing to an entity which has query filter defined then generate a warning because user may have inconsistent graph which violates RI. So everything above stand correct.

jonesty commented 4 years ago

@smitpatel I did see that, thanks. Unfortunately a generated warning doesn't offer much of a way forward. Call me optimistic but I can't help thinking there's a better way to deal with this seemingly common scenario.

smitpatel commented 4 years ago

@jonesty - I have posted detailed response on #11691 Your example involves ignoring query filters for some includes. The API does not work when you want to ignore it on some navigations in the absence of include. Sure, adding the proposed API solves your case but it is not scalable and general enough to be applicable to all navigation scenarios and we are not going to take that direction. It is not a common scenario since most of the customers have data which agrees with model configuration of a relationship. Query uses model metadata for translation and anything which is mismatching between data & model that is unsupported scenario and we don't intend to fix it.

As I said in #11691, consider using own custom solution which comply with your business requirements, global query filter is not the solution for this use case.

jonesty commented 4 years ago

@smitpatel Okay mate, thanks anyway.

hsnsalhi commented 4 years ago

Looking for a solution for the same probleme I liked the workaround proposed in this post https://github.com/dotnet/efcore/issues/11853#issuecomment-385794028

oresttkachukd commented 2 years ago

@hsnsalhi @jonesty I can recommend workaround the issue using Filtered include from: https://docs.microsoft.com/en-us/ef/core/querying/related-data/eager You would have to duplicate your filter in more places, but it could help to resolve redundant data load.