Fixes #3068: Throws ResourceTypeAnnotationNotFirst error when payload… #3069
Conversation
… has both @removed and @odata.type
xuzhg
requested review from
robertmclaws,
gathogojr,
habbes,
mikepizzo and
ElizabethOkerio
and removed request for
robertmclaws
|
|
||
| currentNode = currentNode.Next; | ||
| } | ||
| while (currentNode != this.bufferedNodesHead); |
There was a problem hiding this comment.
Was wondering how currentNode ever be bufferedNodeHead then I confirmed that this is a circular linked list. I think we should abstract traversal and access of the nodes through some helper method or inner class to avoid accidentally accessing the nodes the wrong way.
There was a problem hiding this comment.
I checked the logics in 'BefferingJsonReader' and that's the way to check the end of link.
| break; | ||
| } | ||
|
|
||
| currentNode = currentNode.Next; |
There was a problem hiding this comment.
can currentNode.Next ever return null? Should account for that?
There was a problem hiding this comment.
If we have the 'bufferedNodesHead', the 'Next' is not null because if it's empty, 'bufferedNodesHead.Next == bufferedNodesHead'.
|
|
||
| if (currentNode.NodeType == JsonNodeType.Property && | ||
| currentNode.Value.ToString() == propertyName && | ||
| currentNode.Next != currentNode) // must have another node |
There was a problem hiding this comment.
Why do check whether there's another node after this?
There was a problem hiding this comment.
Just to see if I understand, property and value are stored in separate node, so if we find the expected property node, we want to make sure there's a value node after it. Is that correct?
There was a problem hiding this comment.
That's correct. Property and value are two node types and they are stored in separate node.
| if (this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.PrefixedODataTypePropertyName, true, out value) || | ||
| this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.SimplifiedODataTypePropertyName, true, out value)) |
There was a problem hiding this comment.
nit: make it clear what the true arguments imply
| if (this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.PrefixedODataTypePropertyName, true, out value) || | |
| this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.SimplifiedODataTypePropertyName, true, out value)) | |
| if (this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.PrefixedODataTypePropertyName, removeIfExists: true, out value) || | |
| this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.SimplifiedODataTypePropertyName, removeIfExists: true, out value)) | |
| @@ -114,6 +122,8 @@ internal void ReadResourceTypeName(IODataJsonReaderResourceState resourceState) | |||
|
|
|||
| // Read the annotation value. | |||
| resourceState.Resource.TypeName = this.ReadODataTypeAnnotationValue(); | |||
|
|
|||
| // resourceState.TypeAnnotationFound = true; | |||
There was a problem hiding this comment.
Why was this statement added then commented out? Should we remove it?
| @@ -81,6 +81,64 @@ internal BufferingJsonReader(IJsonReader innerReader, string inStreamErrorProper | |||
| this.currentBufferedNode = null; | |||
| } | |||
|
|
|||
| internal bool TryGetValueFromBuffered(string propertyName, bool removeIfExist, out object value) | |||
There was a problem hiding this comment.
This could have some pretty severe performance ramifications.
If a payload doesn't have the value that we try to get, we will read the entire payload into memory. For very large, or streamed, responses, this could overflow memory.
This kinda defeats the whole point of ordering, which is that we can process the response in a streaming fashion.
The previous implementation assumes that type, if present, is the first thing we read. So we read the first node and, if it's type, we store that, and if it is not type, we assume there is no type and continue reading the payload as normal.
I would have expected we simply expand that logic. That is, we read the first node. If it is one of our "metadata" nodes (context, removed, type, id), then we get the value and repeat for the next node. As soon as we find a node that is not one of our metadata nodes, we break and start reading the payload as normal.
… has both @removed and @odata.type
Issues
This pull request fixes #3068.
Description
Make sure the reordering json reader reorder @context, @removed, @type, @id in this ordering.
@mikepizzo, Why does @removed should be property before @type?
We cannot assume the end customer is reading the payload using delta reader or normal entity reader, so, we don't know whether the @type position is the first property or second property if it contains @context. So, in this PR, try to retrieve the @type directly from the buffer. But, we should make sure:
a) Can a normal entity payload contain 'odata.removed'?
b) Can we allow use 'resource reader' to read the payload containing 'odata.removed'?
@mikepizzo
We can skip the value/validation if the 'odata.removed' has the wrong value. It's same concern that if we do like this solution?
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.