Support non-utf8-encoded query parameters in Req.Test

[craigfurman]

Alternative to #495

Draft to get initial feedback, I'm happy to update the docs here if this is on the right track

[craigfurman]

admittedly, it's a bit weird that I've extended the test plug to accept more options, but no extra options are actually used, since query parameter parsing takes place in steps.ex before this is called. Plug-y things are happened in there though, it seems quite coupled to me

[wojtekmach]

Yeah this could be considered another coupling where so far :plug and Req.Test are pretty orthogonal (Req.Test.Adapter naming notwithstanding). In other words, if this is to be supported there should be a more generic mechanism rather than going through Req.Test. I.e. we should be able to make this work:

iex> Req.get!("/foo?bar=\xFF", plug: &Plug.Conn.send_resp(&1, 200, ""))
** (Plug.Conn.InvalidQueryError) invalid UTF-8 on urlencoded params, got byte 255
    (plug 1.16.1) lib/plug/conn/utils.ex:298: Plug.Conn.Utils.do_validate_utf8!/3
    (plug 1.16.1) lib/plug/conn/query.ex:155: Plug.Conn.Query.decode_www_form/3
    (plug 1.16.1) lib/plug/conn/query.ex:136: Plug.Conn.Query.decode_www_pair/4
    (elixir 1.19.0) lib/enum.ex:2520: Enum."-reduce/3-lists^foldl/2-0-"/3
    (plug 1.16.1) lib/plug/conn/query.ex:123: Plug.Conn.Query.decode/4
    (plug 1.16.1) lib/plug/conn.ex:1086: Plug.Conn.fetch_query_params/2
    (req 0.5.15) lib/req/steps.ex:1035: Req.Steps.run_plug/1
    iex:1: (file)

The only idea I have is:

  Req.get!("/foo?bar=\xFF",
    plug: &Plug.Conn.send_resp(&1, 200, ""),
+   plug_options: [validate_utf8_query: false]
  )

and we'd continue with the Req.Test API you proposed.

That being said, I'm not sure about introducing a whole new mechanism for this pretty niche use case. We already have a bunch of built-in options so im hesitant to add more built-in ones.

This feature is kinda similar to Plug.Test ensuring header names are lowercase with a configuration escape hatch:

iex> Plug.Test.conn(:get, "/") |> Plug.Conn.put_req_header("Foo", "bar")
** (Plug.Conn.InvalidHeaderError) header key is not lowercase: "Foo"
    (plug 1.19.0-dev) lib/plug/conn.ex:1968: Plug.Conn.validate_header_key_normalized_if_test!/2
    (plug 1.19.0-dev) lib/plug/conn.ex:813: Plug.Conn.put_req_header/3
    iex:1: (file)
iex> Application.put_env(:plug, :validate_header_keys_during_test, false)
:ok
iex> Plug.Test.conn(:get, "/") |> Plug.Conn.put_req_header("Foo", "bar") |> then(& &1.req_headers)
[{"Foo", "bar"}]

so I suppose another idea is to have a some kind of similarly looking config :req, :plug_validate_utf8_query, false. But then again, dunno if global option is the answer.

:plug_options seems to me least bad especially if there other :plug aspects we'd like to configure that way down the road but yeah, still unconvinced. Starting to think this is all not worth it and should have shipped your initial PR after all.

[craigfurman]

The initial PR is still open if you prefer that approach btw.

plug_options seems reasonable too, I'm happy to tweak this PR if we want to go in that direction.

I know the benefit of supporting this is low (I guess that depends on how many applications out there actually use non-utf8 query params), but the cost seems pretty low too. Your call of course!

[wojtekmach]

Closing in favour of #495 after all. I really appreciate that you explored this solution, can always go with it down the road.