OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
454 stars 161 forks source link

Error(s) running OData against Cosmos #738

Open michael-d-atl404 opened 1 year ago

michael-d-atl404 commented 1 year ago

Describe the bug I am trying to run OData against a Cosmos database container and while some basic functionality works, when I add $select and other OData actions I start to get a lot of odd errors. I was able to find work arounds for some of them, but these last 2 I seem to be stuck on even when stepping through the Microsoft.Azure.Cosmos source.

Note: I have already posted to the Microsoft.Azure.Cosmos site and they see the issue being that the OData query generator is creating a "Conditional" clause ({IIF(($it == null), null, $it.Changes)}) that cannot be translated to Cosmos SQL: https://github.com/Azure/azure-cosmos-dotnet-v3/issues/3542 . I tried playing with the options HandleNullPropagation and HandleReferenceNavigationPropertyExpandFilter but they did not seem to fix this.

To Reproduce I have created a repo with code that reproduces all of these issues I have been seeing:

https://github.com/michael-d-atl404/ODataWithCosmosErrors

This project was run with a local Cosmos Emulator running: https://learn.microsoft.com/en-us/azure/cosmos-db/local-emulator. I included setup for that in the README.md.

I also included info in the README.md on the first 3 errors I was able to work around as well as the URLs I am still unable to get to work along with the errors I am getting.

These URLs work just fine:

https://localhost:7253/odata/Audits https://localhost:7253/odata/Audits?$select=id,user,date,changes

but then these other URLs are causing me errors:

https://localhost:7253/odata/Audits?$count=true&$select=id,user,date,changes&$orderby=date%20desc&$top=50 https://localhost:7253/odata/Audits?$count=true&$select=id,user,date&$expand=changes($select=prop,old,new)&$orderby=date%20desc&$top=50

The real API being used here has OData running on some EFCore resources and some OData. Everything seemed to be running fine on both from our Vue app until I hit this new Cosmos model for Audits where the document had a child array of simple objects. I should be able to make it work with either pulling the entire child collection or navigating it as a "$expand", I just need one of them to work and would prefer that it did work with paging as it is possible there will be a lot of these records.

Basically I debugged down to this exception being thrown from ExpressionToSql.Translate:

Microsoft.Azure.Cosmos.Linq.DocumentQueryException: Expression with NodeType 'Conditional' is not supported.

where it appears to be running against an IIF null check on the "changes" IEnumerable attribute and there is no code to handle that particular expression tree. Not sure if there is another setting to try to bypass this or not. I attempted to use OData Query Option settings:

HandleNullPropagation = Microsoft.AspNetCore.OData.Query.HandleNullPropagationOption.False

and

HandleReferenceNavigationPropertyExpandFilter = true

but neither of these seemed to fix anything.

Expected behavior Proper, paged OData responses for at least one of those 2 URL patterns.

Actual behavior

I think that the main one I would like fixed is this (for URL "https://localhost:7253/odata/Audits?$count=true&$select=id,user,date,changes&$orderby=date%20desc&$top=50" ):

Full Error Stack:
      Microsoft.Azure.Cosmos.Linq.DocumentQueryException: Expression with NodeType 'Conditional' is not supported., Windows/10.0.19044 cosmos-netstandard-sdk/3.29.4
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.Translate(Expression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMethodCall(MethodCallExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.Translate(Expression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitCollectionExpression(Expression expression, TranslationContext context, String parameterName)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitCollectionExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.CreateSubquery(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitConditional(ConditionalExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitNonSubqueryScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberAssignment(MemberAssignment inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitBindingList(ReadOnlyCollection`1 inputExpressionList, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberInit(MemberInitExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitNonSubqueryScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberAssignment(MemberAssignment inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitBindingList(ReadOnlyCollection`1 inputExpressionList, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberInit(MemberInitExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitNonSubqueryScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberAssignment(MemberAssignment inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitBindingList(ReadOnlyCollection`1 inputExpressionList, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMemberInit(MemberInitExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitNonSubqueryScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitScalarExpression(Expression expression, ReadOnlyCollection`1 parameters, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitSelect(ReadOnlyCollection`1 arguments, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMethodCall(MethodCallExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.Translate(Expression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.VisitMethodCall(MethodCallExpression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.Translate(Expression inputExpression, TranslationContext context)
         at Microsoft.Azure.Cosmos.Linq.ExpressionToSql.TranslateQuery(Expression inputExpression, IDictionary`2 parameters, CosmosLinqSerializerOptions linqSerializerOptions)
         at Microsoft.Azure.Cosmos.Linq.SqlTranslator.TranslateQuery(Expression inputExpression, CosmosLinqSerializerOptions linqSerializerOptions, IDictionary`2 parameters)
         at Microsoft.Azure.Cosmos.Linq.CosmosLinqQuery`1.CreateFeedIterator(Boolean isContinuationExpected)
         at Microsoft.Azure.Cosmos.Linq.CosmosLinqQuery`1.GetEnumerator()+MoveNext()
         at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
         at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
         at Microsoft.Azure.Cosmos.Linq.CosmosLinqQueryProvider.Execute[TResult](Expression expression)
         at System.Linq.Queryable.Count[TSource](IQueryable`1 source)
         at ODataCosmosTest.EnableCosmosQueryAttribute.<>c__DisplayClass0_0.<ApplyQuery>b__0() in C:\Users\michaeld\localProjects\ODataCosmosTest\ODataCosmosTest\EnableCosmosQueryAttribute.cs:line 26
         at Microsoft.AspNetCore.OData.Abstracts.ODataFeature.get_TotalCount()
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.CreateResourceSet(IEnumerable resourceSetInstance, IEdmCollectionTypeReference resourceSetType, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteResourceSetAsync(IEnumerable enumerable, IEdmTypeReference resourceSetType, ODataWriter writer, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteObjectInlineAsync(Object graph, IEdmTypeReference expectedType, ODataWriter writer, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteObjectAsync(Object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.ODataOutputFormatterHelper.WriteToStreamAsync(Type type, Object value, IEdmModel model, ODataVersion version, Uri baseAddress, MediaTypeHeaderValue contentType, HttpRequest request, IHeaderDictionary requestHeaders, IODataSerializerProvider serializerProvider)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResultFilterAsync>g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
         at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
         at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
         at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
         at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
         at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
         at Microsoft.WebTools.BrowserLink.Net.BrowserLinkMiddleware.ExecuteWithFilterAsync(IHttpSocketAdapter injectScriptSocket, String requestId, HttpContext httpContext)
         at Microsoft.AspNetCore.Watch.BrowserRefresh.BrowserRefreshMiddleware.InvokeAsync(HttpContext context)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)

Environment summary

I am running on Windows 10 Pro and using the latest .NET 6, Microsoft.AspNetCore.OData 8.0.11 and Microsoft.Azure.Cosmos 3.31.1

julealgon commented 1 year ago

Nicely documented. I'm sad to hear some basic things are not working properly with the CosmosDB provider since I wanted to use it soon myself.

I think that the main one I would like fixed is this (for URL "https://localhost:7253/odata/Audits?$count=true&$select=id,user,date,changes&$orderby=date%20desc&$top=50" ):

This specific query has a bunch of different components to it. Could you try to figure out a minimal query example that would still reproduce the issue? For example, does it still fail if you remove the $count? What about the $top? Or maybe its just the inclusion of the changes property that triggers it?

Would be nice to know precisely which specific operation is causing this exception.

julealgon commented 1 year ago

Also, the fact that you had to add this: https://github.com/michael-d-atl404/ODataWithCosmosErrors/blob/94d020c60c481a6d7f28c8c85c86d2107cc0e059/ODataCosmosTest/Program.cs#L11-L36

Suggests to me that your action is not being detected as a proper OData route. I suspect it could be because of the "odata" prefix you added when configuring OData but didn't add the prefix to the controller in a [Route] attribute.

Can you please share the output of the $odata debug endpoint?

michael-d-atl404 commented 1 year ago

https://localhost:7253/odata/Audits?$count=true&$select=id,user,date,changes&$orderby=date%20desc&$top=50

@julealgon The breaking factor appears to simply be the inclusion of "changes". We have a few other OData \ Cosmos endpoints running pretty well (other than the 3 hacky kind of settings mentioned). The difference is that this is the first Cosmos document complex child attribute (i.e. the "changes" child array). I am able to get this to work by leaving out the "$select" completely and just pulling back everything which is not really ideal as there is a lot of superfluous data in the document that we would rather leave out.

michael-d-atl404 commented 1 year ago

Also, the fact that you had to add this: https://github.com/michael-d-atl404/ODataWithCosmosErrors/blob/94d020c60c481a6d7f28c8c85c86d2107cc0e059/ODataCosmosTest/Program.cs#L11-L36

Suggests to me that your action is not being detected as a proper OData route. I suspect it could be because of the "odata" prefix you added when configuring OData but didn't add the prefix to the controller in a [Route] attribute.

Can you please share the output of the $odata debug endpoint?

@julealgon Changing Newtonsoft.Json at the root default did seem to be required. I tried to make these settings at the ".AddNewtonsoftJson()" layer during IServiceCollection setup, but the settings there did not change anything.

Without those Newtonsoft settings I was constantly getting infinite loops in the generation of either the OData parsing or the SQL translation. It would be nice to get those fixed better as well, but that is probably not a priority. It kept telling me that my EdmModel had a loop in it, but I looked through many times and did not find any.

Is this the URL you are talking about: https://localhost:7253/odata?$odata?

It returns:

{
  "@odata.context":"https://localhost:7253/odata/$metadata",
  "value":
    [
      {"name":"WeatherForecasts","kind":"EntitySet","url":"WeatherForecasts"},
      {"name":"Audits","kind":"EntitySet","url":"Audits"}
    ]
}
julealgon commented 1 year ago

@julealgon Changing Newtonsoft.Json at the root default did seem to be required. I tried to make these settings at the ".AddNewtonsoftJson()" layer during IServiceCollection setup, but the settings there did not change anything.

It is required because you are not outputing an OData payload, but a standard MVC one. OData doesn't rely on Newtonsoft (or System.Text.Json): it has its own custom json serialization logic.

Once we ensure your endpoint is properly recognized as OData, these changes you've made to Newtonsoft settings should become obsolete.

Is this the URL you are talking about: https://localhost:7253/odata?$odata?

It returns:

{
  "@odata.context":"https://localhost:7253/odata/$metadata",
  "value":
    [
      {"name":"WeatherForecasts","kind":"EntitySet","url":"WeatherForecasts"},
      {"name":"Audits","kind":"EntitySet","url":"Audits"}
    ]
}

No. What you are accessing there is equivalent to the root https://localhost:7253/odata: the ?$odata is being ignored.

I'm talking about this endpoint:

You enable that by adding a middleware on startup as per the readme:

        // Send "~/$odata" to debug routing if enable the following middleware
        // app.UseODataRouteDebug();

This presents a custom UI that helps you identify which of your endpoints is being detected by OData and which aren't. Your action should show there as "non-OData". Please confirm.

michael-d-atl404 commented 1 year ago

This is what I get from:

https://localhost:7253/$odata

image

julealgon commented 1 year ago

This is what I get from:

https://localhost:7253/$odata

Oh ok.... my assumption was wrong then. I think your serialization changes are actually affecting the deserialization from the Cosmos REST API which would be used by the provider, making this part completely unrelated to OData.

Sorry I can't be of further assistance; you'll have to wait for the OData team to respond here.

michael-d-atl404 commented 1 year ago

@julealgon I believe that the issue is that in the OData LINQ query generation, somewhere a "Conditional" clause is being added that looks like: {IIF(($it == null), null, $it.Changes)}

It is a pretty basic clause, probably because the "Changes" attribute is an IEnumerable and not a simple data type.

Unfortunately, when the Microsoft.Azure.Cosmos code tries to convert this part of the LINQ query to Cosmos SQL there is no comparable clause to convert it to. I guess what I am looking for is some kind of ODataQueryOption that will prevent these kind of NULL check conditionals from being added at all so they can be used by the Cosmos database.

habbes commented 1 year ago

@michael-d-atl404 thanks for reporting this and thanks for the detailed explanation and repro-project. I'll investigate this issue and report back.

xuzhg commented 1 year ago

can 'https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/HandleNullPropagationOption.cs#L28' setting work for you?

michael-d-atl404 commented 1 year ago

@xuzhg I believe I tried that along with the HandleReferenceNavigationPropertyExpandFilter property in a number of configurations with no change in results. Unless there is another place to set this than where I was:

https://github.com/michael-d-atl404/ODataWithCosmosErrors/blob/master/ODataCosmosTest/Controllers/AuditsController.cs

wbuck commented 1 year ago

@michael-d-atl404 Just for my own edification, when selecting a property that is not collection, does it work? Or just the inclusion of $select regardless of the selected property types fail?

So, does $select=id, user work? If the above ($select=id, user) query works, does selecting a property that is a collection of primitives (say an integer) work? E.g., $select=id, user, myIntegerCollection

michael-d-atl404 commented 1 year ago

@wbuck In the demo code as-is, the URL https://localhost:7253/odata/Audits?$select=id,user,date,changes does work just fine with the 3 fixes I had found. It only seems to be addition of the "$count=true" with the selection of "changes" that this error kicks in.

We have another object in the system that has an array of int that does seem to be working fine.

wbuck commented 1 year ago

So the only way I got around these issues (I can't use $select at all in my queries) was to not apply the $select query at all. I then take the SelectExpandClause (if it's not null) and build the Expression tree (and potentially build a dynamic runtime type as well) which I then apply to the current IQueryable Expression.

I'd avoid using the EnableQuery attribute and take the query options as a parameter to the controller action instead. This allows you to keep everything async.

wbuck commented 1 year ago

Currently I'm implementing a new feature in AutoMapper.Extensions.OData to support the Cosmos DB LINQ provider. I have a fair amount completed I just need to finish unit and integration tests. All the the queries you showed work as expected.

The feature will support the following query options at the moment:

There is a work around for the lack of $apply which can be read about here.