From 403672aa287decd16730fd9ad184427f27165b3e Mon Sep 17 00:00:00 2001 From: leoMehlig Date: Sat, 2 Sep 2023 16:45:19 +0200 Subject: [PATCH 1/8] Fixes infinite recursion when casting `AnySerializable` to wrong type If a values associated type `Serializable` , equals the values type, `toValue` would infinitely call itself. The test `testWrongCast` reproduces this and I fixed it by comparing the `nextType` with the current type. --- Sources/Defaults/Utilities.swift | 2 +- Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Sources/Defaults/Utilities.swift b/Sources/Defaults/Utilities.swift index aaaee5e..88b3ab7 100644 --- a/Sources/Defaults/Utilities.swift +++ b/Sources/Defaults/Utilities.swift @@ -194,7 +194,7 @@ extension Defaults.Serializable { return anyObject } - guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type else { + guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type, nextType != T.self else { // This is a special case for the types which do not conform to `Defaults.Serializable` (for example, `Any`). return T.bridge.deserialize(anyObject as? T.Serializable) as? T } diff --git a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift index b0f4017..34a77d6 100644 --- a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift +++ b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift @@ -466,4 +466,10 @@ final class DefaultsAnySerializableTests: XCTestCase { waitForExpectations(timeout: 10) } + + func testWrongCast() { + let value = Defaults.AnySerializable(false) + XCTAssertEqual(value.get(Bool.self), false) + XCTAssertNil(value.get(String.self)) + } } From 4c10fc0503ba3a4bc137d5c8afb8d60ab8451a32 Mon Sep 17 00:00:00 2001 From: leoMehlig Date: Sat, 2 Sep 2023 19:04:55 +0200 Subject: [PATCH 2/8] Fix indentation --- Sources/Defaults/Utilities.swift | 2 +- Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Sources/Defaults/Utilities.swift b/Sources/Defaults/Utilities.swift index 88b3ab7..357e0c6 100644 --- a/Sources/Defaults/Utilities.swift +++ b/Sources/Defaults/Utilities.swift @@ -194,7 +194,7 @@ extension Defaults.Serializable { return anyObject } - guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type, nextType != T.self else { + guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type, nextType != T.self else { // This is a special case for the types which do not conform to `Defaults.Serializable` (for example, `Any`). return T.bridge.deserialize(anyObject as? T.Serializable) as? T } diff --git a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift index 34a77d6..5b38c4d 100644 --- a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift +++ b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift @@ -467,9 +467,9 @@ final class DefaultsAnySerializableTests: XCTestCase { waitForExpectations(timeout: 10) } - func testWrongCast() { - let value = Defaults.AnySerializable(false) - XCTAssertEqual(value.get(Bool.self), false) - XCTAssertNil(value.get(String.self)) - } + func testWrongCast() { + let value = Defaults.AnySerializable(false) + XCTAssertEqual(value.get(Bool.self), false) + XCTAssertNil(value.get(String.self)) + } } From 1185c4ecccdf72e424675273a354f92e7d8e98e3 Mon Sep 17 00:00:00 2001 From: leoMehlig Date: Sat, 2 Sep 2023 19:05:50 +0200 Subject: [PATCH 3/8] Remove spaces line --- Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift index 5b38c4d..a0f9e61 100644 --- a/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift +++ b/Tests/DefaultsTests/DefaultsAnySeriliazableTests.swift @@ -466,7 +466,7 @@ final class DefaultsAnySerializableTests: XCTestCase { waitForExpectations(timeout: 10) } - + func testWrongCast() { let value = Defaults.AnySerializable(false) XCTAssertEqual(value.get(Bool.self), false) From 7cfd3401ab707affc4de3de13b45e16cf58fdcd8 Mon Sep 17 00:00:00 2001 From: leoMehlig Date: Sun, 3 Sep 2023 10:25:44 +0200 Subject: [PATCH 4/8] Adds `publisher` function accepting array of keys In my app I have a use case, where I have a dynamic array of keys, which I want to observe using a Publisher (a publisher works better here then AsyncSequence). By giving an option to use either variadic parameters or an array, that would be possible without breaking any code. Additionally, I've discovered a crash when observing multiple keys and immediately cancelling the publisher. Here an observer, which was not yet added, was removed. Added check to make sure, this can't happen. --- Sources/Defaults/Observation+Combine.swift | 14 +++++++++++++- Sources/Defaults/Observation.swift | 7 ++++++- Tests/DefaultsTests/DefaultsTests.swift | 17 +++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/Sources/Defaults/Observation+Combine.swift b/Sources/Defaults/Observation+Combine.swift index 16f4e11..a8be6a8 100644 --- a/Sources/Defaults/Observation+Combine.swift +++ b/Sources/Defaults/Observation+Combine.swift @@ -102,7 +102,7 @@ extension Defaults { - Warning: This method exists for backwards compatibility and will be deprecated sometime in the future. Use ``Defaults/updates(_:initial:)-9eh8`` instead. */ public static func publisher( - keys: _AnyKey..., + keys: [_AnyKey], options: ObservationOptions = [.initial] ) -> AnyPublisher { let initial = Empty(completeImmediately: false).eraseToAnyPublisher() @@ -118,4 +118,16 @@ extension Defaults { combined.merge(with: keyPublisher).eraseToAnyPublisher() } } + + /** + Publisher for multiple `Key` observation, but without specific information about changes. + + - Warning: This method exists for backwards compatibility and will be deprecated sometime in the future. Use ``Defaults/updates(_:initial:)-9eh8`` instead. + */ + public static func publisher( + keys: _AnyKey..., + options: ObservationOptions = [.initial] + ) -> AnyPublisher { + self.publisher(keys: keys, options: options) + } } diff --git a/Sources/Defaults/Observation.swift b/Sources/Defaults/Observation.swift index 858a75c..1610430 100644 --- a/Sources/Defaults/Observation.swift +++ b/Sources/Defaults/Observation.swift @@ -125,6 +125,7 @@ extension Defaults { private weak var object: UserDefaults? private let key: String private let callback: Callback + private var isObserving = false init(object: UserDefaults, key: String, callback: @escaping Callback) { self.object = object @@ -138,10 +139,14 @@ extension Defaults { func start(options: ObservationOptions) { object?.addObserver(self, forKeyPath: key, options: options.toNSKeyValueObservingOptions, context: nil) + self.isObserving = true } func invalidate() { - object?.removeObserver(self, forKeyPath: key, context: nil) + if isObserving { + object?.removeObserver(self, forKeyPath: key, context: nil) + isObserving = false + } object = nil lifetimeAssociation?.cancel() } diff --git a/Tests/DefaultsTests/DefaultsTests.swift b/Tests/DefaultsTests/DefaultsTests.swift index 2108c1c..f1a25c3 100644 --- a/Tests/DefaultsTests/DefaultsTests.swift +++ b/Tests/DefaultsTests/DefaultsTests.swift @@ -733,6 +733,23 @@ final class DefaultsTests: XCTestCase { waitForExpectations(timeout: 10) } + func testImmediatelyFinishingMultiplePublisherCombine() { + let key1 = Defaults.Key("observeKey1", default: false) + let key2 = Defaults.Key("observeKey2", default: "🦄") + let expect = expectation(description: "Observation closure being called without crashing") + + let cancellable = Defaults + .publisher(keys: [key1, key2], options: [.initial]) + .first() + .sink { _ in + expect.fulfill() + } + + cancellable.cancel() + + waitForExpectations(timeout: 10) + } + func testKeyEquatable() { XCTAssertEqual(Defaults.Key("equatableKeyTest", default: false), Defaults.Key("equatableKeyTest", default: false)) } From 8b256c148b027c54893a95335766692a65326d70 Mon Sep 17 00:00:00 2001 From: leoMehlig Date: Sun, 3 Sep 2023 10:32:10 +0200 Subject: [PATCH 5/8] Fix indentation --- Sources/Defaults/Observation+Combine.swift | 22 +++++++-------- Sources/Defaults/Observation.swift | 12 ++++---- Tests/DefaultsTests/DefaultsTests.swift | 32 +++++++++++----------- 3 files changed, 33 insertions(+), 33 deletions(-) diff --git a/Sources/Defaults/Observation+Combine.swift b/Sources/Defaults/Observation+Combine.swift index a8be6a8..bb09754 100644 --- a/Sources/Defaults/Observation+Combine.swift +++ b/Sources/Defaults/Observation+Combine.swift @@ -119,15 +119,15 @@ extension Defaults { } } - /** - Publisher for multiple `Key` observation, but without specific information about changes. - - - Warning: This method exists for backwards compatibility and will be deprecated sometime in the future. Use ``Defaults/updates(_:initial:)-9eh8`` instead. - */ - public static func publisher( - keys: _AnyKey..., - options: ObservationOptions = [.initial] - ) -> AnyPublisher { - self.publisher(keys: keys, options: options) - } + /** + Publisher for multiple `Key` observation, but without specific information about changes. + + - Warning: This method exists for backwards compatibility and will be deprecated sometime in the future. Use ``Defaults/updates(_:initial:)-9eh8`` instead. + */ + public static func publisher( + keys: _AnyKey..., + options: ObservationOptions = [.initial] + ) -> AnyPublisher { + self.publisher(keys: keys, options: options) + } } diff --git a/Sources/Defaults/Observation.swift b/Sources/Defaults/Observation.swift index 1610430..b768288 100644 --- a/Sources/Defaults/Observation.swift +++ b/Sources/Defaults/Observation.swift @@ -125,7 +125,7 @@ extension Defaults { private weak var object: UserDefaults? private let key: String private let callback: Callback - private var isObserving = false + private var isObserving = false init(object: UserDefaults, key: String, callback: @escaping Callback) { self.object = object @@ -139,14 +139,14 @@ extension Defaults { func start(options: ObservationOptions) { object?.addObserver(self, forKeyPath: key, options: options.toNSKeyValueObservingOptions, context: nil) - self.isObserving = true + self.isObserving = true } func invalidate() { - if isObserving { - object?.removeObserver(self, forKeyPath: key, context: nil) - isObserving = false - } + if isObserving { + object?.removeObserver(self, forKeyPath: key, context: nil) + isObserving = false + } object = nil lifetimeAssociation?.cancel() } diff --git a/Tests/DefaultsTests/DefaultsTests.swift b/Tests/DefaultsTests/DefaultsTests.swift index f1a25c3..8ef82e3 100644 --- a/Tests/DefaultsTests/DefaultsTests.swift +++ b/Tests/DefaultsTests/DefaultsTests.swift @@ -733,22 +733,22 @@ final class DefaultsTests: XCTestCase { waitForExpectations(timeout: 10) } - func testImmediatelyFinishingMultiplePublisherCombine() { - let key1 = Defaults.Key("observeKey1", default: false) - let key2 = Defaults.Key("observeKey2", default: "🦄") - let expect = expectation(description: "Observation closure being called without crashing") - - let cancellable = Defaults - .publisher(keys: [key1, key2], options: [.initial]) - .first() - .sink { _ in - expect.fulfill() - } - - cancellable.cancel() - - waitForExpectations(timeout: 10) - } + func testImmediatelyFinishingMultiplePublisherCombine() { + let key1 = Defaults.Key("observeKey1", default: false) + let key2 = Defaults.Key("observeKey2", default: "🦄") + let expect = expectation(description: "Observation closure being called without crashing") + + let cancellable = Defaults + .publisher(keys: [key1, key2], options: [.initial]) + .first() + .sink { _ in + expect.fulfill() + } + + cancellable.cancel() + + waitForExpectations(timeout: 10) + } func testKeyEquatable() { XCTAssertEqual(Defaults.Key("equatableKeyTest", default: false), Defaults.Key("equatableKeyTest", default: false)) From 704167f2bfeed941a9b8f47244faba93d4a2f12b Mon Sep 17 00:00:00 2001 From: Leo Mehlig Date: Sun, 3 Sep 2023 12:37:03 +0200 Subject: [PATCH 6/8] Update Sources/Defaults/Observation.swift Co-authored-by: Sindre Sorhus --- Sources/Defaults/Observation.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Defaults/Observation.swift b/Sources/Defaults/Observation.swift index b768288..e136e5e 100644 --- a/Sources/Defaults/Observation.swift +++ b/Sources/Defaults/Observation.swift @@ -139,7 +139,7 @@ extension Defaults { func start(options: ObservationOptions) { object?.addObserver(self, forKeyPath: key, options: options.toNSKeyValueObservingOptions, context: nil) - self.isObserving = true + isObserving = true } func invalidate() { From b825df88be32a3c0ac9e7bac3b3df5c87d1ae188 Mon Sep 17 00:00:00 2001 From: Leo Mehlig Date: Sun, 3 Sep 2023 12:37:11 +0200 Subject: [PATCH 7/8] Update Sources/Defaults/Observation.swift Co-authored-by: Sindre Sorhus --- Sources/Defaults/Observation.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/Defaults/Observation.swift b/Sources/Defaults/Observation.swift index e136e5e..06d2d74 100644 --- a/Sources/Defaults/Observation.swift +++ b/Sources/Defaults/Observation.swift @@ -147,6 +147,7 @@ extension Defaults { object?.removeObserver(self, forKeyPath: key, context: nil) isObserving = false } + object = nil lifetimeAssociation?.cancel() } From a44434e64739578ce36a3ee5968ae976b41de2c9 Mon Sep 17 00:00:00 2001 From: Leo Mehlig Date: Sun, 3 Sep 2023 16:20:55 +0200 Subject: [PATCH 8/8] Update Sources/Defaults/Observation+Combine.swift Co-authored-by: Sindre Sorhus --- Sources/Defaults/Observation+Combine.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Defaults/Observation+Combine.swift b/Sources/Defaults/Observation+Combine.swift index bb09754..c045f3e 100644 --- a/Sources/Defaults/Observation+Combine.swift +++ b/Sources/Defaults/Observation+Combine.swift @@ -128,6 +128,6 @@ extension Defaults { keys: _AnyKey..., options: ObservationOptions = [.initial] ) -> AnyPublisher { - self.publisher(keys: keys, options: options) + publisher(keys: keys, options: options) } }