Fix delta sync bug #263

royjit · 2019-06-06T23:17:06Z

Delta sync was using wrong hash to store the data in the local cache. This caused
delta sync to fail intermittently.

Issue #:
#232

Description of changes:
Hash calculation used in delta calculation was wrong and was producing different results on the same input. This was caused due to the unordered representation of swift Dictionary object which we used for creating hash. This PR addresses this issue by sorting the dictionary content and then using it for hash calculation.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Delta sync was using wrong hash to store the data in the local cache. This caused delta sync to fail intermittently.

CHANGELOG.md

Co-Authored-By: Rohan Dubal <[email protected]>

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

palpatim · 2019-06-10T23:49:02Z

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

+    }
+
+    func testWithAllValuesFilled() {
+        let subscriptionWithSync = AppSyncSubscriptionWithSync<OnUpvotePostSubscription, ListPostsQuery, ListPostsQuery>(appSyncClient: appsyncClient!,


nit: Here and throughout: please start arguments on the next line after the opening parens to fix indent, or reformat the generic parameters to line these up. As it stands, the actual method call and arguments are off the screen :)

Fixed the indentation, removed the generics

palpatim · 2019-06-10T23:51:21Z

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

+
+class AppSyncSubscriptionWithSyncTests: XCTestCase {
+
+    var appsyncClient: AWSAppSyncClient?


You could make this an implicitly unwrapped optional (var appsyncClient: AWSAppSyncClient!) to avoid constantly force unwrapping in the body of your tests.

Reasoning:

You can't do anything meaningful without the client, so a crash immediately fails the tests instead of getting into an unexpected state (as for example if you had an optional chain or a guard that simply exited early without asserting anything)

You have a proper try/catch block in setUp to catch the setup failures immediately

palpatim · 2019-06-10T23:52:50Z

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

+    }
+}
+
+


Nit, remove extra lines

palpatim · 2019-06-10T23:57:00Z

AWSAppSyncClient.xcodeproj/project.pbxproj

@@ -133,6 +133,7 @@
 		70C68E4D132FE62623DB8C07 /* Pods_AWSAppSyncTestHostApp.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 8C707001F57B091A8A001CAB /* Pods_AWSAppSyncTestHostApp.framework */; };
 		8032C5415EF414C038394D69 /* Pods_AWSAppSyncTestCommon.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 74071C397A83DEA980BB2F4C /* Pods_AWSAppSyncTestCommon.framework */; };
 		A70604C0C722923A70C937A1 /* Pods_AWSAppSyncTestApp.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 57F5A94352E1ABE35159489D /* Pods_AWSAppSyncTestApp.framework */; };
+		B48056BC22A992E200E4F742 /* AppSyncSubscriptionWithSyncTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B48056BB22A992E200E4F742 /* AppSyncSubscriptionWithSyncTests.swift */; };


nit: sort the folder by name

palpatim · 2019-06-11T00:00:19Z

CHANGELOG.md

+### Bug Fixes
+
+* Fix a bug where delta sync was not correctly storing/ retrieving the `lastSyncTime`. See [issue #232](https://github.com/awslabs/aws-mobile-appsync-sdk-ios/issues/232)
+* **Breaking API Change** To fix the delta sync logic, there was a change in the hashing function used internally. This change can cause the existing app to ignore the cache for the first sync and fetch data using base query. Subsequent sync operation should work as normal.


What happens to any existing cache data? Will it be re-used, overwritten, ignored and left to grow stale?

Existing cache data will be ignored.

palpatim · 2019-06-11T00:01:03Z

AWSAppSyncClient/Internal/AppSyncSubscriptionWithSync.swift

-
+        let variables = baseQuery.variables?.sorted(by: { $0.0 < $1.0 }).description ?? ""
+        baseString += type(of: baseQuery).requestString + variables
+        


nit, remove trailing whitespace

palpatim · 2019-06-11T00:01:16Z

AWSAppSyncClient/Internal/AppSyncSubscriptionWithSync.swift

        if let subscription = subscription {
-            let variables = subscription.variables?.description ?? ""
-            baseString = type(of: subscription).requestString + variables
+            let variables = subscription.variables?.sorted(by: { $0.0 < $1.0 }).description  ?? ""


nit: spacing

Fixed spacing

palpatim · 2019-06-11T00:03:48Z

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

+    }
+
+
+    private class GraphGetQuery: GraphQLQuery {


No need for this to be namespaced to the test, just extract to a private class in the same file

Moved outside

palpatim · 2019-06-11T00:05:19Z

AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift

+        XCTAssertEqual(result1, result2, "Hash value should be equal for the two sync operation")
+
+    }
+


Please add a test that ensures that cases that specify the same operation for both baseQuery and deltaQuery work as expected.

Fix delta syn bugc

ccdd708

Delta sync was using wrong hash to store the data in the local cache. This caused delta sync to fail intermittently.

rohandubal requested review from palpatim and rohandubal June 6, 2019 23:22

rohandubal added AppSync contribution Community contribution PRs labels Jun 6, 2019

rohandubal reviewed Jun 6, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

rohandubal reviewed Jun 6, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

royjit and others added 2 commits June 6, 2019 16:34

Typo fix - Update CHANGELOG.md

869402a

Co-Authored-By: Rohan Dubal <[email protected]>

Fix typo Update CHANGELOG.md

0c36fc2

Co-Authored-By: Rohan Dubal <[email protected]>

palpatim requested changes Jun 11, 2019

View reviewed changes

Fixes as per PR review

4909612

royjit requested review from palpatim and rohandubal June 11, 2019 19:09

palpatim approved these changes Jun 13, 2019

View reviewed changes

rohandubal approved these changes Jun 20, 2019

View reviewed changes

palpatim merged commit b2783ba into awslabs:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delta sync bug #263

Fix delta sync bug #263

royjit commented Jun 6, 2019 •

edited by rohandubal

Loading

palpatim Jun 10, 2019

royjit Jun 11, 2019

palpatim Jun 10, 2019

royjit Jun 11, 2019

palpatim Jun 10, 2019

royjit Jun 11, 2019

palpatim Jun 10, 2019

royjit Jun 11, 2019

palpatim Jun 11, 2019

royjit Jun 11, 2019

palpatim Jun 11, 2019

royjit Jun 11, 2019

palpatim Jun 11, 2019

royjit Jun 11, 2019

palpatim Jun 11, 2019

royjit Jun 11, 2019

palpatim Jun 11, 2019

royjit Jun 11, 2019


		class AppSyncSubscriptionWithSyncTests: XCTestCase {

		var appsyncClient: AWSAppSyncClient?


		let variables = baseQuery.variables?.sorted(by: { $0.0 < $1.0 }).description ?? ""
		baseString += type(of: baseQuery).requestString + variables

		XCTAssertEqual(result1, result2, "Hash value should be equal for the two sync operation")

		}

Fix delta sync bug #263

Fix delta sync bug #263

Conversation

royjit commented Jun 6, 2019 • edited by rohandubal Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royjit commented Jun 6, 2019 •

edited by rohandubal

Loading