- Notifications
You must be signed in to change notification settings - Fork3.3k
ClrCollectionAccessor cleanup for complex JSON collections#36618
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:release/10.0
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| varresult=(TResult)collectionAccessor!.Create(); | ||
| // var collectionAccessor = relationship.GetCollectionAccessor(); | ||
| #pragma warning disableEF1001// Internal EF Core API usage. | ||
| varcollectionAccessor=((IRuntimePropertyBase)relationship).GetIndexedCollectionAccessor(); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is pubternal until we are satisfied with the API
| manager.CaptureState(); | ||
| varentity=innerShaper(queryContext,newKeyPropertyValues,jsonReaderData); | ||
| collectionAccessor.AddStandalone(result,entity); | ||
| //collectionAccessor.AddStandalone(result, entity); |
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.
I have no idea what this means (yet) but it doesn't exist on ClrIndexedCollectionAccessor.
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 allows to add an element to the collection without the containing object. We could addSetStandalone andCreateStandalone if necessary
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.
Actually,Create should just be renamed toCreateStandalone as that would reflect the current behavior. Note that because the containing object could be a value type you would need to either useCreateStandalone withSetStandalone to populate the collection before materializing the containing object. Or useCreateStandalone andSet 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.