OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
854 stars 476 forks source link

The property '{propertyname}' cannot be used in the $expand query option #2815

Open mattkleiny opened 10 months ago

mattkleiny commented 10 months ago

Periodically when querying the OData endpoints in one of our products, clients receive an unexpected 400 error stating: The property '{propertyname}' cannot be used in the $expand query option. This occurs on different entity sets.

Normally, these properties are 'expandable' and work correctly. However, when the server is under load they start failing periodically with a 400 error.

Assemblies affected

Microsoft.AspNet.OData 7.7.2

Reproduce steps

  1. Set-up a project in IIS with the OData library.
  2. Have it serve 2 entity sets with 2 different models that each include a complex property.
  3. Add an .Expand() option of allowed to those entity sets and those particular complex properties.
  4. Run a stress test against the endpoint that simultaneously executes those endpoints. Notice that it periodically fails with a 400 error.

Expected result

The response should be successful, and $expand as appropriate.

Actual result

The response will periodically fail with a 400 error stating that "The property '{propertyname}' cannot be used in the $expand query option".

Additional detail

Here's a repository reproducing the issue https://github.com/mattkleiny/odata-bug-repro

xuzhg commented 10 months ago

@mattkleiny Your service has the following configuration:

config.Expand(QueryOptionSetting.Disabled);

This line disables all $expand. Try to use:

config.Expand();
mattkleiny commented 10 months ago

Thanks for getting back to me @xuzhg.

We need to selectively enable .Expand() on specific entity sets. In the repro I gave you, it's disabled by default with config.Expand(QueryOptionSetting.Disabled); in top-level config, and then specifically enabled on the two entity sets.

It will fail intermittently. Some requests do succeed and produce the correct response. Here's an example after just a few seconds of running, and I've updated the repro to more clearly show:

There were 195 successful responses and 25 failed responses for /v3/Organisation?$expand=Contacts
There were 341 successful responses and 18 failed responses for /v3/Contact?$expand=BusinessCards

Debugging a little bit, it looks like isExpandable is false in this block of code when we have many simultaneous requests, and if the default query option was enabled with config.Expand();, it would side-step the issue by not throwing an exception here.

https://github.com/OData/WebApi/blob/46d7ecbc5a5254129b2d3cf31ea68fa420ea52b8/src/Microsoft.AspNet.OData.Shared/Query/Validators/SelectExpandQueryValidator.cs#L333-L360

I was tracing the source of this information back into EdmLibHelpers.IsExpandable and unfortunately it gets quite complicated quite quickly, it seems to be sourced through an annotation of some kind and stored in a versioned dictionary... it does look like there is shared data across multiple requests underneath and I suspect it's a race condition of some sort, but I'm just speculating 🤷‍♂️.

xuzhg commented 10 months ago

Ok, It seems thread-safe problem and I already refactored it in our 8 version at https://github.com/OData/AspNetCoreOData/pull/800.

Do you mind to try our 8.x version? It's easy to update. If you need me to try, please share the 'write' permission to your GitHub repository.

We have a plan to port back the changes from 8 to 7, but it needs time to finish it.

mattkleiny commented 10 months ago

Thanks @xuzhg! I've given you write access, if you'd like to try to update.

I was under the impression that the ASP .NET Core version that you linked to would be incompatible with our old .NET Framework based app. It would be a much larger piece of work to migrate off of .NET Framework, of course. But if there's a way we can simply update to the 8.x package then that's great.

xuzhg commented 10 months ago

@mattkleiny https://github.com/mattkleiny/odata-bug-repro/pull/1

mattkleiny commented 10 months ago

Thanks @xuzhg for opening that PR. I see that the change involved is primarily about using an ASP.NET Core project? That would be great, but sadly we can't update our stack at this point.

Hoping that the fix will get backported into the Web API library at some point. Should I leave this issue open until then? Others might have a similar problem.