[pull] master from vapor:master (#234)

* New works-for-everything CI (vapor#2365)

* New works-for-everything CI

This version of the test workflow can be pasted without changes into any repo whose tests don't have additional dependencies and don't need to separately test that they're still Vapor-compatible (such as console-kit). Improvements over existing CI:

- Runs on pushes to master as well as PRs
- Full coverage of all available Swift runner images for Linux
- Uses latest Xcode without hardcoding on macOS
- Nice names for each test step
- Nice name for the workflow itself

* Apply suggestions from code review

* `Environment` cleanups (vapor#2353)

* Remove numerous unnecessary code stanzas.

* Don't check `isRelease` in `Environment.==`, it will always be equal because the value is effectively a compile-time constant. Also improve the use of MARK comments so Xcode renders the intended divisions nicely.

* Add warning to `Environment.isRelease` explaining the seemingly unusual behavior of its value.

* Redo the documentation comments for `Environment.secret()` (both versions). Significantly simplify the implementation of the path-taking version. IMPORTANT NOTE: This is an interim step of cleanup while a much more complete revamping of this API is worked on.

* Correctly sanitize the excess arguments Xcode passes to test invocations for the testing environment. I forgot to mention in a previous commit that support was added for `VAPOR_ENV` too...

* Add sanitization of raw SwiftPM's invocation of the xctest runner binary. This is necessarily a little specific to the version of SwiftPM and Xcode involved, but should at least be specific enough a check to not interfere with normal operations if the call sequence changes.

* There is no need to hardcode all the logger levels for `LosslessStringConvertible` conformance. `Logger.Level` is already `RawRepresentable` as `String` and that conformance can be used transparently. (And it has `CaseIterable` for good measure, which provides yet another way to scale this particular elevation.)

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>

.github/workflows/test.yml

@@ -1,25 +1,49 @@
-name: test
+name: Test Matrix
+
 on:
-- pull_request
+  pull_request:
+  push:
+    branches:
+    - master
+
 jobs:
-  vapor_macos:
-    runs-on: macos-latest
-    env:
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
-    steps:
-    - uses: actions/checkout@v2
-    - run: xcrun swift test --enable-test-discovery --sanitize=thread
-  vapor_xenial:
-    container: 
-      image: vapor/swift:5.2-xenial
+
+  Linux:
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        image:
+          - swift:5.2-xenial
+          - swift:5.2-bionic
+          - swiftlang/swift:nightly-5.2-xenial
+          - swiftlang/swift:nightly-5.2-bionic
+          - swiftlang/swift:nightly-5.3-xenial
+          - swiftlang/swift:nightly-5.3-bionic
+          - swiftlang/swift:nightly-master-xenial
+          - swiftlang/swift:nightly-master-bionic
+          - swiftlang/swift:nightly-master-focal
+        include:
+          - image: swiftlang/swift:nightly-master-centos8
+            depscmd: dnf install -y zlib-devel
+          - image: swiftlang/swift:nightly-master-amazonlinux2
+            depscmd: yum install -y zlib-devel
+    container: ${{ matrix.image }}
     steps:
-    - uses: actions/checkout@v2
-    - run: swift test --enable-test-discovery --sanitize=thread
-  vapor_bionic:
-    container: 
-      image: vapor/swift:5.2-bionic
-    runs-on: ubuntu-latest
+      - name: Install dependencies if needed
+        run: ${{ matrix.depscmd }}
+      - name: Check out code
+        uses: actions/checkout@v2
+      - name: Run tests with Thread Sanitizer
+        run: swift test --enable-test-discovery --sanitize=thread
+
+  macOS:
+    runs-on: macos-latest
     steps:
-    - uses: actions/checkout@v2
-    - run: swift test --enable-test-discovery --sanitize=thread
+      - name: Select latest available Xcode
+        uses: maxim-lobanov/setup-xcode@1.0
+        with: { 'xcode-version': 'latest' }
+      - name: Check out code
+        uses: actions/checkout@v2
+      - name: Run tests with Thread Sanitizer
+        run: swift test --enable-test-discovery --sanitize=thread

Sources/Vapor/Environment/Environment+Process.swift

@@ -17,12 +17,9 @@ extension Environment {
         ///     Environment.process.DATABASE_PORT // 3306
         public subscript<T>(dynamicMember member: String) -> T? where T: LosslessStringConvertible {
             get {
-                guard let raw = self._info.environment[member], let value = T(raw) else {
-                    return nil
-                }
-
-                return value
+                return self._info.environment[member].flatMap { T($0) }
             }
+
             nonmutating set (value) {
                 if let raw = value?.description {
                     setenv(member, raw, 1)
@@ -38,12 +35,9 @@ extension Environment {
         ///     Environment.process.DATABASE_USER // "root"
         public subscript(dynamicMember member: String) -> String? {
             get {
-                guard let value = self._info.environment[member] else {
-                    return nil
-                }
-
-                return value
+                return self._info.environment[member]
             }
+
             nonmutating set (value) {
                 if let raw = value {
                     setenv(member, raw, 1)

Sources/Vapor/Environment/Environment+Secret.swift

@@ -1,43 +1,61 @@
 extension Environment {
-    /// Reads a file's content for a secret. The secret key represents the name of the environment variable that holds the path for the file containing the secret
+    /// Reads a file's content for a secret. The secret key is the name of the environment variable that is expected to
+    /// specify the path of the file containing the secret.
+    ///
     /// - Parameters:
-    ///     - key: Environment name for the path to the file containing the secret
-    ///     - fileIO: FileIO handler provided by NIO
-    ///     - on: EventLoop to operate on while opening the file
-    /// - Throws: Error.environmentVariableNotFound if the environment variable with the key name does not exist
+    ///   - key: The environment variable name
+    ///   - fileIO: `NonBlockingFileIO` handler provided by NIO
+    ///   - eventLoop: `EventLoop` for NIO to use for working with the file
+    ///
+    /// Example usage:
+    ///
+    /// ````
+    /// func configure(_ app: Application) {
+    ///     // ...
+    ///
+    ///     let databasePassword = try Environment.secret(
+    ///         key: "DATABASE_PASSWORD_FILE",
+    ///         fileIO: app.fileio,
+    ///         on: app.eventLoopGroup.next()
+    ///     ).wait()
+    ///
+    /// ````
+    ///
+    /// - Important: Do _not_ use `.wait()` if loading a secret at any time after the app has booted, such as while
+    ///   handling a `Request`. Chain the result as you would any other future instead.
     public static func secret(key: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture<String?> {
-        guard let filePath = self.get(key) else { return eventLoop.future(nil) }
+        guard let filePath = self.get(key) else {
+            return eventLoop.future(nil)
+        }
         return self.secret(path: filePath, fileIO: fileIO, on: eventLoop)
     }
 
 
-    /// Reads a file's content for a secret. The path is a file path to the file that contains the secret in plain text
+    /// Load the content of a file at a given path as a secret.
+    ///
     /// - Parameters:
-    ///     - path: Path to the file that contains the secret
-    ///     - fileIO: FileIO handler provided by NIO
-    ///     - on: EventLoop to operate on while opening the file
-    /// - Throws: Error.environmentVariableNotFound if the environment variable with the key name does not exist
+    ///   - path: Path to the file containing the secret
+    ///   - fileIO: `NonBlockingFileIO` handler provided by NIO
+    ///   - eventLoop: `EventLoop` for NIO to use for working with the file
+    ///
+    /// - Returns:
+    ///   - On success, a succeeded future with the loaded content of the file.
+    ///   - On any kind of error, a succeeded future with a value of `nil`. It is not currently possible to get error details.
     public static func secret(path: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture<String?> {
         return fileIO
             .openFile(path: path, eventLoop: eventLoop)
-            .flatMap({ (arg) -> EventLoopFuture<ByteBuffer> in
+            .flatMap { handle, region in
                 return fileIO
-                    .read(fileRegion: arg.1, allocator: .init(), eventLoop: eventLoop)
-                    .flatMapThrowing({ (buffer) -> ByteBuffer in
-                        try arg.0.close()
-                        return buffer
-                    })
-            })
-            .map({ (buffer) -> (String) in
-                var buffer = buffer
-                return buffer.readString(length: buffer.writerIndex) ?? ""
-            })
-            .map({ (secret) -> (String) in
-                secret.trimmingCharacters(in: .whitespacesAndNewlines)
-            })
-            .recover ({ (_) -> String? in
+                    .read(fileRegion: region, allocator: .init(), eventLoop: eventLoop)
+                    .always { _ in try? handle.close() }
+            }
+            .map { buffer -> String in
+                return buffer
+                    .getString(at: buffer.readerIndex, length: buffer.readableBytes)!
+                    .trimmingCharacters(in: .whitespacesAndNewlines)
+            }
+            .recover { _ -> String? in
                 nil
-            })
+            }
     }
 }
-

Sources/Vapor/Environment/Environment.swift

@@ -1,3 +1,5 @@
+import ConsoleKit
+
 /// The environment the application is running in, i.e., production, dev, etc. All `Container`s will have
 /// an `Environment` that can be used to dynamically register and configure services.
 ///
@@ -12,6 +14,8 @@
 ///     print(Environment.get("DB_PASSWORD"))
 ///
 public struct Environment: Equatable {
+    // MARK: - Detection
+
     /// Detects the environment from `CommandLine.arguments`. Invokes `detect(from:)`.
     /// - parameters:
     ///     - arguments: Command line arguments to detect environment from.
@@ -21,73 +25,106 @@ public struct Environment: Equatable {
         return try Environment.detect(from: &commandInput)
     }
 
-    /// Detects the environment from `CommandInput`. Parses the `--env` flag.
+    /// Detects the environment from `CommandInput`. Parses the `--env` flag, with the
+    /// `VAPOR_ENV` environment variable as a fallback.
     /// - parameters:
     ///     - arguments: `CommandInput` to parse `--env` flag from.
     /// - returns: The detected environment, or default env.
     public static func detect(from commandInput: inout CommandInput) throws -> Environment {
+        self.sanitize(commandInput: &commandInput)
+
         struct EnvironmentSignature: CommandSignature {
             @Option(name: "env", short: "e", help: "Change the application's environment")
             var environment: String?
-            init() { }
         }
+
         var env: Environment
-        if let value = try EnvironmentSignature(from: &commandInput).environment {
-            switch value {
+        switch try EnvironmentSignature(from: &commandInput).environment ??
+            Environment.process.VAPOR_ENV
+        {
             case "prod", "production": env = .production
-            case "dev", "development": env = .development
+            case "dev", "development", .none: env = .development
             case "test", "testing": env = .testing
-            default: env = .init(name: value)
-            }
-        } else {
-            env = .development
+            case .some(let name): env = .init(name: name)
         }
         env.commandInput = commandInput
         return env
     }
 
-    // MARK: Presets
+    /// Performs stripping of user defaults overrides where and when appropriate.
+    private static func sanitize(commandInput: inout CommandInput) {
+        #if Xcode
+        // Strip all leading arguments matching the pattern for assignment to the `NSArgumentsDomain`
+        // of `UserDefaults`. Matching this pattern means being prefixed by `-NS` or `-Apple` and being
+        // followed by a value argument. Since this is mainly just to get around Xcode's habit of
+        // passing a bunch of these when no other arguments are specified in a test scheme, we ignore
+        // any that don't match the Apple patterns and assume the app knows what it's doing.
+        while (commandInput.arguments.first?.prefix(6) == "-Apple" || commandInput.arguments.first?.prefix(3) == "-NS"),
+              commandInput.arguments.count > 1 {
+            commandInput.arguments.removeFirst(2)
+        }
+        #elseif os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
+        // When tests are invoked directly through SwiftPM using `--filter`, SwiftPM will pass `-XCTest <filter>` to the
+        // runner binary, and also the test bundle path unconditionally. These must be stripped for Vapor to be satisifed
+        // with the validity of the arguments. We detect this case reliably the hard way, by looking for the `xctest`
+        // runner executable and a leading argument with the `.xctest` bundle suffix.
+        if commandInput.executable.hasSuffix("/usr/bin/xctest") {
+            if commandInput.arguments.first?.lowercased() == "-xctest" && commandInput.arguments.count > 1 {
+                commandInput.arguments.removeFirst(2)
+            }
+            if commandInput.arguments.first?.hasSuffix(".xctest") ?? false {
+                commandInput.arguments.removeFirst()
+            }
+        }
+        #endif
+    }
+
+    /// Invokes `sanitize(commandInput:)` over a set of raw arguments and returns the
+    /// resulting arguments, including the executable path.
+    private static func sanitizeArguments(_ arguments: [String] = CommandLine.arguments) -> [String] {
+        var commandInput = CommandInput(arguments: arguments)
+        sanitize(commandInput: &commandInput)
+        return commandInput.executablePath + commandInput.arguments
+    }
+
+    // MARK: - Presets
 
     /// An environment for deploying your application to consumers.
-    public static var production: Environment {
-        return .init(name: "production")
-    }
+    public static var production: Environment { .init(name: "production") }
 
     /// An environment for developing your application.
-    public static var development: Environment {
-        return .init(name: "development", arguments: ["vapor"])
-    }
+    public static var development: Environment { .init(name: "development") }
 
     /// An environment for testing your application.
-    public static var testing: Environment {
-        return .init(name: "testing", arguments: ["vapor"])
-    }
+    ///
+    /// Performs an explicit sanitization step because this preset is often used directly in unit tests, without the
+    /// benefit of the logic usually invoked through either form of `detect()`. This means that when `--env test` is
+    /// explicitly specified, the sanitize logic is run twice, but this should be harmless.
+    public static var testing: Environment { .init(name: "testing", arguments: sanitizeArguments()) }
 
     /// Creates a custom environment.
-    public static func custom(name: String) -> Environment {
-        return .init(name: name, arguments: ["vapor"])
-    }
+    public static func custom(name: String) -> Environment { .init(name: name) }
 
-    // MARK: Env
+    // MARK: - Env
 
     /// Gets a key from the process environment
     public static func get(_ key: String) -> String? {
         return ProcessInfo.processInfo.environment[key]
     }
 
-    // MARK: Equatable
-
-    /// See `Equatable`
-    public static func ==(lhs: Environment, rhs: Environment) -> Bool {
-        return lhs.name == rhs.name && lhs.isRelease == rhs.isRelease
-    }
-
     /// The current process of the environment.
     public static var process: Process {
         return Process()
     }
 
-    // MARK: Properties
+    // MARK: - Equatable
+
+    /// See `Equatable`
+    public static func ==(lhs: Environment, rhs: Environment) -> Bool {
+        return lhs.name == rhs.name
+    }
+
+    // MARK: - Properties
 
     /// The environment's unique name.
     public let name: String
@@ -96,9 +133,13 @@ public struct Environment: Equatable {
     ///
     /// This usually means reducing logging, disabling debug information, and sometimes
     /// providing warnings about configuration states that are not suitable for production.
-    public var isRelease: Bool {
-        return !_isDebugAssertConfiguration()
-    }
+    ///
+    /// - Warning: This value is determined at compile time by configuration; it is not
+    ///   based on the actual environment name. This can lead to unxpected results, such
+    ///   as `Environment.production.isRelease == false`. This is done intentionally to
+    ///   allow scenarios, such as testing production environment behaviors while retaining
+    ///   availability of debug information.
+    public var isRelease: Bool { !_isDebugAssertConfiguration() }
 
     /// The command-line arguments for this `Environment`.
     public var arguments: [String]
@@ -109,7 +150,7 @@ public struct Environment: Equatable {
         set { arguments = newValue.executablePath + newValue.arguments }
     }
 
-    // MARK: Init
+    // MARK: - Init
 
     /// Create a new `Environment`.
     public init(name: String, arguments: [String] = CommandLine.arguments) {

Sources/Vapor/Logging/LoggingSystem+Environment.swift

@@ -22,28 +22,6 @@ extension LoggingSystem {
 }
 
 extension Logger.Level: LosslessStringConvertible {
-    public init?(_ description: String) {
-        switch description.lowercased() {
-        case "trace": self = .trace
-        case "debug": self = .debug
-        case "info": self = .info
-        case "notice": self = .notice
-        case "warning": self = .warning
-        case "error": self = .error
-        case "critical": self = .critical
-        default: return nil
-        }
-    }
-
-    public var description: String {
-        switch self {
-        case .trace: return "trace"
-        case .debug: return "debug"
-        case .info: return "info"
-        case .notice: return "notice"
-        case .warning: return "warning"
-        case .error: return "error"
-        case .critical: return "critical"
-        }
-    }
+    public init?(_ description: String) { self.init(rawValue: description.lowercased()) }
+    public var description: String { self.rawValue }
 }