Update cache clearing helpers by timkimadobe · Pull Request #27 · adobe/aepsdk-testutils-ios

timkimadobe · 2024-01-31T01:44:24Z

Note

This PR also includes an important (unrelated to the features in this PR) bug fix for MockNetworkService double recording sent network requests.

Description

This PR creates a new cache clearing helper as an extension to NamedCollectionDataStore, called clear(). The intent of the changes are:

To support tvOS and iOS handling of device local persistence (both before and after Core 4.2.0 UserDefaults migration (iOS only))
To make clearing all known persistence locations as easy as possible
To allow easy customization for clearing specific persistence locations for more specific test cases

Modeled off of the Core implementation: https://github.com/adobe/aepsdk-core-ios/blob/d91de55cd052b1141a5d78d4a4d433fb486c8fac/AEPServices/Mocks/DatastoreUtilities.swift#L40

Changes

NamedCollectionDataStore.clear() - new method that uses the default args for all underlying calls:

static func clear() {
    UserDefaults.clearAll()
    FileManager.default.clearCache()
    FileManager.default.removeAdobeCacheDirectory()
}

FileManager.clearCache updated to allow for passing custom list of cache locations and isDirectory flag, with default locations if not specified

Questions for reviewers

Core’s version of NamedCollectionDataStore.clear() removes each item in the cache directory individually vs the entire top level cache directory; is that preferable to removing the entire "com.adobe.aep.datastore" directory?
1. Since all calls in the Core FileSystemNamedCollection route through findOrCreateAdobeSubdirectory, removal of the entire directory itself shouldn’t pose an issue?
FileManager clearCache also uses ServiceProvider.shared.dataQueueService and removes the key for each cache item name, is this required?
1. Was brought over from the Edge iOS base implementation: https://github.com/adobe/aepsdk-edge-ios/blob/00fd1a7f4033aa862693b1383fe3e579bed6e4c6/Tests/TestUtils/FileManager%2BTestable.swift#L27
Are there any other default cache locations that should be included in the FileManager.clearCache list?
- For example, with the current list of locations, this is the clear cache result (note the "BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation:" in the middle):

Error removing cache item, with reason: Error Domain=NSCocoaErrorDomain Code=4 "“com.adobe.edge.consent” couldn’t be removed." UserInfo={NSUserStringVariant=(
    Remove
), NSFilePath=/Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/Caches/com.adobe.edge.consent, NSUnderlyingError=0x600000ce5e60 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}
Error removing cache item, with reason: Error Domain=NSCocoaErrorDomain Code=4 "“com.adobe.edge.identity” couldn’t be removed." UserInfo={NSUserStringVariant=(
    Remove
), NSFilePath=/Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/Caches/com.adobe.edge.identity, NSUnderlyingError=0x600000ce5e00 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}
BUG IN CLIENT OF libsqlite3.dylib: database integrity compromised by API violation: vnode unlinked while in use: /Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/Caches/com.adobe.eventHistory
invalidated open fd: 16 (0x11)
Error removing cache item, with reason: Error Domain=NSCocoaErrorDomain Code=4 "“com.adobe.mobile.diskcache” couldn’t be removed." UserInfo={NSUserStringVariant=(
    Remove
), NSFilePath=/Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/Caches/com.adobe.mobile.diskcache, NSUnderlyingError=0x600000c18540 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}
Error removing cache item, with reason: Error Domain=NSCocoaErrorDomain Code=4 "“com.adobe.module.signal” couldn’t be removed." UserInfo={NSUserStringVariant=(
    Remove
), NSFilePath=/Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/Caches/com.adobe.module.signal, NSUnderlyingError=0x600000c185d0 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}
Successfully removed directory at /Users/timothyk/Library/Developer/CoreSimulator/Devices/0B7FD657-BB7E-46E5-93C3-7FA1645B09F7/data/Library/com.adobe.aep.datastore.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Allows test writers to completely clear all persistence locations in a single call, using default values for underlying methods

…g a default list of locations otherwise

kevinlind · 2024-01-31T23:00:48Z

Sources/FileManager+Testable.swift

+        for cacheItem in cacheItems {
            do {
-                try self.removeItem(at: URL(fileURLWithPath: "\(url.relativePath)/\(cacheItem)"))
+                try self.removeItem(at: URL(fileURLWithPath: "Library/Caches/\(cacheItem.name)", isDirectory: cacheItem.isDirectory))


Should this still be \(url.relativePath)/\(cacheItem) instead of hard coding to Library/Caches/?

Same question as Kevin.

Sorry I wasn't sure if Library/Caches/ was equivalent to url.relativePath as I was referring to the Core cache clearing helper logic here: https://github.com/adobe/aepsdk-core-ios/blob/d91de55cd052b1141a5d78d4a4d433fb486c8fac/AEPServices/Mocks/DatastoreUtilities.swift#L31

I've reverted the change back to use the relativePath value instead

kevinlind · 2024-01-31T23:17:57Z

Sources/FileManager+Testable.swift

+        for cacheItem in cacheItems {
            do {
-                try self.removeItem(at: URL(fileURLWithPath: "\(url.relativePath)/\(cacheItem)"))
+                try self.removeItem(at: URL(fileURLWithPath: "Library/Caches/\(cacheItem.name)", isDirectory: cacheItem.isDirectory))


Per your comment on the logged errors, is it possible to check if the directory exists before attempting to delete it so an error isn't logged?

Apple recommends against checking for file existing when predicating behavior: https://developer.apple.com/documentation/foundation/filemanager/1410277-fileexists

Attempting to predicate behavior based on the current state of the file system or a particular file on the file system is not recommended. Doing so can cause odd behavior or race conditions. It’s far better to attempt an operation (such as loading a file or creating a directory), check for errors, and handle those errors gracefully than it is to try to figure out ahead of time whether the operation will succeed. For more information on file-system race conditions, see Race Conditions and Secure File Operations in Secure Coding Guide.

Yes, saw this in the PR: adobe/aepsdk-core-ios#966, specifically:
adobe/aepsdk-core-ios#966 (comment)

So chose to emit an error log in this case, is this enough error handling or should we add more logic here?

kevinlind · 2024-01-31T23:19:59Z

Sources/NamedCollectionDataStore+Test.swift

nit: the file names sometimes use "+Test" and sometimes use "+Testable". Is there a rhyme or reason for this? I'm wondering if it would be more clear to use "+TestApis" to specify these files just add APIs for testing?

or +TestHelper

I think renaming the helper files that have the + (extensions) for consistency would be great!

@kevinlind are you ok with "+TestHelper"? I was thinking "+TestApis" could be interpreted as the file exposing or mocking existing APIs from the class for testing purposes - when it's actually providing helper methods that do testing setup/teardown etc?

kevinlind · 2024-01-31T23:35:50Z

Regarding question 2, ThreadSafeDictionary doesn't contain a clear method so each element must be removed individually. However, there is a keys API so you could loop through all the keys to remove all the elements. I don't see a reason not to clear the queue between tests.

cdhoffmann · 2024-02-05T20:27:22Z

Sources/FileManager+Testable.swift


        guard let directoryUrl = directoryUrl else {
-            Log.error(label: LOG_TAG, "Could not compute the directory URL for removal.")
+            Log.error(label: LOG_TAG, "Could not compute the directory URL for \(directoryName) for removal.")


Github isn't letting me mark the name of the function as the line. But, can we change the name of the function. Technically, this function isn't clearing the "cache", it is clearing the fileSystemStorage which is actually found in the Library directory (not cache). As far as the implementation, have you tested that this actually clears all of the directories? The implementation in the Core helper is different currently. See here.

What do you think of the name:
clearDirectory(_ name: String = "com.adobe.aep.datastore", inAppGroup appGroup: String? = nil)

Notes:

clear instead of remove to align with the other helper methods that have similar functionality of clearing peristence values

Since this method is an extension on FileManager, hopefully just clearDirectory is enough context without including fileSystem in the method name?

Please note that this will be a breaking change, so adopters of this change who have already migrated to use AEPTestUtils will have to update

As for the testing, that is something that was discussed - whether we want separate tests for the test utilities - but we decided against it in favor of having the extension test suites be the "tests" for the utilities working properly (for example, there are functional test cases in Edge that rely on the file system storage being cleared properly between test cases, otherwise they fail - which was used to validate this particular implementation)
What are your thoughts on this approach?

In terms of this particular file system removal implementation, the default args should follow the same logic as the current helper in Core, with the difference being that this one removes the directory altogether, and Core's removes each item in the directory, leaving the directory itself in place. However, this method also allows flexibility in terms of compatibility with app groups, so if extensions need to test that kind of environment the capability is available

Update usages

timkimadobe

Thanks so much for the reviews @kevinlind and @cdhoffmann! Updated based on feedback, please let me know what you think

timkimadobe · 2024-02-06T01:58:33Z

Sources/FileManager+Testable.swift

+        for cacheItem in cacheItems {
            do {
-                try self.removeItem(at: URL(fileURLWithPath: "\(url.relativePath)/\(cacheItem)"))
+                try self.removeItem(at: URL(fileURLWithPath: "Library/Caches/\(cacheItem.name)", isDirectory: cacheItem.isDirectory))


Sorry I wasn't sure if Library/Caches/ was equivalent to url.relativePath as I was referring to the Core cache clearing helper logic here: https://github.com/adobe/aepsdk-core-ios/blob/d91de55cd052b1141a5d78d4a4d433fb486c8fac/AEPServices/Mocks/DatastoreUtilities.swift#L31

I've reverted the change back to use the relativePath value instead

timkimadobe · 2024-02-06T02:01:16Z

Sources/FileManager+Testable.swift

+        for cacheItem in cacheItems {
            do {
-                try self.removeItem(at: URL(fileURLWithPath: "\(url.relativePath)/\(cacheItem)"))
+                try self.removeItem(at: URL(fileURLWithPath: "Library/Caches/\(cacheItem.name)", isDirectory: cacheItem.isDirectory))


Yes, saw this in the PR: adobe/aepsdk-core-ios#966, specifically:
adobe/aepsdk-core-ios#966 (comment)

So chose to emit an error log in this case, is this enough error handling or should we add more logic here?

timkimadobe · 2024-02-06T02:17:08Z

Sources/FileManager+Testable.swift


        guard let directoryUrl = directoryUrl else {
-            Log.error(label: LOG_TAG, "Could not compute the directory URL for removal.")
+            Log.error(label: LOG_TAG, "Could not compute the directory URL for \(directoryName) for removal.")


What do you think of the name:
clearDirectory(_ name: String = "com.adobe.aep.datastore", inAppGroup appGroup: String? = nil)

Notes:

clear instead of remove to align with the other helper methods that have similar functionality of clearing peristence values

Since this method is an extension on FileManager, hopefully just clearDirectory is enough context without including fileSystem in the method name?

Please note that this will be a breaking change, so adopters of this change who have already migrated to use AEPTestUtils will have to update

As for the testing, that is something that was discussed - whether we want separate tests for the test utilities - but we decided against it in favor of having the extension test suites be the "tests" for the utilities working properly (for example, there are functional test cases in Edge that rely on the file system storage being cleared properly between test cases, otherwise they fail - which was used to validate this particular implementation)
What are your thoughts on this approach?

In terms of this particular file system removal implementation, the default args should follow the same logic as the current helper in Core, with the difference being that this one removes the directory altogether, and Core's removes each item in the directory, leaving the directory itself in place. However, this method also allows flexibility in terms of compatibility with app groups, so if extensions need to test that kind of environment the capability is available

timkimadobe · 2024-02-06T02:26:03Z

Sources/NamedCollectionDataStore+Test.swift

I think renaming the helper files that have the + (extensions) for consistency would be great!

@kevinlind are you ok with "+TestHelper"? I was thinking "+TestApis" could be interpreted as the file exposing or mocking existing APIs from the class for testing purposes - when it's actually providing helper methods that do testing setup/teardown etc?

cdhoffmann

Looks great Tim. Thanks for resolving the comments.

timkimadobe added 2 commits January 30, 2024 17:28

Add new complete cache clearing API for NamedCollectionDataStore

0f5c659

Allows test writers to completely clear all persistence locations in a single call, using default values for underlying methods

Refactor clearCache to allow for passing custom cache locations, usin…

fd9bfa7

…g a default list of locations otherwise

timkimadobe requested a review from kevinlind January 31, 2024 01:44

Alphabetize filename order

6526117

timkimadobe requested a review from cdhoffmann January 31, 2024 01:47

timkimadobe added 3 commits January 30, 2024 18:00

Merge branch 'main' into update-cache-clearing

a606500

Update access level to public

e2943d3

Bugfix: double record in MockNetworkService

a7df77c

timkimadobe mentioned this pull request Jan 31, 2024

Refactor functional tests to use JSON comparison (part 2) adobe/aepsdk-edge-ios#438

Merged

10 tasks

kevinlind reviewed Jan 31, 2024

View reviewed changes

cdhoffmann requested changes Feb 5, 2024

View reviewed changes

timkimadobe added 4 commits February 5, 2024 17:57

Revert path construction change for clearing cache items

9fa1cd6

Rename removeAdobeCacheDirectory to clearDirectory and update args

52b2dca

Update usages

Rename extension file suffixes to +TestHelper

91a88a3

Update method docs to better reflect underlying logic

276d203

timkimadobe commented Feb 6, 2024

View reviewed changes

timkimadobe requested review from cdhoffmann and kevinlind February 6, 2024 02:38

cdhoffmann approved these changes Feb 6, 2024

View reviewed changes

timkimadobe merged commit dd373f5 into adobe:main Feb 7, 2024

timkimadobe deleted the update-cache-clearing branch February 7, 2024 05:26

Conversation

timkimadobe commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Questions for reviewers

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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!

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!

kevinlind commented Jan 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe 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!

cdhoffmann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timkimadobe commented Jan 31, 2024 •

edited

Loading