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.

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

Fix subtle bug when deserializing property without value #1308

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

Conversation

gathogojr
Copy link
Contributor

Fixes #1307 by handling a property without value gracefully when deserializing OData payloads

@gathogojr gathogojr force-pushed the fix/1307-subtle-bug-when-deserializing-property-without-value branch from 5e18dff to 3337a09 Compare September 6, 2024 12:08
@@ -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

@@ -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

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.

"\"Description\":\"Whole grain bread\"," +
"\"PublishDate\":\"1997-07-01\"," +
"\"ReleaseDate@odata.type\":\"#Edm.DateTimeOffset\"," + // OData annotation - Property without value
"\"DiscontinuedDate@Is.DateTimeOffset\":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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subtle bug in deserializing payload with property without value
3 participants