Conditional Response Compression #225

dimitribouniol · 2024-07-09T10:56:17Z

Added support for conditional response compression in HTTPResponseCompression.

Motivation:

This change allows libraries like vapor or direct users of the handler to add additional functionality around when a response should be compressed, rather than always or never for the channel's lifetime. This is particularly important for pipelined requests, as some responses (ie. dynamic data) should be dynamically compressed, while others (ie. images) should not.

Two specific use cases immediately came to mind, particularly checking the content type of the response and using some heuristic, or using marker headers (ie. X-Vapor-Compression) to statelessly mark entire routes as compressible or incompressible by default.

Modifications:

Rather than hardcode specific use cases, I went with an approach other handlers have used which is to provide a predicate to HTTPResponseCompression that allows the caller to weigh in on if compression should be performed. This allows a library like vapor to always install the handler, but then conditionally check the headers of the response about to go out to determine if it should be compressed or not based on heuristics and knobs it can choose to expose. Note that this predicate is always called even in cases where the request does not support compression, so the user of the handler does not need to concern themselves ahead of time whether or not to write marker headers based on the requests, and can keep the logic and cleanup contained in one place.

Result:

HTTPResponseCompression now supports conditional response compression allowing for greater flexibility in how the handler is used, which can hopefully increase its general usefulness.

dimitribouniol · 2024-07-09T10:57:51Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

+    @_disfavoredOverload
+    @available(*, deprecated, message: "Deprecated in favor of HTTPResponseCompressor(initialByteBufferCapacity:, responseCompressionPredicate:)")
+    public convenience init(initialByteBufferCapacity: Int = 1024) {
+        // TODO: This version is kept around for backwards compatibility and should be merged with the signature below in the next major version.


Not sure what the best practice is here for deprecating overloads that can break backwards compatibility. I remember this was a thing early on in Swift 5, but not sure if its still relevant?

Let's not deprecate this: we can just have two methods and remove the default argument for the new one.

Should I keep the todo? Is there another mechanism to not forget these on the next major version bump?

Please go ahead and file an issue and we can mark it for the next major version, but I'm happy to keep the TODO as well.

Linked in the code as well: #226

dimitribouniol · 2024-07-09T11:01:00Z

@swift-server-bot test this please

Lukasa

Generally looks really nice. Just a few nits here.

Lukasa · 2024-07-17T17:02:09Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

+    @_disfavoredOverload
+    @available(*, deprecated, message: "Deprecated in favor of HTTPResponseCompressor(initialByteBufferCapacity:, responseCompressionPredicate:)")
+    public convenience init(initialByteBufferCapacity: Int = 1024) {
+        // TODO: This version is kept around for backwards compatibility and should be merged with the signature below in the next major version.


Let's not deprecate this: we can just have two methods and remove the default argument for the new one.

Lukasa · 2024-07-17T17:02:43Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

    /// - Parameter initialByteBufferCapacity: Initial size of buffer to allocate when hander is first added.
-    public init(initialByteBufferCapacity: Int = 1024) {
+    /// - Parameter responseCompressionPredicate: The predicate used to determine if the response should be compressed or not based on its headers. Defaults to `nil`, which will compress every response this handler sees. This predicate is always called wether the client supports compression for this response or not, so it can be used to clean up any marker headers you may use to determine if compression should be performed or not. Please see ``ResponseCompressionPredicate`` for more details.


Suggested change

/// - Parameter responseCompressionPredicate: The predicate used to determine if the response should be compressed or not based on its headers. Defaults to `nil`, which will compress every response this handler sees. This predicate is always called wether the client supports compression for this response or not, so it can be used to clean up any marker headers you may use to determine if compression should be performed or not. Please see ``ResponseCompressionPredicate`` for more details.

/// - Parameter responseCompressionPredicate: The predicate used to determine if the response should be compressed or not based on its headers. Defaults to `nil`, which will compress every response this handler sees. This predicate is always called whether the client supports compression for this response or not, so it can be used to clean up any marker headers you may use to determine if compression should be performed or not. Please see ``ResponseCompressionPredicate`` for more details.

Lukasa · 2024-07-17T17:13:08Z

Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest.swift

+    func testConditionalCompressionEnabled() throws {
+        let predicateWasCalled = expectation(description: "Predicate was called")
+        let compressor = HTTPResponseCompressor { responseHeaders, isCompressionSupported in
+            predicateWasCalled.fulfill()


We should probably fulfil the predicate last.

Put them inside a defer instead so this is guaranteed, unless that would lead to a bad outcome? Its only really trying to make sure the predicate isn't silently skipped.

Defer is just fine.

Lukasa · 2024-07-17T17:18:42Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

+    /// - Parameter isCompressionSupported: Set to `true` if the client requested compatible compression, and if the HTTP response supports it, otherwise `false`.
+    /// - Returns: Return `true` if the compressor should proceed to compress the response, or `false` if the response should not be compressed.
+    ///
+    /// - Note: Returning `true` when compression is not supported will not enable compression, and the modified headers will always be used.


This behaviour is surprising to me. I think we should introduce a new enum for the return value, something like compressIfPossible and doNotCompress as the cases. Just to clarify what will happen.

I was thinking about this too, glad we are on the same page. Are we ok with enum enums, or should I make a struct with private enums for future compatibility?

Went with a struct for futureproofness after all, but let me know if the cases are clearly named (went with an "intent"-like terminology, and clarified the wording in the doc comment)

Lukasa · 2024-07-18T12:56:20Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

+        /// The response should be compressed if supported by the HTTP protocol.
+        public static let compressibleResponse = CompressionIntent(.compressibleResponse)
+        /// The response should not be compressed even if supported by the HTTP protocol.
+        public static let uncompressedResponse = CompressionIntent(.uncompressedResponse)


I think let's make it clear these are action requests:

.compressIfPossible .doNotCompress

Lukasa · 2024-07-18T13:20:04Z

@swift-server-bot add to allowlist

Lukasa · 2024-07-18T13:23:34Z

Sources/NIOHTTPCompression/HTTPResponseCompressor.swift

-    /// Initialise a ``HTTPResponseCompressor``
+    /// Initialize a ``HTTPResponseCompressor``.
+    /// - Parameter initialByteBufferCapacity: Initial size of buffer to allocate when hander is first added.
+    public convenience init(initialByteBufferCapacity: Int) {


Hmm, when I suggested taking off the default argument value I actually meant to take it from the other initializer's responseCompressionPredicate. But I now wonder: why can't we have default values here? There should be no ambiguity, the compiler should be fine.

Initially I ran into trouble, but I forgot what the issue was — let me give it another try

You're right, we can have default arguments for both of them after all, let me add it back

…r than a boolean

Lukasa

I love this, this is fantastic work. Thanks @dimitribouniol!

dimitribouniol commented Jul 9, 2024

View reviewed changes

dimitribouniol force-pushed the dimitri/conditional-compression branch from 088a0e5 to f398c71 Compare July 9, 2024 11:02

dimitribouniol mentioned this pull request Jul 10, 2024

Conditional Content Response Compression vapor/vapor#3215

Merged

dimitribouniol force-pushed the dimitri/conditional-compression branch from f398c71 to 1972e77 Compare July 12, 2024 08:11

Lukasa added the 🆕 semver/minor Adds new public API. label Jul 17, 2024

Lukasa reviewed Jul 17, 2024

View reviewed changes

dimitribouniol force-pushed the dimitri/conditional-compression branch from 1972e77 to 006187b Compare July 17, 2024 20:31

Lukasa reviewed Jul 18, 2024

View reviewed changes

dimitribouniol force-pushed the dimitri/conditional-compression branch from 006187b to 2f2678e Compare July 18, 2024 13:12

Lukasa reviewed Jul 18, 2024

View reviewed changes

dimitribouniol added 3 commits July 18, 2024 06:33

Added support for conditional response compression

b841d47

Updated expectations to be fulfilled via a defer

999fd2c

Updated compression response predicate to return an intent enum rathe…

eab1303

…r than a boolean

dimitribouniol force-pushed the dimitri/conditional-compression branch from 2f2678e to eab1303 Compare July 18, 2024 13:33

Lukasa approved these changes Jul 18, 2024

View reviewed changes

Lukasa merged commit d1ead62 into apple:main Jul 18, 2024
6 of 7 checks passed

dimitribouniol deleted the dimitri/conditional-compression branch July 18, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional Response Compression #225

Conditional Response Compression #225

dimitribouniol commented Jul 9, 2024

dimitribouniol Jul 9, 2024

Lukasa Jul 17, 2024

dimitribouniol Jul 17, 2024

Lukasa Jul 18, 2024

dimitribouniol Jul 18, 2024

dimitribouniol commented Jul 9, 2024

Lukasa left a comment

Lukasa Jul 17, 2024

Lukasa Jul 17, 2024

dimitribouniol Jul 17, 2024

Lukasa Jul 17, 2024

dimitribouniol Jul 17, 2024

Lukasa Jul 18, 2024

Lukasa Jul 17, 2024

dimitribouniol Jul 17, 2024

dimitribouniol Jul 17, 2024

Lukasa Jul 18, 2024

dimitribouniol Jul 18, 2024

Lukasa commented Jul 18, 2024

Lukasa Jul 18, 2024

dimitribouniol Jul 18, 2024

dimitribouniol Jul 18, 2024

Lukasa left a comment

Conditional Response Compression #225

Conditional Response Compression #225

Conversation

dimitribouniol commented Jul 9, 2024

Motivation:

Modifications:

Result:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitribouniol commented Jul 9, 2024

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment