fix: "XML parsing error: Hub reply does not define a root element #4075" #4223
Conversation
| @@ -154,7 +154,7 @@ public Task ProcessRequest(IDictionary<string, object> environment) | |||
| environment.DisableResponseBuffering(); | |||
|
|
|||
| var response = new OwinResponse(environment); | |||
|
|
|||
| response.ContentType = "text/html"; | |||
There was a problem hiding this comment.
Hrm, I think text/html is not a great choice for this. Also, we do set the mime type correctly in some requests (see SendJsonResponse) so I'd rather narrow this to make sure we understand where the Content-Type is missing and add the correct Content-Type values there.
There was a problem hiding this comment.
- In this particular place if you call response.ContentType just after constructor call it will respond "text/html" but "Content-Type" will not be added to the response until you set it explicitly. Don't ask me why.
- there are 2 places where ContentType is missing. But I think that all places which have a specific content type will just override default one. So I think that having default one is a good choice (you do want to have all response with "Content-Type" set don't you?)
There was a problem hiding this comment.
- Don't ask me why.
This is because it is a convention that text/html is the default Content-Type. OwinResponse is using that value to indicate that there is no active Content-Type. However, as you noticed, some browsers dislike that and emit a warning.
2. So I think that having default one is a good choice
I don't think it's a good idea to have an inaccurate content type. I'd much prefer that we set it where we know what the content type is. Accidentally forgetting to set the content type yields an error message in the dev tools console which can easily be ignored and doesn't cause a funtional problem. Accidentally putting the wrong content type in place can yield functional problems. It's a matter of which is more dangerous to forget to do, and simply not setting the Content-Type is much less dangerous.
|
|
|
@anurse Please review |
| }; | ||
|
|
||
| transport.Object.ProcessRequest(transportConnection.Object).Wait(); | ||
| response.VerifySet(r => r.ContentType = It.IsAny<string>(), "ContentType not set"); |
There was a problem hiding this comment.
Could you assert the specific content type? We're trying to avoid simple "was this property set" or "was this method called" test cases moving forward since they are brittle and don't test the actual desired outcome.
| @@ -106,6 +106,7 @@ protected Task ProcessRequestCore(ITransportConnection connection) | |||
| } | |||
| else if (IsAbortRequest) | |||
| { | |||
| Context.Response.ContentType = "text/html"; | |||
There was a problem hiding this comment.
Can we make this text/plain?
|
Thanks for the contribution @lAnubisl ! |
No description provided.