Repository navigation
Add configuration type for HTTP/1 server pipeline setup - #3746
Conversation
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
This is the right direction. Thank you!
| /// - ``outboundHeaderValidation``: `true` | ||
| /// - ``encoderConfiguration``: ``HTTPResponseEncoder/Configuration/init()`` | ||
| /// - ``decoderLimitConfiguration``: ``NIOHTTPDecoderLimitConfiguration/init()`` | ||
| public static var defaults: Self { |
There was a problem hiding this comment.
I think we should move the default value config comments into the properties itself.
What is the benefit of the defaults static property. Why not just init()?
There was a problem hiding this comment.
I've replaced the defaults property with a public init() now. See 1b8e7b2.
| fileprivate init( | ||
| pipeliningAssistance: Bool = true, | ||
| serverUpgrade: UpgradeConfiguration? = nil, | ||
| errorHandling: Bool = true, | ||
| outboundHeaderValidation: Bool = true, | ||
| encoderConfiguration: HTTPResponseEncoder.Configuration = .init(), | ||
| decoderLimitConfiguration: NIOHTTPDecoderLimitConfiguration = .init() | ||
| ) { | ||
| self.pipeliningAssistance = pipeliningAssistance | ||
| self.serverUpgrade = serverUpgrade | ||
| self.errorHandling = errorHandling | ||
| self.outboundHeaderValidation = outboundHeaderValidation | ||
| self.encoderConfiguration = encoderConfiguration | ||
| self.decoderLimitConfiguration = decoderLimitConfiguration | ||
| } |
There was a problem hiding this comment.
What do we need this for? Why don't we use the normal init() and then set the properties accordingly?
There was a problem hiding this comment.
This was to keep the diff minimal in all the existing helper methods. But I've removed this now. See #3746 (comment).
| position: position, | ||
| withPipeliningAssistance: configuration.pipeliningAssistance, | ||
| withServerUpgrade: configuration.serverUpgrade.map { ($0.upgraders, $0.completionHandler) }, | ||
| withErrorHandling: configuration.errorHandling, | ||
| withOutboundHeaderValidation: configuration.outboundHeaderValidation, | ||
| withEncoderConfiguration: configuration.encoderConfiguration, | ||
| withDecoderLimitConfiguration: configuration.decoderLimitConfiguration |
There was a problem hiding this comment.
I think we should pass the new Config struct down here.
| public var upgraders: [HTTPServerProtocolUpgrader] | ||
|
|
||
| /// Called once the upgrade is complete, with the context of the handler performing the upgrade. | ||
| public var completionHandler: @Sendable (ChannelHandlerContext) -> Void |
There was a problem hiding this comment.
this doesn't need to be Sendable
| /// the upgrade. | ||
| public init( | ||
| upgraders: [HTTPServerProtocolUpgrader], | ||
| completionHandler: @escaping @Sendable (ChannelHandlerContext) -> Void |
There was a problem hiding this comment.
this doesn't need to be Sendable
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
AMAZING! Thanks for pushing through
Motivation:
NIOHTTP1.HTTPPipelineSetupcontains helper methods which add HTTP/1 client/server handlers to the provided channel pipeline.Over time, we have had to duplicate these methods when introducing a new configuration option in order to not break API.
As a result, there is now lots of duplication in
NIOHTTP1.HTTPPipelineSetupand introducing more configuration options is very cumbersome. We should instead add a configuration type to solve this problem.Modifications:
NIOHTTPServerPipelineConfiguration, andNIOHTTPServerPipelineSynchronousConfiguration.ChannelPipelineandChannelPipeline.SynchronousOperationsthat requires the configuration type as an argument.ChannelPipelineandChannelPipeline.SynchronousOperations.Result:
Better API.