add support for redirect limits #113

artemredkin · 2019-09-30T12:57:04Z

This PR adds support for 2 types of redirect limits - one that detects loops (by keeping history of all visited URL's for a single request), and one that just counts the number of redirects.

fixes #76

tomerd

some small suggestions and questions

Sources/AsyncHTTPClient/HTTPHandler.swift

Sources/AsyncHTTPClient/HTTPClient.swift

Sources/AsyncHTTPClient/HTTPHandler.swift

Sources/AsyncHTTPClient/HTTPClient.swift

tomerd · 2019-10-01T17:24:15Z

Sources/AsyncHTTPClient/HTTPClient.swift

+        /// Redirects are not followed.
+        case disallow
+        /// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
+        case allow(max: Int, allowCycles: Bool)


nit: "disable" and "enable" seem more natural than "disallow" and "allow"? since this is a feature flag not a security policy?

this struct was renamed to Policy, do you think RedirectPolicy.enable is better than RedirectPolicy.allow? I don't have a strong opinion here, wdyt? cc @ianpartridge

In the tests it reads as redirects: .allow(max: 5, allowCycles: false) which seems fine (although I would prefer redirection: instead of redirects:). .allow makes sense to me because redirection is something that is requested by the remote HTTP server via an HTTP return code, and we decide whether to allow it or not.

How about RedirectConfiguration.disallow and RedirectConfiguration.follow?

I like .follow 🙂

Sources/AsyncHTTPClient/HTTPClient.swift

ianpartridge · 2019-10-02T08:43:45Z

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -423,6 +427,19 @@ extension HTTPClient.Configuration {
            self.read = read
        }
    }
+
+    /// Specifies redirect processing settings.
+    public enum RedirectPolicy {


Thanks! What do people think about calling this RedirectConfiguration to match TLSConfiguration etc?

+1 from me, @tomerd wdyt?

Sources/AsyncHTTPClient/HTTPClient.swift

ianpartridge · 2019-10-02T09:03:15Z

Sources/AsyncHTTPClient/HTTPClient.swift

+        /// Redirects are not followed.
+        case disallow
+        /// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
+        case allow(max: Int, allowCycles: Bool)


In the tests it reads as redirects: .allow(max: 5, allowCycles: false) which seems fine (although I would prefer redirection: instead of redirects:). .allow makes sense to me because redirection is something that is requested by the remote HTTP server via an HTTP return code, and we decide whether to allow it or not.

tomerd · 2019-10-08T19:34:00Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+        XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
+            XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectLimitReached)
+        }
+    }


do we want to test case insensitive loops?

do you mean when schema and path are capitalised differently?

Sources/AsyncHTTPClient/HTTPClient.swift

weissi

this looks really good, two suggestions, most importantly, we shouldn't make the policy enum public.

Sources/AsyncHTTPClient/HTTPClient.swift

ianpartridge · 2019-10-10T08:57:32Z

Sources/AsyncHTTPClient/HTTPClient.swift

        /// Default client timeout, defaults to no timeouts.
        public var timeout: Timeout
        /// Upstream proxy, defaults to no proxy.
        public var proxy: Proxy?
        /// Ignore TLS unclean shutdown error, defaults to `false`.
        public var ignoreUncleanSSLShutdown: Bool

-        public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
-            self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
+        public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {


There's quite a lot of code duplication here, with each initializer having the same hard-coded .allow(max: 5, allowCycles: false) default value. We should pull that out into a separate

static let defaultRedirectPolicy: RedirectPolicy = .allow(max: 5, allowCycles: false)

then this can be:

public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = HTTPClient.defaultRedirectPolicy, timeout: Timeout = Timeout(), proxy: Proxy? = nil) { ... }

then defaultRedirectPolicy would also have to be public, what if it will be optional instead?

Co-Authored-By: Johannes Weiss <[email protected]>

weissi · 2019-10-11T15:05:24Z

@artemredkin

Test Case 'HTTPClientInternalTests.testUploadStreamingBackpressure' started at 2019-10-11 13:58:29.876
/code/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift:208: error: HTTPClientInternalTests.testUploadStreamingBackpressure : XCTAssertEqual failed: ("0") is not equal to ("1") - 
Test Case 'HTTPClientInternalTests.testUploadStreamingBackpressure' failed (0.012 seconds)

artemredkin · 2019-10-14T10:21:56Z

@weissi I rewrote the backpressure, could you please take a look at a comment to see if it makes sense?

weissi · 2019-10-16T17:20:31Z

@artemredkin cool, looks good! On top of the test with actual channels, did the EmbeddedChannel not work?

weissi · 2019-10-23T01:04:53Z

ping @artemredkin on the above question

artemredkin · 2019-10-23T05:44:00Z

@weissi not yet, do you think its critical? we have "integration-style" test for backpressure for now

weissi · 2019-10-23T05:47:24Z

@artemredkin I think it would be good to have both especially given that the integration test might turn out to not be a 100% reliable (I think it is but hard to tell for sure). I also think the integration test will be much easier to follow for people who aren't you & me because we worked on that integration test together.

artemredkin · 2019-10-23T13:00:02Z

@weissi I filed #117 to implement that test for 1.1.0. Test is passing now and test itself is not part of this PR (redirects). wdyt, should we continue as is in order to release 1.0.0?

weissi · 2019-10-23T14:41:00Z

@artemredkin perfect, no, let’s not block 1.0.0

artemredkin · 2019-10-23T16:06:32Z

@weissi @Lukasa @tomerd @ianpartridge any additional comments?

Lukasa · 2019-10-23T16:08:00Z

None from me.

ianpartridge · 2019-10-23T16:27:03Z

LGTM.

weissi

thanks! LGTM

add support for redirect limits

939aa2b

artemredkin requested review from tomerd, weissi and Lukasa September 30, 2019 12:57

artemredkin added 2 commits September 30, 2019 14:38

Merge branch 'master' into redirect_limits

fca5971

add tests on linux

b437111

tomerd reviewed Sep 30, 2019

View reviewed changes

review fix: guard instead of if

99096c5

ianpartridge reviewed Sep 30, 2019

View reviewed changes

Sources/AsyncHTTPClient/HTTPClient.swift Outdated Show resolved Hide resolved

review fixes

b3ffde6

tomerd reviewed Oct 1, 2019

View reviewed changes

ianpartridge reviewed Oct 2, 2019

View reviewed changes

artemredkin added 2 commits October 8, 2019 13:18

review fix: make redirect state be part for request

bbad11f

review fix: add a separate error for redirect cycles

8b0e613

tomerd reviewed Oct 8, 2019

View reviewed changes

weissi reviewed Oct 10, 2019

View reviewed changes

Sources/AsyncHTTPClient/HTTPClient.swift Outdated Show resolved Hide resolved

weissi requested changes Oct 10, 2019

View reviewed changes

Sources/AsyncHTTPClient/HTTPClient.swift Outdated Show resolved Hide resolved

ianpartridge reviewed Oct 10, 2019

View reviewed changes

artemredkin and others added 2 commits October 11, 2019 13:19

Update Sources/AsyncHTTPClient/HTTPClient.swift

aa843de

Co-Authored-By: Johannes Weiss <[email protected]>

review fixes

7e2edf8

artemredkin added 2 commits October 14, 2019 11:08

rewrite backpressure test

5209dc3

fix formatting

cf2d4d6

Merge branch 'master' into redirect_limits

86cf7d8

weissi approved these changes Oct 23, 2019

View reviewed changes

artemredkin merged commit 51dc885 into master Oct 23, 2019

artemredkin deleted the redirect_limits branch October 23, 2019 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for redirect limits #113

add support for redirect limits #113

artemredkin commented Sep 30, 2019

tomerd left a comment

tomerd Oct 1, 2019 •

edited

Loading

artemredkin Oct 1, 2019

ianpartridge Oct 2, 2019

artemredkin Oct 11, 2019

ianpartridge Oct 11, 2019

ianpartridge Oct 2, 2019

artemredkin Oct 8, 2019

artemredkin Oct 11, 2019

ianpartridge Oct 2, 2019

tomerd Oct 8, 2019

artemredkin Oct 11, 2019

weissi left a comment

ianpartridge Oct 10, 2019

artemredkin Oct 11, 2019

weissi commented Oct 11, 2019

artemredkin commented Oct 14, 2019

weissi commented Oct 16, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

Lukasa commented Oct 23, 2019

ianpartridge commented Oct 23, 2019

weissi left a comment

add support for redirect limits #113

add support for redirect limits #113

Conversation

artemredkin commented Sep 30, 2019

tomerd left a comment

Choose a reason for hiding this comment

tomerd Oct 1, 2019 • edited Loading

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

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi commented Oct 11, 2019

artemredkin commented Oct 14, 2019

weissi commented Oct 16, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

weissi commented Oct 23, 2019

artemredkin commented Oct 23, 2019

Lukasa commented Oct 23, 2019

ianpartridge commented Oct 23, 2019

weissi left a comment

Choose a reason for hiding this comment

tomerd Oct 1, 2019 •

edited

Loading