Add content negotiation to the default error handler #1911
Conversation
|
@thecodejunkie: Any feedback on this PR? This resolves issue #1905. |
|
@jonathanfoster Currently you're duplicating a lot of code from the |
|
Duplicate code is no bueno. I'll dive into the Content Negotiation wiki page so I can get a better idea of where the |
|
@jonathanfoster did you ever resolve the code duplication issue? Thanks |
|
Ping @jonathanfoster :) |
|
I'm not dead yet! No, I haven't resolved the duplication issue yet. It took me some time to grok the code base and I think I have a better plan for integration now. I had done some spikes and I can't remember where I left off before getting pulled into something else. I'll take a look again this weekend to see if I can wrap things up. If not I'll close the PR and let someone else takeover. |
|
👍 |
|
So I was able to remove the duplicate code by leveraging the |
jonathanfoster
changed the title
|
@thecodejunkie, everything is working now. I wrapped the call to This PR now includes 2 potential breaking changes. The |
| string errorPage; | ||
|
|
||
| if (!this.errorPages.TryGetValue(statusCode, out errorPage)) | ||
| if (!this.errorMessages.ContainsKey(statusCode) || !this.errorPages.ContainsKey(statusCode)) |
There was a problem hiding this comment.
Is this condition correct? We bail out if the statusCode is not in both dictionaries. Is that right?
There was a problem hiding this comment.
Yes, that's correct. This is a protection against calling this method without first checking HandlesStatusCode.
There was a problem hiding this comment.
Sorry. I don't understand that. How is this protecting against calling without first checking the status code?
I read it as ensuring that there is always a message and an error page for the status code.
There was a problem hiding this comment.
You're right, this protects against a status that doesn't have a message and an error page. I was referring to calling code that doesn't check HandlesStatusCode before calling Handle. Then someone could inadvertently pass in a non-supported status.
There was a problem hiding this comment.
OK, I think I finally understand 😴
| /// <summary> | ||
| /// Default error handler result | ||
| /// </summary> | ||
| public class DefaultStatusCodeHandlerResult |
There was a problem hiding this comment.
This is just an implementation detail of the DefaultStatusCodeHandler, right? Maybe this should just be a private class?
There was a problem hiding this comment.
I made this an internal class since it's referenced in functional tests.
|
Replaced by #2081 |
...r now supports application/json, application/xml, and text/html. The default content type is text/html when all media types are supported (/). If a non-supported media type is provided then the response status code will be 406 not acceptable.
I made a change to
ContextExtensions.GetExceptionDetailsthat could be considered breaking. I changed the method to returnstring.Emptyinstead of "None" when no exception is found. I didn't see any other usages in the solution, but I could see a situation where a consumer of the library has a check for "None" that will now fail. I don't think it's a huge concern, but something to consider. I added a unit test to ensure the method returnsstring.Emptyin the future. If there's an issue with this change then I could always revert it. I just thought thatstring.Emptyis a more idomatic return value in this situation.