From 90110faa06f7666a0d58d224c92da976fff5d930 Mon Sep 17 00:00:00 2001 From: Leo Dion Date: Mon, 31 Aug 2026 12:48:30 -0400 Subject: [PATCH] Resolve booleans via a bool primitive, not string parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolvedBool` read every source through `string(forKey:)`, which cannot see a valueless command-line flag: swift-configuration's CLI provider reports `--verbose` only through `bool(forKey:)`, and returns nil from the string accessor. The `if source == .commandLine { return true }` branch was therefore unreachable for the very case its comment describes, and reachable only when a value *had* been supplied — which it then discarded. Measured against a real ConfigReader before the fix: --verbose -> false (should be true) --verbose false -> true (should be false) --verbose true -> true FLAG=banana -> false (should be ignored) FLAG=on -> false (should be ignored) The environment path was wrong too, and independently: the truthiness test was `normalized == "true" || "1" || "yes"`, so every unrecognized non-empty value collapsed to `false` rather than nil. A typo'd `FLAG=ture` silently *disabled* a flag whose default was true, instead of falling through to that default. Adds `bool(forKey:isSecret:fileID:line:) -> Bool?` as a fourth protocol primitive alongside string/int/double, and reduces `resolvedBool` to the same one-line delegation the other three already used. `ConfigReader.bool` has the identical shape to `ConfigReader.string`, so it witnesses the requirement with no change to the retroactive conformance. The requirement ships with a default implementation that parses the string value, so existing conformers keep compiling unchanged. That default is itself corrected: `true`/`1`/`yes` and `false`/`0`/`no` are recognized case-insensitively and everything else yields nil. Why this was never caught: MockConfigValueReader modelled a bare flag as an empty *string*, which the old code read as presence. The real provider returns nil there, so the double disagreed with production precisely where the bug lived. The mock now carries a native `bools` dictionary, and a new StringOnlyConfigValueReader covers the string-parsing default, so both reader shapes are exercised. 63 tests pass; swift-format, SwiftLint and the header check are clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BB4QwYjmEPMC2Fo5HW4cKd --- .../ConfigValueReading+Bool.swift | 62 +++++++++ Sources/ConfigKeyKit/ConfigValueReading.swift | 36 ++--- .../ConfigValueReadingBoolTests.swift | 126 ++++++++++++++++++ .../ConfigValueReadingTests.swift | 59 -------- .../MockConfigValueReader.swift | 16 +++ .../StringOnlyConfigValueReader.swift | 61 +++++++++ 6 files changed, 279 insertions(+), 81 deletions(-) create mode 100644 Sources/ConfigKeyKit/ConfigValueReading+Bool.swift create mode 100644 Tests/ConfigKeyKitTests/ConfigValueReadingBoolTests.swift create mode 100644 Tests/ConfigKeyKitTests/StringOnlyConfigValueReader.swift diff --git a/Sources/ConfigKeyKit/ConfigValueReading+Bool.swift b/Sources/ConfigKeyKit/ConfigValueReading+Bool.swift new file mode 100644 index 0000000..051a2de --- /dev/null +++ b/Sources/ConfigKeyKit/ConfigValueReading+Bool.swift @@ -0,0 +1,62 @@ +// +// ConfigValueReading+Bool.swift +// ConfigKeyKit +// +// Created by Leo Dion. +// Copyright © 2026 BrightDigit. +// +// Permission is hereby granted, free of charge, to any person +// obtaining a copy of this software and associated documentation +// files (the "Software"), to deal in the Software without +// restriction, including without limitation the rights to use, +// copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following +// conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +// OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +// HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +// WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +// OTHER DEALINGS IN THE SOFTWARE. +// + +internal import Foundation + +// swiftlint:disable discouraged_optional_boolean +extension ConfigValueReading { + /// Parses a boolean from the reader's string value. + /// + /// `true` / `1` / `yes` and `false` / `0` / `no` are recognized, case-insensitively. + /// Anything else — including an empty value — yields `nil`, so resolution falls through + /// to the next source and ultimately to the key's default. An unrecognized value must + /// not be treated as `false`: a typo would then silently *disable* a flag rather than + /// being ignored. + public func bool( + forKey key: Key, + isSecret: Bool, + fileID: String, + line: UInt + ) -> Bool? { + guard + let value = string(forKey: key, isSecret: isSecret, fileID: fileID, line: line) + else { + return nil + } + switch value.lowercased().trimmingCharacters(in: .whitespaces) { + case "true", "1", "yes": + return true + case "false", "0", "no": + return false + default: + return nil + } + } +} +// swiftlint:enable discouraged_optional_boolean diff --git a/Sources/ConfigKeyKit/ConfigValueReading.swift b/Sources/ConfigKeyKit/ConfigValueReading.swift index fb47721..4864ddd 100644 --- a/Sources/ConfigKeyKit/ConfigValueReading.swift +++ b/Sources/ConfigKeyKit/ConfigValueReading.swift @@ -71,6 +71,17 @@ public protocol ConfigValueReading { /// Reads a double value for the native key, or `nil` if absent. func double(forKey key: Key, isSecret: Bool, fileID: String, line: UInt) -> Double? + + /// Reads a boolean value for the native key, or `nil` when this source supplies + /// nothing usable — absent, empty, or not recognizable as a boolean. + /// + /// A default implementation parses the string value, so existing conformers keep + /// working unchanged. Readers with a native boolean accessor — `ConfigReader` among + /// them — witness this requirement directly, which matters: a command-line provider + /// reports a *valueless* flag (`--verbose`) only through its boolean accessor. Its + /// string accessor returns `nil`, so resolving booleans through strings cannot see + /// flag presence at all. + func bool(forKey key: Key, isSecret: Bool, fileID: String, line: UInt) -> Bool? } extension ConfigValueReading { @@ -126,13 +137,11 @@ extension ConfigValueReading { resolvedDouble(key) } - // swiftlint:disable discouraged_optional_boolean /// Reads an optional boolean value, or `nil` if no source provides one /// (same truthiness rules as the required boolean overload). public func read(_ key: OptionalConfigKey) -> Bool? { resolvedBool(key) } - // swiftlint:enable discouraged_optional_boolean /// Reads an optional value parsed from a source string with `transform`. /// @@ -193,26 +202,9 @@ extension ConfigValueReading { resolved(key) { double(forKey: $0, isSecret: $1, fileID: #fileID, line: #line) } } - // swiftlint:disable:next discouraged_optional_boolean private func resolvedBool(_ key: any ConfigurationKey) -> Bool? { - for source in sourcePriority { - guard let keyString = key.key(for: source) else { continue } - guard - let value = string( - forKey: makeConfigKey(keyString), isSecret: key.isSecret, fileID: #fileID, line: #line - ) - else { continue } - if source == .commandLine { - // Flag presence indicates true (e.g. `--verbose`). - return true - } - let normalized = value.lowercased().trimmingCharacters(in: .whitespaces) - if normalized.isEmpty { - // An empty value is treated as absent; consult the next source. - continue - } - return normalized == "true" || normalized == "1" || normalized == "yes" - } - return nil + resolved(key) { bool(forKey: $0, isSecret: $1, fileID: #fileID, line: #line) } } } + +// swiftlint:enable discouraged_optional_boolean diff --git a/Tests/ConfigKeyKitTests/ConfigValueReadingBoolTests.swift b/Tests/ConfigKeyKitTests/ConfigValueReadingBoolTests.swift new file mode 100644 index 0000000..8ef191b --- /dev/null +++ b/Tests/ConfigKeyKitTests/ConfigValueReadingBoolTests.swift @@ -0,0 +1,126 @@ +// +// ConfigValueReadingTests.swift +// ConfigKeyKit +// +// Created by Leo Dion. +// Copyright © 2026 BrightDigit. +// +// Permission is hereby granted, free of charge, to any person +// obtaining a copy of this software and associated documentation +// files (the "Software"), to deal in the Software without +// restriction, including without limitation the rights to use, +// copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following +// conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +// OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +// HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +// WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +// OTHER DEALINGS IN THE SOFTWARE. +// + +import Testing + +@testable import ConfigKeyKit + +/// Boolean resolution across sources. +/// +/// Split from the main suite because booleans are the one type whose resolution differs +/// per reader: one with a native boolean accessor (``MockConfigValueReader``, as +/// `ConfigReader` is) sees a valueless command-line flag, while one supplying only +/// strings (``StringOnlyConfigValueReader``) falls back to the protocol's parsing. +@Suite("ConfigValueReading: booleans") +internal struct ConfigValueReadingBoolTests { + @Test("Required bool: CLI flag presence is true") + internal func boolCLIPresence() throws { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) + let cli = try #require(boolKey.key(for: .commandLine)) + let reader = MockConfigValueReader(bools: [cli: true]) + #expect(reader.read(boolKey) == true) + } + + @Test("Required bool: an explicit CLI false is honored, not overridden by presence") + internal func boolCLIExplicitFalse() throws { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) + let cli = try #require(boolKey.key(for: .commandLine)) + #expect(MockConfigValueReader(bools: [cli: false]).read(boolKey) == false) + } + + @Test( + "Required bool: ENV truthy strings, via the string-parsing default", + arguments: [ + ("true", true), ("1", true), ("YES", true), ("yes", true), + ("false", false), ("0", false), ("no", false), ("NO", false), + ] + ) + internal func boolENVParsing(value: String, expected: Bool) throws { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) + let env = try #require(boolKey.key(for: .environment)) + let reader = StringOnlyConfigValueReader(strings: [env: value]) + #expect(reader.read(boolKey) == expected) + } + + @Test( + "Required bool: an unrecognized value is ignored, never coerced to false", + arguments: ["banana", "on", "off", "ture", "2"] + ) + internal func boolUnrecognizedFallsThrough(value: String) throws { + // Regression: these used to resolve as `false`, so a typo silently *disabled* a + // flag whose default was `true` instead of being ignored. + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) + let env = try #require(boolKey.key(for: .environment)) + #expect(StringOnlyConfigValueReader(strings: [env: value]).read(boolKey) == true) + + let optionalKey = OptionalConfigKey("verbose", envPrefix: "BRIGHTDIGIT") + let optionalEnv = try #require(optionalKey.key(for: .environment)) + #expect(StringOnlyConfigValueReader(strings: [optionalEnv: value]).read(optionalKey) == nil) + } + + @Test("Required bool: default when absent") + internal func boolDefault() { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) + #expect(MockConfigValueReader().read(boolKey) == true) + } + + @Test("Optional bool: CLI presence true, ENV truthy, nil when absent") + internal func optionalBool() throws { + let boolKey = OptionalConfigKey("verbose", envPrefix: "BRIGHTDIGIT") + let cli = try #require(boolKey.key(for: .commandLine)) + let env = try #require(boolKey.key(for: .environment)) + #expect(MockConfigValueReader(bools: [cli: true]).read(boolKey) == true) + #expect(StringOnlyConfigValueReader(strings: [env: "yes"]).read(boolKey) == true) + #expect(StringOnlyConfigValueReader(strings: [env: "false"]).read(boolKey) == false) + #expect(MockConfigValueReader().read(boolKey) == nil) + } + + @Test("Required bool honors sourcePriority: ENV value wins over CLI flag when reversed") + internal func boolReversedPriority() throws { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) + let cli = try #require(boolKey.key(for: .commandLine)) + let env = try #require(boolKey.key(for: .environment)) + // CLI flag present (true) and ENV explicitly "false": precedence decides. + let forward = MockConfigValueReader(bools: [cli: true, env: false]) + #expect(forward.read(boolKey) == true) + let reversed = MockConfigValueReader( + bools: [cli: true, env: false], + sourcePriority: [.environment, .commandLine] + ) + #expect(reversed.read(boolKey) == false) + } + + @Test("Required bool: empty ENV is treated as absent, default used") + internal func boolEmptyENVUsesDefault() throws { + let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) + let env = try #require(boolKey.key(for: .environment)) + #expect(StringOnlyConfigValueReader(strings: [env: ""]).read(boolKey) == true) + #expect(StringOnlyConfigValueReader(strings: [env: " "]).read(boolKey) == true) + } +} diff --git a/Tests/ConfigKeyKitTests/ConfigValueReadingTests.swift b/Tests/ConfigKeyKitTests/ConfigValueReadingTests.swift index cb2cb15..b88218f 100644 --- a/Tests/ConfigKeyKitTests/ConfigValueReadingTests.swift +++ b/Tests/ConfigKeyKitTests/ConfigValueReadingTests.swift @@ -67,31 +67,6 @@ internal struct ConfigValueReadingTests { #expect(reader.read(key) == "from-env") } - @Test("Required bool: CLI flag presence is true") - internal func boolCLIPresence() throws { - let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) - let cli = try #require(boolKey.key(for: .commandLine)) - let reader = MockConfigValueReader(strings: [cli: ""]) - #expect(reader.read(boolKey) == true) - } - - @Test( - "Required bool: ENV truthy strings", - arguments: [("true", true), ("1", true), ("YES", true), ("false", false), ("0", false)] - ) - internal func boolENVParsing(value: String, expected: Bool) throws { - let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) - let env = try #require(boolKey.key(for: .environment)) - let reader = MockConfigValueReader(strings: [env: value]) - #expect(reader.read(boolKey) == expected) - } - - @Test("Required bool: default when absent") - internal func boolDefault() { - let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) - #expect(MockConfigValueReader().read(boolKey) == true) - } - @Test("Optional int: parsed with precedence, nil when absent") internal func optionalInt() throws { let intKey = OptionalConfigKey("episode-number", envPrefix: "BRIGHTDIGIT") @@ -136,40 +111,6 @@ internal struct ConfigValueReadingTests { #expect(MockConfigValueReader().read(intKey) == -1) } - @Test("Optional bool: CLI presence true, ENV truthy, nil when absent") - internal func optionalBool() throws { - let boolKey = OptionalConfigKey("verbose", envPrefix: "BRIGHTDIGIT") - let cli = try #require(boolKey.key(for: .commandLine)) - let env = try #require(boolKey.key(for: .environment)) - #expect(MockConfigValueReader(strings: [cli: ""]).read(boolKey) == true) - #expect(MockConfigValueReader(strings: [env: "yes"]).read(boolKey) == true) - #expect(MockConfigValueReader(strings: [env: "false"]).read(boolKey) == false) - #expect(MockConfigValueReader().read(boolKey) == nil) - } - - @Test("Required bool honors sourcePriority: ENV value wins over CLI flag when reversed") - internal func boolReversedPriority() throws { - let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: false) - let cli = try #require(boolKey.key(for: .commandLine)) - let env = try #require(boolKey.key(for: .environment)) - // CLI flag present (true) and ENV explicitly "false": precedence decides. - let forward = MockConfigValueReader(strings: [cli: "", env: "false"]) - #expect(forward.read(boolKey) == true) - let reversed = MockConfigValueReader( - strings: [cli: "", env: "false"], - sourcePriority: [.environment, .commandLine] - ) - #expect(reversed.read(boolKey) == false) - } - - @Test("Required bool: empty ENV is treated as absent, default used") - internal func boolEmptyENVUsesDefault() throws { - let boolKey = ConfigKey("verbose", envPrefix: "BRIGHTDIGIT", default: true) - let env = try #require(boolKey.key(for: .environment)) - #expect(MockConfigValueReader(strings: [env: ""]).read(boolKey) == true) - #expect(MockConfigValueReader(strings: [env: " "]).read(boolKey) == true) - } - @Test("Optional date: falls through to next source when higher precedence fails to parse") internal func optionalDateParseFallthrough() throws { let dateKey = OptionalConfigKey("published-at", envPrefix: "BRIGHTDIGIT") diff --git a/Tests/ConfigKeyKitTests/MockConfigValueReader.swift b/Tests/ConfigKeyKitTests/MockConfigValueReader.swift index eced1c5..7253859 100644 --- a/Tests/ConfigKeyKitTests/MockConfigValueReader.swift +++ b/Tests/ConfigKeyKitTests/MockConfigValueReader.swift @@ -27,15 +27,24 @@ // OTHER DEALINGS IN THE SOFTWARE. // +// swiftlint:disable discouraged_optional_boolean @testable import ConfigKeyKit /// Dict-backed ``ConfigValueReading`` keyed by the exact per-source key strings /// that `ConfigKey` / `OptionalConfigKey` produce, so the shared `read(_:)` /// resolution can be exercised without any configuration framework. +/// +/// Models a reader with a **native** boolean accessor, as `ConfigReader` has. A +/// command-line provider reports a valueless flag (`--verbose`) only through that +/// accessor, so booleans are seeded in ``bools`` rather than as strings. Seeding a bare +/// flag as an empty string — which this double used to do — is exactly what hid the +/// resolution bug: the real provider returns `nil` from `string(forKey:)` there. +/// ``StringOnlyConfigValueReader`` covers the string-parsing default instead. internal struct MockConfigValueReader: ConfigValueReading { internal var strings: [String: String] = [:] internal var ints: [String: Int] = [:] internal var doubles: [String: Double] = [:] + internal var bools: [String: Bool] = [:] internal var sourcePriority: [ConfigKeySource] = ConfigKeySource.priority internal func makeConfigKey(_ string: String) -> String { string } @@ -57,4 +66,11 @@ internal struct MockConfigValueReader: ConfigValueReading { ) -> Double? { doubles[key] } + + internal func bool( + forKey key: String, isSecret _: Bool, fileID _: String, line _: UInt + ) -> Bool? { + bools[key] + } } +// swiftlint:enable discouraged_optional_boolean diff --git a/Tests/ConfigKeyKitTests/StringOnlyConfigValueReader.swift b/Tests/ConfigKeyKitTests/StringOnlyConfigValueReader.swift new file mode 100644 index 0000000..5a08fee --- /dev/null +++ b/Tests/ConfigKeyKitTests/StringOnlyConfigValueReader.swift @@ -0,0 +1,61 @@ +// +// MockConfigValueReader.swift +// ConfigKeyKit +// +// Created by Leo Dion. +// Copyright © 2026 BrightDigit. +// +// Permission is hereby granted, free of charge, to any person +// obtaining a copy of this software and associated documentation +// files (the "Software"), to deal in the Software without +// restriction, including without limitation the rights to use, +// copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following +// conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +// OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +// HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +// WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +// OTHER DEALINGS IN THE SOFTWARE. +// + +@testable import ConfigKeyKit + +/// A ``ConfigValueReading`` that supplies **only** strings, so it inherits the protocol's +/// default boolean parsing. +/// +/// Exists to pin that default: a reader with no native boolean accessor must still +/// resolve `true`/`1`/`yes` and `false`/`0`/`no`, and must yield `nil` — not `false` — +/// for anything it cannot recognize. +internal struct StringOnlyConfigValueReader: ConfigValueReading { + internal var strings: [String: String] = [:] + internal var sourcePriority: [ConfigKeySource] = ConfigKeySource.priority + + internal func makeConfigKey(_ string: String) -> String { string } + + internal func string( + forKey key: String, isSecret _: Bool, fileID _: String, line _: UInt + ) -> String? { + strings[key] + } + + internal func int( + forKey _: String, isSecret _: Bool, fileID _: String, line _: UInt + ) -> Int? { + nil + } + + internal func double( + forKey _: String, isSecret _: Bool, fileID _: String, line _: UInt + ) -> Double? { + nil + } +}