-
Notifications
You must be signed in to change notification settings - Fork 352
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
Fixed the handling of empty http attributes in Batch Requests #2705
Fixed the handling of empty http attributes in Batch Requests #2705
Conversation
…emPropertiesCache.cs
…emPropertiesCache.cs
…ailure due to an Exception
…PropertiesCache, throwing an exception for duplicate properties, except header properties.
…ow also throw an exception.
…ded test cases for duplicate headers and properties
…on property unit tests
…dling of JSON property and removed corresponding unit tests.
…t-handling-of-duplicate-http-attributes Bugfix/issue 2656/batch request handling of duplicate http attributes
…sonLightBatchPayloadItemPropertiesCache, refs: OData#2656
…emPropertiesCache.cs
…PropertiesCache, throwing an exception for duplicate properties, except header properties.
…ow also throw an exception.
…dling of JSON property and removed corresponding unit tests.
…sonLightBatchPayloadItemPropertiesCache, refs: OData#2656
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) |
@gathogojr would it be possible to review this follow-up task of PR-2669? |
Hi @apiaskowski We do squash the commits when merging but a reviewer who's not familiar with the background might wonder and indeed ask why there are so many commits. If it's not too much of a hassle, consider squashing them. |
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.
Sure, I'll do. Typically I'd "squash & merge", but since I am not able to do so here, I'd have to do it here manually by git rebase? |
Yeah. Basically |
Issues
This pull request fixes Issue-2656
Description
OData batching does return an HTTP 500 error, if the request includes a empty
null
value HTTP attributes.This issue is caused by not proper validation of the JSON property value, when serializing it into a String variable.
Checklist (Uncheck if it is not completed)
Additional work necessary
The current implementation does handle empty http attributes, by validating, if the JSON property is empty, i.e.
null
.But as discussed in previous PR-2669, these should not be handled in this way and instead throw an
ODataException
.If so, the odata specification need to be adjusted and highlight, that empty http attributes / JSON properties are not allowed.