Make Tesla.Env maintain both request and response

[chulkilee]

Right now, Tesla.Env is used for two use cases - to represent a request, or a response. That makes middlewares to keep the request before running Tesla.run if the middleware needs both information (e.g. logger).

I'm just wondering what if Tesla.Env contains both request and response - like req_headers and resp_headers, etc. like Plug.Conn.

[teamon]

Regarding "what if" - not much, besides the need for duplicated headers manipulation functions and a breaking change for existing codebase, not only for headers but also body.

You can keep request info easily with a tiny middleware like #156.

[chulkilee]

Thanks for the feedback!

First of all, I'm aware that this will introduce breaking changes. I'm not asking for this changes right now but instead trying to discuss long-term changes for explicit and simple interface.

I see Tesla.put_opt can be used to store "state" across middleware. However I don't think that's a good pattern - another middleware may touch the value under the same key. Putting them under Tesla.Env structure makes it clear (although still random middleware manipulate them :) )

Also if there is a middleware putting random headers, currently callers cannot get the actual request header, unless the remote server response it back.

If the purpose of Tesla.Env is to capture the whole environment around the request (e.g. it also contains __client__), then why is it overwriting body and headers with values from response?

Or even further, it may be better to extract Tesla.Env to have Tesla.Request and Tesla.Response.

{:ok, %{body: %{"id" => id}}} = Tesla.get(client, "/hello")

{:ok, %{response: %{body: %{"id" => id}}}} = Tesla.get(client, "/hello")

[teamon]

I think adding one more level of nesting (:request, :response keys) would make pattern matching less pleasant.

Your points are perfectly valid, can't argue with that. However, at the same time it is quite a hypothetical discussion - I haven't yet encountered a case where having single body/headers fields caused an issue. Do you have any real-world example?

[chulkilee]

1. middleware wrapping Tesla.run

https://github.com/teamon/tesla/blob/v0.10.0/lib/tesla/middleware/logger.ex#L114

    env
    |> log_request
    |> log_headers("> ")
    |> log_params("> ")
    |> log_body("> ")
    |> Tesla.run(next)
    |> log_response
    |> log_headers("< ")
    |> log_body("< ")

could be written like this, which is more explicit.

    env
    |> log_request
    |> log_req_headers
    |> log_req_body
    |> Tesla.run(next)
    |> log_response
    |> log_resp_headers
    |> log_resp_body

defp log_headers(headers, prefix) # take headers, not Tesla.Env

defp log_req_headers(env), do: log_headers(env.req_headers, ">")

Also note that the current implemention has temporal coupling - you ask for body from Tesla.Env, and it can be request body or response body. Although that's manageable inside a middleware, I still think it's better to remove that

2. Investigate request details once completed

It's not easy in the current implementation.

---

Since Tesla.Env is only available for users as return value, adding req_headers and resp_headers won't be a breaking change. I know #156 would work but since request and response are the core concept of HTTP request lifecycle anyway, why don't we make them as first-citizen in Tesla.Env?

---

Splitting Tesla.Request is somewhat different topic so I'll leave it out from this issue (I was considering ExAws style to split building a request and performing a request for https://github.com/chulkilee/ex_force. But if we do so, then it's better to make it as part of Tesla.Env anyway..

[teamon]

I can't see the benefit of adding more layers - these are all private functions, used in a very limited scope. Besides, the 1.0 branch already has a new Logger (combining Logger & DebugLogger from 0.x) - https://github.com/teamon/tesla/blob/1.0/lib/tesla/middleware/logger.ex

Also note that the current implemention has temporal coupling - you ask for body from Tesla.Env, and it can be request body or response body. Although that's manageable inside a middleware, I still think it's better to remove that

Can you explain this point a bit more? I don't think I completely understand what you mean.

Since Tesla.Env is only available for users as return value, adding req_headers and resp_headers won't be a breaking change

It's hard to keep it compatible & consistent at the same time. Just adding these fields and populating them before/after request isn't enough. All calls to Tesla.put_header/set_header/etc would require keeping headers and req_headers/resp_headers in sync.

I know #156 would work but since request and response are the core concept of HTTP request lifecycle anyway, why don't we make them as first-citizen in Tesla.Env?

I think it's the same reason why it's preferable to use simple maps/kwlists instead of deeply nested named structures - convenience. As I mentioned before, pattern matching on response is very important part of tesla's design. You may like to read 0.x to 1.0 Migration Guide to see upcoming changes. All feedback is highly appreciated.

I was considering ExAws style to split building a request and performing a request for https://github.com/chulkilee/ex_force

What "style" do you mean? (Unfortunately ex_force has no README I could look at for examples)

[chulkilee]

Thanks for the feedback! Let me try to make it clear.

1. Temporal coupling in a middleware needs both request and response

(Originally I called them as middleware wrapping Tesla.run)

(I know that logger is for 0.10 (I put the link with the tag) and it is being replaced. I'm using it as an example for a middleware which needs both request and response.)

env has significantly different data under same key (headers and body). If you acecss them before Tesla.run, they return request headers and data. If you access them after Tesla.run, it returns response headers and body.

This is not necessarily bad, since Tesla functions return only response data (not request data) in Tesla.Env so all are "internal". Also it's "manageble" since it's pretty easy to know when Tesla.run is called inside single middleware.

However, if we split request data and response data in Tesla.Env, then we can reduce (even small) cognitive overhead of guessing "this is req header or resp header". :)

2. Having both req_headers and resp_headers

It's hard to keep it compatible & consistent at the same time. Just adding these fields and populating them before/after request isn't enough. All calls to Tesla.put_header/set_header/etc would require keeping headers and req_headers/resp_headers in sync.

I don't know your plan on 1.0 (e.g. is it "rc" or any future work). But if we add resp, then I think it's better to rename fields and functions to have req_ and resp_ prefix. Like Plug.put_req_header.

+ Nested

I think it's the same reason why it's preferable to use simple maps/kwlists instead of deeply nested named structures - convenience. As I mentioned before, pattern matching on response is very important part of tesla's design.

I agree! I don't think it's necessary to introduce nested structure to return request data in Tesla.Env.

I'm bring up this idea only because it makes easy to move on "ExAws" style (see the next section)

+ ExAws style

ExAws separate 1) building a request and 2) performing the request. I considered this pattern when writing ex_force.

S3.list_objects |> ExAws.request!

I'm not sure this is necessary better. It has some benefits but also introduces some complexity. Even Tesla supports it, we still need "helper function" to do everything in one function.

Again, this is out of scope of this issue so once I have better idea I'll open another issue for discussion :)

[chulkilee]

As we have Tesla.Middleware.KeepRequest - closing it.