-
Notifications
You must be signed in to change notification settings - Fork 186
Enable the nested delta collection deep update #747
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -144,7 +144,7 @@ public override bool TrySetPropertyValue(string name, object value) | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (value is IDelta) | ||||||
| if (value is IDelta || value is IDeltaSet) | ||||||
| { | ||||||
| return TrySetNestedResourceInternal(name, value); | ||||||
| } | ||||||
|
|
@@ -225,6 +225,14 @@ internal bool TryGetNestedPropertyValue(string name, out object value) | |||||
| object deltaNestedResource = _deltaNestedResources[name]; | ||||||
|
|
||||||
| Contract.Assert(deltaNestedResource != null, "deltaNestedResource != null"); | ||||||
|
|
||||||
| //If DeltaSet collection, we are handling delta collections so the value will be that itself and no need to get instance value | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| if (deltaNestedResource is IDeltaSet) | ||||||
| { | ||||||
| value = deltaNestedResource; | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| Contract.Assert(DeltaHelper.IsDeltaOfT(deltaNestedResource.GetType())); | ||||||
|
|
||||||
| value = deltaNestedResource; | ||||||
|
|
@@ -339,6 +347,15 @@ public void CopyChangedValues(T original) | |||||
| { | ||||||
| // Patch for each nested resource changed under this T. | ||||||
| dynamic deltaNestedResource = _deltaNestedResources[nestedResourceName]; | ||||||
|
|
||||||
| if (deltaNestedResource is IDeltaSet) | ||||||
| { | ||||||
| // TODO: That's the bulk insert OData Path handler feature, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of leaving a TODO in the code, I'd suggest adding a link to an issue in github documenting this feature.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #resolved. |
||||||
| // See the comments in https://github.com/OData/AspNetCoreOData/issues/748 | ||||||
| // So far, Let's skip DeltaSet and figure it out later. | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| dynamic originalNestedResource = null; | ||||||
| if (!TryGetPropertyRef(original, nestedResourceName, out originalNestedResource)) | ||||||
| { | ||||||
|
|
@@ -705,11 +722,14 @@ private bool TrySetNestedResourceInternal(string name, object deltaNestedResourc | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| PropertyAccessor<T> cacheHit = _allProperties[name]; | ||||||
| // Get the Delta<{NestedResourceType}>._instance using Reflection. | ||||||
| FieldInfo field = deltaNestedResource.GetType().GetField("_instance", BindingFlags.NonPublic | BindingFlags.Instance); | ||||||
| Contract.Assert(field != null, "field != null"); | ||||||
| cacheHit.SetValue(_instance, field.GetValue(deltaNestedResource)); | ||||||
| if (!(deltaNestedResource is IDeltaSet)) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the above one. That's C# 9 feature and I 'd skip it in this PR. Thanks. |
||||||
| { | ||||||
| PropertyAccessor<T> cacheHit = _allProperties[name]; | ||||||
| // Get the Delta<{NestedResourceType}>._instance using Reflection. | ||||||
| FieldInfo field = deltaNestedResource.GetType().GetField("_instance", BindingFlags.NonPublic | BindingFlags.Instance); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was already like this in the previous code, but why do we use reflection on a type that we own? Or am I missing something here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think deltaNestedResource is a generic type instance, DeltaSet< T >. We don't know the real type until running. |
||||||
| Contract.Assert(field != null, "field != null"); | ||||||
| cacheHit.SetValue(_instance, field.GetValue(deltaNestedResource)); | ||||||
| } | ||||||
|
|
||||||
| // Add the nested resource in the hierarchy. | ||||||
| // Note: We shouldn't add the structural properties to the <code>_changedProperties</code>, which | ||||||
|
|
||||||
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.
Can we use
orpattern matching here: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.
That's C# 9 feature and it seems we have to update the csproj to enable C# 9 language.
I don't want to introduce that changes in this PR. Thanks.