-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature/remote content hash #296
Open
Deco354
wants to merge
10
commits into
wordpress-mobile:trunk
Choose a base branch
from
Deco354:feature/remote-content-hash
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
066520a
Add SHAHasher
Deco354 7b38d49
Add contentHash to RemotePost
Deco354 240a524
Add RemotePost Hashing tests
Deco354 a3596bb
Add more detail to contentHash documentation
Deco354 9269015
Remove hash performance test
Deco354 cc6ada5
Add SHAHasher to WordPressKit framework header
Deco354 f795a27
Test autosaves don't alter hash
Deco354 ca291e0
Add documentation on hash exclusions & exceptions
Deco354 7dc25bb
Bump pod version number
Deco354 428ba3c
Rename sha256StringFromData
Deco354 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // | ||
| // SHAHasher.h | ||
| // WordPressKit | ||
| // | ||
| // Created by Declan McKenna on 20/10/2020. | ||
| // Copyright © 2020 Automattic Inc. All rights reserved. | ||
| // | ||
| #import <Foundation/Foundation.h> | ||
|
|
||
| @interface SHAHasher : NSObject | ||
| + (NSString *)combineHashes:(NSArray<NSData *>*) hashArray; | ||
| + (NSData *)hashForStringArray:(NSArray *) array; | ||
| + (NSData *)hashForString:(NSString *) string; | ||
| + (NSData *)hashForNSInteger:(NSInteger)integer; | ||
| + (NSData *)hashForDouble:(double)dbl; | ||
| + (NSData *)hashForBool:(BOOL)boolean; | ||
| + (NSString *)hexStringFromData:(NSData *)data; | ||
| @end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // | ||
| // SHAHasher.m | ||
| // WordPressKit | ||
| // | ||
| // Created by Declan McKenna on 20/10/2020. | ||
| // Copyright © 2020 Automattic Inc. All rights reserved. | ||
| // | ||
| #import "SHAHasher.h" | ||
| #import <Foundation/Foundation.h> | ||
| #import <CommonCrypto/CommonDigest.h> | ||
| @implementation SHAHasher | ||
|
|
||
| + (NSString *)combineHashes:(NSArray<NSData *>*) hashArray | ||
| { | ||
| NSMutableData *mutableData = [NSMutableData data]; | ||
| for (NSData *hash in hashArray) { | ||
| [mutableData appendData:hash]; | ||
| } | ||
|
|
||
| unsigned char finalDigest[CC_SHA256_DIGEST_LENGTH]; | ||
|
|
||
| CC_SHA256(mutableData.bytes, (CC_LONG)mutableData.length, finalDigest); | ||
|
|
||
| return [self hexStringFromData:[NSData dataWithBytes:finalDigest length:CC_SHA256_DIGEST_LENGTH]]; | ||
| } | ||
|
|
||
| + (NSData *)hashForStringArray:(NSArray *) array { | ||
| NSString *joinedArrayString = [array componentsJoinedByString:@""]; | ||
| return [self hashForString:joinedArrayString]; | ||
| } | ||
|
|
||
| + (NSData *)hashForString:(NSString *) string { | ||
| if (!string) { | ||
| return [[NSMutableData dataWithLength:CC_SHA256_DIGEST_LENGTH] copy]; | ||
| } | ||
|
|
||
| NSData *encodedBytes = [string dataUsingEncoding:NSUTF8StringEncoding]; | ||
| unsigned char digest[CC_SHA256_DIGEST_LENGTH]; | ||
|
|
||
| CC_SHA256(encodedBytes.bytes, (CC_LONG)encodedBytes.length, digest); | ||
|
|
||
| return [NSData dataWithBytes:digest length:CC_SHA256_DIGEST_LENGTH]; | ||
| } | ||
|
|
||
| + (NSData *)hashForNSInteger:(NSInteger)integer { | ||
| unsigned char digest[CC_SHA256_DIGEST_LENGTH]; | ||
|
|
||
| CC_SHA256(&integer, sizeof(integer), digest); | ||
|
|
||
| return [NSData dataWithBytes:digest length:CC_SHA256_DIGEST_LENGTH]; | ||
| } | ||
|
|
||
| + (NSData *)hashForDouble:(double)dbl { | ||
| unsigned char digest[CC_SHA256_DIGEST_LENGTH]; | ||
|
|
||
| CC_SHA256(&dbl, sizeof(dbl), digest); | ||
|
|
||
| return [NSData dataWithBytes:digest length:CC_SHA256_DIGEST_LENGTH]; | ||
| } | ||
|
|
||
| + (NSData *)hashForBool:(BOOL)boolean { | ||
| unsigned char digest[CC_SHA256_DIGEST_LENGTH]; | ||
|
|
||
| CC_SHA256(&boolean, sizeof(boolean), digest); | ||
|
|
||
| return [NSData dataWithBytes:digest length:CC_SHA256_DIGEST_LENGTH]; | ||
| } | ||
|
|
||
| + (NSString *)hexStringFromData:(NSData *)data { | ||
| NSMutableString *mutableString = [NSMutableString string]; | ||
|
|
||
| const char *hashBytes = [data bytes]; | ||
|
|
||
| for (int i = 0; i < data.length; i++) { | ||
| [mutableString appendFormat:@"%02.2hhx", hashBytes[i]]; | ||
| } | ||
|
|
||
| return [mutableString copy]; | ||
| } | ||
|
|
||
| @end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // | ||
| // RemotePostTests.swift | ||
| // WordPressKitTests | ||
| // | ||
| // Created by Declan McKenna on 19/10/2020. | ||
| // Copyright © 2020 Automattic Inc. All rights reserved. | ||
| // | ||
|
|
||
| import XCTest | ||
| @testable import WordPressKit | ||
|
|
||
| class RemotePostTests: XCTestCase { | ||
|
|
||
| func testHashWithNilValues() { | ||
| XCTAssertEqual(RemotePost().contentHash(), RemotePost().contentHash()) | ||
| } | ||
|
|
||
| func testHashWithNilValuesIsPersistent() { | ||
| let expectedHash = "72bcdd41f59ecd51f66ada667342a04765ff8f17997a4d48ea708e6eabbf5580" | ||
| XCTAssertEqual(RemotePost().contentHash(), expectedHash) | ||
| } | ||
|
|
||
| func testRemotePostHashIsSameForSameContent() { | ||
| let post = noNullPropertyPost | ||
| let identicalPost = noNullPropertyPost | ||
| XCTAssertEqual(post.contentHash(), identicalPost.contentHash()) | ||
| } | ||
|
|
||
| func testRemotePostHashDiffersForDifferentContent() { | ||
| let post = noNullPropertyPost | ||
| let modifiedPost = noNullPropertyPost | ||
| modifiedPost.tags.append("new tag") | ||
| XCTAssertNotEqual(post.contentHash(), modifiedPost.contentHash()) | ||
| } | ||
|
|
||
| func testRemotePostHashIsPersistent() { | ||
| let post = noNullPropertyPost | ||
| let expectedHash = "729a3df7c916699c5efb548dc4f53f43dec11d5516dd63ff6787c81904d464f1" | ||
| XCTAssertEqual(post.contentHash(), expectedHash) | ||
| } | ||
|
|
||
| func testAutosaveDoesntAlterHash() { | ||
| let post = RemotePost() | ||
| let hash = RemotePost().contentHash() | ||
| post.autosave = RemotePostAutosave() | ||
| let autosavedPostHash = post.contentHash() | ||
| XCTAssertEqual(hash, autosavedPostHash) | ||
| } | ||
| } | ||
|
|
||
| private extension RemotePostTests { | ||
| /// Remote post with all properties set to non null to ensure hash is consistent for all fields | ||
| var noNullPropertyPost: RemotePost { | ||
| let remotePost = RemotePost() | ||
| remotePost.postID = 90210 | ||
| remotePost.siteID = 101 | ||
| remotePost.authorAvatarURL = "www.test.com" | ||
| remotePost.authorDisplayName = "jk" | ||
| remotePost.authorEmail = "[email protected]" | ||
| remotePost.authorURL = "swiftdec.com" | ||
| remotePost.authorID = 9001 | ||
| remotePost.date = Date(timeIntervalSince1970: 0) | ||
| remotePost.title = "Lorem Ipsum" | ||
| remotePost.url = URL(string: "lemonparty.com") | ||
| remotePost.shortURL = URL(string: "www.why.com") | ||
| remotePost.content = "Dolor Sit Amet" | ||
| remotePost.excerpt = "...." | ||
| remotePost.slug = "lorem-ipsum" | ||
| remotePost.suggestedSlug = "~!!" | ||
| remotePost.status = "draft" | ||
| remotePost.parentID = 42 | ||
| remotePost.postThumbnailID = 420 | ||
| remotePost.postThumbnailPath = "Arakis" | ||
| remotePost.type = "" | ||
| remotePost.format = "" | ||
| remotePost.commentCount = 666 | ||
| remotePost.likeCount = 555 | ||
| remotePost.tags = ["lorem,ipsum,test"] | ||
| remotePost.pathForDisplayImage = "!.com" | ||
| remotePost.isStickyPost = true | ||
| remotePost.isFeaturedImageChanged = false | ||
| return remotePost | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we have support to hash each component in the methods below, and then combine the hash of each component (through
combineHashes), is there a reason behind the choice of using a big string concatenating all of the post elements?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance mostly.
Hashing every property will likely minimize collisions but it took ~ 0.1s when I performance tested this. I fear this could result in a significant delay when dealing with many posts.
Property hash performance
For an
AbstractPostupload I don't think this is a problem as a post upload isn't an action that blocks the user. However for downloading aRemotePostupdate this could result in real user delays as the non-arrival of this update will block them from continued use of the app.ConcatenatedStringHash Performance
After concatenating the strings we're down to < 0.01s
I'm still a little bit concerned that this has the potential to be a bit too slow if we're going to be checking large numbers of posts each update. The swift algorithm I wrote earlier this month just performs a bitwise shift operation, this would be much faster than the others if it prove necessary.
Let me know your thoughts, perhaps I've missed something somewhere.
How are posts currently downloaded when in large numbers? Do we download them in batches?