Include with InMemory provider does not return records when required navigation are null

[stevejgordon]

I have a question and possible bug to report in EF Core 2.0.

In our project we have been using the InMemory provider with EF as part of our unit testing strategy. We are able to setup a clean InMemory "database" with a set of test data which we can then use to validate our query code is working as expected.

After upgrading to 2.0.0 I noticed that we had multiple test failures across the project. On investigating I found that in many cases it was that queries were returning not returning the same records as in 1.x. On investigation I noticed that it was in cases were we were not setting related entities for the navigational properties, but where the query specific Included these.

In the test case we only setup the specific entity and related entities for those elements we will be testing.

Question: Has this behaviour changed by design in 2.0.0? It now seems that if a query has Includes in it but the navigation properties are null, those records will not be returned. Previously we got the object with null navigational properties. I couldn't see any release notes or blogs posts indicating a change to this behaviour.

Possible bug: As we normally include a specific property for the foreign key and don't mark them as nullable integers, those relationships should be considered required. However, even in 2.0.0 it seems to allow the creation of an entities, without the navigational item. Is this because of the InMemory provider? I do recall reading that perhaps it did not enforce full referential integrity.

Steps to reproduce

Sample code to reproduce is available at https://github.com/stevejgordon/EfCoreIssue

Running the tests will demonstrate the differences.

Marking the foreign key as nullable does appear to then allow tests to pass in both cases.

Further technical details

EF Core version: 2.0.0
Database Provider: InMemory
Operating system: Windows 10
IDE: VS 2017 (15.3)

[ajcvickers]

@maumar Let's consider for patch after investigation, and also check whether or not it is a regression.

[olivierr91]

I have exactly the same problem (just closed my duplicate issue).

[olivierr91]

For testing purposes, we often do not want to setup the full entity tree in the test database and we often do not need all navigational properties during a test. Allowing includes on navigational properties mapped to a non-nullable field should return the primary entity whether the related entity exists or not. Marking the fields as nullable just for testing purposes does not seem a good permanent solution to me.

[maumar]

The reason for the behavior change is that now include is using navigation rewrite logic to construct the queries (whereas before we manually crafted include statements). Navigation rewrite produces INNER JOIN pattern for required relationships and LEFT JOIN pattern for optional. Before we would always hand-craft LEFT JOIN pattern, regardless of the relationship requiredness between child and parent.

As @stevejgordon said this issue is specific to InMemory due to lack of referential integrity checks.

[smitpatel]

Is this issue arising because we "saved" data which would violate integrity constraint otherwise? I think the Include pipeline is behaving correctly here.

[olivierr91]

@smitpatel Yes, indeed. I understand your point. The pipeline is behaving correctly, even though the previous behavior allowed us to simplify greatly the data setup of unit tests.

Where previously, I could persist an entity for testing purpose like:
var student = new Student();

Will now become:
var student = new Student() {Program = new Program() {School = new School() {City = new City() {Country = new Country()}}}};
... which can quickly become cumbersome when doing simple tests for complex entities having a deep navigational tree.

I guess I will have no choice to comply with the integrity constraints even in the tests.

[olivierr91]

If the in-memory database ever perform integrity checks the tests would fail anyway on SaveChanges(), so better make our test data comply with the non-nullable integrity constraints. I have fixed my tests and everything runs fine. The issue is closed for me.

[smitpatel]

Closing this as by design.
In this case Include pipeline is working correctly. If your FK is required (i.e. there will be parent record present always) then Inner join is more efficient and left outer join is redundant.
It is true that this is regression but this breaking change improves faulty behavior. So we would want it in 2.0.0.
From EF perspective, we always go based on the domain model provided. So if user defines a required FK but deletes the constraint in database manually, user can insert data manually with null value in FK property. But EF would still assume there is constraint and work accordingly. Hence there will be Inner join rather than left join.
Thinking more about database providers which does not enforce integrity checks (non-relational providers like cosmosdb/mongodb), it seems fine to have InMemory not enforcing integrity checks. General advice for such database providers is to make sure integrity is not violated at application layer and only allow your application layer to change data. i.e. the test data should be valid in terms of constraints. EF will just utilize the model assuming all data is in valid state in data store. If there are mismatch in terms of schema/constraints between domain & database model then results could be incorrect. (most times it throws exception while running queries)

[divega]

Reopening to make sure we have a conversation about enforcing requiredness in the in-memory provider as mentioned in the twitter thread:

https://twitter.com/stevejgordon/status/898592262468370436

[ajcvickers]

See also #2166

[divega]

I just realized reading the discussion in #2166 that this is not about enforcing requiredness but the referential constraint, which we then decided not to do. I.e. in the repro provided a default value of 0 is stored in the foreign key, and we don’t have a good way to know it isn’t a valid key, without help from the store.

So the conversation can be about whether we have new data to revisit the original decision.

[stevejgordon]

In our case we can update the tests. There are quite a few broken as a result of the change. Would it be worth documenting this change so others know to be prepared for this? Also, could it be possible to enable to original behaviour via configuration?

[ajcvickers]

@divega Agreed that we should revisit that decision. I think the discussion is really about the strength semantics for foreign keys defined in the model. For relational the semantics have always been strong--you can't lie about whether an FK is required or not, or whether the constraint really exists in the database or not, and then expect to get correct results if your data doesn't match what you say your model is. This allows us to be more efficient in queries and simplifies code in other places.

When Smit and I discussed this we came down on the side of maintaining this strong semantics for other providers for now because it allows us to continue being more efficient/simpler in EF. However, if it turns out that idiomatically different uses come out of other non-relational providers, then we may need to relax the FK semantics at the EF level, but if we do that we should investigate other places where we make the assumption that if an FK is set, then it really does reference a principal.

For the in-memory provider, I don't think there is a strong argument to relax the FK semantics, but it changes the discussion on #2166 because now the in-memory provider explicitly has these semantics when used with EF, and hence it makes sense to enforce them at the model level.

[ajcvickers]

@stevejgordon We will try to get this documented and we'll discuss an opt-out.

[ajcvickers]

Hi @stevejgordon, @orobert91. We are gathering information on the use of EF Core pre-release builds. You reported this issue shortly after the release of 2.0.0 RTM. It would be really helpful if you could let us know:

• Did you consider testing your code against the pre-release builds?
• Was there anything blocking you from using pre-release builds?
• What do you think could make it easier for you to use pre-release builds in the future?

Thanks in advance for any feedback. Hopefully this will help us to increase the value of pre-release builds going forward.

[stevejgordon]

Sorry for the delay @ajcvickers:

In this case I hadn't been using the pre-release builds of EF Core. Nothing was blocking me from doing so. I've used some of the ASP.NET pre-release build in the past with no specific issues or questions.

[paulovila]

Hi @ajcvickers this is still happening in EFCore InMemory 5.0.3

var  options = new DbContextOptionsBuilder<MyContext>()
                .UseInMemoryDatabase(DateTime.Now.Ticks.ToString())
                .EnableSensitiveDataLogging()
                .Options;
var context = new MyContext(options);
context.DataSeriesItems.AddRange(
                new DataSeriesItem { ModelRunId = 5 },
                new DataSeriesItem { ModelRunId = 2 },
                new DataSeriesItem { ModelRunId = 5 });
await context.SaveChangesAsync();
var seriesItems = await context
   .DataSeriesItems
   .Include(w => w.DataSource) // Fails when this line is included
   .Where(w => w.ModelRunId == 5)
   .ToArrayAsync();
Assert.AreEqual(seriesItems.Count(), 2);

Notice that DataSeriesItem.Id and DataSource.Id PKs are IDENTITY(1,1)

[jeremyZX]

I can understand the rationale for in-memory accepting entities into the in-memory store which would otherwise be invalid if it went into a relational store, and I can also understand the rationale for treating includes like an inner join, but but is this expected behavior? I've been wrestling with it in converting our unit tests to use EF Core InMemory:

Edit: EF Core 5.0.10. Unfortunately I can't seem to load EF Core 6 into LINQPad to test.

var saveGame = new SaveGame(); // SaveGame has a mandatory relationship to Game
dbContext.SaveGames.Add(saveGame);
dbContext.SaveChanges();
...
var anySaveGames1 = dbContext.SaveGames.Include(g => g.Game).Any(); // returns true
var anySaveGames2 = dbContext.SaveGames.Include(g => g.Game).ToList().Any(); // returns false

var saveGameCount1 = dbContext.SaveGames.Include(g => g.Game).Count(); // returns 1
var saveGameCount2 = dbContext.SaveGames.Include(g => g.Game).ToList().Count(); // returns 0