Use consistent casing for headers#379
Conversation
| @io.write(HTTP_11) | ||
| @headers.each do |k, v| | ||
| next if k == "Content-Type" | ||
| next if k == "content-type" |
There was a problem hiding this comment.
This was the root cause for the behaviour described in the description.
The middleware sets content-type to application/json; charset=utf-8 in the default headers: https://github.com/discourse/message_bus/blob/main/lib/message_bus/rack/middleware.rb#L84-L88
The intent if this line appears to be to "skip" writing the content-type to the string buffer because the client will do so later. But since the casings didn't match, the default content-type ended up in the buffer. Then later...
|
|
||
| TYPE_TEXT = "Content-Type: text/plain; charset=utf-8\r\n".freeze | ||
| TYPE_JSON = "Content-Type: application/json; charset=utf-8\r\n".freeze | ||
| TYPE_TEXT = "content-type: text/plain; charset=utf-8\r\n".freeze |
There was a problem hiding this comment.
The client added the Content-Type: text/plain; charset=utf-8 here (or at least does for chunked encoding). This is why content-type was in the http response twice.
|
@moberegger Thank you for this! We started encountering issues when deploying the new gem as well. |
|
@tgxworld Oops! I just realized I forgot to add an entry to the change log. Do you mind taking care of that for me when you gear up the next release? |
|
@moberegger No worries. I'll add it when I cut the new release. |
Since the new 4.5 release, I've noticed the same header being written to the
rack.hijackbuffer multiple times. For example(Note: I trimmed out some headers for brevity)
content-typeis there twice; once withapplication/json; charset=utf-8, once withtext/plain; charset=utf-8. Notice that the casing is different. Root cause appears to be changes in #371 that switched casing in some places, but not others. For exampleMessageBus::Clientwould check forContent-TypebutMessageBus::Rack::Middlewaresetcontent-type. Easier to explain in my code annotations below.Simplest fix is to just use lowercase headers everywhere. I considered using
Rackconstants instead, but figured this was easier.content-typeis the only header that I noticed this behaviour with, but I felt it was best to be consistent with the casing across the board to help prevent/catch future regressions.