Fix regression causing navigation properties to be auto-expanded in typeless scenarios

[gathogojr]

Fixes #1412

Fix regression causing navigation properties to be auto-expanded in typeless scenarios

The challenge we face is that EdmEntityObject, EdmComplexObject, EdmDeltaResourceObject, EdmDeltaComplexObject, and EdmDeltaDeletedResourceObject (together with their typed internal equivalents TypedEdmEntityObject and TypedEdmComplexObject) all implemement IDelta, IDeltaSetItem, and IEdmChangedObject interfaces directly or indirectly.

Illustration:

using Microsoft.AspNetCore.OData.Deltas;
using Microsoft.AspNetCore.OData.Formatter.Value;
using Microsoft.OData.Edm;

var orderEntityType = new EdmEntityType("NS", "Order");
var addressComplexType = new EdmComplexType("NS", "Address");

var orderEntityObject = new EdmEntityObject(orderEntityType);
var addressComplexObject = new EdmComplexObject(addressComplexType);
var orderDeltaResourceObject = new EdmDeltaResourceObject(orderEntityType);
var addressDeltaComplexObject = new EdmDeltaComplexObject(addressComplexType);
var orderDeltaDeletedResourceObject = new EdmDeltaDeletedResourceObject(orderEntityType);

Console.WriteLine($"EdmEntityObject - Is IDelta: {orderEntityObject is IDelta}, Is DeltaSetItem: {orderEntityObject is IDeltaSetItem}, Is IEdmChangedObject: {orderEntityObject is IEdmChangedObject}, Is DeltaResource: {orderEntityObject.IsDeltaResource()}");
Console.WriteLine($"EdmComplexObject - Is IDelta: {addressComplexObject is IDelta}, Is DeltaSetItem: {addressComplexObject is IDeltaSetItem}, Is IEdmChangedObject: {addressComplexObject is IEdmChangedObject}, Is DeltaResource: {addressComplexObject.IsDeltaResource()}");
Console.WriteLine($"EdmDeltaResourceObject - Is IDelta: {orderDeltaResourceObject is IDelta}, Is DeltaSetItem: {orderDeltaResourceObject is IDeltaSetItem}, Is IEdmChangedObject: {orderDeltaResourceObject is IEdmChangedObject}, Is DeltaResource: {orderDeltaResourceObject.IsDeltaResource()}");
Console.WriteLine($"EdmDeltaComplexObject - Is IDelta: {addressDeltaComplexObject is IDelta}, Is DeltaSetItem: {addressDeltaComplexObject is IDeltaSetItem}, Is IEdmChangedObject: {addressDeltaComplexObject is IEdmChangedObject}, Is DeltaResource: {addressDeltaComplexObject.IsDeltaResource()}");
Console.WriteLine($"EdmDeltaDeletedResourceObject - Is IDelta: {orderDeltaDeletedResourceObject is IDelta}, Is DeltaSetItem: {orderDeltaDeletedResourceObject is IDeltaSetItem}, Is IEdmChangedObject: {orderDeltaDeletedResourceObject is IEdmChangedObject}, Is DeltaResource: {orderDeltaDeletedResourceObject.IsDeltaResource()}");

Result:

EdmEntityObject - Is IDelta: True, Is DeltaSetItem: True, Is IEdmChangedObject: True, Is DeltaResource: True
EdmComplexObject - Is IDelta: True, Is DeltaSetItem: True, Is IEdmChangedObject: True, Is DeltaResource: True
EdmDeltaResourceObject - Is IDelta: True, Is DeltaSetItem: True, Is IEdmChangedObject: True, Is DeltaResource: True
EdmDeltaComplexObject - Is IDelta: True, Is DeltaSetItem: True, Is IEdmChangedObject: True, Is DeltaResource: True
EdmDeltaDeletedResourceObject - Is IDelta: True, Is DeltaSetItem: True, Is IEdmChangedObject: True, Is DeltaResource: True

For the above reason, you can't tell by interrogating the object itself where it's a delta resource - specifically for EdmEntityObject and EdmComplexObject that can be present in both delta and non-delta payloads.

What this basically means, is that if you have a controller action like the typeless scenario below, where Customer is a navigation property, the navigation property is auto-expanded whether or not there's an $expand on the URL:

public EdmEntityObjectCollection Get()
{
    var customerObject = new EdmEntityObject(TypelessEdmModel.CustomerEntityType);
    customerObject.TrySetPropertyValue("Id", 1);
    customerObject.TrySetPropertyValue("Name", "Sue");

    var orderObject = new EdmEntityObject(TypelessEdmModel.OrderEntityType);
    orderObject.TrySetPropertyValue("Id", 1);
    orderObject.TrySetPropertyValue("Amount", 310m);
    orderObject.TrySetPropertyValue("Customer", customerObject);

    return new EdmEntityObjectCollection(
        new EdmCollectionTypeReference(
            new EdmCollectionType(
                new EdmEntityTypeReference(TypelessEdmModel.OrderEntityType, false))))
    {
        orderObject
    };
}

The buggy logic is here:

AspNetCoreOData/src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSerializer.cs

Line 497 in c60d9e0

bool isDelta = graph is IDelta || graph is IEdmChangedObject;

We use the ODataResourceSerializer to serialize both delta and non-delta payloads. When the above statement is evaluated, it'll always return true for nearly all (if not all) typeless Edm objects, including those in the code sample above, that shouldn't be serialized as a delta.

Telling whether an object is a delta or non-delta resource in typeless scenarios becomes a dicey affair.
I propose in this pull request to use the serializer context (via ODataSerializerContext) to indicate whether its a delta payload being written or not.

This change fixes the regression and addresses the challenge described above.

[ElizabethOkerio]

Are we using these method calls to determine when the writeContext.isDelta is set? What if someone overrides these methods? They'll have to set this property explicitly? What happens if they don't?

[anasik]

@ElizabethOkerio , in #1413, I solved the same bug with a simple type comparison that only modifies one function in one file.

It's practically a one-liner and I would appreciate it if someone could review it. I think it's the most viable fix for this problem. I also added some tests very specific to the reported use-case.

[gathogojr]

@ElizabethOkerio Following our offline discussion, I have addressed the issue by setting the property from the output formatter where the serializer context is initialized

[anasik]

The TypelessDataModel you created here under seems very similar to the one under Untyped. Are you sure your new tests wouldn't be well suited to that folder?

In fact, your entire Typeless folder seems very similar to the Untyped folder.

Meanwhile, the existing Typeless folder looks very different from this so adding these tests to that folder would be a drastic change.

[gathogojr]

The TypelessDataModel you created here under seems very similar to the one under Untyped. Are you sure your new tests wouldn't be well suited to that folder?

In fact, your entire Typeless folder seems very similar to the Untyped folder.

Meanwhile, the existing Typeless folder looks very different from this so adding these tests to that folder would be a drastic change.

@anasik I went through the Untyped folder and I don't see a significant overlap. As a matter of fact, the Edm model in the Untyped folder is typed. In addition, the Typeless folder already had existing tests for typeless scenario

[ElizabethOkerio]

This is not a typeless data model. I don't know can we use a name for these tests and this folder that depicts what is being tested. Exa. DeltasWithTyplessModels or something like that.

[gathogojr]

@ElizabethOkerio Do note the comment I added before the first model:

// Types included for comparison with typeless scenario

The main focus of the tests is typeless scenario but I added the models to verify that the serialized result is the same

[WanjohiSammy]

LGTM