httpclient Response object references original Request; refs #16685

[daehee]

• Add request field to Response & AsyncResponse objects,
• Update related tests in thttpclient.nim
• Clear "ambiguous identifier" error on tasynchttpserver.nim

[daehee]

@dom96 thanks for the feedback, updated

[timotheecour]

DRY:

Suggested change

	doAssert clientAgent.contains("Nim httpclient")
	doAssert clientAgent == defUserAgent

[timotheecour]

Suggested change

	let clientAgent = $req.headers["user-agent"]
	let clientAgent = req.headers["user-agent"]

[timotheecour]

LGTM after addressing remaining comments

[timotheecour]

let's wait until #16710

[Araq]

What are the use cases? In general you should avoid keeping everything in memory just because somebody might need it later. The "later" may never come but everybody pays the price. (Higher memory consumption.)

[daehee]

@Araq I'd have to think more on the broader range of use cases for the community, but here are some immediate examples:

1. To see what ultimately is constructed by httpclient request, which can be used e.g. calculate full size of the original Request (sum of method, URL, headers and body)
2. Get the history of 3xx redirects e.g. https://requests.readthedocs.io/en/master/user/quickstart/?highlight=history#redirection-and-history

Without the original request attached to the Response, the original Uri, HttpHeader, other related request data needs to be passed along with the Response and may lead to more verbose, error-prone code.

Just as a quick hacky example:

import uri, httpclient, options, strformat

proc getRedirectTarget(resp: Response, u: Uri): tuple[url: Uri, code: HttpCode] =
  if resp.headers.hasKey("location") and resp.code.is3xx():
    var target = parseUri(resp.headers["location"])
    if not target.isAbsolute():
      target = parseUri(&"{u.scheme}://{u.hostname}{target}")
    result = (target, resp.code)

let url = "https://tinyurl.com/examplezoom"
let client = newHttpClient(maxRedirects=0)
let resp = client.get(url)
echo resp.getRedirectTarget(parseUri(url))

[Araq]

To see what ultimately is constructed by httpclient request, which can be used e.g. calculate full size of the original Request (sum of method, URL, headers and body)

For debugging, make the debugging code opt-in.

[juancarlospaco]

This should be compile-time opt-in,
for performance, debugging and backwards compat.

[timotheecour]

For debugging, make the debugging code opt-in.

it's not just for debugging, but how about this:
add:

type RequestOption* = object
  preparedRequest*: bool
  # can grow in future
proc initRequestOption(): RequestOption = RequestOption(preparedRequest: false)
proc request(..., option = initRequestOption()) = ...

rationale for object instead of set[SomeOption]: same as in jsonutils #15133 (comment)

[dom96]

For the best of both worlds, just put these additions behind a define: when defined(httpRespReqRef) (or something, feel free to choose a better name).

[timotheecour]

I don't see how when defined(httpRespReqRef) is the "best of both worlds", whether to populate RequestOption should be decided at call site, not via some global all-or-nothing flag; if some 3rd party dependency requires it, you'd be forced to use it. defined that controls API's should be used sparingly, eg for legacy behavior, not for things that can co-exist long in the future.

Also, defined flags should have nim prefix.

[daehee]

On a related note what's the possibility of the Request type in asynchttpserver being refactored out and shared with httpclient? This new request object discussed here would create redundancy with what already exists in asynchttpserver:

Nim/lib/pure/asynchttpserver.nim

Lines 59 to 66 in a90f7a6

	Request* = object
	client*: AsyncSocket # TODO: Separate this into a Response object?
	reqMethod*: HttpMethod
	headers*: HttpHeaders
	protocol*: tuple[orig: string, major, minor: int]
	url*: Uri
	hostname*: string ## The hostname of the client that made the request.
	body*: string

[juancarlospaco]

I like the idea of a shared Request object, if it is possible.

If you really taking the effort to improve it, would be better first to fix httpclient to be reusable, as it is now is kinda single-shot thing,
1st request always work, multiple requests with the same httpclient may eventually fail, thats even more broken.

[ringabout]

let's wait until #16710

@timotheecour

[Araq]

This is not opt-in. Please re-open once addressed.

[bung87]

how about add a property like preserveRequest to client type and give a bool value when constructor new client ?