Configure client connections with a builder #733
Conversation
glbrntt
added
kind/enhancement
Motivation: Configuring a client without TLS is easy to do, since no tls is the default in the client connection configuration. This is bad. The API should make it clearer that the default is insecure. The current API for configuring clients falls a bit short in a number of areas. In no particular order: 1. It can't be easily extended without SemVer major changes (since we'd have to provide a new init whenever new properties are added), 2. Nested configuration (i.e. TLS) is unnecessarily verbose, 3. The initializer requires properties to be passed in the correct order, and 4. TLS is disabled by default (!) and the API doesn't make this clear that this is insecure Modifications: - create a `ClientConnectionBuilder` whose public API is limited to configuring the private properties; this is similar to NIOs bootstraps - provide static methods on `ClientConnection` to provide a `secure` or `insecure` builder - update tests in a couple of places to demonstrate usage Result: - Configuring a connection is less verbose - Not using TLS is more obviously 'insecure'
glbrntt
force-pushed
the
gb-channel-builders
branch
from
f5c3b3d to
0c29466
Compare
| @@ -101,6 +101,9 @@ public class ClientConnection { | |||
| } | |||
|
|
|||
| /// Creates a new connection from the given configuration. | |||
| /// | |||
| /// - Important: Users should prefer using `ClientConnection.secure(group:)` to build a connection | |||
| /// with TLS, or `ClientConnection.insecure(group:)` to build a connection without TLS. | |||
| public init(configuration: Configuration) { | |||
There was a problem hiding this comment.
This is still alpha right? You could make an API-breaking change to hide these initialisers and force people to use the new secure and insecure?
There was a problem hiding this comment.
That's right but it's impossible to provide fix-its for this and it's quite an annoying change to have to make. I'm leaning towards keeping it for now. The longer term plan would be to not add anything to it to gently force people to the builder.
There was a problem hiding this comment.
Please do not hide this initialiser, as this would disallow precise configuration by hands, which is often needed.
There was a problem hiding this comment.
@be4rd what's your use case here? Everything should be representable by the builder as well.
There was a problem hiding this comment.
@glbrntt if you'd like to keep it another option to dissuade use would be to deprecate or provide a warning for direct use
There was a problem hiding this comment.
@glbrntt sorry, did not saw TLS configuration methods in builder when I left a comment 😅
| /// multiplier to 1.0. | ||
| @discardableResult | ||
| public func withConnectionBackoff(fixed amount: TimeAmount) -> Self { | ||
| let seconds = Double(amount.nanoseconds) / Double(1_000_000_000) |
There was a problem hiding this comment.
Constant 1_000_000_000 is used a couple of times. Perhaps make attached it to Double to reduce potential error? (E.g. Double.Nanosecond, or just make it a constant somewhere)
| @@ -101,6 +101,9 @@ public class ClientConnection { | |||
| } | |||
|
|
|||
| /// Creates a new connection from the given configuration. | |||
| /// | |||
| /// - Important: Users should prefer using `ClientConnection.secure(group:)` to build a connection | |||
| /// with TLS, or `ClientConnection.insecure(group:)` to build a connection without TLS. | |||
| public init(configuration: Configuration) { | |||
There was a problem hiding this comment.
@glbrntt sorry, did not saw TLS configuration methods in builder when I left a comment 😅
Motivation:
Configuring a client without TLS is easy to do, since no tls is the
default in the client connection configuration. This is bad. The API
should make it clearer that the default is insecure.
The current API for configuring clients falls a bit short in a number of
areas. In no particular order:
have to provide a new init whenever new properties are added),
order, and
this is insecure
Modifications:
ClientConnectionBuilderwhose public API is limited toconfiguring the private properties; this is similar to NIOs bootstraps
ClientConnectionto provide asecureorinsecurebuilderResult: