Proxy Change Detection

[orionstudt]

Addresses #10949

This was getting a bit bigger than I originally anticipated so making a draft pull request for any early feedback.

Note that the tests are currently all in LazyLoadingProxyTests and as part of this PR I have split them out into 3 files (change detection, lazy loading, proxy) since they don't all fall under "Lazy-Loading" anymore. All the tests still exist in their current form. Hopefully that's not an issue.

Things that still need to be done:

• Tests that ensure proxy change detection convention is enforced.
• Tests that ensure that the correct combination of interfaces/interceptors are proxied given whether lazy loading and/or change detection is enabled, and the entity's change tracking strategy.
• Tests that ensure events are raised by interceptors when necessary.

Some questions copied from #10949, which still stand:

1. If the entity already implements one of the notify interfaces, should we just not proxy that interface and leave it up to the consumer to raise those events? Edit: alternatively we could just continue to proxy and potentially raise 2 events if the consumer is already raising 1. Currently I am not even checking for this. At the moment I don't think this will be an issue, but could and probably will make tests against entities that are already implementing these interfaces in order to verify.

2. In a similar vein, for collection navigations we can bubble a collection change up to the parent entity if the underlying collection is one of the observable collection types. But it will be up to the consumer to instantiate that collection as one of the observable types in their entity's constructor, is that ok?

3. With what I've currently written out, if a consumer enables UseChangeDetectionProxies the default ChangeDetectionStrategy will still be Snapshot so none of the entities will receive the INPC proxies. Should we also be updating the ChangeDetectionStrategy to some other default value when change detection proxies are first enabled?

Current issue that I wouldn't mind insight on:

IProxyFactory now relies on ProxiesOptionsExtension in order to determine how to proxy entities. Since IProxyFactory is registered as a "provider-specific service" it is apparently not able to resolve IDbContextOptions to retrieve the extension. Is there some existing mechanism for a "provider-specific service" to get access to an extension? I couldn't seem to find one.

Edit: looking at it again and the IProxyFactory.Create takes in DbContext - so I'm just realizing that IProxyFactory is not tied to a DbContext (whoops). Will either need to change that or have other methods provide DbContext when necessary...

[dnfclas]

All CLA requirements met.

[ajcvickers]

@orionstudt I made a pass through and generally this looks great. I will do another review when I am properly back at work in the new year.

Your specific questions here:

1. I'm leaning towards not implementing again. On the one hand it's not guaranteed that the existing implementation does exactly what we need. But on the other hand having two implementations means we (and others) need to be sure we use the correct one when subscribing.

2. I think it's reasonable that if the entity creates its own collection instance, then it must be observable so we can use it. We should generate an error if we find this isn't the case--if I remember rightly, we already have code for this when not using proxies, so that may be sufficient.

3. Yes, the change detection strategy should be changed automatically so that the proxies are automatically used.

[orionstudt]

@ajcvickers Awesome. Some thoughts:

1. I'm leaning towards not implementing again. On the one hand it's not guaranteed that the existing implementation does exactly what we need. But on the other hand having two implementations means we (and others) need to be sure we use the correct one when subscribing.

Sounds good to me. I will add that check on my next run through.

2. I think it's reasonable that if the entity creates its own collection instance, then it must be observable so we can use it. We should generate an error if we find this isn't the case--if I remember rightly, we already have code for this when not using proxies, so that may be sufficient.

Also sounds good. Makes that a bit easier. I'll have to see if I can find the code for the error you mentioned. It's occurring to me that this is also not specific to navigations since owned types can also be collections, and may get interesting with the indexer properties that I noticed you guys are working on.

3. Yes, the change detection strategy should be changed automatically so that the proxies are automatically used.

Any thoughts on which of the 3 strategies we should default to?

Enjoy your time off, and happy new year.

[ajcvickers]

@orionstudt The existing error is https://github.com/aspnet/EntityFrameworkCore/blob/b36ea0a3b7dc4aef662d7b2ead8fbce3b6b50d85/src/EFCore/Properties/CoreStrings.resx#L204

The strategy should be ChangingAndChangedNotifications. This is the most efficient since it avoids snapshotting current/original values as much as possible.

[orionstudt]

@ajcvickers So I've been working on defaulting to ChangingAndChangedNotifications and hit a bit of a dilemma, or I'm just missing something.

I'm setting the default ChangeTrackingStrategy in ProxyBindingRewriter which sets all the conventions for the proxy lib, and occurs before ValidatingConvention. At which point ValidatingConvention runs ModelValidator which checks the ChangeTrackingStrategy and will throw an error if IEntityType.ClrType doesn't implement the INPC interfaces.

I tried having a separate convention that runs after ValidatingConvention but that throws a different error because at that point the model is read only. It also doesn't appear that I can derive from and replace ValidatingConvention because it just calls a catch-all validation method on ModelValidator.

It also doesn't look like the proxies lib, not being a database provider, should do any overloading of ModelValidator itself.

Thoughts on how to get around that?

[orionstudt]

Disregard previous comment, I slept on it. Came up with a solution by mimicking the Annotation used in the ChangeDetector implementation. Curious if you think that is an OK solution.

Tests are now written, everything looks kosher. Only issue is that when you add an entity to the context the first thing it tries to do is start tracking that entity. The problem is consumers obviously are providing the CLR type so the entity hasn't been proxied and doesn't have the INPC interfaces yet...

[ajcvickers]

@orionstudt Making great progress here! With regard to integration with model validation, I think it would be better to do something a bit more robust. I will look into this a bit more and discuss with @AndriySvyryd. I think ideally the extension should be able to:

• Remove/replace the existing validation for notification interfaces when proxies are configured
• Instead validate that the given entity types are suitable for proxies

This is closely related to your about what should happen if the application directly creates an entity instance an then attempts to track it. In EF6, effective use of notification entities was hampered by supporting mixed graphs where some entities are proxies and some are not. For example, entity snapshots and DetectChanges might still be needed. In addition, it was confusing when people accidentally didn't create proxy instances and then didn't get the behavior they were expecting.

This is why EF Core is a lot more strict about actually using notification entities if they have been configured--hence the validation discussed above. Based on this we should not allow a non-proxy instance to be tracked. (This might just mean updating the current exception message.)

On the other hand, forcing proxy instances to be used can be a burden to the application since any place that needs to create an entity instance needs to do so directly or indirectly using EF Core.

I'm leaning towards requiring proxy instance, but I'll also discuss this with the team.

One final thing: Take a look at ProxyGraphUpdatesTestBase and the related GraphUpdatesTestBase. These are pretty large functional test suites that run lots of variations over different types of entity. It would be great to get at least a couple more variations: one using change-detection proxies, and one using both lazy-loading and change-detection via proxies.

[orionstudt]

@ajcvickers Awesome. Glad you responded because I went down a little rabbit hole, lol.

I got it to a point where it was using Snapshot on entities that hadn't been saved yet, but then had the issue of if you requested an entity that the DbContext was still holding in memory it still hasn't been proxied and you'd get the same issue - so you're probably right in that the cleanest solution would be if the entity were proxied early on. I will delay any changes on this front until I hear from you guys on how you'd like to tackle it.

I'll take a look at those tests next time I get a chance and see if I can get more variation in there.

[ajcvickers]

@orionstudt After discussion with the team:

• I'm going to do some investigation in to what is currently possible for modifying the validation conventions
• We think a proxy instance should always be required, at least for now. This is the safest behavior in that it doesn't lock us into support for mixed notification/non-notification entities. If we gather enough feedback that this is too limiting, then we can revisit this later without breaking existing apps.

[orionstudt]

@ajcvickers So I added a test to check for your "setting a navigation property to null before it is included / lazy loaded" scenario. We can get it to snapshot and raise when CheckEquality == false but we would need to hook into ChangeDetector.DetectNavigationChange() or sooner so that the snapshot value is non null and the modification gets recorded.

[ajcvickers]

@orionstudt See my comment here with regard to the model validation: #15660 (comment)

I expect that we will not block this PR waiting for a solution to that issue, but let me discuss with the team tomorrow.

With regard to the setting a null navigation to null scenario, I think we can break that out into a separate issue--maybe I will just re-open the original one. Then we should fix that issue so that it works consistently either with proxies or manually implemented notification entities.

[orionstudt]

@ajcvickers Cool. Sounds like we'll have it all split out into separate issues then - so this PR is about ready for review. I will backtrack the null navigation test so that we are passing and then flag this ready. Let me know if you think the existing tests need to be any more robust than they are.

[ajcvickers]

@orionstudt Thanks for this high quality PR and for collaborating well on working through the issues. Much appreciated.

[orionstudt]

Thanks @ajcvickers ! It's been a pleasure.