-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Limit change tracking proxies to only changing _and_ changed strategies #22278
Conversation
@Pilchie RC1 |
/cc @JonPSmith FYI |
break; | ||
if (!_notifyPropertyChangingInterface.IsAssignableFrom(entityType.ClrType)) | ||
{ | ||
interfacesToProxy.Add(_notifyPropertyChangingInterface); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If interfacesToProxy
is empty we shouldn't create a proxy for change tracking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this in another PR
c072747
to
c474ec2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional things to fix in the next PR:
- Currently
context.CreateProxy<Dictionary<string, object>>()
throws "The entity type '{entityType}' was not found. Ensure that the entity type has been added to the model.". If the type is shared or weak we should instead suggest callingcontext.Set<T>("TypeName").CreateProxy()
- If change tracking proxies are enabled and there are entity types using
Dictionary<string, object>
with non-PK properties we should throw inProxyChangeTrackingConvention.OnModelFinalized
We should also validate that if the entity type is set up to use proxies then |
@AndriySvyryd Yes, we require all properties to be virtual, but we're missing checking indexer properties--that was one of the issues I mentioned on the call earlier. This is the only reason our tests are passing right now. No foot shooting allowed. For lazy-loading proxies, you don't have to create a proxy instance, but for change tracking proxies we don't allow you to use a non-proxy instance. This can be a pain, but prevents some serious foot shooting. |
Approved for RC1 |
c474ec2
to
2c1757c
Compare
Fixes #21920