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

Implement change tracking proxies #10949

Closed
ajcvickers opened this issue Feb 12, 2018 · 15 comments
Closed

Implement change tracking proxies #10949

ajcvickers opened this issue Feb 12, 2018 · 15 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Contributor

Using the lazy-loading proxy infrastructure coupled with INotifyPropertyChanging and INotifyPropertyChanged.

Also, consider partial notifications, for example to detect setting a navigation property or FK to null--see this discussion: dotnet/ef6#452

@orionstudt
Copy link
Contributor

I'd like to give this a go if that's not a problem. Is this on any existing roadmaps? I can write up a plan here and I'll probably have a couple simple questions before I start anything.

@ajcvickers
Copy link
Contributor Author

@orionstudt This isn't currently planned for an upcoming release; a good PR is certainly something we would be interested in. Writing a plan here would be a good place to start.

@orionstudt
Copy link
Contributor

orionstudt commented Dec 13, 2019

@ajcvickers Alright, cool. Here is my initial thoughts/plan:

  • Add an additional boolean switch to proxies extension, ProxiesOptionsExtension, UseChangeDetectionProxies.

  • An additional third switch, not sure what to call it. Something like CheckEquality - default to true. The default behavior being if the incoming value is the same as the current value than no change notification will be raised. Setting to false should cover the setting a navigation or FK to null scenario.

  • Modify logic in ProxyBindingRewriter to indicate different convention. If UseChangeDetectionProxies is enabled it will require all properties to be virtual, not just navigations. Also if only UseChangeDetectionProxies is enabled we won't need to setup lazy loader service property.

  • Write 2 additional castle interceptors, 1 for INotifyPropertyChanged & 1 for INotifyPropertyChanging.

  • ProxyFactory will now rely on ProxiesOptionsExtension in order to determine how to build the proxies (which interfaces to proxy, and which interceptors to use).

  • Will use IEntityType.GetChangeTrackingStrategy() to determine which INPCs to proxy on each entity, if any.

Questions:

  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.

  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?

@ajcvickers
Copy link
Contributor Author

@orionstudt Thanks--this seems like a well thought out approach, and generally in the correct direction. I will discuss your questions with the team and get back to you.

@ajcvickers ajcvickers removed this from the Backlog milestone Dec 16, 2019
@orionstudt
Copy link
Contributor

I will start this probably over the holiday or shortly thereafter and get something together for you guys to review. Happy holidays!

@ajcvickers
Copy link
Contributor Author

@orionstudt Sounds good.

@orionstudt
Copy link
Contributor

@ajcvickers Made an initial draft PR with the current state of the work and copied the questions over there as I figured that might be a better place for any discussion.

@ajcvickers ajcvickers added this to the 5.0.0 milestone Jan 14, 2020
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 14, 2020
ajcvickers added a commit that referenced this issue Jan 25, 2020
Issue #10949

Found and fixed a couple of bugs:
* Proxies were reading directly from the property. This can cause recursive loops in some cases. Fixed by using EF's compiled delegates for writing to the field directly.
* Comparisons were being done by the default for the property/navigation. Instead force use of configured value comparer for regular properties or reference equality for navigations.

Also added overloads to `CreateProxy` to make it easier to do creation of proxy graphs inline.
ajcvickers added a commit that referenced this issue Jan 26, 2020
Issue #10949

Found and fixed a couple of bugs:
* Proxies were reading directly from the property. This can cause recursive loops in some cases. Fixed by using EF's compiled delegates for writing to the field directly.
* Comparisons were being done by the default for the property/navigation. Instead force use of configured value comparer for regular properties or reference equality for navigations.

Also added overloads to `CreateProxy` to make it easier to do creation of proxy graphs inline.
ajcvickers added a commit that referenced this issue Jan 26, 2020
Issue #10949

Found and fixed a couple of bugs:
* Proxies were reading directly from the property. This can cause recursive loops in some cases. Fixed by using EF's compiled delegates for writing to the field directly.
* Comparisons were being done by the default for the property/navigation. Instead force use of configured value comparer for regular properties or reference equality for navigations.

Also added overloads to `CreateProxy` to make it easier to do creation of proxy graphs inline.
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@JonPSmith
Copy link

JonPSmith commented Jul 22, 2020

Hi @orionstudt and @ajcvickers ,

I'm updating my book Entity Framework Core in Action to EF Core 5 and I will soon updating the part about INotifyPropertyChanged. Can you point me to an classes/unit tests that use this feature so I can understand it.

No rush.

@orionstudt
Copy link
Contributor

orionstudt commented Jul 22, 2020

Hi @JonPSmith. All the code for this feature is in the src/EFCore.Proxies project and the tests are in test/EFCore.Proxies.Tests.

Specifically most of the code is in:
ProxyFactory.cs
PropertyChangedInterceptor.cs
PropertyChangingInterceptor.cs

@ajcvickers
Copy link
Contributor Author

@JonPSmith
Copy link

JonPSmith commented Jul 23, 2020

Hi @orionstudt and @ajcvickers ,

That should keep me busy. I will work though this and maybe get you to check that I have understand it correctly.

@JonPSmith
Copy link

Hi @orionstudt and @ajcvickers ,

I am writing about change tracking proxies. What I have worked out so far are:

  • It works like Lazy Loading version that uses the UseLazyLoadingProxies() builder command.
  • You add builder.UseChangeTrackingProxies() to the creation the DbContextOptionsBuilder
  • Every property has to be to be virtual in all entities that are linked to the DbContext

Things I don't understand are

  • How do I set up the ChangingAndChangedNotifications? Is it linked to the Proxy Create??

Questions I have

  • I don't think you can have a mix of normal entity classes (i.e. not using the ChangeTrackingProxies and not having virtual properties) and entity classes that use ChangeTrackingProxies in the same DbContext. Is that correct?
  • Can you have UseLazyLoadingProxies() and .UseChangeTrackingProxies() at the same time?

@orionstudt
Copy link
Contributor

How do I set up the ChangingAndChangedNotifications? Is it linked to the Proxy Create??

I'm not sure I'm understanding this question correctly but, the ChangeTrackingStrategy will default to ChangingAndChangedNotifications when you call .UseChangeTrackingProxies(). This is done in Microsoft.EntityFrameworkCore.Proxies.Internal.ProxyChangeTrackingConvention.

I don't think you can have a mix of normal entity classes (i.e. not using the ChangeTrackingProxies and not having virtual properties) and entity classes that use ChangeTrackingProxies in the same DbContext. Is that correct?

That is correct. This was a design decision from the EFCore team. @ajcvickers discussed this here & here.

Can you have UseLazyLoadingProxies() and .UseChangeTrackingProxies() at the same time?

Yes this should absolutely be ok. You can see in Microsoft.EntityFrameworkCore.Proxies.Internal.ProxyFactory that on proxy creation it will pass in interceptors for one, the other, or both.

Hope that helps.

@JonPSmith
Copy link

Hi @orionstudt,

Thanks for coming back to me. I have worked it out now:

  • If I am using ChangingAndChangedNotifications I need to create the entity class via the CreateProxy method.
  • If I am using ChangingNotifications I can create the entity class with a normal new.

That what works in my unit test but is that what you recommend? The use of the CreateProxy method isn't obvious and I suspect you need some documentation for that.

It also looks like you can't have a ChangingAndChangedNotificationsWithOriginalValues version. Not sure you need one but just checking that I haven't missed it.

Thanks for the other answers. They all make sense.

@ajcvickers
Copy link
Contributor Author

@JonPSmith To add to what @orionstudt said, this is the case where I wouldn't expect to set different values for ChangeTrackingStrategy. The proxies will always implement both changing and changed notifications, I don't think there would be any value in telling EF to not use them. That being said, ChangingAndChangedNotificationsWithOriginalValues would make sense to set.

I filed #21920

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants