Plug.Conn.Query encode question about plug HOT 11 CLOSED

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

from plug.

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é

from plug.

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

from plug.

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.

from plug.

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.

from plug.

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.

from plug.

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

from plug.

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.

from plug.

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

from plug.

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!

from plug.

Makes total sense, thanks!

from plug.