Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #3068: Throws ResourceTypeAnnotationNotFirst error when payload… #3069

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixes #3068: Throws ResourceTypeAnnotationNotFirst error when payload… #3069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a98f2b7
Fixes #3068: Throws ResourceTypeAnnotationNotFirst error when payload…
xuzhg Sep 20, 2024
4f16d6e
Address the comments
xuzhg Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/Microsoft.OData.Core/Json/BufferingJsonReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,64 @@ internal BufferingJsonReader(IJsonReader innerReader, string inStreamErrorProper
this.currentBufferedNode = null;
}

internal bool TryGetValueFromBuffered(string propertyName, bool removeIfExist, out object value)
Copy link
Member

@mikepizzo mikepizzo Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
bool found = false;
BufferedNode currentNode = this.bufferedNodesHead;
if (this.bufferedNodesHead != null)
{
do
{
if (currentNode.NodeType == JsonNodeType.StartObject ||
currentNode.NodeType == JsonNodeType.StartArray)
{
break;
}

if (currentNode.NodeType == JsonNodeType.Property &&
currentNode.Value.ToString() == propertyName &&
currentNode.Next != currentNode) // must have another node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do check whether there's another node after this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. Property and value are two node types and they are stored in separate node.

{
found = true;
break;
}

currentNode = currentNode.Next;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can currentNode.Next ever return null? Should account for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have the 'bufferedNodesHead', the 'Next' is not null because if it's empty, 'bufferedNodesHead.Next == bufferedNodesHead'.

}
while (currentNode != this.bufferedNodesHead);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the logics in 'BefferingJsonReader' and that's the way to check the end of link.

}

value = null;
if (!found)
{
return false;
}

BufferedNode valueNode = currentNode.Next;

if (removeIfExist)
{
currentNode.Previous.Next = valueNode.Next;
valueNode.Next.Previous = currentNode.Previous;

if (this.bufferedNodesHead == currentNode)
{
this.bufferedNodesHead = valueNode.Next;
}

if (this.isBuffering)
{
if (this.currentBufferedNode == currentNode || this.currentBufferedNode == valueNode)
{
this.currentBufferedNode = this.bufferedNodesHead;
}
}
}

value = valueNode.Value;
return true;
}

/// <summary>
/// The type of the last node read.
/// </summary>
Expand Down
17 changes: 17 additions & 0 deletions src/Microsoft.OData.Core/Json/ODataJsonResourceDeserializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@ internal void ReadResourceTypeName(IODataJsonReaderResourceState resourceState)
Debug.Assert(resourceState != null, "resourceState != null");
this.AssertJsonCondition(JsonNodeType.Property, JsonNodeType.EndObject);

object value;
if (this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.PrefixedODataTypePropertyName, removeIfExist: true, out value) ||
this.JsonReader.TryGetValueFromBuffered(ODataJsonConstants.SimplifiedODataTypePropertyName, removeIfExist: true, out value))
{
resourceState.Resource.TypeName = ReaderUtils.AddEdmPrefixOfTypeName(ReaderUtils.RemovePrefixOfTypeName(value as string));
return;
}

// If the current node is the odata.type property - read it.
if (this.JsonReader.NodeType == JsonNodeType.Property)
{
Expand Down Expand Up @@ -821,10 +829,16 @@ internal object ReadEntryInstanceAnnotation(string annotationName, bool anyPrope
return this.ReadAndValidateAnnotationStringValue(annotationName);

case ODataAnnotationNames.ODataRemoved: // 'odata.removed'
// If the value of 'odata.removed' is an object, let's throw exception since it should be not read here.
if (this.JsonReader.NodeType == JsonNodeType.StartObject)
{
throw new ODataException(ODataErrorStrings.ODataJsonResourceDeserializer_UnexpectedDeletedEntryInResponsePayload);
}

// for others, let's skip it
this.JsonReader.SkipValue();
return null;

default:
ODataAnnotationNames.ValidateIsCustomAnnotationName(annotationName);
Debug.Assert(
Expand Down Expand Up @@ -905,6 +919,9 @@ internal void ApplyEntryInstanceAnnotation(IODataJsonReaderResourceState resourc
mediaResource.ETag = (string)annotationValue;
break;

case ODataAnnotationNames.ODataRemoved: // 'odata.removed'
break;

default:
ODataAnnotationNames.ValidateIsCustomAnnotationName(annotationName);
Debug.Assert(
Expand Down
153 changes: 153 additions & 0 deletions ...ore.Tests/IntegrationTests/Evaluation/AutoComputePayloadMetadataInJsonIntegrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,159 @@ private static IEdmModel GetModelWithAbstractType(out EdmEntitySet entitySet)
return model;
}

[Fact]
public void ReadingResourceWithInstanceAnnotationAndODataTypeWorks1()
{
const string payload =
"{\"@odata.context\":\"http://svc/$metadata#EntitySet/$entity\"," +
"\"@odata.type\":\"#Namespace.EntityType\"," +
"\"Name\":\"SampleName\"," +
"\"@removed\":false," +
"\"ID\":89}";

EdmEntitySet entitySet = EntitySet;
IEdmModel model = Model;
IEdmEntityType entityType = EntitySet.EntityType;

InMemoryMessage message = new InMemoryMessage();
message.SetHeader("Content-Type", "application/json;odata.metadata=minimal");
message.Stream = new MemoryStream(Encoding.UTF8.GetBytes(payload));

ODataResource topLevelResource = null;
ODataMessageReaderSettings settings = new ODataMessageReaderSettings(ODataVersion.V401)
{
};

using (var messageReader = new ODataMessageReader((IODataResponseMessage)message, settings, model))
{
var reader = messageReader.CreateODataResourceReader(entitySet, entityType);
while (reader.Read())
{
switch (reader.State)
{
case ODataReaderState.ResourceEnd:
topLevelResource = (ODataResource)reader.Item;
break;
}
}
}

Assert.NotNull(topLevelResource);
Assert.Equal(new Uri("http://svc/EntitySet(89)"), topLevelResource.Id);

Assert.Equal("Namespace.EntityType", topLevelResource.TypeName);
Assert.Equal("SampleName", Assert.IsType<ODataProperty>(topLevelResource.Properties.First(p => p.Name == "Name")).Value);
Assert.Equal(89, Assert.IsType<ODataProperty>(topLevelResource.Properties.First(p => p.Name == "ID")).Value);
}

[Fact]
public void ReadingResourceWithInstanceAnnotationAndODataTypeWorks()
{
const string payload =
"{\"@odata.context\":\"http://svc/$metadata#EntitySet/$entity\"," +
"\"@odata.type\":\"#Namespace.EntityType\"," +
"\"Name\":\"SampleName\"," +
"\"@NS.removed\":false," +
"\"ID\":89}";

EdmEntitySet entitySet = EntitySet;
IEdmModel model = Model;
IEdmEntityType entityType = EntitySet.EntityType;

InMemoryMessage message = new InMemoryMessage();
message.SetHeader("Content-Type", "application/json;odata.metadata=minimal");
message.Stream = new MemoryStream(Encoding.UTF8.GetBytes(payload));

ODataResource topLevelResource = null;
ODataMessageReaderSettings settings = new ODataMessageReaderSettings(ODataVersion.V401)
{
ShouldIncludeAnnotation = (annotation) => true
};

using (var messageReader = new ODataMessageReader((IODataResponseMessage)message, settings, model))
{
var reader = messageReader.CreateODataResourceReader(entitySet, entityType);
while (reader.Read())
{
switch (reader.State)
{
case ODataReaderState.ResourceEnd:
topLevelResource = (ODataResource)reader.Item;
break;
}
}
}

Assert.NotNull(topLevelResource);
Assert.Equal(new Uri("http://svc/EntitySet(89)"), topLevelResource.Id);

Assert.Equal("Namespace.EntityType", topLevelResource.TypeName);
Assert.Equal("SampleName", Assert.IsType<ODataProperty>(topLevelResource.Properties.First(p => p.Name == "Name")).Value);
Assert.Equal(89, Assert.IsType<ODataProperty>(topLevelResource.Properties.First(p => p.Name == "ID")).Value);

ODataInstanceAnnotation annotation = Assert.Single(topLevelResource.InstanceAnnotations);
Assert.Equal("NS.removed", annotation.Name);
Assert.Equal(false, annotation.Value.FromODataValue());
}

[Fact]
public void ReadingDeletedResourceContainsODataTypeWorks()
{
const string payload =
"{\"@odata.context\":\"http://svc/$metadata#EntitySet/$delta\"," +
"\"value\":[" +
"{" +
"\"@odata.type\":\"#Namespace.EntityType\"," +
"\"Name\":\"SampleName\"," +
"\"@removed\":{\"reason\":\"deleted\"}," +
"\"ID\":89" +
"}" +
"]" +
"}";

EdmEntitySet entitySet = EntitySet;
IEdmModel model = Model;
IEdmEntityType entityType = EntitySet.EntityType;

InMemoryMessage message = new InMemoryMessage();
message.SetHeader("Content-Type", "application/json;odata.metadata=minimal");
message.Stream = new MemoryStream(Encoding.UTF8.GetBytes(payload));

ODataMessageReaderSettings settings = new ODataMessageReaderSettings(ODataVersion.V401);

ODataDeletedResource deletedResource = null;
using (var messageReader = new ODataMessageReader((IODataResponseMessage)message, settings, model))
{
var reader = messageReader.CreateODataDeltaResourceSetReader(entitySet, entityType);
while (reader.Read())
{
switch (reader.State)
{
case ODataReaderState.DeltaResourceSetStart:
break;

case ODataReaderState.DeltaResourceSetEnd:
break;

case ODataReaderState.DeletedResourceStart:
deletedResource = (ODataDeletedResource)reader.Item;
break;
case ODataReaderState.DeletedResourceEnd:
break;
}
}
}

Assert.NotNull(deletedResource);
Assert.Equal(new Uri("http://svc/EntitySet(89)"), deletedResource.Id);

Assert.Equal("Namespace.EntityType", deletedResource.TypeName);

Assert.Equal(2, deletedResource.Properties.Count());
Assert.Equal("SampleName", Assert.IsType<ODataProperty>(deletedResource.Properties.First(p => p.Name == "Name")).Value);
Assert.Equal(89, Assert.IsType<ODataProperty>(deletedResource.Properties.First(p => p.Name == "ID")).Value);
}

[Theory]
[InlineData("minimal")]
[InlineData("full")]
Expand Down
81 changes: 81 additions & 0 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/Json/ReorderingJsonReaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ await SetupReorderingJsonReaderAndRunTestAsync(
await reorderingReader.ReadPrimitiveValueAsync());
});
}

[Fact]
public async Task ReadReorderedPayloadContainingSimplifiedODataAnnotationsAsync()
{
Expand Down Expand Up @@ -345,6 +346,86 @@ await SetupReorderingJsonReaderAndRunTestAsync(
});
}

[Fact]
public async Task ReadReorderedPayload_OrderingContextRemovedTypeIdAsync()
{
var payload = "{" +
"\"Id\":1," +
"\"@odata.id\":\"http://tempuri.org/Customers(1)\"," +
"\"Name\":\"Sue\"," +
"\"@odata.removed\":{\"reason\":\"deleted\"}," +
"\"@odata.type\":\"SomeEntityType\"," +
"\"@odata.context\":\"any\"" +
"}";

await SetupReorderingJsonReaderAndRunTestAsync(
payload,
async (reorderingReader) =>
{
// 1. should be '@odata.context'
Assert.Equal("@odata.context", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip context value

// 2. should be '@odata.removed'
Assert.Equal("@odata.removed", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

// 3. should be '@odata.type'
Assert.Equal("@odata.type", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

// 4. should be '@odata.id'
Assert.Equal("@odata.id", await reorderingReader.ReadPropertyNameAsync());
Assert.Equal("http://tempuri.org/Customers(1)", await reorderingReader.ReadPrimitiveValueAsync());

Assert.Equal("Id", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

Assert.Equal("Name", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value
});
}

[Fact]
public async Task ReadReorderedPayload_OrderingContextRemovedTypeId_ContainingSimplifiedAnnotationAsync()
{
var payload = "{" +
"\"Id\":1," +
"\"@id\":\"http://tempuri.org/Customers(1)\"," +
"\"Name\":\"Sue\"," +
"\"@removed\":{\"reason\":\"deleted\"}," +
"\"@context\":\"any\"," +
"\"@type\":\"SomeEntityType\""+
"}";

await SetupReorderingJsonReaderAndRunTestAsync(
payload,
async (reorderingReader) =>
{
// 1. should be '@context'
Assert.Equal("@context", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip context value

// 2. should be '@removed'
Assert.Equal("@removed", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

// 3. should be '@type'
Assert.Equal("@type", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

// 4. should be '@id'
Assert.Equal("@id", await reorderingReader.ReadPropertyNameAsync());
Assert.Equal("http://tempuri.org/Customers(1)", await reorderingReader.ReadPrimitiveValueAsync());

Assert.Equal("Id", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value

Assert.Equal("Name", await reorderingReader.ReadPropertyNameAsync());
await reorderingReader.SkipValueAsync(); // Skip object value
});
}

[Fact]
public async Task ReadBinaryValueAsync()
{
Expand Down