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

Add support for ignoring query filter on Include #21093

Closed
jonesty opened this issue May 31, 2020 · 23 comments
Closed

Add support for ignoring query filter on Include #21093

jonesty opened this issue May 31, 2020 · 23 comments

Comments

@jonesty
Copy link

jonesty commented May 31, 2020

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:

  • Non-deleted cars
  • The manufacturer (always - whether soft-deleted or not)
  • Non-deleted features

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
Copy link
Contributor

maumar commented Jun 1, 2020

related issue: #19801

@maumar
Copy link
Contributor

maumar commented Jun 5, 2020

@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

@maumar maumar closed this as completed Jun 5, 2020
@maumar maumar removed their assignment Jun 5, 2020
@jonesty
Copy link
Author

jonesty commented Jun 5, 2020

@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
Copy link
Author

jonesty commented Jun 5, 2020

@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
Copy link
Member

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
Copy link
Author

jonesty commented Jun 6, 2020

@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
Copy link
Member

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
Copy link
Author

jonesty commented Jun 6, 2020

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
Copy link
Member

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
Copy link
Author

jonesty commented Jun 6, 2020

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

@maumar
Copy link
Contributor

maumar commented Jun 6, 2020

@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
Copy link
Member

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
Copy link
Author

jonesty commented Jun 6, 2020

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
Copy link
Member

@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
Copy link
Author

jonesty commented Jun 6, 2020

@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
Copy link
Author

jonesty commented Jun 7, 2020

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
Copy link
Author

jonesty commented Jun 9, 2020

Related issues: #11691, #14151, #20771

@smitpatel
Copy link
Member

@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
Copy link
Author

jonesty commented Jun 9, 2020

@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
Copy link
Member

@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
Copy link
Author

jonesty commented Jun 10, 2020

@smitpatel Okay mate, thanks anyway.

@hsnsalhi
Copy link

hsnsalhi commented Aug 7, 2020

Looking for a solution for the same probleme I liked the workaround proposed in this post #11853 (comment)

@oresttkachukd
Copy link

@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.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants