Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make HTTP decompression limit configurable #2203

Merged
merged 2 commits into from
Feb 27, 2020

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Feb 27, 2020

…'s enabled. Kept the default of 10x ratio. Added unit test for compression support and the limit handling.
@gwynne gwynne added the semver-minor Contains new API label Feb 27, 2020
@gwynne gwynne self-assigned this Feb 27, 2020
@gwynne gwynne added this to In Progress in Vapor 4 via automation Feb 27, 2020
@gwynne gwynne added the enhancement New feature or request label Feb 27, 2020
Copy link
Member

@calebkleveter calebkleveter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there should be a way to combine both the supportDecompression and decompressionLimit parameters, but I can't think of a good way to do it at this point.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tanner0101
Copy link
Member

@calebkleveter agreed. I can't think of a good way either though. If we find something we can deprecate these methods.

@tanner0101 tanner0101 merged commit 5651d7e into master Feb 27, 2020
Vapor 4 automation moved this from In Progress to Done Feb 27, 2020
@tanner0101 tanner0101 deleted the configurable-http-compression-limit branch February 27, 2020 16:04
@tanner0101
Copy link
Member

These changes are now available in 4.0.0-beta.4.1

@t089
Copy link

t089 commented Feb 27, 2020

Instead of a Boolean you could use an enum

enum Decompression {
   .disabled
   .enabled(limit: NIOHTTPDecompression.DecompressionLimit)
}

@tanner0101
Copy link
Member

@t089 that's a good idea. An enum-like struct could be even better so that we can add new "cases" in the future.

pull bot pushed a commit to scope-demo/vapor that referenced this pull request Feb 27, 2020
* Add the ability to configure the limits of HTTP decompression when it's enabled. Kept the default of 10x ratio. Added unit test for compression support and the limit handling.

* Tweak a unit test so it doesn't hang forever on network failures.
nachoBonafonte pushed a commit to scope-demo/vapor that referenced this pull request Feb 27, 2020
* Add the ability to configure the limits of HTTP decompression when it's enabled. Kept the default of 10x ratio. Added unit test for compression support and the limit handling.

* Tweak a unit test so it doesn't hang forever on network failures.
@gwynne
Copy link
Member Author

gwynne commented Feb 27, 2020

@t089 Yeah, I was thinking of something like that, but I couldn't quite put it together in my head without breaking the existing API (which I try to avoid). The enum you suggested looks great, I think it makes sense to just use that instead of trying to stick to API consistency while still in beta (just look at how much Fluent changed in the last few days 😆).

@calebkleveter
Copy link
Member

The best part is that now enum cases with associated values can have default values.

@tanner0101
Copy link
Member

public struct HTTPDecompressionConfiguration {
   public static var disabled: Self {
        .init(isEnabled: false, limit: ...)
   }
   
   public static func enabled(limit: NIOHTTPDecompression.DecompressionLimit = .ratio(10)) -> Self {
       .init(isEnabled: true, limit: limit)
   }

   var isEnabled: Bool
   var limit: NIOHTTPDecompression.DecompressionLimit
}

That's how you could do it with a struct so we can add new cases going forward.

@t089
Copy link

t089 commented Feb 29, 2020

Even more elegant would be to use a private enum inside the struct to model the different states. Eg

public struct HTTPDecompressionConfiguration {
   public static var disabled: Self {
        .init(decompression: .disabled)
   }
   
   public static func enabled(limit: NIOHTTPDecompression.DecompressionLimit = .ratio(10)) -> Self {
       .init(decompression: .enabled(limit: limit)
   }

    enum Decompression {
       .disabled
       .enabled(limit: NIOHTTPDecompression
   }

    var decompression: Decompression
}

@tanner0101
Copy link
Member

I've implemented an improved API here: #2214.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new API
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants