Implement a more comprehensive way to get connection information

[mtrudel]

Scope of changes:

• Add optional c:Plug.Conn.Adapter.get_sock_data/1 and c:Plug.Conn.Adapter.get_ssl_data/1 callbacks
• Add Plug.Conn.get_sock_data/1 and Plug.Conn.get_ssl_data/1

[mtrudel]

Hrm. Looking into the work required to implement this in Cowboy, it's not insignificant. Cowboy easily provides all of the existing info in peer_data, but basically nothing else (I suspect that this is probably why the existing Plug.Conn.Adapter contract looks the way it does). It'd be a whole set of kinda subtle PRs all the way up to Ranch to get this information sourced, for what amounts to a pretty niche use case. Not entirely sure this is a good use of effort, TBH.

WDYT?

[ns-blee]

Out of curiosity here... It is not possible to do an adapter check, return nil if Cowboy, and the value if Bandit? Or is the expectation that it will remain agnostic? If so, is there not some way to expose the methods and allow the user to do the adapter check, if needed, though implied? Without access I will be forced to use a proxy and handle TLS termination there so I can inject the values in the stream...

[josevalim]

LGTM! Just two tiny comments.

[mtrudel]

Out of curiosity here... It is not possible to do an adapter check, return nil if Cowboy, and the value if Bandit?

I think the better option here might be to make get_connection_data/1 optional on the Adapter behaviour and keep the Plug.Conn plumbing for get_peer_data/1 as-is (at least until someone gets the relevant work done upstream in Cowboy). I can write up the Plug.Conn.get_connection_data/1 function to return {:error, :not_supported}, similar to how we used to do push promises before we deprecated them.

[josevalim]

I think the better option here might be to make get_connection_data/1 optional on the Adapter behaviour and keep the Plug.Conn plumbing for get_peer_data/1 as-is (at least until someone gets the relevant work done upstream in Cowboy).

We could also make the other keys in connection_data optional. Both options are fine by me!

[mtrudel]

Trying a different tack here with separate get_peer_data/1. get_sock_data/1 and get_ssl_data/1 callbacks that I think is a whole lot simpler (and doesn't require and upstream changes in Plug.Cowboy at all).

[josevalim]

Mostly to keep ti adapter agnostic :)

Suggested change

	raise "get_sock_data not supported by #{inspect(adapter)}." <>
	"You should either delete the call to `get_sock_data/1` or switch to an " <>
	"adapter that does support this call such as Bandit."
	end
	end
	@doc """
	Returns SSL data for the connection if present. If the connection is not SSL, returns nil
	"""
	@spec get_ssl_data(t) :: Plug.Conn.Adapter.ssl_data()
	def get_ssl_data(%Conn{adapter: {adapter, payload}}) do
	if function_exported?(adapter, :get_ssl_data, 1) do
	adapter.get_ssl_data(payload)
	else
	raise "get_ssl_data not supported by #{inspect(adapter)}." <>
	"You should either delete the call to `get_ssl_data/1` or switch to an " <>
	"adapter that does support this call such as Bandit."
	raise "get_sock_data not supported by #{inspect(adapter)}"
	end
	end
	@doc """
	Returns SSL data for the connection if present. If the connection is not SSL, returns nil
	"""
	@spec get_ssl_data(t) :: Plug.Conn.Adapter.ssl_data()
	def get_ssl_data(%Conn{adapter: {adapter, payload}}) do
	if function_exported?(adapter, :get_ssl_data, 1) do
	adapter.get_ssl_data(payload)
	else
	raise "get_ssl_data not supported by #{inspect(adapter)}"

[josevalim]

Minor nits and we can ship this!!!

[mtrudel]

Ready for review - since the new callbacks are optional this can go out anytime and underlying support can come out on its own schedule (I'll be getting Bandit's support done in the next few days, bad weather permitting)

[josevalim]

💚 💙 💜 💛 ❤️

[mtrudel]

Aw shoot sorry I missed those changes - GitHub stacked all of your suggestions such that I had to do them manually.

[josevalim]

No worries!