AspNetCoreOData icon indicating copy to clipboard operation
AspNetCoreOData copied to clipboard

$orderby no longer working after upgrading from Microsoft.AspNetCore.OData 8.2.4 to 8.2.5

Open jacobjohnston opened this issue 11 months ago • 16 comments

Assemblies affected Microsoft.AspNetCore.OData 8.2.5

Describe the bug Using $orderby on a field returns an error saying the field does not exist. However, if I do a query where I just use a $filter on the field, it works entirely fine.

This started with 8.2.5. Rolling back to 8.2.4 and everything works fine again.

Reproduce steps Add $orderby to my query, and I get the following error:

Could not find a property named 'modifiedDate' on type 'MyNamespace.MyModel'.
[2024-03-05T22:26:20.554Z] Result: Could not find a property named 'modifiedDate' on type 'MyNamespace.MyModel'.
[2024-03-05T22:26:20.554Z] Exception: Microsoft.OData.ODataException: Could not find a property named 'modifiedDate' on type 'MyNamespace.MyModel'.
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.EndPathBinder.GeneratePropertyAccessQueryForOpenType(EndPathToken endPathToken, SingleValueNode parentNode)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.EndPathBinder.BindEndPath(EndPathToken endPathToken)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.MetadataBinder.BindEndPath(EndPathToken endPathToken)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.MetadataBinder.Bind(QueryToken token)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.OrderByBinder.ProcessSingleOrderBy(BindingState state, OrderByClause thenBy, OrderByToken orderByToken)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.OrderByBinder.BindOrderBy(BindingState state, IEnumerable`1 orderByTokens)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.ODataQueryOptionParser.ParseOrderByImplementation(String orderBy, ODataUriParserConfiguration configuration, ODataPathInfo odataPathInfo)
[2024-03-05T22:26:20.554Z]    at Microsoft.OData.UriParser.ODataQueryOptionParser.ParseOrderBy()
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.OrderByQueryOption.get_OrderByClause()
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.OrderByQueryOption.get_OrderByNodes()
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.OrderByQueryOption.ApplyToCore(IQueryable query, ODataQuerySettings querySettings)
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.OrderByQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings)
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings)
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.ODataQueryOptions`1.ApplyTo(IQueryable query, ODataQuerySettings querySettings)
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query)
[2024-03-05T22:26:20.554Z]    at Microsoft.AspNetCore.OData.Query.ODataQueryOptions`1.ApplyTo(IQueryable query)
[2024-03-05T22:26:20.554Z]    at MyNamespace.MyRequest`1.PerformODataQuery[TEntity](FetchRequestBase request, IQueryable`1 baseQuery) in /Users/jacob/Location 2 Functions/Endpoints/Endpoint Functions/FetchEndpointBase.cs:line 188
[2024-03-05T22:26:20.554Z]    at MyNamespace.MyRequest`1.FetchData[TEntity](FetchRequestBase request, IQueryable`1 baseQuery, UserContext userContext, String valuesName) in /Users/jacob/Location 2 Functions/Endpoints/Endpoint Functions/FetchEndpointBase.cs:line 53

Data Model

public class MyModel
{
    /// <summary>
    /// The ID
    /// </summary>
    public long ID { get; init; }

    /// <summary>
    /// Whom the record was created by
    /// </summary>
    public string CreatedBy { get; init; }

    /// <summary>
    /// The date the record was created
    /// </summary>
    [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
    public DateTime CreatedDate { get; init; }

    /// <summary>
    /// Whom the record was last modified by
    /// </summary>
    public string ModifiedBy { get; init; }

    /// <summary>
    /// The date the record was modified
    /// </summary>
    [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    public DateTime ModifiedDate { get; init; }
}

jacobjohnston avatar Mar 05 '24 22:03 jacobjohnston

Does it change the error if you use ModifiedDate instead of modifiedDate?

julealgon avatar Mar 05 '24 23:03 julealgon

Does it change the error if you use ModifiedDate instead of modifiedDate?

Yes!

It looks like my title should be "$orderby case sensitive after 8.2.5"

jacobjohnston avatar Mar 05 '24 23:03 jacobjohnston

Does it change the error if you use ModifiedDate instead of modifiedDate?

Yes!

It looks like my title should be "$orderby case sensitive after 8.2.5"

Thanks for confirming, that was precisely my intent with the question 😉

There weren't many changes in the 8.2.5 release, so hopefully someone on the team will be able to quickly find which commit caused this regression.

julealgon avatar Mar 05 '24 23:03 julealgon

My guess is it has been broken by https://github.com/OData/AspNetCoreOData/pull/1164 @xuzhg can you check this?

justin-mellor avatar Mar 07 '24 12:03 justin-mellor

I think you need $skip and top in the odata query as well for it to fail

justin-mellor avatar Mar 07 '24 12:03 justin-mellor

I think you need $skip and top in the odata query as well for it to fail

Just did a bit of testing and it looks like you need a $top OR $skip value to induce this bug.

My queries happen to get a default $top value if the request doesn't specify one, so I didn't catch that nuance.

jacobjohnston avatar Mar 07 '24 15:03 jacobjohnston

@jacobjohnston Can you share your failing request Uri with the query? and your service configuration?

xuzhg avatar Mar 07 '24 16:03 xuzhg

@xuzhg For us an example query is

https://localhost:4200/api/assistants?$orderby=displayName%20desc&top=15&$skip=0

Not sure what bit service configuration you want. We have Swashbuckle on the REST site in case that is relevant

we use services.AddControllers().AddOData();

also slight subtlety is services.AddMvc(options => { // Stop Swagger crashing with OData. https://github.com/OData/WebApi/issues/1177#issuecomment-462088814 // Needs more work to get actual full odata support options.InputFormatters.RemoveType<ODataInputFormatter>(); options.OutputFormatters.RemoveType<ODataOutputFormatter>(); })

and app.UseEndpoints(endpoints => { endpoints.MapDefaultControllerRoute(); });

justin-mellor avatar Mar 07 '24 16:03 justin-mellor

@xuzhg For us an example query is

https://localhost:4200/api/assistants?$orderby=displayName%20desc&top=15&$skip=0

Not sure what bit service configuration you want. We have Swashbuckle on the REST site in case that is relevant

we use services.AddControllers().AddOData();

also slight subtlety is services.AddMvc(options => { // Stop Swagger crashing with OData. OData/WebApi#1177 (comment) // Needs more work to get actual full odata support options.InputFormatters.RemoveType(); options.OutputFormatters.RemoveType(); })

and app.UseEndpoints(endpoints => { endpoints.MapDefaultControllerRoute(); });

@justin-mellor Do you mind sharing your repro? attached the project here or share a github repository link? Thanks

xuzhg avatar Mar 07 '24 16:03 xuzhg

@xuzhg I will see if I can put one together

justin-mellor avatar Mar 07 '24 16:03 justin-mellor

@xuzhg I will see if I can put one together

Thanks.

xuzhg avatar Mar 07 '24 17:03 xuzhg

Hi @xuzhg I have attached a repro. There are examples in the .http file. If you reference 8.2.4 then all the cases pass. In 8.2.5 then the orderby with a top fails ODataOrderBy.zip

justin-mellor avatar Mar 08 '24 12:03 justin-mellor

Hi @xuzhg . Decided to make it easier for everyone and push the repro to github https://github.com/justin-mellor/ODataOrderBy

justin-mellor avatar Mar 08 '24 14:03 justin-mellor

@justin-mellor

Thanks for your reproduce, and it shows your "usage" clearly.

Basically, if you follow up odata 'basic' usage as:

1

Build the Edm model and use it build the odata endpoint:

public static IEdmModel GetEdm()
{
    var builder = new ODataConventionModelBuilder();
    builder.EntitySet<Book>("Books");
    builder.EntitySet<Press>("Presses");
    return builder.GetEdmModel();
}
builder.Services.AddControllers()
    .AddOData(opt => opt.AddRouteComponents(..., GetEdm());

It works fine.

2

If you continue to use the OData query only without the OData configuration. You can do the following as workaround: Only change the Program.cs and add a middleware to configure the service.

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddDbContext<BookStoreContext>(opt => opt.UseInMemoryDatabase("BookLists"));
builder.Services.AddControllers()
    .AddOData();

var app = builder.Build();

// Configure the HTTP request pipeline.

app.UseODataRouteDebug();

app.Use(async (context, next) =>
{
    context.Request.ODataFeature().Services = BuildProvider();
    await next.Invoke();
});

app.UseHttpsRedirection();

app.UseAuthorization();

app.MapControllers();

app.Run();


static IServiceProvider BuildProvider()
{
    var services = new ServiceCollection();
    services.AddSingleton<ODataQuerySettings>();
    services.AddSingleton<ODataUriParserSettings>();
    services.AddSingleton(sp => new ODataUriResolver
    {
        EnableCaseInsensitive = true,
        EnableNoDollarQueryOptions = true
    });
    services.AddSingleton<ODataSimplifiedOptions>();
    return services.BuildServiceProvider();
}

It works fine at my side for $orderby and $top. (It also works without the "$" prefix).

Root cause

The root cause is that: In your usage, there's no OData property name case configuration, So, by default, OData library treats the property name case senstitive.

I have a PR at #1192, please take a look and share your comments.

xuzhg avatar Mar 08 '24 19:03 xuzhg

@xuzhg

Thanks for your response. Configuring the middleware does work for us, but I would rather wait for the default Case Insensitive change from your PR to be released. Realised the missing $ from $top was just a typo, hadn't even realised that was a feature.

Hopefully this solves it for @jacobjohnston too.

Thanks

justin-mellor avatar Mar 11 '24 11:03 justin-mellor

Hello @xuzhg , first thing I do not understand is why $filter works fine whereas $orderBy became case sensitive between versions. From your explanation and example it seems that EnableCaseInsensitive should effect the resolver globally and not just regarding $orderBy. Another strange thing is case insensitivity of property named id.

❌ http://localhost:51994/project-statuses?$filter=((date+eq+2024-03-14))&$orderby=date+desc&$skip=0&$top=25 ✔️ http://localhost:51994/project-statuses?$filter=((date+eq+2024-03-14))&$orderby=Date+desc&$skip=0&$top=25 ✔️ http://localhost:51994/projects?$orderby=id+desc&$skip=0&$top=25 ✔️ http://localhost:51994/projects?$orderby=Id+desc&$skip=0&$top=25

Second thing I do not understand is how presence of EDM model influcences this as in your example you did not specify case (in)sensitivity nor anything regarding name mapping. Does EDM model automatically make all properties camelCased?

cts-tradeit avatar Mar 14 '24 14:03 cts-tradeit

@cts-tradeit

For your first thing. First, for ODataQueryOptions< T >, it enable case-insensitive by default if there's no OData service configured. So, you can see the codes at https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs#L1156C17-L1163.

All built-in Query options classes accept/use the same setting, because each query option takes the same "ODataQueryOptionParser". For example: https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs#L983

So, $filter or other query options work fine except the $orderby. It's because if 'stable sorting enabled', we need to add the extra key into the orderby clause to make it stable sorting. For example, if the request query likes "?$orderby=date desc&$top=25", we need to add the "key" into the sorting. So the query should be changed (internally) as " ?$orderby=date desc,Id&$top=25".

Then, we use 'https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs#L765" to rebuild a OrderByQueryOption, then it creates a new ODataQueryOptionParser at 'https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/Query/OrderByQueryOption.cs#L104'. Since the new parser doesn't change the Uri resolver setting, then the Parser only supports the case sensitive by default.

What I am thinking is that, why 'https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs#L765' doesn't pass the existing ODataQueryOptionParser into the OrderByQueryOption constructor?

For your second thing, Does EDM model automatically make all properties camelCased? ==> The answer is no. But, if you are using OData model builder and configured "EnableLowerCamelCase", the property and type name will be made as cameCased.

Basically, OData model builder does two things for type and property. one is the create the EdmType and EdmProperty based on the C# type and C# propertyInfo. The other is the create the mapping between "EdmType, EdmProperty" and "C# type, C# propertyInfo".

xuzhg avatar Mar 18 '24 04:03 xuzhg

The workaround given above (adding ODataUriResolver to services) didn't work for me, and this bug is definitely still present in Microsoft.AspNetCore.OData 8.2.5 (latest available via Nuget)...

wizofaus avatar Jul 19 '24 02:07 wizofaus