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

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

Closed
jacobjohnston opened this issue Mar 5, 2024 · 19 comments · Fixed by #1192
Assignees
Labels
bug Something isn't working

Comments

@jacobjohnston
Copy link

jacobjohnston commented Mar 5, 2024

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 jacobjohnston added the bug Something isn't working label Mar 5, 2024
@julealgon
Copy link
Contributor

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

@jacobjohnston
Copy link
Author

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"

@julealgon
Copy link
Contributor

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.

@justin-mellor
Copy link

My guess is it has been broken by
#1164
@xuzhg can you check this?

@justin-mellor
Copy link

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

@jacobjohnston
Copy link
Author

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.

@xuzhg
Copy link
Member

xuzhg commented Mar 7, 2024

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

@justin-mellor
Copy link

@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();
});

@xuzhg
Copy link
Member

xuzhg commented Mar 7, 2024

@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

@justin-mellor
Copy link

@xuzhg I will see if I can put one together

@xuzhg
Copy link
Member

xuzhg commented Mar 7, 2024

@xuzhg I will see if I can put one together

Thanks.

@justin-mellor
Copy link

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
Copy link

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

@xuzhg
Copy link
Member

xuzhg commented Mar 8, 2024

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

@justin-mellor
Copy link

@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

@cts-tradeit
Copy link

cts-tradeit commented Mar 14, 2024

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?

@xuzhg
Copy link
Member

xuzhg commented Mar 18, 2024

@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 added a commit that referenced this issue May 13, 2024
* Fixes #1190: Enable case insensitive by default for query

* upate the test cases

* The Json string switches randomly, so change the codes to avoid random ordering.

* Address the comment and rebase to main
@wizofaus
Copy link

wizofaus commented Jul 19, 2024

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)...

@jholovacs
Copy link

Why is this closed? This is still broken in the latest 8.2.* version. This is a breaking change from 8.2.4 and needs to be fixed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants