-
Notifications
You must be signed in to change notification settings - Fork 476
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
Fix behaviour on status code requests. #2512
Fix behaviour on status code requests. #2512
Conversation
8cb6f42
to
ec3166d
Compare
ec3166d
to
90b86d0
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -228,46 +228,44 @@ public override void OnActionExecuted(ActionExecutedContext actionExecutedContex | |||
{ | |||
// actionExecutedContext.Result might also indicate a status code that has not yet | |||
// been applied to the result; make sure it's also successful. | |||
StatusCodeResult statusCodeResult = actionExecutedContext.Result as StatusCodeResult; | |||
if (statusCodeResult == null || IsSuccessStatusCode(statusCodeResult.StatusCode)) | |||
ObjectResult responseContent = actionExecutedContext.Result as ObjectResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other return result types other than ObjectResult
that we should care about? I see that IStatusCodeActionResult
is the common interface of all action results that have a status code. Why not use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking further, it seems that there are a few other result types that have a Value
, such as JsonResult
, and there's also ContentResult
which has Content
. Is the assumption here that ObjectResult
is the only one that has a Value
that can be processed by [EnableQuery]
and so we don't care about the rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface exists for net core 2.2 and above. I just went through the code and patched the behaviour as we don't look at other items such as JsonResult and Content result.
//throw Error.Argument("actionExecutedContext", SRResources.QueryingRequiresObjectContent, | ||
// actionExecutedContext.Result.GetType().FullName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could remove this commented code.
It seems hat it as meant to throw an exception if the result was not an ObjectResult. I wonder why it was commented out. But I think it should be removed if it's not used.
Issues
This pull request fixes issue #2511 .
Description
Briefly describe the changes of this pull request.
Fixes the usage of enable query for status code requests as they are not used within AspNetCore OData.
The api
this.StatusCode(int, obj)
returns anObjectResult
instead of aStatusCodeResult
. and Object result is not a child of status code result.The original intent seems to be when the ObjectResult is not null proceed to apply the query options.
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.