ronantakizawa/codereview-bench huggingface dataset:
Repository: vapor/vapor
File: Tests/VaporTests/ApplicationTests.swift (Swift)

Code before:
        let b = eventLoop.makePromise(of: Int.self)

        let c = [a.futureResult, b.futureResult].flatten(on: eventLoop)

        a.succeed(1)
        b.succeed(2)

        try XCTAssertEqual(c.wait(), [1, 2])
    }

    func testBoilerplate() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.get("hello") { req in
            "Hello, world!"
        }

        app.environment.arguments = ["serve"]
        try app.start()

        let res = try app.client.get("http://localhost:8080/hello").wait()
        XCTAssertEqual(res.body?.string, "Hello, world!")
    }

    func testAutomaticPortPickingWorks() throws {
        let app = Application(.testing)
        app.http.server.configuration.hostname = "127.0.0.1"
        app.http.server.configuration.port = 0
        defer { app.shutdown() }

        app.get("hello") { req in
            "Hello, world!"
        }

        XCTAssertNil(app.http.server.shared.localAddress)

        XCTAssertNoThrow(try app.start())

        XCTAssertNotNil(app.http.server.shared.localAddress)
        XCTAssertEqual("127.0.0.1", app.http.server.shared.localAddress?.ipAddress ?? "n/a")

        let port = app.http.server.shared.localAddress?.port
        XCTAssertGreaterThan(port ?? 0, 0)

        let res = try app.client.get("http://localhost:\(port ?? 0)/hello").wait()
        XCTAssertEqual(res.body?.string, "Hello, world!")
    }
}


Review comment (suggestion):
We're moving towards non-throwing tests as XCTest doesn't handle throwing tests very and show the line that threw

Code after:
        let b = eventLoop.makePromise(of: Int.self)

        let c = [a.futureResult, b.futureResult].flatten(on: eventLoop)

        a.succeed(1)
        b.succeed(2)

        try XCTAssertEqual(c.wait(), [1, 2])
    }

    func testBoilerplate() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.get("hello") { req in
            "Hello, world!"
        }

        app.environment.arguments = ["serve"]
        try app.start()

        let res = try app.client.get("http://localhost:8080/hello").wait()
        XCTAssertEqual(res.body?.string, "Hello, world!")
    }

    func testAutomaticPortPickingWorks() {
        let app = Application(.testing)
        app.http.server.configuration.hostname = "127.0.0.1"
        app.http.server.configuration.port = 0
        defer { app.shutdown() }

        app.get("hello") { req in
            "Hello, world!"
        }

        XCTAssertNil(app.http.server.shared.localAddress)

        XCTAssertNoThrow(try app.start())

        XCTAssertNotNil(app.http.server.shared.localAddress)
        guard let localAddress = app.http.server.shared.localAddress,
              let ip = localAddress.ipAddress,
              let port = localAddress.port else {
            XCTFail("couldn't get ip/port from \(app.http.server.shared.localAddress.debugDescription)")
            return
        }

        XCTAssertEqual("127.0.0.1", ip)
        XCTAssertGreaterThan(port, 0)

        XCTAssertEqual("Hello, world!",
Repository: vapor/vapor
File: Tests/VaporTests/WebSocketTests.swift (Swift)

Code before:
import Vapor
import XCTest

final class WebSocketTests: XCTestCase {
    func testWebSocketClient() throws {
        let server = Application(.testing)

        server.http.server.configuration.port = 8086

        server.webSocket("echo") { req, ws in
            ws.onText { ws.send($1) }
        }
        server.environment.arguments = ["serve"]
        try server.start()

        let app = Application(.testing)
        defer {
            app.shutdown()
            server.shutdown()
        }

        app.get("ws") { req -> EventLoopFuture<String> in
            let promise = req.eventLoop.makePromise(of: String.self)
            return WebSocket.connect(
                to: "ws://localhost:8086/echo",
                on: req.eventLoop
            ) { ws in
                ws.send("Hello, world!")
                ws.onText { ws, text in
                    promise.succeed(text)
                    ws.close().cascadeFailure(to: promise)
                }
            }.flatMap {

Review comment (suggestion):
To make this potentially even less flaky, we could also use the automatic free port picking:
https://github.com/vapor/vapor/pull/2572/files

Code after:
import Vapor
import XCTest

final class WebSocketTests: XCTestCase {
    func testWebSocketClient() throws {
        let server = Application(.testing)

        server.http.server.configuration.port = 0

        server.webSocket("echo") { req, ws in
            ws.onText { ws.send($1) }
        }
        server.environment.arguments = ["serve"]
        try server.start()

        defer {
            server.shutdown()
        }

        guard let localAddress = server.http.server.shared.localAddress, let port = localAddress.port else {
            XCTFail("couldn't get port from \(server.http.server.shared.localAddress.debugDescription)")
            return
        }

        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1)

        let promise = elg.next().makePromise(of: String.self)
        let string = try WebSocket.connect(
            to: "ws://localhost:\(port)/echo",
            on: elg.next()
        ) { ws in
            ws.send("Hello, world!")
            ws.onText { ws, text in
Repository: vapor/vapor
File: Sources/Vapor/Multipart/FormDataDecoder+Content.swift (Swift)

Code before:
extension FormDataDecoder: ContentDecoder {
    public func decode<D>(_ decodable: D.Type, from body: ByteBuffer, headers: HTTPHeaders) throws -> D
        where D: Decodable
    {
        guard let boundary = headers.contentType?.parameters["boundary"] else {
            throw Abort(.unsupportedMediaType)
        }
        if headers.contentType?.parameters.count ?? 1 == 1  {
            throw Abort(.unprocessableEntity, identifier: "emptyMultipartFormData")
        }
        var body = body
        let buffer = body.readBytes(length: body.readableBytes) ?? []
        return try self.decode(D.self, from: buffer, boundary: boundary)
    }
}


Review comment (suggestion):
I'd prefer this to be a guard statement to match the rest of the code and better indicate an early exit. And we can chain the statements to avoid defaulting to 1

Code after:
import MultipartKit
import NIOHTTP1
import NIOCore

extension FormDataDecoder: ContentDecoder {
    public func decode<D>(_ decodable: D.Type, from body: ByteBuffer, headers: HTTPHeaders) throws -> D
        where D: Decodable
    {
        try self.decode(D.self, from: body, headers: headers, userInfo: [:])
    }
    
    public func decode<D>(_ decodable: D.Type, from body: ByteBuffer, headers: HTTPHeaders, userInfo: [CodingUserInfoKey: Any]) throws -> D
        where D: Decodable
    {
        guard let boundary = headers.contentType?.parameters["boundary"] else {
            throw Abort(.unsupportedMediaType)
        }

        var body = body
        let buffer = body.readBytes(length: body.readableBytes) ?? []

        guard Array("--\(boundary)\r\n--\(boundary)--\r".utf8) != buffer else {
            throw Abort(.unprocessableEntity, identifier: "emptyMultipartFormData")
        }

        if !userInfo.isEmpty {
            var actualDecoder = self // Changing a coder's userInfo is a thread-unsafe mutation, operate on a copy
            actualDecoder.userInfo.merge(userInfo) { $1 }
            return try actualDecoder.decode(D.self, from: buffer, boundary: boundary)
        } else {
            return try self.decode(D.self, from: buffer, boundary: boundary)
        }
    }
Repository: vapor/vapor
File: Sources/Vapor/Middleware/FileMiddleware.swift (Swift)

Code before:
                absPath = absPath.addTrailingSlash() + defaultFile
            }
            
            // If the default file doesn't exist, pass on request
            guard FileManager.default.fileExists(atPath: absPath) else {
                return next.respond(to: request)
            }
        }
        
        // stream the file
        let res = request.fileio.streamFile(at: absPath)
        return request.eventLoop.makeSucceededFuture(res)
    }
}

fileprivate extension String {
    /// Determines if input path is absolute based on a leading slash
    func isAbsolute() -> Bool {
        return self.hasPrefix("/")
    }

    /// Makes a path relative by removing all leading slashes
    func removeLeadingSlashes() -> String {
        var newPath = self
        while newPath.hasPrefix("/") {
            newPath = String(newPath.dropFirst())
        }
        return newPath
    }

    /// Adds a trailing slash to the path if one is not already present
    func addTrailingSlash() -> String {
        var newPath = self
        if !newPath.hasSuffix("/") {
            newPath += "/"
        }
        return newPath
    }
}


Review comment (suggestion):
```suggestion
            newPath.removeFirst()
```

Code after:
                absPath = absPath.addTrailingSlash() + defaultFile
            }
            
            // If the default file doesn't exist, pass on request
            guard FileManager.default.fileExists(atPath: absPath) else {
                return next.respond(to: request)
            }
        }
        
        // stream the file
        let res = request.fileio.streamFile(at: absPath)
        return request.eventLoop.makeSucceededFuture(res)
    }
}

fileprivate extension String {
    /// Determines if input path is absolute based on a leading slash
    func isAbsolute() -> Bool {
        return self.hasPrefix("/")
    }

    /// Makes a path relative by removing all leading slashes
    func removeLeadingSlashes() -> String {
        var newPath = self
        while newPath.hasPrefix("/") {
            newPath.removeFirst()
        }
        return newPath
    }

    /// Adds a trailing slash to the path if one is not already present
    func addTrailingSlash() -> String {
        var newPath = self
        if !newPath.hasSuffix("/") {
            newPath += "/"
        }
        return newPath
    }
}
Repository: vapor/vapor
File: Tests/VaporTests/Utilities/ThreadSafe.swift (Swift)

Code before:
    public init(wrappedValue value: Value) {
        self.value = value
    }
    
    public var wrappedValue: Value {
        get { return lock.run { return value } }
        set { lock.run { value = newValue } }
    }
}

fileprivate class Lock {
    private let nslock = NSLock()
    init() {}
    
    func lock() {
        nslock.lock()
    }
    
    func unlock() {
        nslock.unlock()
    }
    
    func run(_ closure: () throws -> Void) rethrows {
        lock()
        try closure()
        unlock()
    }
    
    func run<T>(_ closure: () throws -> T) rethrows -> T {
        lock()
        defer { unlock() }
        return try closure()
    }
}


Review comment (suggestion):
This will leave the lock locked if the closure throws.
```suggestion
        defer { unlock() }
        try closure()
```

Code after:
    public init(wrappedValue value: Value) {
        self.value = value
    }
    
    public var wrappedValue: Value {
        get { return lock.run { return value } }
        set { lock.run { value = newValue } }
    }
}

fileprivate class Lock {
    private let nslock = NSLock()
    init() {}
    
    func lock() {
        nslock.lock()
    }
    
    func unlock() {
        nslock.unlock()
    }
    
    func run(_ closure: () throws -> Void) rethrows {
        lock()
        defer { unlock() }
        try closure()
    }
    
    func run<T>(_ closure: () throws -> T) rethrows -> T {
        lock()
        defer { unlock() }
        return try closure()
    }
}
Repository: vapor/vapor
File: Sources/Vapor/HTTP/Client/EventLoopHTTPClient.swift (Swift)

Code before:
extension HTTPClient {
    func delegating(to eventLoop: EventLoop, logger: Logger) -> Client {
        EventLoopHTTPClient(
            http: self,
            eventLoop: eventLoop,
            logger: logger
        )
    }
}

private struct EventLoopHTTPClient: Client {
    let http: HTTPClient
    let eventLoop: EventLoop
    var logger: Logger?

    func send(
        _ client: ClientRequest
    ) -> EventLoopFuture<ClientResponse> {
        guard let url = URL(string: client.url.string) else {
            self.logger?.error("\"\(client.url.string)\" is an invalid URL")
            return self.eventLoop.makeSucceededFuture(ClientResponse())
        }
        do {
            let request = try HTTPClient.Request(
                url: url,
                method: client.method,
                headers: client.headers,
                body: client.body.map { .byteBuffer($0) }
            )
            return self.http.execute(
                request: request,
                eventLoop: .delegate(on: self.eventLoop),
                logger: logger
            ).map { response in
                let client = ClientResponse(
                    status: response.status,
                    headers: response.headers,
                    body: response.body
                )
                return client
            }
        } catch {
            return self.eventLoop.makeFailedFuture(error)
        }
    }


Review comment (bug):
This should return a failed future, probably an `.internalServerError`

Code after:
extension HTTPClient {
    func delegating(to eventLoop: EventLoop, logger: Logger) -> Client {
        EventLoopHTTPClient(
            http: self,
            eventLoop: eventLoop,
            logger: logger
        )
    }
}

private struct EventLoopHTTPClient: Client {
    let http: HTTPClient
    let eventLoop: EventLoop
    var logger: Logger?

    func send(
        _ client: ClientRequest
    ) -> EventLoopFuture<ClientResponse> {
        let urlString = client.url.string
        guard let url = URL(string: urlString) else {
            self.logger?.debug("\(urlString) is an invalid URL")
            return self.eventLoop.makeFailedFuture(Abort(.internalServerError, reason: "\(urlString) is an invalid URL"))
        }
        do {
            let request = try HTTPClient.Request(
                url: url,
                method: client.method,
                headers: client.headers,
                body: client.body.map { .byteBuffer($0) }
            )
            return self.http.execute(
                request: request,
                eventLoop: .delegate(on: self.eventLoop),
                logger: logger
            ).map { response in
                let client = ClientResponse(
                    status: response.status,
                    headers: response.headers,
                    body: response.body
                )
                return client
            }
        } catch {
            return self.eventLoop.makeFailedFuture(error)
        }
    }
Repository: vapor/vapor
File: Sources/Vapor/Response/ResponseCodable.swift (Swift)

Code before:
/// Can convert `self` to a `Response`.
///
/// Types that conform to this protocol can be returned in route closures.
public protocol ResponseEncodable {
    /// Encodes an instance of `Self` to a `Response`.
    ///
    /// - parameters:
    ///     - for: The `Request` associated with this `Response`.
    /// - returns: An `Response`.
    func encodeResponse(for request: Request) -> EventLoopFuture<Response>
}

/// Can convert `Request` to a `Self`.
///
/// Types that conform to this protocol can decode requests to their type.
public protocol RequestDecodable {
    /// Decodes an instance of `Request` to a `Self`.
    ///
    /// - parameters:
    ///     - request: The `Request` to be decoded.
    /// - returns: An asynchronous `Self`.
    static func decodeRequest(_ request: Request) -> EventLoopFuture<Self>
}

extension Request: RequestDecodable {
    public static func decodeRequest(_ request: Request) -> EventLoopFuture<Request> {
        return request.eventLoop.makeSucceededFuture(request)
    }
}

// MARK: Convenience

extension ResponseEncodable {
    /// Asynchronously encodes `Self` into a `Response`, setting the supplied status and headers.

Review comment (suggestion):
```suggestion
    /// - returns: A `Response`.
```

Code after:
/// Can convert `self` to a `Response`.
///
/// Types that conform to this protocol can be returned in route closures.
public protocol ResponseEncodable {
    /// Encodes an instance of `Self` to a `Response`.
    ///
    /// - parameters:
    ///     - for: The `Request` associated with this `Response`.
    /// - returns: A `Response`.
    func encodeResponse(for request: Request) -> EventLoopFuture<Response>
}

/// Can convert `Request` to a `Self`.
///
/// Types that conform to this protocol can decode requests to their type.
public protocol RequestDecodable {
    /// Decodes an instance of `Request` to a `Self`.
    ///
    /// - parameters:
    ///     - request: The `Request` to be decoded.
    /// - returns: An asynchronous `Self`.
    static func decodeRequest(_ request: Request) -> EventLoopFuture<Self>
}

extension Request: RequestDecodable {
    public static func decodeRequest(_ request: Request) -> EventLoopFuture<Request> {
        return request.eventLoop.makeSucceededFuture(request)
    }
}

// MARK: Convenience

extension ResponseEncodable {
    /// Asynchronously encodes `Self` into a `Response`, setting the supplied status and headers.
Repository: vapor/vapor
File: Sources/Vapor/Validation/Validation.swift (Swift)

Code before:
                    let result = validations.validate(nested)
                    results.append(result.results)
                }
                result = ValidatorResults.NestedEach(results: results)
            } catch {
                result = ValidatorResults.Codable(error: error)
            }
            return .init(key: key, result: result, customFailureDescription: customFailureDescription)
        }
    }
    
    init(key: ValidationKey, result: ValidatorResult, customFailureDescription: String?) {
        self.init { decoder in
            .init(key: key, result: result, customFailureDescription: customFailureDescription)
        }
    }
    
    init(run: @escaping (KeyedDecodingContainer<ValidationKey>) -> ValidationResult) {
        self.run = run
    }
}

public struct ValidationResult {
    public let key: ValidationKey
    public let result: ValidatorResult
    public let customFailureDescription: String?
}

extension ValidationResult: ValidatorResult {
    public var isFailure: Bool {
        self.result.isFailure
    }
    
    public var successDescription: String? {
        self.result.successDescription
            .map { "\(self.key) \($0)" }
    }
    
    public var failureDescription: String? {
        self.result.failureDescription
            .map { "\(self.key) \($0)" }
    }
}


Review comment (suggestion):
We're changing the public API here so will need a new `init()` that matches the existing behaviour to avoid any breaking changes

Code after:
                    let result = validations.validate(nested)
                    results.append(result.results)
                }
                result = ValidatorResults.NestedEach(results: results)
            } catch {
                result = ValidatorResults.Codable(error: error)
            }
            return .init(key: key, result: result, customFailureDescription: customFailureDescription)
        }
    }
    
    init(key: ValidationKey, result: ValidatorResult, customFailureDescription: String?) {
        self.init { decoder in
            .init(key: key, result: result, customFailureDescription: customFailureDescription)
        }
    }
    
    init(run: @escaping (KeyedDecodingContainer<ValidationKey>) -> ValidationResult) {
        self.run = run
    }
}

public struct ValidationResult {
    public let key: ValidationKey
    public let result: ValidatorResult
    public let customFailureDescription: String?
    
    init(key: ValidationKey, result: ValidatorResult, customFailureDescription: String? = nil) {
        self.key = key
        self.result = result
        self.customFailureDescription = customFailureDescription
    }
}

extension ValidationResult: ValidatorResult {
    public var isFailure: Bool {
        self.result.isFailure
    }
    
    public var successDescription: String? {
        self.result.successDescription
            .map { "\(self.key) \($0)" }
    }
    
    public var failureDescription: String? {
        self.result.failureDescription
            .map { "\(self.key) \($0)" }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Validation/Validators/Email.swift (Swift)

Code before:
extension Validator where T == String {
    /// Validates whether a `String` is a valid email address.
    public static var email: Validator<T> {
        .init {
            guard
                let range = $0.range(of: regex, options: [.regularExpression]),
                range.lowerBound == $0.startIndex && range.upperBound == $0.endIndex,
                // FIXME: these numbers are incorrect and too restrictive
                $0.count <= 255, // total length
                $0.split(separator: "@")[0].count <= 64 // length before `@`
            else {
                return ValidatorResults.Email(isValidEmail: false)
            }
            return ValidatorResults.Email(isValidEmail: true)
        }
    }
    
    public static var internationalEmail: Validator<T> {
        .init {
            guard
                let range = $0.range(of: regexInt, options: [.regularExpression]),
                range.lowerBound == $0.startIndex && range.upperBound == $0.endIndex,
                // FIXME: these numbers are incorrect and too restrictive
                $0.count <= 255, // total length
                $0.split(separator: "@")[0].count <= 64 // length before `@`
            else {
                return ValidatorResults.Email(isValidEmail: false)
            }
            return ValidatorResults.Email(isValidEmail: true)
        }
    }
}


Review comment (refactor):
If the numbers are now correct, we can remove this. Would be good to link to https://emailregex.com/email-validation-summary/ as the reason why we have these numbers

Code after:
extension Validator where T == String {
    /// Validates whether a `String` is a valid email address.
    public static var email: Validator<T> {
        .init {
            guard
                let range = $0.range(of: regex, options: [.regularExpression]),
                range.lowerBound == $0.startIndex && range.upperBound == $0.endIndex,
                // The numbers beneath here are as defined by https://emailregex.com/email-validation-summary/
                $0.count <= 320, // total length
                $0.split(separator: "@")[0].count <= 64 // length before `@`
            else {
                return ValidatorResults.Email(isValidEmail: false)
            }
            return ValidatorResults.Email(isValidEmail: true)
        }
    }
    
    public static var internationalEmail: Validator<T> {
        .init {
            guard
                let range = $0.range(of: regexInternationalEmail, options: [.regularExpression]),
                range.lowerBound == $0.startIndex && range.upperBound == $0.endIndex,
                // The numbers beneath here are as defined by https://emailregex.com/email-validation-summary/
                $0.count <= 320, // total length
                $0.split(separator: "@")[0].count <= 64 // length before `@`
            else {
                return ValidatorResults.Email(isValidEmail: false)
            }
            return ValidatorResults.Email(isValidEmail: true)
        }
    }
}