fix: support for lazy loading proxies #793
Conversation
|
Is there some common type that is used for proxies that can be checked for, instead of checking for general inheritance? This looks wrong, given that resources may derive from other resources. |
|
Did some research and I found a way to discriminate dynamic proxies from any regular class in the project. Since You'll notice I updated the tests to use actual proxies now. I generate them with |
|
Thanks, this is a lot better! I'd prefer to avoid adding a dependency on the Castle.Proxies nuget package from JADNC (for tests it is fine, though I think it should depend on Microsoft.EntityFrameworkCore.Proxies instead). Can we have a runtime check, so it lights up when available? Something along the lines of GetType(), perhaps cache the resulting type in a private field for performance. It would be nice to have an integration test (using TestServer) that shows your original use case (query succeeds). I have never used EF Core lazy loading, but looking at the docs this seems like all-or-nothing. So unless we can configure this for one resource/relationship, it's probably too much to ask. |
|
About setting up lazy loading for just one resource: as far as I know this is possible but not using lazy loading proxies. The ideal course of action to have integration tests for this particular use case would be for me to add another example app and its corresponding test project, which I can do no problem! In fact, I modified the Getting Started example to test the idea, I could just make this new project be the same one with those changes if you think that's alright. As for the other comments, I'll have to do some more digging in order to figure out a way to identify proxies without adding any dependencies. Did you have any suggestions as to how I could approach this? |
|
Tests: Another approach could be to add a second DbContext to the existing example project, but you'd need a fix for #739 first. Alternatively, #792 adds support for integration tests where DbContext is defined in the test project. I'm okay with leaving it out for now, we can add such a test later. Dynamic loading of types: this may help to get you started. |
|
Managed to do the check without the dependency, thanks for the tip! As you said, I did add a dependency to |
| @@ -13,10 +13,12 @@ namespace JsonApiDotNetCore.Internal | |||
| public class ResourceGraph : IResourceGraph | |||
| { | |||
| private List<ResourceContext> Resources { get; } | |||
| private Type ProxyInterface { get; } | |||
There was a problem hiding this comment.
I believe this field should be named proxyTargetAccessorType, because that is what it is. Additionally, its value does not change during execution, so it can be evaluated once:
private static readonly proxyTargetAccessorType = Type.GetType("Castle.DynamicProxy.IProxyTargetAccessor, Castle.Core");| @@ -125,6 +129,8 @@ public RelationshipAttribute GetInverse(RelationshipAttribute relationship) | |||
| $"For example: 'article => article.Title' or 'article => new {{ article.Title, article.PageCount }}'."); | |||
| } | |||
|
|
|||
| private bool IsDynamicProxy(Type resourceType) => ProxyInterface?.IsAssignableFrom(resourceType) ?? false; | |||
There was a problem hiding this comment.
A better name for this method would indicate what it functionally represents, for example: IsLazyLoadingProxyForResourceType(). The usage of a specific interface to answer the question is an implementation detail.
| @@ -41,12 +43,47 @@ public void Adding_DbContext_Members_That_Do_Not_Implement_IIdentifiable_Logs_Wa | |||
| Assert.Equal("Entity 'UnitTests.Internal.ResourceGraphBuilder_Tests+TestContext' does not implement 'IIdentifiable'.", loggerFactory.Logger.Messages[0].Text); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void GetResourceContext_Yields_Right_Type_For_Proxy() | |||
There was a problem hiding this comment.
Consider: GetResourceContext_Yields_Right_Type_For_LazyLoadingProxy
|
Just did all the things you requested 😄 |
I found that when I enabled lazy loading proxies queries would fail.
This was because the
GetResourceContext(Type)method withinResourceGraphwas checking for strict equality of the registered and provided resource types. I added a call toType.IsAssignableFrom(Type)after that equality check and now when the passed type is a lazy loading proxy the graph is able to identify the entity that corresponds to it.I kept the equality check because some tests would break if I didn't. The reason is, I assume, that whenever you had two or more entities that had a common base type calls to
SingleOrDefaultthrow that the resulting sequence has more than one element. Keeping the equality check prioritizes types that are exactly equal to those you're searching for.Please let me know if you think this could be polished in any way!