phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.47k stars 2.88k forks source link

weird behaviour when deep merging endpoint options #5758

Open sasa1977 opened 8 months ago

sasa1977 commented 8 months ago

Environment

Actual behavior

Phoenix endpoint supports providing options overrides via the start_link argument. Suppose that in runtime.exs we have the following:

config :my_app, MyEndpoint,
  http: [
    transport_options: [socket_opts: [:inet6]],
    ...
  ]

And then we use the childspec {MyEndpoint, http: [transport_options: [socket_opts: [debug: true]]]}.

The final configuration will be: socket_opts: [:inet6]. I.e. the provided socket_opts override was ignored.

OTOH, if socket_opts in the config is a keyword list, merging will succeed, and the final socket_opts will be a result of merging both kw lists. In this case, the socket_opts list passed to start_link will take precedence (i.e. it will overwrite the values with the same keys from app env).

example script ```elixir Application.put_env(:phoenix, :json_library, Jason) Application.put_env(:sample, SamplePhoenix.Endpoint, http: [ port: 4000, transport_options: [socket_opts: [:inet6]] ], server: true, secret_key_base: String.duplicate("a", 64) ) Mix.install([ {:plug_cowboy, "~> 2.5"}, {:jason, "~> 1.0"}, {:phoenix, "~> 1.7.0"} ]) defmodule SamplePhoenix.Endpoint do use Phoenix.Endpoint, otp_app: :sample end {:ok, _} = Supervisor.start_link( [{SamplePhoenix.Endpoint, http: [transport_options: [socket_opts: [debug: true]]]}], strategy: :one_for_one ) IO.inspect(SamplePhoenix.Endpoint.config(:http)) ```

Expected behavior

Not sure :-)

Ideally, two socket_opts lists would be merged, but I understand that this is a tricky decision which might cause implicit issues in other places.

If there's no merging, then I'd at least expect the option passed to start_link to take precedence, i.e. to overwrite the one from the app env. Because that's how it behaves when both values are proper kw lists.

Yet another option is to warn/error, but I'm not sure this is a good idea in general.

mtrudel commented 8 months ago

The tricky thing here is that the transport_opts list (which gets passed more or less directly to the underlying transport; either :gen_tcp or :ssl) isn't a keyword list, it's a proplist. Moreover, :gen_tcp and :ssl don't use the 'first value wins' semantic for duplicate entries in this list, which makes merging an explicit affair. See https://github.com/mtrudel/thousand_island/pull/113 and https://github.com/mtrudel/thousand_island/pull/111 for details.

I feel pretty strongly that Thousand Island should expose the exact same interface for configuration as the underlying transport does (that is, I really don't want to be in the business of rewriting people's configurations as it's a never-ending treadmill of special cases). Unsure of the best way to resolve this, TBH. Open to options!