SQLite persistent normalized cache #79

kompfner · 2017-04-11T07:02:47Z

This PR does the following:

Introduces SqliteNormalizedCache, which persists records in a SQLite database.
Introduces the SqliteNormalizedCache subspec, which is necessary for two reasons:
- It allows consumers of the Apollo pod to decide whether or not they need SQLite persistence for their application - if they don’t need it, they don’t need to take a dependency on SQLite.swift.
- It disables SqliteNormalizedCache usage on tvOS projects, which don’t have access to the file system.
Shares existing unit tests to test InMemoryNormalizedCache and SqliteNormalizedCache.
Adds a unit test verifying that cached data is persisted in SqliteNormalizedCache.

Known remaining work before this PR can be merged:

Properly handle versioning both Apollo.xcodeproj and ApolloSQLite.xcodeproj
Naming: "Sql" --> "SQL"
In loadRecords(forKeys:), implement correct expected behavior of returning nil for keys that don't have corresponding records (instead of the misunderstood expected behavior described in the comment)

Next steps:

Clear cached records when Apollo version changes.

apollo-cla · 2017-04-11T07:02:47Z

@kompfner: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

…ill depend on Sqlite pod)

…framework directory by the time StarWarsAPI generate API build phase is run

…loads

…l] instead of []. This is an odd behavior, but is what is currently required by ApolloStore. This should probably be revisited.

…“reference”)

…sqlite caches

…ume a specific type of cache

…is cached in a persistent way

…ached by the time the fetch’s completion block was called

justinmakaila · 2017-04-19T23:48:55Z

But SQLite isn't lightweight. It requires a new dependency, new xcodeproj, and adds more overhead if you're not a CocoaPods user. Lunar doesn't. It literally just links CoreData. Not to mention it pretty much is this SQLiteNormalizedCache, just with DB transactions expressed in CoreData logic, not SQLite.

If you want to use Apollo and Lunar, you add this to your Cartfile:

github "justinmakaila/Lunar"

It will clone Lunar, clone Apollo, and compile both to .frameworks for you. You then can link both of them as necessary. Sure, when you initialize your ApolloStore, you have to pass it a LunarCache, but that's not even painful. It literally removes the black magic of Apollo just using some "Magic Cache" that you don't have access to.

(Side note: I initially brought this up and started development on Lunar because I wanted to use our already existing model framework. After exploring the code shared here, I realized that I don't need that dependency anymore, because Lunar now has a single entity, LunarRecord, which provides two properties: "key" and "record".)

I'm not happy, as a contributor, a maintainer of my own cache (that literally does the same thing without the dependency), and more specifically, as a Carthage user, that we're trending towards including something because it was PR'd first and is easy to maintain if you use CocoaPods.

My dependency build time is going to go up because I now have to build Apollo, Lunar, and SQLite (which I don't use), even though I cut my local dependencies out (no more model or sync frameworks).

I share the general sentiment here that we should be able to get people up and running with persistence quickly, and that's why I'm still proposing that we just provide install instructions with how to include whatever specific community-maintained cache. It's really not beyond our audience to have to make some choices on their own backend and maybe, if there's no community support, help support the community and write some code.

justinmakaila · 2017-04-19T23:50:54Z

Apollo's beauty isn't even in it's caching, nor should it concern itself with persistence. Apollo's beauty is that it was a lightweight interface that could live on top of your network and persistence layer without being opinionated about either.

This takes a step away from that, and starts to add weight and overhead, absolutely.

justinmakaila · 2017-04-19T23:53:33Z

At this point, I've said all there is to say. If you guys are set on merging this in and forcing SQLite on Carthage users, than there's nothing I can do. I'm just trying to share what I've learned in working with Moya and what I'm trying to use Apollo for. SQLite has nothing to do with Apollo, IMO, and should not be included in the repository.

…ping JSONSerializationFormat is meant to convert from/to GraphQL types, but the fields in a record already contain JSON values or Reference.

martijnwalraven · 2017-04-20T10:40:48Z

@kompfner: I cleaned up some code related to promises and the fields serialization. We no longer need to special case NS types now. I tried to clarify typing by adding some typealiases to Record, but I’m not sure if this actually makes things more or less readable. Definitely something we should also document. Anyway, the rest of the code looks pretty simple, and all tests seem to pass, so I think we’re ready to merge (squash)!

…r fields

kompfner · 2017-04-20T16:59:14Z

ApolloSQLite/SQLiteNormalizedCache.swift

-        reject(error)
-      }
-    }
+    return Promise { try mergeRecords(records: records) }


Oh, sweet, your Promise implementation automatically turns thrown errors into rejections. 👍

kompfner · 2017-04-20T17:02:59Z

Apollo/JSONStandardTypeConversions.swift

@@ -117,14 +117,13 @@ extension Dictionary: JSONEncodable {
  public var jsonObject: JSONObject {
    var jsonObject = JSONObject(minimumCapacity: count)
    for (key, value) in self {
+      print("key: \(key), \(type(of: key))")


Should these be removed?

kompfner · 2017-04-20T17:10:59Z

ApolloSQLite/SQLiteNormalizedCache.swift

-    let recordJSON = try SQLiteSerialization.deserialize(data: recordData)
-    return Record(key: row[key], recordJSON)
+    let fields = try SQLiteSerialization.deserialize(data: recordData)
+    return Record(key: row[key], fields)
  }
 }

 private let serializedReferenceKey = "reference"

 final class SQLiteSerialization {


Hm, I think this should probably be private.

kompfner · 2017-04-20T17:12:47Z

ApolloSQLite/SQLiteNormalizedCache.swift

  }

-  private static func deserialize(valueJSON: Any) throws -> Any {
-    switch valueJSON {
+  private static func deserialize(jsonValue: JSONValue) throws -> Record.Value {


How about this for the naming of the serialize and deserialize method parameters, for even greater clarity?

serialize(fieldValue:)

serialize(fieldJSON:)

kompfner · 2017-04-20T17:14:34Z

ApolloSQLite/SQLiteNormalizedCache.swift

    var objectToSerialize = JSONObject()
    for (key, value) in fields {
      objectToSerialize[key] = try serialize(value: value)
    }
-    return try JSONSerializationFormat.serialize(value: objectToSerialize)


The cleanup in this commit is great! Perhaps JSONSerializationFormat should have a more descriptive name, to make its purpose more distinct than the system-provided JSONSerialization?

kompfner · 2017-04-20T17:18:20Z

ApolloSQLite/SQLiteNormalizedCache.swift

@@ -88,7 +88,7 @@ public final class SQLiteNormalizedCache: NormalizedCache {
  }
 }

-private let serializedReferenceKey = "reference"
+private let serializedReferenceKey = "$reference"


Is this really necessary? What distinguishes a reference field from any other field is that it's a dictionary (of the form [reference: <key>]), whereas other fields are primitive types.

Yeah, you're right, I wasn't thinking clearly, there are no nested objects in records of course. Although we may want to leave open the possibility of storing JSON in a field if something like that becomes part of the GraphQL spec? May be too forward looking.

kompfner · 2017-04-20T17:22:24Z

@martijnwalraven I'm going to do a tiny bit more cleanup based on the comments I left on your last few commits. Please let me know if you agree with those. Those commits were really nice improvements, btw!

…id CI-only crash

… debuggin CI-only crash

kompfner · 2017-04-21T19:52:27Z

I think the reason the ApolloSQLite unit tests were failing was due to race conditions around multiple tests attempting to open the same file on disk. My fix was to have each test open a SQLite file at a different file URL.

…rpose of debuggin CI-only crash" This reverts commit 28feedf.

…eem to be available on Travis

kompfner · 2017-04-21T19:55:08Z

Disabling iOS 9.3 unit tests, since that simulator doesn't seem to be an available option on Travis right now.

kompfner force-pushed the sqlite-normalized-cache branch from fc82f3e to 961477e Compare April 11, 2017 21:56

Paul Kompfner added 28 commits April 11, 2017 17:22

Placeholder SqliteNormalizedCache

ddee83a

Add SQLite.swift dependency to Apollo.podspec

55e8b8d

Create Example project to test Cocoapods installation

647884e

Move tests from Apollo project to ApolloExample project (since they w…

3a094a1

…ill depend on Sqlite pod)

Workaround to tests not running due to Apollo.framework not being in …

6384fb4

…framework directory by the time StarWarsAPI generate API build phase is run

Crude implementation of SqliteNormalizedCache

d4349f5

More progress on crude implementation of SqliteNormalizedCache

e34dd89

Temporary workaround for JSON type conversion bug

9f8b5fb

Replace temporary workaround with better fix

135ac3f

Remove in-memory initializer from SqliteNormalizedCache

76cdf91

Avoid duplicating overwriting updated records in sqlite normalized cache

9680d22

Better organize code in SqliteNormalizedCache merge method

8cba5f5

Fix unit tests by setting NSAppTransportSecurity to enable arbitrary …

829157d

…loads

SqliteNormalizedCache fulfills loadRecords(forKeys:) promise with [ni…

deeab16

…l] instead of []. This is an odd behavior, but is what is currently required by ApolloStore. This should probably be revisited.

Store record references in SQLite as JSON objects with a single key (…

6f5192b

…“reference”)

Update FetchQueryTests to use both in-memory and sqlite caches

43b2cb5

Update LoadQueryFromStoreTests to use both in-memory and sqlite caches

1ae3e37

Update StarWarsServerCachingRoundtripTests to use both in-memory and …

6d9ef48

…sqlite caches

Update StarWarsServerTests to use both in-memory and sqlite caches

abaa650

Update StoreTransactionTests to use both in-memory and sqlite caches

589096f

Update WatchQueryTests to use both in-memory and sqlite caches

20fa038

Remove ApolloClient and ApolloStore convenience initializers that ass…

26a2625

…ume a specific type of cache

Add a test to CachePersistenceTests, verifying that a query response …

3e7a0ee

…is cached in a persistent way

Add SqliteNormalizedCache subspec (which is not supported on tvos)

5245bf8

Make “ApolloExample” and “TestHost iOS” shared schemes

da1f9e7

Move SqliteNormalizedCache file into subspec

b802c7b

Fix persistence unit test. The problem was that data wasn’t already c…

ae5b4c0

…ached by the time the fetch’s completion block was called

Fix Apollo.xcodeproj building (without sqlite cache)

35fed37

Better organize directory structure

92fb642

martijnwalraven added 3 commits April 20, 2017 11:09

Replace TestHost iOS scheme with ApolloPerformanceTests

31ad83d

Clean up Promise construction in SQLiteNormalizedCache

e5dac42

Avoid JSONSerializationFormat in SQLiteNormalizedCache and clarify ty…

1f344e0

…ping JSONSerializationFormat is meant to convert from/to GraphQL types, but the fields in a record already contain JSON values or Reference.

martijnwalraven added 2 commits April 20, 2017 13:07

Use $reference when serializing Reference to avoid conflicts with use…

eb67621

…r fields

Add ApolloSQLite jobs to .travis.yml

6ee21ba

kompfner commented Apr 20, 2017

View reviewed changes

Minor cleanup and clarity fixes

9f9d17f

kompfner force-pushed the sqlite-normalized-cache branch from f70b390 to 9f9d17f Compare April 20, 2017 18:17

Paul Kompfner added 5 commits April 21, 2017 10:05

Write SQLite database to recommended MacOS directory to hopefully avo…

c029a48

…id CI-only crash

Temporarily disable all tests except ApolloSQLite, for the purpose of…

28feedf

… debuggin CI-only crash

Keep trying things to prevent CI crash

76de545

Keep trying things to prevent CI crash

cab7b64

Keep trying things to prevent CI crash

95cd1af

Paul Kompfner added 2 commits April 21, 2017 12:53

Revert "Temporarily disable all tests except ApolloSQLite, for the pu…

5e4e3b5

…rpose of debuggin CI-only crash" This reverts commit 28feedf.

Disable unit tests on iOS 9.3 for now, since that simulator doesn’t s…

998c1d2

…eem to be available on Travis

kompfner merged commit b3cf1c6 into apollographql:master Apr 24, 2017

ttrznadel mentioned this pull request Jun 24, 2020

"Optional is only JSONEncodable if Wrapped is" fatal error occurs on Optional #1271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQLite persistent normalized cache #79

SQLite persistent normalized cache #79

kompfner commented Apr 11, 2017 •

edited

Loading

apollo-cla commented Apr 11, 2017

justinmakaila commented Apr 19, 2017

justinmakaila commented Apr 19, 2017 •

edited

Loading

justinmakaila commented Apr 19, 2017 •

edited

Loading

martijnwalraven commented Apr 20, 2017

kompfner Apr 20, 2017

kompfner Apr 20, 2017

kompfner Apr 20, 2017

kompfner Apr 20, 2017

kompfner Apr 20, 2017

kompfner Apr 20, 2017

martijnwalraven Apr 21, 2017

kompfner commented Apr 20, 2017

kompfner commented Apr 21, 2017 •

edited

Loading

kompfner commented Apr 21, 2017 •

edited

Loading

SQLite persistent normalized cache #79

SQLite persistent normalized cache #79

Conversation

kompfner commented Apr 11, 2017 • edited Loading

apollo-cla commented Apr 11, 2017

justinmakaila commented Apr 19, 2017

justinmakaila commented Apr 19, 2017 • edited Loading

justinmakaila commented Apr 19, 2017 • edited Loading

martijnwalraven commented Apr 20, 2017

kompfner Apr 20, 2017

Choose a reason for hiding this comment

kompfner Apr 20, 2017

Choose a reason for hiding this comment

kompfner Apr 20, 2017

Choose a reason for hiding this comment

kompfner Apr 20, 2017

Choose a reason for hiding this comment

kompfner Apr 20, 2017

Choose a reason for hiding this comment

kompfner Apr 20, 2017

Choose a reason for hiding this comment

martijnwalraven Apr 21, 2017

Choose a reason for hiding this comment

kompfner commented Apr 20, 2017

kompfner commented Apr 21, 2017 • edited Loading

kompfner commented Apr 21, 2017 • edited Loading

kompfner commented Apr 11, 2017 •

edited

Loading

justinmakaila commented Apr 19, 2017 •

edited

Loading

justinmakaila commented Apr 19, 2017 •

edited

Loading

kompfner commented Apr 21, 2017 •

edited

Loading

kompfner commented Apr 21, 2017 •

edited

Loading