Support type cast in group by

[clemvnt]

Hello,

Cast type support in group by.

This PR needs OData/odata.net#2879.

Issues

#1180

Description

For the following group by : groupby((MyAddress/Fully.Qualified.Namespace.HomeAddress/HomeNO))

The response was an error :

System.NotSupportedException: The query specified in the URI is not valid. Binding OData QueryNode of kind 'SingleResourceCast' is not supported by 'AggregationBinder'.

Now, the response is :

[
  {
    "myAddress": {
        "homeNO": "Value1"
    }
  },
  {
    "myAddress": {
        "homeNO": "Value2"
    }
  }
]

[clemvnt]

@microsoft-github-policy-service agree

[wachugamaina]

Could we stick with using var for consistency ?

[clemvnt]

This method is based on the following method :

AspNetCoreOData/src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.cs

Lines 692 to 703 in 7e2e707

	public virtual Expression BindSingleResourceCastNode(SingleResourceCastNode node, QueryBinderContext context)
	{
	CheckArgumentNull(node, context);
	IEdmStructuredTypeReference structured = node.StructuredTypeReference;
	Contract.Assert(structured != null, "NS casts can contain only structured types");
	Type clrType = context.Model.GetClrType(structured);
	Expression source = BindCastSourceNode(node.Source, context);
	return Expression.TypeAs(source, clrType);
	}

There doesn't seem to be any consistency between explicit types and var in the project.

But if someone of the organization feels the same way, I'll make the changes.

[wachugamaina]

Could we stick with using var for consistency ?

[clemvnt]

See previous comment.

[wachugamaina]

Could we stick with using var for consistency ?

[clemvnt]

See previous comment.

[wachugamaina]

Could we document these public classes and methods ?

[clemvnt]

Here, we're in a test file, so the other classes in this file have no comments.

But if someone of the organization feels the same way, I'll make the changes.

[ElizabethOkerio]

@clemvnt Will you continue working on this PR?

[clemvnt]

Yes. I'm waiting for a new version of Microsoft.OData.Core with the OData/odata.net#2879 fix before publishing the PR.

[clemvnt]

I've changed the base branch to main. This PR requires the next version of Microsoft.OData.Core (8.0.2). Other than that, it's ready.

[clemvnt]

This PR is ready.

[WanjohiSammy]

@clemvnt Are you still working on this PR?

[clemvnt]

I fixed the merge conflict. The PR is ready.

cc. @WanjohiSammy

[gathogojr]

@clemvnt Please check out the failing tests

Also test for derived complex type (in particular) as per scenario:

class A
{
    public string Prop { get; set; }
}

class DerivedFromA : A
{
    public string PropInDerived { get; set; }
}

class Product
{
    // ...
    public A A { get; set; }
    // ...
}

// ...
modelBuilder.ComplexType<A>();
modelBuilder.ComplexType<DerivedFromA>();

Request URL

Products?groupby((A/Fully.Qualified.Namespace.DerivedFromA/PropInDerived))

[clemvnt]

• Merged with main.
• Fixed tests.
• Added a new test case :

Products?groupby((Category/Microsoft.AspNetCore.OData.Tests.Models.DerivedCategory/DerivedCategoryName))`)

[gathogojr]

• Merged with main.
• Fixed tests.
• Added a new test case :

Products?groupby((Category/Microsoft.AspNetCore.OData.Tests.Models.DerivedCategory/DerivedCategoryName))`)

@clemvnt The specific scenario I asked you to verify is one where the derived type is a complex type, not entity type. Here's an illustration of a scenario that seem to proceed unexpected results (sharing only the bits you'd need to repro):

// Data models
namespace Microsoft.AspNetCore.OData.E2E.Tests.DollarApply
{   
    public class MyCustomer
    {
        public int Id { get; set; }
        public MyAddress Address { get; set; }
    }

    public class MyAddress
    {
        public string Street { get; set; }
    }

    public class MyHomeAddress : MyAddress
    {
        public string HomeNo { get; set; }
        public MyCity City { get; set; }
    }

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


// Edm model
var builder = new ODataConventionModelBuilder();
            
builder.EntitySet<MyCustomer>("MyCustomers");
builder.ComplexType<MyAddress>();
builder.ComplexType<MyHomeAddress>();
builder.ComplexType<MyCity>();

var model = builder.GetEdmModel();


// Controller
public class MyCustomersController : ODataController
{
    private static readonly List<MyCustomer> myCustomers = new List<MyCustomer>
    {
        new MyCustomer
        {
            Id = 1,
            Address = new MyHomeAddress { Street = "High Street", HomeNo = "#1", City = new MyCity { Name = "Belfast" } }
        },
        new MyCustomer
        {
            Id = 2,
            Address = new MyHomeAddress { Street = "Moore Street", HomeNo = "#2", City = new MyCity { Name = "Dublin" } }
        }
    };

    [EnableQuery]
    public ActionResult<IEnumerable<MyCustomer>> Get()
    {
        return myCustomers;
    }
}


// Tests
[Fact]
public async Task TestPrimitivePropertyInDerivedTypeAsync()
{
    var queryUrl = "default/MyCustomers?$apply=groupby((Address/Microsoft.AspNetCore.OData.E2E.Tests.DollarApply.MyHomeAddress/HomeNo))";
    var request = new HttpRequestMessage(HttpMethod.Get, queryUrl);
    request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json;odata.metadata=none"));
    var client = CreateClient(); // Initialize client

    var response = await client.SendAsync(request);

    // Assert
    Assert.Equal(HttpStatusCode.OK, response.StatusCode);
            
    var content = await response.Content.ReadAsStringAsync();
    // Response as expected: {"value":[{"Address":{"HomeNo":"#1"}},{"Address":{"HomeNo":"#2"}}]}
}

[Fact]
public async Task TestComplexPropertyInDerivedTypeAsync()
{
    var queryUrl = "default/MyCustomers?$apply=groupby((Address/Microsoft.AspNetCore.OData.E2E.Tests.DollarApply.MyHomeAddress/City/Name))";
    var request = new HttpRequestMessage(HttpMethod.Get, queryUrl);
    request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json;odata.metadata=none"));
    var client = CreateClient(); // Initialize client

    var response = await client.SendAsync(request);

    // Assert
    Assert.Equal(HttpStatusCode.OK, response.StatusCode);

    var content = await response.Content.ReadAsStringAsync();
    // Response NOT as expected: {"value":[{"Address":{}},{"Address":{}}]}
}

[clemvnt]

Thank you for the clarification.

It seems the issue arises from the ODataResourceSerializer, which is unable to locate the City property on the structured type Microsoft.AspNetCore.OData.E2E.Tests.DollarApply.MyAddress :

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

Lines 334 to 335 in 0ceeb08

	IEdmProperty edmProperty = structuredType?.Properties()
	.FirstOrDefault(p => p.Name.Equals(prop.Key, StringComparison.Ordinal));

Looking at the GroupByWrapper instance, there doesn't appear to be any information indicating the EDM type of the value. I’m unsure how to infer that the type should be MyHomeAddress instead of MyAddress within the ODataResourceSerializer logic. Do you have any insights on how to address this?

For context, in my specific use case, I don’t encounter this issue because I bypass the OData formatter. Instead, I manually apply OData clauses to IQueryable objects.

[gathogojr]

Thank you for the clarification.

It seems the issue arises from the ODataResourceSerializer, which is unable to locate the City property on the structured type Microsoft.AspNetCore.OData.E2E.Tests.DollarApply.MyAddress :

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

Lines 334 to 335 in 0ceeb08

	IEdmProperty edmProperty = structuredType?.Properties()
	.FirstOrDefault(p => p.Name.Equals(prop.Key, StringComparison.Ordinal));

Looking at the GroupByWrapper instance, there doesn't appear to be any information indicating the EDM type of the value. I’m unsure how to infer that the type should be MyHomeAddress instead of MyAddress within the ODataResourceSerializer logic. Do you have any insights on how to address this?

For context, in my specific use case, I don’t encounter this issue because I bypass the OData formatter. Instead, I manually apply OData clauses to IQueryable objects.

@clemvnt I'll need to do some digging to figure out how we can remedy it. At least we now know about it. If it's okay with you, we could merge your fix since it addresses a gap, then open a new ticket to track this scenario.

[gathogojr]

Thinking about it some more...

It seems the issue arises from the ODataResourceSerializer, which is unable to locate the City property on the structured type Microsoft.AspNetCore.OData.E2E.Tests.DollarApply.MyAddress
I’m unsure how to infer that the type should be MyHomeAddress instead of MyAddress within the ODataResourceSerializer logic

Somehow it works for the primitive HomeNo property declared on MyHomeAddress... We'll need to figure out why that works and why it doesn't work if the property is complex.

[clemvnt]

If it's okay with you, we could merge your fix since it addresses a gap, then open a new ticket to track this scenario.

Sure, I’d be happy to create a new issue for this case and include all the information I have gathered about it.

Somehow it works for the primitive HomeNo property declared on MyHomeAddress... We'll need to figure out why that works and why it doesn't work if the property is complex.

The EDM property isn’t found even for the primitive HomeNo property. However, it works because the following condition is satisfied:

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

Lines 368 to 372 in 0ceeb08

	property = new ODataProperty
	{
	Name = prop.Key,
	Value = prop.Value
	};

For the City property, we don’t meet the same condition since it’s a navigation property.