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

Issue(s) with Updating Entity #118

Closed
dthk-cogmatix opened this issue Dec 3, 2019 · 6 comments
Closed

Issue(s) with Updating Entity #118

dthk-cogmatix opened this issue Dec 3, 2019 · 6 comments
Labels

Comments

@dthk-cogmatix
Copy link

@dthk-cogmatix dthk-cogmatix commented Dec 3, 2019

Hello:

First, thank you for your time, effort, and contribution to the community.

Second, I'm having some issue(s) with the Change Tracking / Updating. My commentary assumes I'm following the code 100%. (I've also hacked together Session/Tracking support so I'm not sure if what I'm experiencing is an artifact of that).

a) The EntityCollection update logic doesn't make 100% sense to me.

	public void Update(TEntity entity, EntityEntryState state)
	{
		var entry = GetEntry(entity);
		if (entry != null)
		{
                             //What assumption are we making for Entity.Equals (Same object reference or same Id / natural key?)
			if (entry.Entity.Equals(entity))
			{
                                      //Shouldn't we replace entry.State with entry.Refresh(entry)?
				entry.State = state;
			}
			else
			{
                             //Assuming we are comparing for Id key above, why would this ever happen since we matched on Id key for GetEntry?
				Entries.Remove(entry);
				Entries.Add(new EntityEntry<TEntity>(entity, state));
			}
		}
		else
		{
			Entries.Add(new EntityEntry<TEntity>(entity, state));
		}
	}

b) Assuming some of the above holds true entry.Refresh(entity) and my understanding of the Refresh method is to compare the "original state" to the new/proposed change/update. Then I think the OriginalValues and CurrentValues is reversed and EntityEntry should really look like:

Should be like:
public BsonDocument OriginalValues => Entity.ToBsonDocument();
public BsonDocument CurrentValues { get; private set; }

	public void Refresh(TEntity entity)
	{
		CurrentValues = entity.ToBsonDocument();
		State = this.HasChanges() ? EntityEntryState.Updated : EntityEntryState.NoChanges;
	}
@dthk-cogmatix

This comment has been minimized.

Copy link
Author

@dthk-cogmatix dthk-cogmatix commented Dec 3, 2019

Actually, I think I'm better understanding how EntityEntry works...... And issues I'm facing could very well be due to my hacked implementation of sessions....(More digging required)

EntityEntry is holding a reference to the current object and when that is being changed, that is how it's getting tracked. (Which makes sense why CurrentValues = Entity.ToBsonDocument()).

I think somewhere along the line I'm getting a new object of the same entity. So, when I'm making changes to this entity, it's not reflecting in EntityEntry.

Flipping things around resolved the issue, but isn't "in the spirit" of the originally intended code...

@dthk-cogmatix

This comment has been minimized.

Copy link
Author

@dthk-cogmatix dthk-cogmatix commented Dec 3, 2019

Ok..... I may have found the actual issue (and helped answer my questions around EntityCollection and the comparison).

if (entry.Entity.Equals(entity))
{
entry.State = state;
}

should be changed to:

if(Object.ReferenceEquals(entity, entry.Entity))
{
entry.State = state;
}

The original intent of code was to check the object references with the default behavior of .Equals. The problem is: If the entities being tracked have an overridden Equals (which isn't uncommon and actually recommended in Domain Driven Design), then you aren't actually comparing the object references.

I believe the right fix (to stay inline with existing spirit of code) is to do an explicit Object.ReferenceEquals.

@Turnerj

This comment has been minimized.

Copy link
Member

@Turnerj Turnerj commented Dec 3, 2019

Your assumption is right - the entry.Entity.Equals(entity) is effectively a reference comparison. It might have been more clear if I used object.ReferenceEquals(entry.Entity, entity) instead. The goal here is basically "if these entities are exactly the same thing, just update the state".

The "else" branch in that code block is for the ID check. If two entities match via ID but are different via entity reference, we want to remove the old entry and add the new entry.

With EntityEntry, it is like you've mentioned, the properties are effectively there for change tracking. We keep the entity reference so we can do the comparisons for EntityCollection as well as the current values as BSON. We keep the original values as a way to do the diff between existing and new values - this allows us to perform writes only on the properties that have changed.

Feel free to share your implementation of sessions here if you want any pointers - I know my internal API is a little unstable between releases. I've been toying with the idea to implement sessions/transaction support (see #34) though there have been a few roadblocks to implementing something like it natively in the library. There are also a few other things I'd like to land before supporting sessions too (#112 and #110) which will delay me implementing it even further.

@Turnerj

This comment has been minimized.

Copy link
Member

@Turnerj Turnerj commented Dec 3, 2019

Didn't see your latest comment as I was writing mine but yeah, I agree. Will do that change shortly and may release it as a patch version too.

Turnerj added a commit that referenced this issue Dec 3, 2019
See #118 for details
@Turnerj

This comment has been minimized.

Copy link
Member

@Turnerj Turnerj commented Dec 3, 2019

The fix for using ReferenceEquals is out in 0.19.0 along with a few other minor changes. https://www.nuget.org/packages/MongoFramework/0.19.0

@dthk-cogmatix

This comment has been minimized.

Copy link
Author

@dthk-cogmatix dthk-cogmatix commented Dec 3, 2019

Initial Issue resolved. Closing this.

Will start a separate thread for session discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.