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

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

Closed
stevejgordon opened this Issue Aug 18, 2017 · 16 comments

Comments

Projects
None yet
6 participants
@stevejgordon

stevejgordon commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Aug 18, 2017

Member

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

Member

ajcvickers commented Aug 18, 2017

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

@orobert91

This comment has been minimized.

Show comment
Hide comment
@orobert91

orobert91 Aug 18, 2017

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

orobert91 commented Aug 18, 2017

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

@orobert91

This comment has been minimized.

Show comment
Hide comment
@orobert91

orobert91 Aug 18, 2017

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.

orobert91 commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Aug 18, 2017

Contributor

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.

Contributor

maumar commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Aug 18, 2017

Contributor

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

Contributor

smitpatel commented Aug 18, 2017

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

@orobert91

This comment has been minimized.

Show comment
Hide comment
@orobert91

orobert91 Aug 18, 2017

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

orobert91 commented Aug 18, 2017

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

@orobert91

This comment has been minimized.

Show comment
Hide comment
@orobert91

orobert91 Aug 18, 2017

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.

orobert91 commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Aug 18, 2017

Contributor

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)

Contributor

smitpatel commented Aug 18, 2017

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)

@smitpatel smitpatel closed this Aug 18, 2017

@smitpatel smitpatel changed the title from Behaviour change - Records not returned when navigational properties are null in 2.0.0 to Include with InMemory provider does not return records when required navigation are null Aug 18, 2017

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Aug 19, 2017

Member

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

Member

divega commented Aug 19, 2017

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

@divega divega reopened this Aug 19, 2017

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Aug 19, 2017

Member

See also #2166

Member

ajcvickers commented Aug 19, 2017

See also #2166

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Aug 19, 2017

Member

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.

Member

divega commented Aug 19, 2017

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.

@divega divega removed this from the 2.1.0 milestone Aug 19, 2017

@stevejgordon

This comment has been minimized.

Show comment
Hide comment
@stevejgordon

stevejgordon Aug 19, 2017

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?

stevejgordon commented Aug 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Aug 19, 2017

Member

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

Member

ajcvickers commented Aug 19, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Aug 19, 2017

Member

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

Member

ajcvickers commented Aug 19, 2017

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

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Sep 12, 2017

Member

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.

Member

ajcvickers commented Sep 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stevejgordon

stevejgordon Oct 3, 2017

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.

stevejgordon commented Oct 3, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment