Skip to content

Conversation

pintsized
Copy link
Member

Fixes #217

@pintsized pintsized requested a review from hamishforbes March 6, 2021 11:53
Copy link
Collaborator

@hamishforbes hamishforbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just silently ignoring non-strings isn't ideal.
We should at least log a message that you're trying to send something that's being ignored.

Would it be better to try and convert to a string? Just with tostring, no serialising or anything.
If you set body to a number or a bool its reasonable to expect that to work and just send a stringified version? Certainly for numbers anyway.

@pintsized
Copy link
Member Author

I guess I was of the opinion that the API is perfectly clear (as is tcpsock:send incidentally so I'm surprised it allows Lua numbers) but we should do something because it's possible to get the client into a weird state. I just wanted to make the minimal change possible really.

But yeah, it's perhaps more idiomatic to assume we'll send string representations magically, and I guess that's harmless enough. I wouldn't be surprised if LuaJIT elides tostring(string) where possible anyway.

@pintsized pintsized changed the title Avoid sending non-string body values Ensure request body Content-Length is set correctly in simple interface Mar 9, 2021
@pintsized pintsized changed the title Ensure request body Content-Length is set correctly in simple interface Ensure request body Content-Length is set correctly Mar 9, 2021
@pintsized
Copy link
Member Author

pintsized commented Mar 9, 2021

On closer inspection this is actually about not having a Content-Length set in the request (we were only calculating on strings), and so the body is left on the wire. Also tcpsock:send supports a table of strings so I added explicit support for that as well.

I also thought I'd be good and return an error if your body is an iterator and you haven't set a length. But I'd forgotten about the get_client_body_reader in chunked transfer mode. Test coverage appears to be non-existent here. @hamishforbes Do you remember anything about this? Is it a supported thing - the comment suggests maybe not? Perhaps we need a flag the user must set, to treat the request body iterator as chunked or not? Or we drop my error handling and just assume chunked if there's no length?

This all feels like some foggy distant memory so any thoughts appreciated!

@hamishforbes
Copy link
Collaborator

Chunked request bodies aren't supported by ngx.req.socket so that's why they aren't tested / won't work with get_client_body_reader

But you might be sourcing a chunked body from somewhere else.

I think if you're setting the body to a function (and you're not proxying and therefore copying the request headers) you're in a fairly advanced use case and should be able to figure out content length / chunking yourself?

Co-authored-by: Thijs Schreijer <[email protected]>
@pintsized pintsized requested a review from hamishforbes March 15, 2021 14:47
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 this pull request may close these issues.

When body is not string next request on same host sends wrong method
3 participants