elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.84k stars 582 forks source link

1.15 changes decoding behaviour of `[]` #1175

Closed SteffenDE closed 10 months ago

SteffenDE commented 11 months ago

Hi there,

updating to Plug 1.15, we noticed a breaking change with how parameters from a phoenix form are parsed:

%{
  "_target" => "role[permissions][][foo]",
  "role[permissions][][foo]" => "false",
  "role[permissions][][bar]" => "false",
  "role[permissions][][baz]" => "true",
}
|> URI.encode_query()
|> Plug.Conn.Query.decode()

1.14:

%{
  "_target" => "role[permissions][][foo]",
  "role" => %{"permissions" => [%{"bar" => "false"}, %{"baz" => "true"}, %{"foo" => "false"}]}
}

1.15:

%{
  "_target" => "role[permissions][][foo]",
  "role" => %{"permissions" => [%{"bar" => "false", "baz" => "true", "foo" => "false"}]}
}

Just wanted to ask whether this is an expected change or not.

josevalim commented 11 months ago

I will try to revert it to the previous behaviour but please note this has always been unspecified, as it has always been ambiguous. :) If you can avoid relying on it, it would be beter. What is your use case?

SteffenDE commented 11 months ago

If you can avoid relying on it, it would be beter. What is your use case?

Some legacy code that was quickly hacked together and an Ecto schema refactor leading to the changeset wanting other parameters than we use in the form.

(The form sends %{"role" => %{"permissions" => [%{"bar" => "false"}, %{"baz" => "true"}, %{"foo" => "false"}]}} - we use checkboxes for each permission - and Ecto wants %{"role" => %{"permissions" => [%{"permission" => "baz"}]}. In the database, there is one row for each permission that is active.)

I think you're right that we should not rely on it. I'll probably refactor this when I find some spare time...

josevalim commented 10 months ago

You can also fix it by adding a continuous number:

%{
  "_target" => "role[permissions][][foo]",
  "role[permissions][0][foo]" => "false",
  "role[permissions][1][bar]" => "false",
  "role[permissions][2][baz]" => "true",
}
|> URI.encode_query()
|> Plug.Conn.Query.decode()

And then getting Map.values of the permissions map.

josevalim commented 10 months ago

Unfortunately this is not a straight-forward change :( I have added warnings to the docs and CHANGELOG.

SteffenDE commented 10 months ago

Just to be clear: this was more of an "is this expected" issue than a real problem. The fix in our code was straightforward, just unexpected because there was nothing in the changelog. So no worries, I'm happy that this is documented as unspecified behaviour.

Thank you!