Skip to content

Collect function fix #672

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Apr 4, 2023
Merged

Collect function fix #672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
668b33a
Collect function fix
carolinacass Mar 16, 2023
6276e51
minor change
carolinacass Mar 16, 2023
104eff5
Merge branch 'main' into cc-collectResponse
carolinacass Mar 16, 2023
2ad4a6b
putting back in tests accidentally deleted
carolinacass Mar 16, 2023
e919181
Fixing comment
carolinacass Mar 20, 2023
df627a5
incorrect ordering
carolinacass Mar 20, 2023
8156844
Main to this file
carolinacass Mar 20, 2023
9b63815
Checking the Content Length through requestMethod and status
carolinacass Mar 21, 2023
e8d8f85
minor move
carolinacass Mar 21, 2023
0de9e27
Changing placement and fixing tests to expectecdContentLength
carolinacass Mar 22, 2023
045a063
suggested fixes
carolinacass Mar 23, 2023
9dc6f33
fixing comments and deleting excess functions
carolinacass Mar 23, 2023
ecbcd90
getting rid of request Method
carolinacass Mar 23, 2023
ae17940
adding tests for expectedContentLength and public init
carolinacass Mar 23, 2023
483a3f2
Fixing test and spelling
carolinacass Mar 27, 2023
14cfdd4
running generate_linux_tests.rb
carolinacass Mar 30, 2023
e60d65b
fixing formatting
carolinacass Apr 3, 2023
2fbef23
Merge branch 'main' into cc-collectResponse
Lukasa Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ extension HTTPClientResponse {
@inlinable public func makeAsyncIterator() -> AsyncIterator {
.init(storage: self.storage.makeAsyncIterator())
}

/// Accumulates an ``Swift/AsyncSequence`` of ``ByteBuffer``s into a single ``ByteBuffer``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can be clearer here: this accumulates Body.

/// - Parameters:
/// - maxBytes: The maximum number of bytes this method is allowed to accumulate
/// - Throws: `NIOTooManyBytesError` if the the sequence contains more than `maxBytes`.
/// - Returns: the number of bytes collected over time
func collect(maxBytes: Int) async throws -> ByteBuffer {
return try await self.collect(upTo: maxBytes)
Copy link
Collaborator

@dnadoba dnadoba Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If the response head already announces the body size we can check that size before downloading anything. This is e.g. the case if the response contains a Content-Length header which contains the expected body size. If it is bigger than maxBytes we should throw before calling self.collection(upTo:). To actually do that we need to move this extension from HTTPClientResponse.Body to HTTPClientResponse or have some kind of reference in HTTPClientResponse.Body to the headers.

Be careful though, there are edge cases where we don't expect a body even if Content-Length is set. This is e.g. the case if the request was just a HEAD request. I think there are more edge cases but that's the only one I know of the top of my head.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think this is appropriate for helper functions to check the Content-Length, but will confirm that it deals with it correctly.

}
}
}

Expand Down
42 changes: 27 additions & 15 deletions Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], ["4"])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand All @@ -137,7 +137,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand All @@ -160,7 +160,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand All @@ -183,7 +183,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], ["4"])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand All @@ -210,7 +210,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand All @@ -233,7 +233,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response.body.collect()
try await response.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
Expand Down Expand Up @@ -522,7 +522,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else {
return
}
guard let body = await XCTAssertNoThrowWithResult(try await response.body.collect()) else { return }
guard let body = await XCTAssertNoThrowWithResult(try await response.body.collect(maxBytes: 1024)) else { return }
var maybeRequestInfo: RequestInfo?
XCTAssertNoThrow(maybeRequestInfo = try JSONDecoder().decode(RequestInfo.self, from: body))
guard let requestInfo = maybeRequestInfo else { return }
Expand Down Expand Up @@ -583,7 +583,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response1.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response1.body.collect()
try await response1.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))

Expand All @@ -592,18 +592,30 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
) else { return }
XCTAssertEqual(response2.headers["content-length"], [])
guard let body = await XCTAssertNoThrowWithResult(
try await response2.body.collect()
try await response2.body.collect(maxBytes: 1024)
) else { return }
XCTAssertEqual(body, ByteBuffer(string: "1234"))
}
}
}

extension AsyncSequence where Element == ByteBuffer {
func collect() async rethrows -> ByteBuffer {
try await self.reduce(into: ByteBuffer()) { accumulatingBuffer, nextBuffer in
var nextBuffer = nextBuffer
accumulatingBuffer.writeBuffer(&nextBuffer)
func testBytesOverflowError() {
guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
XCTAsyncTest {
let bin = HTTPBin(.http2(compress: false)) { _ in HTTPEchoHandler() }
defer { XCTAssertNoThrow(try bin.shutdown()) }
let client = makeDefaultHTTPClient()
defer { XCTAssertNoThrow(try client.syncShutdown()) }
let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/")
request.method = .POST
request.body = .bytes(ByteBuffer(string: "123456"))

guard let response = await XCTAssertNoThrowWithResult(
try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)
) else { return }
XCTAssertEqual(response.headers["content-length"], ["6"])
await XCTAssertThrowsError(
try await response.body.collect(maxBytes: 1))
}
}
}
Expand Down