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

http.request api should be improved #436

Open
johnd0e opened this issue Sep 10, 2024 · 4 comments · May be fixed by #438
Open

http.request api should be improved #436

johnd0e opened this issue Sep 10, 2024 · 4 comments · May be fixed by #438

Comments

@johnd0e
Copy link

johnd0e commented Sep 10, 2024

Sinks (as well as filters) require response metadata to properly process data. There's a significant difference between receiving a 200 or 404 status code, or whether the Content-Type is application/json, text/html, or text/event-stream.

Without this metadata, I’m forced to rely on unreliable heuristics in the sink/filter code, which is far from ideal.

It's unfortunate because the response metadata is already retrieved but simply isn't accessible within the sink.

@alerque
Copy link
Member

alerque commented Sep 10, 2024

Contributions welcome! I don't have much development time for this right now but would be happy to step in as a maintainer role to facilitate merging PRs if somebody tackles this.

@johnd0e
Copy link
Author

johnd0e commented Sep 10, 2024

Well, at least I have an idea for a backward-compatible improvement:

When a sink is not specified, the first value returned by http.request could be more than just 1. It could be a function that retrieves the body, with the sink passed as an argument.

@bassklampfe
Copy link

I'm having similar issues.
I'd prefer not to change the API that much. A simple callback between shouldredirect and shouldrecevicebody fullfills my need.

*** http-orig.lua	2024-09-15 10:18:23.000000000 +0200
--- http.lua	2024-09-15 10:22:48.242167029 +0200
***************
*** 336,347 ****
--- 336,353 ----
      -- at this point we should have a honest reply from the server
      -- we can't redirect if we already used the source, so we report the error
      if shouldredirect(nreqt, code, headers) and not nreqt.source then
          h:close()
          return tredirect(reqt, headers.location)
      end
+ 
+ 	-- check if headers
+ 	if nreqt.processheaders then
+ 		nreqt.processheaders(headers)
+ 	end
+ 
      -- here we are finally done
      if shouldreceivebody(nreqt, code) then
          h:receivebody(headers, nreqt.sink, nreqt.step)
      end
      h:close()
      return 1, code, headers, status

My code then looks like this

	:::      
	local content_length
	local function processheaders(hdr)
		content_length=assert(tonumber(hdr["content-length"]),"no content-length in header")
	end
	local request=
	{
		method = "GET",
		url = url,
		processheaders = processheaders,
		sink = sink,
	}
	:::
``

@johnd0e
Copy link
Author

johnd0e commented Oct 14, 2024

@alerque

Contributions welcome!

May I please request that my PR be reviewed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants