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

Headers should be lower-cased #35

Closed
opqdonut opened this issue Nov 12, 2024 · 5 comments · Fixed by #36
Closed

Headers should be lower-cased #35

opqdonut opened this issue Nov 12, 2024 · 5 comments · Fixed by #36

Comments

@opqdonut
Copy link
Member

Currently, ring-http-response uses header names like "Content-Type". However, the ring spec defines that headers should be lower-cased: https://github.com/ring-clojure/ring/blob/master/SPEC.md#headers-1

This is important in case there are middleware in the pipeline that expect to be able to parse eg. the content-type header, and are looking for it under the "content-type" name.

I think we should label this fix as a breaking change, since somebody might've been relying on the exact naming of the headers.

@opqdonut
Copy link
Member Author

Interesting, the "Content-Type" header comes directly from ring.util.response:

https://github.com/ring-clojure/ring/blob/f7dfe7c9b98c335e4573e61d94c523d5df9a6667/ring-core/src/ring/util/response.clj#L184

@opqdonut
Copy link
Member Author

Let's keep this open while the original ring bug is open.

@opqdonut opqdonut reopened this Nov 14, 2024
@weavejester
Copy link

Thanks for discovering this bug. The error was in the spec. It looks like when I converted it to markdown the "lowercased" adjective was added to the response header definition erroneously. I've since fixed this and the spec should now be correct.

@opqdonut
Copy link
Member Author

Yeah, thanks for the clarification!

@opqdonut
Copy link
Member Author

The original fix was reverted in #37

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 a pull request may close this issue.

2 participants