ClrCollectionAccessor cleanup for complex JSON collections #36618
Conversation
| var result = (TResult)collectionAccessor!.Create(); | ||
| // var collectionAccessor = relationship.GetCollectionAccessor(); | ||
| #pragma warning disable EF1001 // Internal EF Core API usage. | ||
| var collectionAccessor = ((IRuntimePropertyBase)relationship).GetIndexedCollectionAccessor(); |
There was a problem hiding this comment.
GetIndexedCollectionAccessor() is only available on IRuntimePropertyBase (unlike GetCollectionAccessor() which is available on IPropertyBase). I don't have strong feelings here but maybe seems like they should be consistent?
Also note that GetIndexedCollectionAccessor is pubternal (another thing to maybe change).
There was a problem hiding this comment.
Yes, this is pubternal until we are satisfied with the API
| @@ -1098,7 +1103,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync( | |||
| { | |||
| manager.CaptureState(); | |||
| var entity = innerShaper(queryContext, newKeyPropertyValues, jsonReaderData); | |||
| collectionAccessor.AddStandalone(result, entity); | |||
| // collectionAccessor.AddStandalone(result, entity); | |||
There was a problem hiding this comment.
I have no idea what this means (yet) but it doesn't exist on ClrIndexedCollectionAccessor.
There was a problem hiding this comment.
It allows to add an element to the collection without the containing object. We could add SetStandalone and CreateStandalone if necessary
There was a problem hiding this comment.
Actually, Create should just be renamed to CreateStandalone as that would reflect the current behavior. Note that because the containing object could be a value type you would need to either use CreateStandalone with SetStandalone to populate the collection before materializing the containing object. Or use CreateStandalone and Set after materializing it.
@AndriySvyryd, this is some WIP on switching from ClrCollectionAccessor to ClrIndexCollectionAccessor for complex JSON collections in the shaper, as discussed offline. See below for some possible issues - feel free to push changes to this PR from your side (or completely take over it if you want.