express
express copied to clipboard
expressjs
fix(res.send): add Content-Length header only if Transfer-Encoding is not present
Intent
- Because
Content-LengthandTransfer-Encodingcan't be present in the response headers together,Content-Lengthshould be added only if there is noTransfer-Encodingheader.
Hey @dougwilson , Does this PR look good to you or do I need to adjust something else?
Hey, sorry I was just trying to get the new release out. It looks like you still need to add a test for the change to res.redirect, as that was changed too.
I also took a look in the Node.js source and they also have logic for content-length vs transfer-encoding, but node.js only does stuff if transfer-encoding contains chunked while here is it any value (even a single space character). Is that intentional? Should we follow what the node.js logic is? It may help if the underlying issue you ran in to for this change was disclosed.
Hey @dougwilson,
No worries. As far as I understand it doesn't matter if transfer-encoding contains chunked or not. transfer-encoding header can't be present with content-length in response headers at the same time. Please correct me if I'm wrong.
About the issue we are running in.
We are building this https://github.com/sasjs/server NodeJS wrapper for calling the SAS binary executable and we are building some endpoints for test purposes that mock some external API. And to do so we need to respond with exact same headers and payload. And this external API uses transfer-encoding header that I can't set because in such case I'll have both transfer-encoding header and content-length header in my response. I've been also trying to delete content-length header from response before setting transfer-encoding, but it didn't work.
Ah, ok. Can you add tests for other values of transfer-encoding besides chunked as well, then? We want to make sure that res.send will work with other values too.
As for mocking exact replicas of other apis, you typically want to use the lower-level apis for that like res.write and res.end instead of res.send.
Hey @dougwilson,
Sorry for the late responce- I've been on vacation.
And thank you for suggestion to use res.write.
I reverted changes to res.redirect to dedicate current PR to res.send. I adjusted my unit test to cover all tranfer-encodings.
Please let me know if something else is needed to be done.
Regards, Yury
Hey! It's no problem at all :) Thanks for adding to the tests. But I think there is an issue with them: though you added other values, it does not seem like res.send is actually honoring them. For example, if you have Transfer-Encoding: gzip set, can the test show that res.send will actually send it as gzip? Example I would expect res.set('transfer-encoding').send({foo:'bar'}) to be a gzip response with this changeset, right?
Hey @dougwilson!
Can we actually test that our response transforms to gzip? I think its very low level and I'm not sure that nodejs can transform our response to gzip/compress/deflate. Because I did not find any mentions about transfer-encoding that it can be a gzip/compress/deflate response, only it’s a chucked(nodejs documentation). But okay, we exactly know that our response can be transformed to chunks and how we can test it? I don’t have a clue.
Of course tools like zlib can be used for content-encoding but it's not the same as transfer-encoding. (mention it to not to be confused)
I have a suggestion to check our response for having a transfer-encoding header with value that we set on the server but I’m not sure about do we need this. And if we need this check, can I do this commit? I want to make my first contribution to open source!
Please, correct me guys if I’m wrong. Thank you!
You can check the spec for transfer-encoding at https://datatracker.ietf.org/doc/html/rfc9112#name-transfer-encoding
The very first example shows how it can invlue gzip.
@dougwilson Do you expect that response has corresponding Transfer-Encoding? e.g. if we set Transfer-Encoding: gzip on server, we should get it in response on client?