LoadRelatedEntities for 1-1 with Non-Matching Foreign Keys

[weitzhandler]

Hi,

I have this graph:

Contact : Person
- Address : Address

(a Contact class with an Address property which is also an entity.

When calling _collectionChangeTracker.MergeChanges(contact), even if the Address property's entity is Unchanged, the property is still updated with the empty data from server. Unchanged entities should be ignored by the CTC.

[tonysneed]

Hi Shimmy! Hope all is well .. I'll look into this.

[weitzhandler]

Thanks Tony, good to see you again, hope you're all well!

I tried to dig into this issue and I have a question, I see that IIdentifiable only exposes SetEntityIdentifier, is there a way to retrieve an EntityIdentifier from an ITrackable or IIdentifiable so I can skip those entities on MergeChanges? This is a blocker issue in my app, any other workarounds would be welcome too.

Best, Shimmy

[weitzhandler]

Do you think we could have EntityBase or IIdentifiable implement IMergeable so that it exposes the EntityIdentifier property? Even as readonly.

This is not a breaking change and makes no difference in the structure of the project.

[weitzhandler]

OK I figured it, IIdentifiable implements IEquatable<IIdentifiable> which gave me a means to check and merge only matching entities. Please check out PR #200.

[tonysneed]

I’ll have a look shortly.

[tonysneed]

Your PR #200 looks good at first glance. I'll run it locally and merge.

[weitzhandler]

Tx!

[tonysneed]

By the way, the MyGet CI build is failing because of a NuGet version issue. I hope to fix that soon, but I will publish a patch to the official NuGet.org site, so you'll be able to use that.

[weitzhandler]

I was using an older version now, thanks for your warning.

[tonysneed]

@weitzhandler Had to revert the PR because there are 4 failing tests that need to be fixed. Can you please re-create #200 and fix those tests? Thanks!

[weitzhandler]

Really sorry my eye missed it away. Will double-check next time.

I had the tests list filtered that's how I could have missed the failing tests, again, sorry.

[weitzhandler]

I see blank IDs were intentionally allowed so that if the server returns a whole new entity (based on a changed ref ID in the parent entity) it should merged it. I'll instead try to dig into LoadRelatedEntitiesAsync which throws errors and see if I can fix something there.

[weitzhandler]

Apparently the issue is related to the server side LoadRelatedEntitiesAsync which throws this exception:

'Id' is not a member of type 'MyProject.Models.MyEntity' in the currently loaded schemas. Near simple identifier, line 1, column 56.

Since my graph is a bit complex, the easiest way I can tunnel into this without trying to build a failing test method (or in order to be able to build such a method...), I'd need to be able to debug the symbol source, hence #204.

[tonysneed]

I'm a bit confused by your reference to LoadRelatedEntitiesAsync. I thought your issue had to do with MergeChanges. Can you provide me a repro?

[weitzhandler]

Using the symbol server, I found the issue, here's what my model looks like:

public class Contact
{
  public int Id { get; set; }
}

public class ContactData
{
  [Key, ForeignKey(nameof(Contact))]
  public int ContactId { get; set; }
  public Contact Contact { get; set; }
}

When I call LoadRelatedEntities, it assumes theId` field names match the parent key name.

In this method, it uses FromProperties while it should have use the ToProperties instead. I'm gonna try to change it and see if the tests pass, otherwise, we'll have to expand the functionality so it can deal with non-matching foreign key names of related entities.

[tonysneed]

Fixed by #206.

[tonysneed]

It looks like the test I added isn't failing, so I'm unable to replicate the issue: LoadRelatedEntities_Should_Populate_Entities_With_NonMatching_Foreign_Keys.

@weitzhandler Can you please have a look at this test to see if I set it up correctly and whether it is property replicating your issue? If not, please correct it in a PR. Otherwise, please add your own failing test. Thanks!

Update: I'm looking into this now ...

[tonysneed]

The bug does not show up with M-1 relations, but just 1-1 relations. Adding a test for that. The fix will need to check the relationship type. I'll implement.

[tonysneed]

@weitzhandler So it looks like supporting non-matching keys for 1-1 relations with LoadRelatedEntities requires a great deal of code refactoring. I don't have the bandwidth to take this on, but I'm willing to accept a PR that gets the failing test to pass in PR #210, so long as all the other unit and integration tests still pass.

However, the workaround for this is for you to set the null reference types in your controller actions. LoadRelatedEntities is merely a convenience method that handles the most general cases, relieving the developer from needing to set reference properties after updates most of the time. But there is nothing to stop you from setting these properties yourself to handle edge cases. Let me know, and I can show you an example of what the workaround would look like.

[weitzhandler]

Let me know, and I can show you an example of what the workaround would look like.

Please. Even a mere comment might be helpful. Thank you so much!

[tonysneed]

You bet .. so what you want to do is to replace the code in the POST and/or PUT actions your controller that calls LoadRelatedEntities with code that explicitly loads the properties you want populated. Here are the docs on this.

For example, to load the Customer property on an order:

await _dbContext.Entry(order).Reference(o => o.Customer).LoadAsync();

Hope this helps!

[weitzhandler]

1. The problem is that they have a different entity identifier, which the client fails to recognize
2. Certain entities, I prefer not to load since there is no need for them
3. The whole idea is automation of loading the entities in the whole graph, which in my project is spread over 4-6 generations of classes with many relationships and references.

[tonysneed]

1. The problem is that they have a different entity identifier, which the client fails to recognize Can you provide an example or more info? Im not sure how the entity identifier is related to loading related entities.

[tonysneed]

PS. I can fix LRE so it doesn’t throw an exception if 1-1 relations with non-matching keys are in your model. And I’ll accept a PR if you want to implement the feature so that the entities are populated.

[tonysneed]

@weitzhandler Update: PR #210 has been merged so that LRE will no longer throw an exception if there are 1-1 relations in your model with non-matching keys. I left asserts commented out in the test, so that you can try to implement support for your requested feature.

2. Certain entities, I prefer not to load since there is no need for them
3. The whole idea is automation of loading the entities in the whole graph, which in my project is spread over 4-6 generations of classes with many relationships and references.

What I would suggest to add an overload of LoadRelatedEntities that accepts a lambda so you can provide a callback in which you can place code that manually loads the related entity. There you can place logic for determining whether or not to load the related entity. Would this seem like a viable option for you?

[weitzhandler]

@tonysneed Can we detect on a relation that it's gonna fail, which in case we'll skip loading it from the DB to avoid a global exception when calling LoadRelatedEntities?

[tonysneed]

You should not be getting an exception when you call LRE with the latest MyGet package. Is that still happening for you?

I’ll check the MyGet symbols to see what’s happening there.

[weitzhandler]

Maybe I had to clean the solution in order to erase the old DLLs. After doing so, it looks like the problem is gone. So I'm closing this issue for now.

Thank you very much for everything Tony!

[tonysneed]

You bet .. happy to help! Let me know if a callback parameter in LRE would be helpful, which would provide a reference to the entity entry so that you could manually call LoadProperty on it. That way, related entities could be loaded manually all the way down the object graph.