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

Support EngineClient redirection #1480

Merged
merged 20 commits into from Feb 23, 2018

Conversation

Projects
None yet
4 participants
@Joannis
Copy link
Member

Joannis commented Feb 2, 2018

No description provided.

@Joannis Joannis added the enhancement label Feb 2, 2018

@Joannis Joannis self-assigned this Feb 2, 2018

@Joannis Joannis requested review from gwynne and tanner0101 Feb 2, 2018

for req: Request,
redirecting: Int
) throws -> Future<Response> {
if httpRes.status.code >= 301 && httpRes.status.code <= 303 {

This comment has been minimized.

@gwynne

gwynne Feb 2, 2018

Contributor

This does not cover the range of potential redirect responses. Most of the 3xx range is relevant, particularly 307 and 308.

}

req.http.uri = try location.makeURI()

This comment has been minimized.

@gwynne

gwynne Feb 2, 2018

Contributor

It is incorrect but still-common practice for servers to return "relative" redirects, e.g. Location: /path/on/same/server. This case should be detected and the URI defaulted to the previous URI's schema and hostname.


/// Create a new EngineClientConfig.
public init(maxResponseSize: Int) {
self.maxResponseSize = maxResponseSize
self.maxRedirections = 3

This comment has been minimized.

@gwynne

gwynne Feb 2, 2018

Contributor

I would suggest a slightly higher default, probably 8


let response = try client.get("http://www.google.com/").blockingAwait()
XCTAssertEqual(response.http.status, 200)
}

This comment has been minimized.

@gwynne

gwynne Feb 2, 2018

Contributor

Add tests for the "relative" redirect case and for the various 3xx codes.

Joannis added some commits Feb 2, 2018

case 301, 307, 308:
return try redirect(httpRes, for: req, redirecting: redirecting)
case 302, 303:
req.http.method = .get

This comment has been minimized.

@gwynne

gwynne Feb 2, 2018

Contributor

When transforming to GET, make sure to remove any request body and Content-Type header.

@Joannis

This comment has been minimized.

Copy link
Member Author

Joannis commented Feb 2, 2018

[TCP.TCPError.getAddressInfo: Success] is a really great error

Joannis and others added some commits Feb 9, 2018

@tanner0101 tanner0101 added this to Todo in Vapor 3 Feb 12, 2018

@tanner0101 tanner0101 added this to the 3.0.0-beta.2 milestone Feb 13, 2018

}
}

/// Responds to a Request using TCP client.
private func plaintextRespond(to req: Request) throws -> Future<Response> {
private func plaintextRespond(to req: Request, redirecting: Int) throws -> Future<Response> {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

I know this is internal API, but I think it should be maxRedirections:

This comment has been minimized.

@gwynne

gwynne Feb 13, 2018

Contributor

Agreed, redirecting reads more like a boolean value than a limit count.


guard let location = response.headers[.location] else {
throw VaporError(
identifier: "invalid-redirect",

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

camelCase 😭

} else {
return try self.plaintextRespond(to: req)
var path = newURI.path

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

can this chunk of code get moved to an extension/method that explains what it does?

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

or a comment..

This comment has been minimized.

@gwynne

gwynne Feb 13, 2018

Contributor

I would go with a well-named extension method myself, yeah.

looks like everything has been solved

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Feb 13, 2018

Also needs tests passing

@tanner0101 tanner0101 modified the milestones: 3.0.0-beta.2, 3.0.0-beta.3 Feb 13, 2018


let client = try app.make(Client.self)

let response = try client.get("http://httpbin.org/relative-redirect/8").blockingAwait()

This comment has been minimized.

@skyweb07

skyweb07 Feb 17, 2018

These tests are always hitting the network so they will depend on network status and may be flaky cause we're depending on the network connection, we should avoid those ones no?

This comment has been minimized.

@Joannis

Joannis Feb 17, 2018

Author Member

I agree @skyweb07, but I don't want to completely rely on an isolated implementation either. Support Vapor 3 has completely broken support for redirection on both server and client side. The tests won't show this because the tests wouldn't be hitting a good implementation.

@Joannis

This comment has been minimized.

Copy link
Member Author

Joannis commented Feb 22, 2018

Ping @tanner0101 it was working by the way, but I guess something changed causing the Application eventloop to be idle by default. I changed the tests accordingly.

@tanner0101 tanner0101 modified the milestones: 3.0.0-beta.3, 3.0.0-rc.1 Feb 23, 2018

@tanner0101 tanner0101 merged commit 8a2b243 into beta Feb 23, 2018

2 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
dependency-ci Dependencies checked
Details

@tanner0101 tanner0101 deleted the client-redirection branch Feb 23, 2018

@VaporNationTravis VaporNationTravis referenced this pull request Feb 23, 2018

Merged

Week 4 (WIP) #20

@Joannis Joannis moved this from Todo to Done in Vapor 3 Feb 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment