OData / AspNetCoreOData

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

Invalid response from Get() when an EF crash happens #867

Open awbacker opened 1 year ago

awbacker commented 1 year ago

I have a simple controller with a Get method, which reads from the database. The EntityFramework model had a bug (column was null). When reading, an exception was raised:

Microsoft.AspNetCore.Server.Kestrel[13]
      ... An unhandled exception was thrown by the application.
      System.InvalidCastException: Column 'company_id' is null.

This causes the OData library to:

Data Model

class User {
      public int Id {get; set;}
      public int CompanyId { get; set; } // this should have been int?
}

Request/Response

curl -v localhost:5279/odata/v1/Users
*   Trying 127.0.0.1:5279...
* Connected to localhost (127.0.0.1) port 5279 (#0)
> GET /odata/v1/Users HTTP/1.1
> Host: localhost:5279
> User-Agent: curl/7.86.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: application/json; odata.metadata=minimal; odata.streaming=true; charset=utf-8
< Date: Wed, 22 Mar 2023 08:01:24 GMT
< Server: Kestrel
< Transfer-Encoding: chunked
< OData-Version: 4.0
<
* transfer closed with outstanding read data remaining
* Closing connection 0
curl: (18) transfer closed with outstanding read data remaining
{
      "@odata.context":"http://localhost:5279/odata/v1/$metadata#Users",
      "value": [ 
          {object-1}, 
          {object-2}
          // <-- crash happened here, content stops after last `}`

Expected behavior

awbacker commented 11 months ago

Is this perhaps somewhat intended, as part of a streaming response?

BlaineM-SeriouslyRAD commented 6 months ago

We have an EF data context which is used across multiple platforms; desktop, api, webapi. This context intercepts the EF query expressions for many purposes; one of which is to ensure that the current user has read permission for all the entity sets they are attempting to access. Should this fail, an appropriate “access denied” exception is thrown.

By virtue of IQueryable, AspNetCore OData (8.2.5) begins writing to the response body before the entities have been materialized. If an exception occurs while materializing the entities, the response is abandoned - but with a 200 OK status. Any exception handlers also fail because the response has already started and cannot be changed.

Other exceptions may be thrown during the materialization of entities (e.g. database / network issues), so this isn’t great behaviour.

For now, we’ve worked around the issue by overriding the default implementation of WriteObjectAsync in ODataResourceSetSerializer, ensuring entities are materialized into concrete lists before the base method is called. Doing so ensures that any exception occurs before AspNetCore OData has started to respond, at the possible cost of some performance:

public override async Task WriteObjectAsync(object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
{
    // Microsoft.AspNetCore.OData (8.2.5) starts the HttpResponse
    // before materializing EF entities.

    // Any errors during materialization will not get returned because the
    // response has already started and cannot be changed.

    // This results in a 200 OK and a truncated response.

    // So... let's materialize EF entities early.
    // Any exception will now be thrown before the response has started.

    if (graph is IAsyncEnumerable<object> asyncEnumerable)
        graph = await asyncEnumerable.ToListAsync();
    else if (graph is IEnumerable enumerable)
        graph = enumerable.Cast<object>().ToList();

    await base.WriteObjectAsync(graph, type, messageWriter, writeContext);
}

Definitely the same issue as https://github.com/OData/WebApi/issues/2704.