Add ability to transform values on migrate by dfed · Pull Request #228 · square/Valet

dfed · 2020-05-20T09:38:51Z

This PR resolves #226 by adding the ability to compactMap over key:value pairs as they are being migrated.

I do not intend to merge this PR prior to the release of Valet 4.0.

dfed · 2020-05-20T09:39:28Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

        }
    }

-    func test_migrateObjectsMatching_withAccountNameAsData_doesNotRaiseException() throws


This test name was incorrect.

codecov-commenter · 2020-05-20T17:30:35Z

Codecov Report

Merging #228 into master will increase coverage by 1.06%.
The diff coverage is 93.90%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   85.68%   86.74%   +1.06%     
==========================================
  Files          15       16       +1     
  Lines         992     1049      +57     
==========================================
+ Hits          850      910      +60     
+ Misses        142      139       -3

Impacted Files	Coverage Δ
Sources/Valet/Internal/Keychain.swift	`89.50% <88.88%> (+1.14%)`	⬆️
Sources/Valet/Valet.swift	`90.74% <95.83%> (+1.71%)`	⬆️
Sources/Valet/MigratableKeyValuePair.swift	`100.00% <100.00%> (ø)`

dfed · 2020-05-20T19:52:37Z

Sources/Valet/Internal/Keychain.swift

-        var keysToMigrate = Set<String>()
+        var keyValuePairsToMigrate = [String: Data]()
        for keychainEntry in retrievedItemsToMigrateWithData {
-            guard let key = keychainEntry[kSecAttrAccount as String] as? String, key != Keychain.canaryKey else {


We'll force the key to be a String later on as part of the compactMap. For now, we just want to make sure we have a key.

dfed · 2020-05-20T21:28:42Z

Tests/ValetObjectiveCBridgeTests/VALValetTests.m

    XCTAssertNil(valet);
 }

+- (void)test_containsObjectForKey_returnsTrueWhenObjectExistsInKeychain;


I realized while I was here that I didn’t have an explicit objective-c test for this wrapper method, so I wrote a couple

dfed · 2020-05-20T21:29:03Z

Tests/ValetObjectiveCBridgeTests/VALValetTests.m


+- (BOOL)testEnvironmentIsSigned;
+{
+    return NSBundle.mainBundle.bundleIdentifier != nil && ![NSBundle.mainBundle.bundleIdentifier isEqualToString:@"com.apple.dt.xctest.tool"];


This logic is the same logic used in the swift tests

README.md

Sources/Valet/Internal/Keychain.swift

Tests/ValetIntegrationTests/KeychainIntegrationTests.swift

NickEntin · 2020-06-04T01:20:29Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

    }
-
+
+    func test_migrateObjectsMatchingCompactMap_successfullyMigratesTransformedValue() throws {


Should we also add a test that transforms the key? (Same for the Valet -> Valet equivalent below)

Good call! Will add

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

Sources/Valet/Valet.swift

fdiaz

I mostly looked at the public API 😉

fdiaz · 2020-06-05T10:22:06Z

Sources/Valet/MigratableKeyValuePair.swift

+import Foundation
+
+/// A struct that represented a key:value pair that can be migrated.
+public struct MigratableKeyValuePair<KeyType: Hashable>: Hashable {


Maybe?

Suggested change

public struct MigratableKeyValuePair<KeyType: Hashable>: Hashable {

public struct MigratableKeyValuePair<Key: Hashable>: Hashable {

https://swift.org/documentation/api-design-guidelines#name-according-to-roles

fdiaz · 2020-06-05T10:27:00Z

Sources/Valet/MigratableKeyValuePair.swift

+
+    /// A sentinal `ObjectiveCCompatibilityMigratableKeyValuePairOutput` that conveys that the migration should be prevented.
+    @objc
+    public static func preventMigration() -> ObjectiveCCompatibilityMigratableKeyValuePairOutput {


This method sounds to me more than its actually doing the action to prevent migration on an instance rather than creating an instance that prevents the migration.

Maybe migrationDisabled / migrationPrevented?

Yeah the tough thing here is it isn't super clear that we're doing this work before the migration has started. This is why I wanted to use a future tense. A past-tense phrase here might make it read as if returning it prevented/disabled the migration of just this pair (or future pairs).

In the context where it's used, I hope this method reads well:

Valet/Tests/ValetObjectiveCBridgeTests/VALValetTests.m

Lines 475 to 484 in c7cec75

[otherValet migrateObjectsMatching:@{

(__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword,

(__bridge id)kSecAttrService : @"VAL_VALValet_initWithIdentifier:accessibility:_identifier_AccessibleWhenUnlocked",

} compactMap:^VALMigratableKeyValuePairOutput * _Nullable(VALMigratableKeyValuePairInput * _Nonnull input) {

if ([input.key isEqualToString:key1]) {

return [[VALMigratableKeyValuePairOutput alloc] initWithKey:input.key value:input.value];;

} else {

return VALMigratableKeyValuePairOutput.preventMigration;

}

} error:nil];

The documentation on the migrateObjectsMatching Objective-C method also makes it clear how to use this method:

Valet/Sources/Valet/Valet.swift

Lines 661 to 668 in c7cec75

/// Migrates objects matching the input query into the receiving Valet instance.

/// - Parameters:

/// - query: The query with which to retrieve existing keychain data via a call to SecItemCopyMatching.

/// - compactMap: A closure that transforms a key:value pair from the raw pair currently in the keychain into a key:value pair we'll insert into the destination Valet. Returning `nil` from this closure will cause that key:value pair not to be migrated. Returning `VALMigratableKeyValuePairOutput.preventMigration` will prevent migrating any key:value pairs.

/// - error: An error of type `KeychainError` or `MigrationError`.

@available(swift, obsoleted: 1.0)

@objc(migrateObjectsMatching:compactMap:error:)

public func 🚫swift_migrateObjects(matching query: [String : AnyHashable], compactMap: (ObjectiveCCompatibilityMigratableKeyValuePairInput) -> ObjectiveCCompatibilityMigratableKeyValuePairOutput?) throws {

While I agree that reading this method by itself is a bit confusing, I think within the context it's supposed to be used it makes sense? Let me know if not @fdiaz.

That said, I should make this method available only to Objective-C

…able

Co-authored-by: Nick Entin <entin@squareup.com>

dfed · 2020-06-13T23:45:23Z

Force push had no functional changes. Only change was branch history after merging develop--4.0 into master

dfed commented May 20, 2020

View reviewed changes

dfed changed the title ~~[Draft][RFC] Add ability to transform values on migrate~~ [Draft] Add ability to transform values on migrate May 20, 2020

dfed commented May 20, 2020

View reviewed changes

dfed requested review from NickEntin and fdiaz May 20, 2020 20:16

dfed marked this pull request as ready for review May 20, 2020 20:16

dfed changed the title ~~[Draft] Add ability to transform values on migrate~~ Add ability to transform values on migrate May 20, 2020

dfed force-pushed the dfed--transform-on-migrate branch from fddf1ce to 9ed7cd6 Compare May 20, 2020 20:22

dfed commented May 20, 2020

View reviewed changes

dfed linked an issue May 21, 2020 that may be closed by this pull request

Support arbitrary transforms of data during migration #226

Closed

dfed self-assigned this May 21, 2020

NickEntin reviewed Jun 4, 2020

View reviewed changes

NickEntin approved these changes Jun 4, 2020

View reviewed changes

dfed commented Jun 4, 2020

View reviewed changes

Sources/Valet/Valet.swift Outdated Show resolved Hide resolved

fdiaz approved these changes Jun 5, 2020

View reviewed changes

Base automatically changed from develop--4.0 to master June 13, 2020 23:33

dfed and others added 11 commits June 13, 2020 16:42

Add ability to transform values on migrate

043f4c9

On macOS, kSecAttrAccount must always be a string value to be retriev…

1676cf5

…able

Up test coverage

5ebeebe

whitespace

63d1611

Better naming for objc compatibility private method

7b89056

Mention new method in README

997b144

Apply suggestions from Nick's code review

d5312fe

Co-authored-by: Nick Entin <entin@squareup.com>

Add better parameter documentation

99873ee

Add successfullyMigratesTransformedKey tests

d2a8ecc

Consolidate comments

f1bedd9

fdiaz feedback

abb6d42

dfed force-pushed the dfed--transform-on-migrate branch from 093d029 to abb6d42 Compare June 13, 2020 23:42

whitespace

f62a360

dfed merged commit 1879bdb into master Jun 22, 2020

dfed deleted the dfed--transform-on-migrate branch June 22, 2020 00:12

		}


		func test_migrateObjectsMatchingCompactMap_successfullyMigratesTransformedValue() throws {

	public struct MigratableKeyValuePair<KeyType: Hashable>: Hashable {
	public struct MigratableKeyValuePair<Key: Hashable>: Hashable {

	[otherValet migrateObjectsMatching:@{
	(__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword,
	(__bridge id)kSecAttrService : @"VAL_VALValet_initWithIdentifier:accessibility:_identifier_AccessibleWhenUnlocked",
	} compactMap:^VALMigratableKeyValuePairOutput * _Nullable(VALMigratableKeyValuePairInput * _Nonnull input) {
	if ([input.key isEqualToString:key1]) {
	return [[VALMigratableKeyValuePairOutput alloc] initWithKey:input.key value:input.value];;
	} else {
	return VALMigratableKeyValuePairOutput.preventMigration;
	}
	} error:nil];

	/// Migrates objects matching the input query into the receiving Valet instance.
	/// - Parameters:
	/// - query: The query with which to retrieve existing keychain data via a call to SecItemCopyMatching.
	/// - compactMap: A closure that transforms a key:value pair from the raw pair currently in the keychain into a key:value pair we'll insert into the destination Valet. Returning `nil` from this closure will cause that key:value pair not to be migrated. Returning `VALMigratableKeyValuePairOutput.preventMigration` will prevent migrating any key:value pairs.
	/// - error: An error of type `KeychainError` or `MigrationError`.
	@available(swift, obsoleted: 1.0)
	@objc(migrateObjectsMatching:compactMap:error:)
	public func 🚫swift_migrateObjects(matching query: [String : AnyHashable], compactMap: (ObjectiveCCompatibilityMigratableKeyValuePairInput) -> ObjectiveCCompatibilityMigratableKeyValuePairOutput?) throws {

Conversation

dfed commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fdiaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Jun 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dfed commented May 20, 2020 •

edited

Loading

codecov-commenter commented May 20, 2020 •

edited

Loading