-
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
Add events for entity state changes #10896
Conversation
_queryTrackingBehavior = context | ||
.GetService<IDbContextOptions>() | ||
.Extensions | ||
.OfType<CoreOptionsExtension>() | ||
.FirstOrDefault() | ||
?.QueryTrackingBehavior | ||
?? QueryTrackingBehavior.TrackAll; | ||
|
||
stateManager.EntityStateChangingEventBridge = OnEnityStateChanging; |
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.
typo
/// <summary> | ||
/// <c>True</c> if the change is for an entity that has just been queried from the database; <c>false</c> otherwise. | ||
/// </summary> | ||
public virtual bool FromQuery { get; } |
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.
Is the state really changing when we are loading from query? Should this be a separate "EntityMaterialized" event?
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.
EntityMaterialized is different--that would fire for tracking and no-tracking queries. I added this flag because sometimes certain things, like validation, can be skipped if the entity starts tracking after being queried, but it's still a state change from Detached to Unchanged.
256d920
to
e44b16d
Compare
@anpete New version up. |
public EntityEntryEventArgs( | ||
[NotNull] InternalEntityEntry internalEntityEntry) | ||
{ | ||
_internalEntityEntry = internalEntityEntry; |
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.
Check not null
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.
It's an internal constructor.
/// <summary> | ||
/// Event arguments for events relating to tracked <see cref="EntityEntry" />s. | ||
/// </summary> | ||
public class EntityEntryEventArgs : EventArgs |
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.
Provide DbContext here, too?
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.
Its available on the EntityEntry
EntityState newState) | ||
: base(internalEntityEntry) | ||
{ | ||
OldState = oldState; |
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.
As above
/// <summary> | ||
/// <c>True</c> if the entity is being tracked as part of a database query; <c>false</c> otherwise. | ||
/// </summary> | ||
public virtual bool FromQuery { get; } |
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.
/// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
/// directly from your code. This API may change or be removed in future releases. | ||
/// </summary> | ||
void OnTracked(InternalEntityEntry internalEntityEntry, bool fromQuery); |
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.
Having these On methods to allow individual entries to raise events on the state manager caught my attention.
What do folks think about the idea of pushing the event down on to the entries themselves? That way, that state manager would just subscribe to the event on the entries. cc @AndriySvyryd @bricelam @divega
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.
Seems like it would use a lot more memory if every entry had a reference to the handlers.
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.
Yeah, that's likely true.
e44b16d
to
0188578
Compare
Issue #10895 (Part of #626)