You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As suggested by @DerGuteMoritz in clj-commons#728. This fixes the issue and makes
the test added in the previous commit pass.
Keeping the `client-ssl-context` call in `http-connection` as is,
even though it might seem superfluous considering the code path taken in
the test, but `http-connection` is a public API, so we have to keep
the call (which for us is a no-op, if we ignore the repeated ALPN check)
even for our case when the protocol is https and `ssl-context` is supplied.
NOTE: This highlights a difference we are introducing here. Previously,
if we specified ssl-context, but the protocol wasn't https, we would just
ignore the ssl-context. Currently, we are coercing it ahead-of-time,
before knowing the request protocol. This could be alleviated by wrapping
the coercion in a `delay`, so it won't happen until needed. However, given
how unlikely this scenario seems, I have doubts whether it'd be worth it.
I slightly dislike the repetition of `[:http1]` default value,
but since it server as a documentation in `http-connection`,
I decided to keep it as is rather than to extract it out.
Also, I slightly dislike the repetition of a pattern to call
`ensure-consistent-alpn-config` and then `coerce-ssl-client-context`
but it's only now in 2 places, which I think is a better alternative
than adding yet another ssl-coercion layer/wrapping function.
Obviously, we cannot just move `ensure-consistent-alpn-config` to
`ssl-client-context`, since ALPN is only for HTTP.
0 commit comments