-
Notifications
You must be signed in to change notification settings - Fork 473
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
bulk ops deserialization #2686
bulk ops deserialization #2686
Conversation
2862c29
to
afa6425
Compare
src/Microsoft.AspNet.OData.Shared/DataModificationExceptionType.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Builder/IODataInstanceAnnotationContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DataModificationExceptionType.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceSetDeserializer.cs
Show resolved
Hide resolved
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.
Left some comments, suggestions and a few things that looked like potential bugs.
The PR seems to introduce potentially breaking changes, is it meant for 7.x?
There seems to be a lot of changes made to existing tests that I didn't quite understand, which lowered my confidence in figuring out whether the PR doesn't break things.
src/Microsoft.AspNet.OData.Shared/Builder/IODataInstanceAnnotationContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
@habbes the main reason there are some changes to the tests is because there is some logic that didn't work as expected before. The logic was changed and now works correctly hence the reason why some tests were removed completely or changes made to them. |
2c5617b
to
829e5d7
Compare
eaf46cc
to
e7cad64
Compare
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/DeltaDeletedEntityObjectOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/CollectionDeserializationHelpers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/CollectionDeserializationHelpers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/DeserializationHelpers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceDeserializer.cs
Outdated
Show resolved
Hide resolved
c9dfae8
to
a06c376
Compare
b1726e8
to
f3a8ab7
Compare
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.
LGTM
2efb99e
to
598ce56
Compare
src/Microsoft.AspNet.OData.Shared/Builder/IODataInstanceAnnotationContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Builder/IODataInstanceAnnotationContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataReaderExtensions.cs
Outdated
Show resolved
Hide resolved
if (TypeHelper.HasDefaultConstructor(newType)) | ||
{ | ||
newOriginalNestedResource = Activator.CreateInstance(newType); | ||
} |
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 believe we should throw an exception if the type has no default constructor. Otherwise return a null will result into a NullReferenceException
when we later call CopyChangedValues
. Would actually be better if you handled that scenario and added a test to confirm that an exception is thrown
e78a146
to
8176c82
Compare
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'm still a bit worried about the ODataResourceDeserializerHelpers
class. The logic in some of those methods look/feel brittle and complex to me, starting with the ApplyIdPath
method. I just hope you have validated as much logic as possible in that class
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
Description
This pull request is a sub of this big PR #2656. We have broken the big PR into 3PRs - deserialization/serialization/ApiHandlers to hasten the review. This PR contains only deserialization changes for bulk operations.
N/B: This PR seems big but it is not really big, most of the changes are minor changes - like removing or adding whitespace where needed.
A sample delta payload will look like below:
some of the main changes are:
We have added the following classes and interfaces to assist in the deserialization of delta payloads- like the one shown above.:
@odata.id
- in parsed format. Used by both POCO objects and DeltaChanges have also been made to the
ODataReaderExtensions
- we check for the readerstate - if there is a deltaresourcesetstart state, we'll create an odatadeltaresourceset.Changes have also been made to the
ODataResourceSetDeserializer
andODataResourceDeserializer
to handle creating a deltaset from a delta request payload.Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.