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

Fix subtle bug when deserializing property without value #1308

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix subtle bug when deserializing property without value #1308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion src/Microsoft.AspNetCore.OData/Formatter/Deserialization/ODataResourceDeserializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,14 @@ public virtual void ApplyStructuralProperties(object resource, ODataResourceWrap
throw new ArgumentNullException(nameof(resourceWrapper));
}

foreach (ODataProperty property in resourceWrapper.Resource.Properties)
foreach (ODataPropertyInfo propertyInfo in resourceWrapper.Resource.Properties)
Copy link
Member

Choose a reason for hiding this comment

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

why don't use 'OfType', so we don't need line 494 to 499.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was deliberate. I believe OfType would be more expensive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xuzhg Here's the code for a simple benchmark to compare the two:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Diagnosers;
using System.Collections.Generic;

[MemoryDiagnoser]
public class Benchmarks
{
    private IEnumerable<ODataPropertyInfo> properties;

    public Benchmarks()
    {
        List<ODataPropertyInfo> properties = new()
        {
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataPropertyInfo { Name = "A" },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataPropertyInfo { Name = "A" },
            new ODataProperty { Name = "A", Value = 1 },
            new ODataProperty { Name = "A", Value = 1 },
        };

        this.properties = properties;
    }

    [Benchmark]
    public int CountPropertyValues_WithNestedCondition()
    {
        int count = 0;
        foreach (ODataPropertyInfo propertyInfo in properties)
        {
            if (propertyInfo is ODataProperty property) {
                count += property.Value;
            }
        }

        return count;
    }

    [Benchmark]
    public int CountPropertyValues_WithOfType()
    {
        int count = 0;
        foreach (ODataProperty property in properties.OfType<ODataProperty>())
        {
            count += property.Value;
        }

        return count;
    }
}

public class ODataPropertyInfo
{
    public string Name { get; set; }
}

public class ODataProperty : ODataPropertyInfo
{
    public int Value { get; set; }
}

And here are the results:
image

I think that LINQ overheads are responsible for the difference

{
if (!(propertyInfo is ODataProperty property))
{
// Cannot deserialize property without value
continue;
}

ApplyStructuralProperty(resource, property, structuredType, readContext);
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/Microsoft.AspNetCore.OData/Formatter/Wrapper/ODataReaderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ private static void ReadODataItem(ODataReader reader, Stack<ODataItemWrapper> it
resourceSetParentWrapper.Items.Add(new ODataPrimitiveWrapper((ODataPrimitiveValue)reader.Item));
break;

case ODataReaderState.NestedProperty:
// Property without value - do nothing
Copy link
Member

Choose a reason for hiding this comment

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

Why do nothing? need more details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xuzhg What do you suggest we should do here?

Copy link
Member

Choose a reason for hiding this comment

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

I think we 'd add at least:

  1. Why do we need to catch this reader state in this reading loop?
  2. Why do we do nothing in this reader state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't we need a case for this state before?

Copy link
Contributor Author

@gathogojr gathogojr Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@xuzhg @habbes The block for the ODataReaderState.NestedProperty should have been added when we upgraded the ODL dependency to the version that contained this change OData/odata.net#2786.

In OData/odata.net#2786, we introduced support for reading property without value. Before that, we would throw an exception if we came across a property without value. When we changed that, the ODataReaderState.NestedProperty state gets toggled when we read such a property. It's just that after upgrading the ODL dependency, we didn't add any test to verify behaviour when a payload containing a property without value is deserialized. It's easy to verify my theory here. If you add the test in this PR to ASP.NET Core OData release-8.x branch, the control flow will reach the default block and the Debug.Assert statement will fail. That's the reason we need the block that I introduced.

break;

default:
Contract.Assert(false, "We should never get here, it means the ODataReader reported a wrong state.");
break;
Expand Down
34 changes: 34 additions & 0 deletions ...rosoft.AspNetCore.OData.Tests/Formatter/Deserialization/ODataResourceDeserializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1553,6 +1553,40 @@ public void ReadAsync_ThrowsOnUnknownEntityType()
typeof(Product), _readContext).Wait(), "The property 'Concurrency' does not exist on type 'ODataDemo.Product'. Make sure to only use property names that are defined by the type or mark the type as open type.");
}

[Fact]
public async Task ReadAsync_ForPropertyWithoutValue()
{
// Arrange
string content = "{" +
"\"ID\":0," +
"\"Name\":\"Bread\"," +
"\"Description\":\"Whole grain bread\"," +
"\"PublishDate\":\"1997-07-01\"," +
"\"[email protected]\":\"#Edm.DateTimeOffset\"," + // OData annotation - Property without value
"\"[email protected]\":true," + // Custom annotation - Property without value
Copy link
Member

Choose a reason for hiding this comment

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

How about the other type of properties with value? (Complex, enum)

"\"Rating\":4," +
"\"Price\":2.5" +
"}";

ODataResourceDeserializer deserializer = new ODataResourceDeserializer(_deserializerProvider);

// Act
Product product = await deserializer.ReadAsync(
GetODataMessageReader(content, _edmModel),
typeof(Product),
_readContext) as Product;

// Assert
Assert.Equal(0, product.ID);
Assert.Equal(4, product.Rating);
Assert.Equal(2.5m, product.Price);
Assert.Equal(product.PublishDate, new Date(1997, 7, 1));
Assert.Equal("Bread", product.Name);
Assert.Equal("Whole grain bread", product.Description);
Assert.Null(product.ReleaseDate);
Assert.Null(product.DiscontinuedDate);
}

private static ODataMessageReader GetODataMessageReader(IODataRequestMessage oDataRequestMessage, IEdmModel edmModel)
{
return new ODataMessageReader(oDataRequestMessage, new ODataMessageReaderSettings(), edmModel);
Expand Down
Loading