Skip to content

Conversation

@wedgberto
Copy link
Contributor

Purpose

This PR "buffers" the OData message wrapper in a memory stream until the OData query has been enumerated, then copies it into the HTTP response body only when no exception has been thrown.

Scenario

A WebAPI endpoint is serviced by an Entity Framework 6.0 IQueryable.

[HttpGet]
[EnableQuery]
public IQueryable<dto> Get()
{
    return this.dtoService.GetDtos();
}

When the SQL query is executed but the configured SQL command timeout is exceeded, our exception handling middleware successfully catches the SqlException, but is unable to set the HTTP status code to 500 because the HTTP response body is already started.

The endpoint returns a 200 OK HTTP status code and a body containing malformed JSON:

{"@odata.count":2,"value":[

@wedgberto
Copy link
Contributor Author

This might resolve: #704

@wedgberto
Copy link
Contributor Author

@microsoft-github-policy-service agree

}

await serializer.WriteObjectAsync(value, type, messageWriter, writeContext).ConfigureAwait(false);
await request.HttpContext.Response.Body.WriteAsync(responseBody.ToArray());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to create array of unknown size here?

Copy link
Contributor Author

@wedgberto wedgberto Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i used responseBody.CopyTo(request.HttpContext.Response.Body) initially, but it didn't work how I expected so I resorted to this. I'll find a better way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I was missing rewinding the buffer response body stream back to position zero

ODataPath path = request.ODataFeature().Path;
IEdmNavigationSource targetNavigationSource = path.GetNavigationSource();
HttpResponse response = request.HttpContext.Response;
var responseBody = new MemoryStream();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume to use memory stream pooling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean Microsoft.IO.RecyclableMemoryStream ?

@wedgberto wedgberto requested a review from Airex March 14, 2023 16:34
Copy link
Contributor

@mikepizzo mikepizzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wedgberto -- thanks for your contribution!

Note that we do not want to buffer in memory by default -- this can have a significant performance impact for large responses, and we do everything possible to make sure that we are able to stream a response without buffering.

In order to support efficient streaming, the OData protocol specifically describes instream error scenarios, calling for the response body to be malformed JSON.

@wedgberto
Copy link
Contributor Author

wedgberto commented Mar 17, 2023

@wedgberto -- thanks for your contribution!

Note that we do not want to buffer in memory by default -- this can have a significant performance impact for large responses, and we do everything possible to make sure that we are able to stream a response without buffering.

In order to support efficient streaming, the OData protocol specifically describes instream error scenarios, calling for the response body to be malformed JSON.

So rather than return an appropriate HTTP error status code, the spec states to return a 200 OK with a malformed JSON response body. If that is the case then I guess this PR is obsolete and can be aborted. We'll just have to code our Angular consumer to compensate and not assume that 200 OK responses contain valid JSON.

There are other issues in this repo that mention incomplete JSON in the response that might find this specified behaviour useful. #760 #219 #109

using Microsoft.AspNetCore.OData.Extensions;
using Microsoft.AspNetCore.OData.Formatter.MediaType;
using Microsoft.AspNetCore.OData.Formatter.Serialization;
using Microsoft.AspNetCore.OData.Formatter.Value;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test for this functionality ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants