Set-cookie headers get overridden about tesla HOT 27 CLOSED

@amatalai Thank you, it seems I had to check the result of Tesla.run(next) for errors, but that and a little change to one of the tests was enough to get it working. I'll be waiting for a proper solution :)

This was the exact changes if someone is interested:

# lib/tesla/middleware/core.ex
defmodule Tesla.Middleware.Normalize do
  def call(env, next, _opts) do
    env2 =
      env
      |> normalize
      |> Tesla.run(next)

    case env2 do
      env2=%Tesla.Env{} ->
        env2
        |> Tesla.put_opt(:raw_headers, env2.headers)
        |> normalize
      other ->
        normalize(other)
    end
  end

  ...
end

from tesla.

from tesla.

Well, I've decided to make the headers a Map simply because it is convenient (i.e. resp.headers["content-type"] syntax), but this does has a cost of overriding duplicate headers.

I think I'd like to keep that as the default (unless there is some other nice solution), but there should be a way to override this behaviour.

For now I have two ideas:

1. Make the Normalize middleware optional (which will break all current code using tesla)
2. Save raw headers into Tesla.Env so that they can be accessed when needed.

I guess the 2 would be better (could maybe be even generalized as env.raw_response). Let me know what do you think of that.

from tesla.

I like the option 2 more, but correct me if I'm wrong: the env.raw_response would be different depending on the adapter you are using?

Maybe make a way to provide your own normalize functions when building a client (actually when injecting the middleware)?

from tesla.

from tesla.

@teamon maybe you could add something like this (I can clean this up and add tests if needed) :)

from tesla.

I gave this some more thought: how about letting the default_middleware function be overridable, and keep the rest of the code like in my previous comment?

from tesla.

Hey @BogdanHabic, sorry for the delay 🎄 (I'll also be off since tomorrow until 3.01)

The longer I think about this one the more convinced I am to make a breaking change in favour of Tesla.Middleware.HeadersMap optional middleware. In the README the first example of a client is already using three middlewares (BaseUrl, Headers and JSON) and I think one more should not be a big issue - I definitely should add some more comments to that first example anyway.
This way it is explicit that headers will be a map and there is no need for overrides.

I'll get back on that later next week and check the impact of such change in my production projects.

from tesla.

Hey @teamon, happy holidays!

If I can help in any way, do let me know. :D

from tesla.

Questions / TODOs:

• How big is the impact on existing codebases?
• How to make Headers middleware work with both Map and List?
• Should Normalize middleware still convert headers to String & downcase?
• How to handle headers access required by middlewares like JSON?
• Improve README

from tesla.

@BogdanHabic After giving this a lot of thoughts, to implement the first option (switch to map) would result in:

• How big is the impact on existing codebases?
  • changing all calls to env.headers["foo"] into something like Tesla.get_header(env, "foo") - not cool, but not that crazy or impossible
• How to make Headers middleware work with both Map and List?
  • possible matching both cases and using appropriate modules functions (List/Map)
• Should Normalize middleware still convert headers to String & downcase?
  • probably not, but that would make headers lookup much slower
• How to handle headers access required by middlewares like JSON?
  • all access would have to go via Tesla.get_header(env, name)
• Any other downsides?
  • pattern matching on headers map is out, no more this

Considering all the above I'm more in favour of providing env.orig_headers :: [{String.t, String.t}] plus a utility function Tesla.get_orig_header(env, name) that would perform a case-insensitive search in the orig_headers list.

Btw @amatalai, does this use case match any of the :private ones?

from tesla.

@teamon I like the solution you described.

Just a quick question: Why would the search be case-insensitive?

If you need the original headers, not the normalized ones, I would assume you would also know the casing of the header?

from tesla.

Arr, you're right (it's late...). I guess we don't really need that function at all (using Enum.find/filter should be enough)

from tesla.

@teamon You could also store it as a map where the key is the header, and the value is a list of header values.

That way you get quick lookup, and for headers you know have just value you can use hd (or wrap it into a get_single_orig_header).

from tesla.

@teamon yes, things like :raw_headers and/or :raw_body is what I would like to store inside env.private :)

from tesla.

Are there any workarounds for now? I'm working with a webserver that sends multiple Set-Cookie headers and I'm new to the lib so I couldn't think of a quick solution.

from tesla.

@edupc Easiest/fastest workaround which I can imagine right now is to fork Tesla and change normalize middleware to save raw headers in env.opts.

Something like this:

# this code wasn't tested in any way
defmodule Tesla.Middleware.Normalize do
  def call(env, next, _opts) do
    env2 =
      env
      |> normalize
      |> Tesla.run(next)

    env2
    |> Tesla.put_opt(:raw_headers, env2.headers)
    |> normalize
  end

...
end

Btw I'm planning to make some proposal PR in next few days that will cover both this issue and #48

from tesla.

Hey guys,

I fixed this issue by populating duplicate headers as a list in the headers map:
https://github.com/farhadi/tesla/commit/048391f6e7906ad735b1dc8f91fa3d9a5770cf77

let me know what you think.
I can write tests if the solution is acceptable to be merged.

from tesla.

How about the ability to prepend middleware? Then one could do some work before Normalize and tuck whatever is needed away into env.

Apropos to this discussion: https://stackoverflow.com/questions/4371328/are-duplicate-http-response-headers-acceptable

from tesla.

How about the ability to prepend middleware? Then one could do some work before Normalize and tuck whatever is needed away into env.

I think duplicate headers like set-cookie are too common to be handled manually, the library should support them by default.

from tesla.

I'm sorry @farhadi, but your solution is not acceptable - it leads to almost nondeterministic behaviour of headers map values being sometimes lists and sometimes binaries requiring the end user to check for both cases.

I think the only reasonable approach here is to make a breaking change, with Normalize returning headers as map(binary => [binary]) plus a separate, backward-compatible middleware SingleHeaders (draft name) that would use only first value for each header thus preserving the current behaviour.

from tesla.

If the map(binary => [binary]) path is chosen I would be more than willing to add it (I actually have a fork that is already doing this, I would just need to clean it up).

from tesla.

from tesla.

What about having a DuplicateHeader middleware which accepts a list of headers that should be returned as a list while keeping others as strings?

from tesla.

Why not just have a utility function which would check if the header exists in the map, and if it does, returns the result of List.first(headers["key"]) or nil if the header doesn't exist?

That way the JSON middleware will instead of accessing the map directly use the helper instead.

from tesla.

With #104 in place it is now possible to do something like this:

defmodule Client do
  use Tesla

  def new do
    Tesla.build_client [], [
      :keep_original_headers
    ]
  end

  def keep_original_headers(env, next) do
    env = Tesla.run(env, next)
    Tesla.put_opt(env, :original_headers, env.headers)
  end
end

and then

iex> env = Client.new |> Client.get("http://google.com")
iex> env.opts[:original_headers]
[{'cache-control', 'private'}, {'date', 'Sat, 07 Oct 2017 16:38:24 GMT'},
 {'location',
  'http://www.google.pl/?gfe_rd=cr&dcr=0&ei=AAPZWfGtM-Sv8weEg4igDg'},
 {'content-length', '268'}, {'content-type', 'text/html; charset=UTF-8'},
 {'referrer-policy', 'no-referrer'}]

@BogdanHabic would that solve your issue?

from tesla.

hi there - sorry for piggybacking on this older thread. It is about cookies, and I just started using Tesla. Are there any examples of how to handle the cookies, with Tesla. Right now I just want to be able to send back the cookies I am getting from the server i.e JSESSIONID - Thank you

from tesla.