Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify cache to not read file modification date #969

Merged

Conversation

praveek
Copy link
Contributor

@praveek praveek commented Sep 22, 2023

  • Modify CacheService to stop using fileModificationDate attribute to store cacheExpiry.
  • Clean up logs in FileSystemNamedCollection.

This change will cause a one time cache miss for currently cached content.

Fixed IdentityIntegrationTests .testGetExperienceCloudIdWithinPermissibleTimeOnInstall - #954

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #969 (a9c689f) into migrateOffUserDefaults (b431e5a) will decrease coverage by 0.29%.
The diff coverage is 87.50%.

@@                    Coverage Diff                     @@
##           migrateOffUserDefaults     #969      +/-   ##
==========================================================
- Coverage                   90.07%   89.78%   -0.29%     
==========================================================
  Files                         133      133              
  Lines                        8321     8330       +9     
==========================================================
- Hits                         7495     7479      -16     
- Misses                        826      851      +25     

Copy link
Member

@sbenedicadb sbenedicadb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple small things - take them or leave them

@@ -49,7 +49,7 @@ class FileSystemNamedCollection: NamedCollectionProcessing {
do {
try updatedStorageData.write(to: fileUrl, options: .atomic)
} catch {
Log.warning(label: self.LOG_TAG, "Error when writing to file: \(error)")
Log.warning(label: self.LOG_TAG, "Error \(error) when writing \(key) to collection \(collectionName)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would vote to put our dynamic content in single quotes so it's clear to the reader exactly where the error, key, and collection name begin and end. should we also announce key (e.g. - ...when writing key '\(key)') like we are for the other variables and in the subsequent log?

Comment on lines 95 to 104
guard let storageData = try? Data(contentsOf: fileUrl) else {
return nil
}

guard let jsonResult = try? JSONSerialization.jsonObject(with: storageData) as? [String: Any] else {
Log.warning(label: LOG_TAG, "Failed to read dictionary from collection \(collectionName)")
return nil
}
Log.warning(label: LOG_TAG, "Failed to get Dictionary from data store")
return nil

return jsonResult
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return jsonResult
}
guard let storageData = try? Data(contentsOf: fileUrl) else {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we provide a message here indicating we were unable to retrieve the file contents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a lot of false warnings during a new install.

}

// As date stores the timestamp in milliseconds, compare double value with accuracy greater than milliseconds.
let accuracy = 0.00001;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this just be 0.001? my two cents is we could make it even a little more forgiving than this so we don't have random failures with this equality check

@praveek praveek merged commit 0fe0e1e into adobe:migrateOffUserDefaults Oct 2, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants