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

support ActionResult<IAsyncEnumerable<T>> #1082

Conversation

ElizabethOkerio
Copy link
Contributor

@ElizabethOkerio ElizabethOkerio commented Nov 1, 2023

We added support for the IAsyncEnumerable interface. The current support can only be used like this:

[HttpGet] 
 [EnableQuery] 
 public IAsyncEnumerable<Customer> Get() 
 { 
     return this.context.Customers.AsAsyncEnumerable(); 
 } 

This PR adds support for this format:

[EnableQuery] 
public ActionResult<IAsyncEnumerable<Customer>> Get() 
{ 
    var query = this.context.Customers.AsAsyncEnumerable(); 
    return this.Ok(query); 
} 

In the second format, the declared type of the returned object is null ObjectResult responseContent = actionExecutedContext.Result as ObjectResult. This is the type that is used to determine how to retrieve items from the database during serialization. That's if the type is IAsyncEnumerable<T> then we use await foreach otherwise we just use a foreach.

The changes here get the type T in ActionResult<T> and check if T is of type IAsyncEnumerable<>. If it is, then we update the response content's DeclaredType to IAsyncEnumerable<>

@ElizabethOkerio ElizabethOkerio changed the title support ActionResult<IAsyncEnumerable<Customer>> support ActionResult<IAsyncEnumerable<T>> Nov 1, 2023
@@ -255,6 +256,20 @@ public override void OnActionExecuted(ActionExecutedContext actionExecutedContex
if (statusCodeResult?.StatusCode == null || IsSuccessStatusCode(statusCodeResult.StatusCode.Value))
{
ObjectResult responseContent = actionExecutedContext.Result as ObjectResult;

ControllerActionDescriptor controllerActionDescriptor = actionDescriptor as ControllerActionDescriptor;
Copy link
Contributor

@gathogojr gathogojr Nov 7, 2023

Choose a reason for hiding this comment

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

Curious... Our library currently supports the following without the code you added:

public ActionResult<IEnumerable<Customer>> Get()
{
   // ...
}

How is it that we need the code you added to support a return type of ActionResult<IAsyncEnumerable<T>> but we didn't/don't need it to support a return type of ActionResult<IEnumerable<T>>?

Is there a feature gap in the ASP.NET Core framework such that a return type of ActionResult<IEnumerable<T>> is supported out of the box while a return type of ActionResult<IAsyncEnumerable<T>> isn't?

Or do we need to identify what tweak is in place to make ActionResult<IEnumerable<T>> work and apply a similar tweak for ActionResult<IAsyncEnumerable<T>>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why this works is because, before adding support for IAsyncEnumerable, we converted the object to be serialized to an IEnumerable and used a foreach to enumerate the IEnumerable.

When we added support for IAsyncEnumerable, then we added a check during serialization that will check the type of the object being serialized.. If the type is an IAsyncEnumerable then an await foreach will be used,,, otherwise a normal foreach is used..

Without this fix, when we check the type of the object being serialized, since the declared type of the object is null and we can't tell whether it is an IAsyncEnumerable or not we default to IEnumerable and use foreach to enumerate the collection which is not suppossed to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ElizabethOkerio Would it make better sense to set the declared type even in the case of ActionResult<IEnumerable<T>> so we don't have to rely on null to default to IEnumerable?

Copy link
Contributor Author

@ElizabethOkerio ElizabethOkerio Nov 7, 2023

Choose a reason for hiding this comment

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

I think that since all the other return types can be represented as IEnumerable and are enumerated using foreach it's ok not to worry about the declared type here.. This is because then we'll have to start checking for the return types for all the other scenarios like ActionResult<Customer> for example.

Copy link
Contributor

@gathogojr gathogojr left a comment

Choose a reason for hiding this comment

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

:shipit:

@ElizabethOkerio ElizabethOkerio merged commit 7980e55 into OData:main Nov 8, 2023
2 checks passed
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.

3 participants