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

Including entity with a query filter can cause not loading the root entity #11691

Closed
jirikanda opened this issue Apr 16, 2018 · 19 comments
Closed

Comments

@jirikanda
Copy link

I would expect the following code returns the instance of Child (the same instances).
But it depends on a query filter. If child's master is filtered, the second line returns null.

Child child = dbContext.Set<Child>().Where(c => c.Id == 1).FirstOrDefault();
Child childInludingMaster = dbContext.Set<Child>().Include(m => m.Master /* has a query filter */).Where(c => c.Id == 1).FirstOrDefault();

Steps to reproduce

  1. Install NuGet packages
  2. Create database IssueDemo (or change connection string)
  3. Generate database migration
  4. Run unit test
	[TestClass]
	public class TestIssue
	{
		[TestMethod]
		public void DbSet_ShouldReturnSameInstancesWheneverIncludeIsCalled()
		{
			// Arrange
			#region Data initialization
			int childId;

			using (var dbContext = new MyDbContext())
			{
				dbContext.Database.Migrate();

				Child child = new Child { IsDeleted = false };
				Master master = new Master { IsDeleted = true, Children = new List<Child> { child } };

				dbContext.Set<Master>().AddRange(master);
				dbContext.SaveChanges();

				childId = child.Id;
			}
			#endregion

			// Act
			using (var dbContext = new MyDbContext())
			{
				Child child = dbContext.Set<Child>().Where(c => c.Id == childId).FirstOrDefault();
				Child childInludingMaster = dbContext.Set<Child>().Include(m => m.Master /* has a query filter */).Where(c => c.Id == childId).FirstOrDefault();

				// Assert				
				Assert.AreSame(Object.ReferenceEquals(child, childInludingMaster), "Instances should be the same.");
			}
		}
	}

	#region Model
	public class Child
	{
		public int Id { get; set; }

		public Master Master { get; set; }
		public int MasterId { get; set; }

		public bool IsDeleted { get; set; }
	}

	public class Master
	{
		public int Id { get; set; }
		
		public List<Child> Children { get; set; }

		public bool IsDeleted { get; set; }
	}
	#endregion

	#region MyDbContext
	public class MyDbContext : DbContext
	{
		protected override void OnModelCreating(ModelBuilder modelBuilder)
		{
			base.OnModelCreating(modelBuilder);

			modelBuilder.Entity<Master>().HasQueryFilter(item => !item.IsDeleted);
			modelBuilder.Entity<Child>().HasQueryFilter(item => !item.IsDeleted);
		}

		protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
		{
			base.OnConfiguring(optionsBuilder);

			optionsBuilder.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=IssueDemo;Trusted_Connection=True;");
		}
	}
	#endregion

Further technical details

EF Core version: 2.0.2
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Target .NET Framework: 4.6.1

@ajcvickers
Copy link
Member

@jirikanda We discussed this in triage and decided we needed some more information on what you are trying to do. It seems like the case is this:

  • There is a relationship in your model that says every Child has a Master
  • There is a query filter that filters out some Masters
  • A query is executed that queries explicitly for a Child when its Master would have been filtered out

In the case where there is no Include, this results in a Child being returned even though there its Master would never be returned. This basically violates the relationship defined between Child and Master.

In the case where the Include is used, the results could be considered correct, because the filter says that that Master should not be returned, and hence by the definition of the relationship there cannot be any Child with a reference to it.

So, it seems like the problem here is that the combination of a filter on the Master and a query directly against a Child bypassing the Master is resulting in the data not matching what the model says. At least, that's one interpretation. We would like to know how you are thinking about the scenario so that we can try to compare our expectations to yours and decide on a plan of action.

@jirikanda
Copy link
Author

Thank you very much for the answer.

My description contains a synthetic scenario, that’s why Child/Master classes are not well understandable. I will describe it in more details.
I am preparing guidelines how to use Entity Framework Core in our applications (tens of line of business applications, tailor-made software development). The issue is related to soft deletes.
Softdelete-aware object in for us an object which is no more usable. This for us means:

  • Retrieving list of (softdelete-aware) objects
    When retrieving list of objects, a deleted object must not be returned. We want to be able to suppress this constraint to enable undelete scenario (like IgnoreQueryFilters).
    Example: If a user see list of cars, no deleted car is in the list.

  • Handling collections of (softdelete-aware) objects
    When an object A has a collection of objects B, we USUALLY do not want to load deleted objects B in the A's collection.
    We would like to have an option to decide (in model definition) whether the collections should or should not contain deleted object.
    Example: Let’s say, we have a Group and it has a collection of Members in the group (group.Members). When we load a group, we do not want deleted members to be in the collection group.Members.

  • Handling references to (softdelete-aware) objects
    When an object A has a reference to object B, we want never to filter out this object.
    Example: Let’s say Car has a (required) Color. We want to have a car with color even when the color is deleted.
    Example: An Invoice was issued by an Accountant and we would like to know who it was even when the Accountant is deleted.

There is a conflict in our goal (we never filter out references, but we USUALLY filter out collections), but it fits our practice and business requirements. That's our interpretation of soft delete ;-).

I tried the behavior of the Entity Framework Core and I did not expect the Include method should limit the root objects. Furthermore, I did not notice such information in the documentation (https://docs.microsoft.com/en-us/ef/core/querying/related-data).
XML documentation of Include extension method (https://github.com/aspnet/EntityFrameworkCore/blob/a588722e10f34a12b9cea3d655fd26aafd91051b/src/EFCore/EntityFrameworkQueryableExtensions.cs) is “Specifies related entities to include in the query results.“ (in brief). So I did not expect…

@ajcvickers
Copy link
Member

@jirikanda Thanks for the detailed description. This is very useful.

@ajcvickers
Copy link
Member

Triage: we will fix this post 2.1 by updating the query generation to not use inner join in this case--that is, when principal has a filter.

@jirikanda For now, if you make the relationship optional, then it should use a left join. You might also look into using a filter that uses a navigation property to make the behavior consistent between the Include and no Include case--see #8881

@ajcvickers ajcvickers added this to the 2.2.0 milestone Apr 18, 2018
@f135ta
Copy link

f135ta commented Apr 25, 2018

Could this be linked to issue #8576?

@htuomola
Copy link

Not sure if you need more feedback but I have the exact same use case as @jirikanda. Changing the relationship as optional is not really a good workaround IMO as it would require the FK field to be made nullable and I don't really want to change the DB schema forth and back because of this (also people would be confused by the nullable field). I'll rather disable the query filter for the given entity type or maybe ignore on per-use basis.

@jirikanda
Copy link
Author

Because of this behavior we do not use query filters at all.

@hisuwh
Copy link

hisuwh commented Dec 13, 2018

I have the same problem as @jirikanda and a similar use case. In my case I have an Archived property on the parent entity.

We have a view where we use IgnoreQueryFilters to explicitly view Archived parents. When we drill into these records we then cannot view the child records because we are including some information from the parent and so they get filtered out because they are archived.

We don't want to IgnoreQueryFilters on the child collection to get around this because we may add additional Global Filters on the child collection. (In fact, side point, it would be helpful to be able to key global filters so that we ignore them individually , e.g. .IgnoreQueryFilters("Archived"))

@smitpatel smitpatel modified the milestones: Backlog, 3.1.0 Nov 25, 2019
@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed punted-for-3.0 try-on-latest labels Nov 25, 2019
@jgoyvaerts
Copy link

@smitpatel Could you give some more information on how this is fixed?

I have the exact same usecase as @jirikanda .

Handling references to (softdelete-aware) objects
When an object A has a reference to object B, we want never to filter out this object.
Example: Let’s say Car has a (required) Color. We want to have a car with color even when the color is deleted.
Example: An Invoice was issued by an Accountant and we would like to know who it was even when the Accountant is deleted.

We upgraded to 3.1 but the entity is still not loaded. Is there now an option on the .Include method to ignore the query filter for this specific include? Are we missing something else?

@maumar
Copy link
Contributor

maumar commented Jan 29, 2020

related issue: #19649

@smitpatel
Copy link
Member

@jgoyvaerts - File a new issue with repro code please.

@jonesty
Copy link

jonesty commented May 25, 2020

@smitpatel - I'm facing this exact issue now in 3.1 also. We really need a way to control query filters on a per-Include basis. For example, assuming all entity types have a soft-delete query filter defined, this would be ideal:

db.Set<Car>() // Only non-deleted cars should come back.
  .Include(c => c.Manufacturer, ignoreQueryFilter: true) // ALWAYS include manufacturer (even if soft-deleted)!
  .Include(c => c.Features) // Only non-deleted features should come back.

In this example, the join onto Manufacturer would not contain the query filter condition. Is there any other way to achieve this behaviour at present?

@smitpatel smitpatel removed this from the 3.1.0 milestone Jun 10, 2020
@smitpatel smitpatel added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Jun 10, 2020
@smitpatel
Copy link
Member

After going through all the linked issues here

  • Global query filters are supposed to follow referential integrity constraint defined in the model. If each child object is required to have a parent then then all the child pointing to a parent which is soft-deleted must be soft-deleted. Otherwise it causes orphan children which according to the model is not possible.
  • We explored idea of always treating the relationship as optional when query filter but Left join can be perf expensive compared to inner join. Doing so in all cases feels like a penalty common cases have to pay to support a misuse of query filters.

Looking at this issue all the related issues,

  • User wants to apply query filters for some collection navigations & always ignore for reference navigations.
  • User does not have proper filters. i.e. Parent has filter but child does not have filter hence violating the RI.
  • User wants to ignore filters for some include but not for raw navigations.

Overall, global query filters are filters which will be applied to DbSet whenever querying for certain entity type. Further, referential integrity constraint should match model, with & without filter because model metadata is used to generate appropriate join. None of above scenarios are actual use case for global query filters.

  • If referential integrity constraint is not maintained then user is responsible for writing whatever is the required join to generate the result. EF Core cannot do that based on model. Hence for such scenarios navigations MUST not be used.
  • Applying filters for some navigations only would at best refer to mapping of a navigation property. E.g. if Car has a query filter but when querying for Manufacturer.Cars, it should not be used but when querying for Color.Cars, it should be used. It cannot be specified at entity level hence cannot use global query filter.
  • Adding a parameter to ignore query filter on include does not work since if user wants to ignore the query filter for some navigation then there is no API for that. Further different navigations can have different requirements about ignoring which goes back to point above.

Folks above who are facing issue or anyone new who arrives here with similar issue, consider using a different solution than global query filters. If the data cannot conform to model configuration about a relationship with & without query filter then it is not supported scenario with query filter. Consider using different refactoring, including manual joins for your case.

Marking this as closed by design.

@stevendarby
Copy link
Contributor

stevendarby commented Feb 24, 2021

In a DB First scenario, would there be any unexpected side effects in modelling non-null int FKs in the DB as nullable int properties in EF, so that the relationship was considered optional and LEFT JOINs used, getting around issues where filters are used?

If using ClientNoAction delete behaviour, no attempts to null the FK when deleting entities.

We would mandate/validate the true ‘requiredness’ in our create/update DTOs.

Anything else to consider?

—-

I really do think this ‘strict’ approach you’re taking to global filters is unnecessarily complicating it.

Like, EF isn’t actually the database so why can’t we use extremely useful features built into EF, like query filters, to make querying the database easier? Being able to rely on navigation properties instead of manual join syntax everywhere makes life so much easier, means DTOs can be projected with AutoMapper with easy to reuse code (query TEntity, project to TDto). Having to rewrite to use custom queries everywhere for each specific TEntity to do the manual joins is a pain- feels a step closer to just having to write the SQL.

I think maybe there is too much focus on ‘soft delete’ for which maybe there is an argument should conform to strict database referential integrity. (But even then- it doesn’t have to, does it? That’s why it’s soft and not an actual delete.) However, query filters could actually be used for anything; perhaps forgetting soft delete might help reason about this.

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

Lots of emails and notifications with these reopenings…

@ajcvickers
Copy link
Member

@stevendarby We're moving non-fixed issues from closed as "complete" to closed as "not planned". The notifications are unfortunate.

@stevendarby
Copy link
Contributor

@ajcvickers ah ok, yeah, though I thought I’d seen direct “closed as complete” to “closed as not planned” before without the reopening between - which is the bit I’m getting notified about. But anyway, no big deal!

@ajcvickers
Copy link
Member

The API returns an error trying to do it without opening first.

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

10 participants