Environment cleanups (#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.)

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 }
 }