Plug.Conn.Query encode question

[mnussbaumer]

Hi,

I ran into two situations when encoding parameters for x-www-formurlencoded using Plug.Conn.Query.encode for body requests and I wanted to understand if this is working according to the spec and is stripe's parsing/documentation that is out of spec, or if this out of scope for this module, or is something that should be changed (enabled/allowed), and if so, if a PR for it is welcomed - since I will do some work on this for my own needs anyway.

First is the error that is raised when a map has more than 1 key inside a list.

req = %{
      success_url: "http://localhost:4000/payments/success",
      cancel_url: "http://localhost:4000/payments/cancel",
      line_items: [
        %{
          price_data: %{
            currency: "EUR",
            product_data: %{name: "Stuff"},
            unit_amount: 100
          },
          quantity: amount
        }
      ],
      payment_method_types: %{"0" => :card, "1" => :acss_debit, "2" => :sepa_debit},
      mode: "payment",
      metadata: %{user_id: user_id}
    }

results in:

 Plug.Conn.Query.encode(req)
** (ArgumentError) cannot encode maps inside lists when the map has 0 or more than 1 element, got: %{price_data: %{currency: "EUR", product_data: %{name: "Gold Coins"}, unit_amount: 100}, quantity: 5}
    (plug 1.13.6) lib/plug/conn/query.ex:226: anonymous fn/3 in Plug.Conn.Query.encode_pair/3
    (elixir 1.13.4) lib/enum.ex:4086: Enum.flat_map_list/2
    (plug 1.13.6) lib/plug/conn/query.ex:235: Plug.Conn.Query.encode_pair/3
    (plug 1.13.6) lib/plug/conn/query.ex:262: anonymous fn/3 in Plug.Conn.Query.encode_kv/3
    (elixir 1.13.4) lib/enum.ex:1213: anonymous fn/3 in Enum.flat_map/2
    (stdlib 3.16.1) maps.erl:410: :maps.fold_1/3
    (elixir 1.13.4) lib/enum.ex:2408: Enum.flat_map/2
    (plug 1.13.6) lib/plug/conn/query.ex:266: Plug.Conn.Query.encode_kv/3
    (plug 1.13.6) lib/plug/conn/query.ex:204: Plug.Conn.Query.encode/2

Here I'm not sure why the raise was implemented so I might be missing something? If this was

[0][map_key_1]=xxx
[0][map_key_2]=yyy
[1][map_key_1]=zzz
[1][map_key_2]=www

That seems to encode properly the information?

The second is related to that as well, even if there's only a single key, it will encode lists as [][map_key] instead of [n_index][map_key].
I've noticed something that is probably related to this when using phoenix with embeds/assocs and inputs_for where the params submitted through the form are decoded as a map keyed by the index instead of a list. Changesets deal with it as if it was a list, but the first time I thought something was wrong on my forms now I see that this might be somehow related.

Stripe does accept the [] notation for primitive lists though, so it's also not very consistent, although it seems to also accept the [n_index]=primitive in those cases as well, but doesn't accept [] for some cases.

I've ran into this as I've been writing a lib to generate SDK clients from open api 3 specs ex_aopi stripe. And when generating them for stripe, there's instances (i.e.: line items, https://stripe.com/docs/api/checkout/sessions/create ) where both their doc and open api spec include define lists (arrays in oapi) for a field with object items (it might also be that for this instance, since the order of the line items is possibly important, they require index fields, but given an array/list is naturally ordered it seems they could do away with it... not sure if it's any shortcoming or to ease their parsing side either).

To summarize, wanted to know if someone has looked into this previously, and the reasons for it working as it is.

If wanted, a simple solution would be to add another optional arg, to how to encode lists (this would keep backwards compatibility, while enabling one to pass lists_as: :indexed), and do indexed based encoding for lists

Thanks!

[josevalim]

Given we raise today, We can simply support the new format, so a pull request That does so is welcome. We need to make sure we parse it correctly too

[mnussbaumer]

By parse you mean the decode function then?
Should it work the same way, if passed an option it assembles an ordered list, and if not keeps the current behaviour?

I see the fun sig has a few more params than the encode one,

def decode(
        query,
        initial \\ %{},
        invalid_exception \\ Plug.Conn.InvalidQueryError,
        validate_utf8 \\ true
      )

But until a new major plug version the current signature can't change, so the solution would be adding a new optional keyword param at the end?

The usual approach for optional params is keyword and get the option from it right? It will only be a single param right now, but not sure if we should translate it into a map on the first iteraction, to then be able to do cleaner guards in the inner funs?

ps: thanks for all the work you do José

[josevalim]

We may not need an option. I think that it will work just fine but you need to double check.

[mnussbaumer]

Decoding it seems to work as of now to parse, where field[index][key] parses as a map.

ie:

 Plug.Conn.Query.decode("test=yeap&payment_method_types[0]=card&payment_method_types[1]=acss_debit")
%{
  "payment_method_types" => %{"0" => "card", "1" => "acss_debit"},
  "test" => "yeap"
}

Although one might expect to see a list on payment_method_types. I see the reason for it though, it makes parsing way more straightforward and there's plenty of ambiguity (like if it's an actual object but it just has ints as the keys).

I've just written something that seems to work, but it needs to generate a couple function clauses through metaprogramming:

# simple lists
Plug.Conn.Query.decode("test=yeap&payment_method_types[0][test]=card&payment_method_types[1][test]=acss_debit")

# nested maps
Plug.Conn.Query.decode("test=yeap&payment_method_types[0][test]=card&something_else=20&payment_method_types[1][test]=acss_debit")

# complex nesting
Plug.Conn.Query.decode("test=yeap&payment_method_types[0][test]=card&something_else=20&payment_method_types[1][second][0]=acss_debit&payment_method_types[1][second][1]=anotheR_one")

Would parse respectively as:

# simple lists
%{
  "payment_method_types" => ["card", "acss_debit"],
  "something_else" => "20",
  "test" => "yeap"
}

# nested maps
%{
  "payment_method_types" => [%{"test" => "card"}, %{"test" => "acss_debit"}],
  "something_else" => "20",
  "test" => "yeap"
}

# complex nesting
%{
  "payment_method_types" => [
    %{"test" => "card"},
    %{"second" => ["acss_debit", "anotheR_one"]}
  ],
  "something_else" => "20",
  "test" => "yeap"
}

In case an array is specified by [n] notation but there's no values < n present, a list would still be returned with every index that wasn't provided as nil. But then there's still the case if it actually is an object that just happens to have ints as keys, but if it was behind a opt switch (disabled by default) it could perhaps be worth adding. Another option to try to prevent that, is to keep some more info while accumulating, so that instead of immediately building the list, we could perhaps check after the nested levels return if the map has a key "0" and then reduce_while, accumulating as an ordered list somehow or bailing with the original map if any key is not an int representation, or there's gaps. Although doable is more complex. We can put some checks with map_size to prevent exploits but I have a feeling that it might be exploitable any way.

[mnussbaumer]

The thing is, if it's worth adding any of this to Plug. The encoding won't hurt to have in plug I guess. Because for my usage the parsing doesn't really pose a problem because changesets can handle it the way it's built. It's just the outgoing body that I need to encode (in this particular case) without required the user to pass params that in the spec show as lists, as a map.

[josevalim]

Yes, lists are only if you don't specify any key at all. So it is expected for all of these to be maps and I wouldn't change it. In fact, I now think your answer is to use maps. Your input should be this:

req = %{
      success_url: "http://localhost:4000/payments/success",
      cancel_url: "http://localhost:4000/payments/cancel",
      line_items: %{
        0 => %{
          price_data: %{
            currency: "EUR",
            product_data: %{name: "Stuff"},
            unit_amount: 100
          },
          quantity: amount
        }
      },
      payment_method_types: %{"0" => :card, "1" => :acss_debit, "2" => :sepa_debit},
      mode: "payment",
      metadata: %{user_id: user_id}
    }

Also note that, afaik, there is no specification for this, so everyone pretty much rolls with whatever they want.

[mnussbaumer]

Yeap, I can agree with that, hence why I decided to ask first. I know indeed it works when done like that, my issue is trying to reconcile automatically generating typespecs from oapi specs with the encoding (I'll personally opt to allow the list mode, but since I was using Plug.Conn.Query for the body wondered if this was something wanted as encoding doesn't present any problem), because in this particular case if you read the stripe docs, it says a list of objects, and you might jump to writing it as a list in your function calls - you would need to read the typespec very carefully/remember the gotcha to see it would be a map and not a list. And the typespecs get pretty long, https://hexdocs.pm/exoapi_stripe/0.1.3/ExOAPI.Stripe.SDK.Checkout.html#post_checkout_sessions/2

I could also special case this type when generating the typespecs but that might even be more complex. Anyway, thanks. I'll close this, if wanted at some point just re-open

[josevalim]

We could change encode with the caveat that decode will return a different structure and I am not sure if that’s a good idea.

[josevalim]

If OpenAPI specifies how things should be encoded, then perhaps it should be part of the OpenAPI lib. :)

[mnussbaumer]

This is indeed a problem with spec/stripe side not Plug.Conn.Query per se José, but since I'll have to write it anyway was checking if it would make sense to PR, just that!

[josevalim]

Makes total sense, thanks!