Add Normalized Cache structure and Response normalization #260

BenSchwab · 2017-03-01T06:04:04Z

This PR will focus on the general structure of the normalized cache, and response normalization (writing to the cache). Reading from the cache is not done.

Some change summaries:

Give BufferedResponseReader a ResponseReaderShadow to allow normalization while reading network response
GraphQLResponseNormalizer contains the meat of the normalization construction process, which mostly matches iOS implementation
Temporarily use unbounded in memory cache InMemoryNormalizedCacheStore (but may prove useful as an intermediate store)
NormalizedCacheStore allows user to plug in how cached data is stored
Set up rough structure of NormalizedCache

Closes 258

BenSchwab · 2017-03-01T06:13:33Z

@sav007 @digitalbuddha

Still working on writing tests, cleaning things up etc. But let me know any early thoughts/concerns.

This should roughly set the structure of the normalized cache (similar pattern to network cache), and I will be able to break out a lot of smaller tasks including:

Preparing ApolloRecord for easy disk/serialized cache support
Adding reading from NormalizedCache via interceptor
Cache expiration, fetch strategy
Query watching

digitalbuddha · 2017-03-01T14:49:35Z

apollo-integration/src/main/AndroidManifest.xml

@@ -0,0 +1,7 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.ben_schwab.topshelf.apollointegration">
+
+  <application android:allowBackup="true" android:label="@string/app_name" android:supportsRtl="true">


my understanding of android:allowBackup flag is that if any manifest that is being merged has that flag set then the client app will have it no matter what their main manifest says. Our internal policy is to have libraries define android:allowBackup="false" so that client apps can still overwrite it.

oops, yeah I have been burnt by this before

digitalbuddha · 2017-03-01T14:50:17Z

...tion/src/test/graphql/com/apollographql/android/impl/normalizer/EpisodeHeroNameResponse.json

+      "name": "R2-D2"
+    }
+  }
+}


missing newline eof, checkstyle might have trouble.

digitalbuddha · 2017-03-01T14:50:52Z

...integration/src/test/graphql/com/apollographql/android/impl/normalizer/HeroNameResponse.json

@@ -0,0 +1,7 @@
+{


seems identical to EpisodeHeroNameResponse.json confirming that both are needed

It is identical, just thought for organizational purposes 1 graphql query = 1 graphql response of the same name.

digitalbuddha · 2017-03-01T14:51:15Z

apollo-integration/src/test/java/com/apollographql/android/impl/ResponseNormalizationTest.java

+
+  private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'", Locale.US);
+
+  ApolloClient apolloClient;


any reason not to make fields private?

digitalbuddha · 2017-03-01T14:53:06Z

apollo-integration/src/test/java/com/apollographql/android/impl/ResponseNormalizationTest.java

+        .serverUrl(server.url("/"))
+        .okHttpClient(okHttpClient)
+        .normalizedCache(cacheStore, new CacheKeyResolver() {
+          @Nullable @Override public String cacheKeyForJsonObject(Map<String, Object> jsonObject) {


nitpick: method should be named resolve unless there are other variants of how the resolver can work

digitalbuddha · 2017-03-01T14:53:57Z

apollo-integration/src/test/java/com/apollographql/android/impl/ResponseNormalizationTest.java

+    Response<HeroName.Data> body = call.execute();
+    assertThat(body.isSuccessful()).isTrue();
+
+    ApolloRecord record = cacheStore.read("QUERY_ROOT");


lets move the next few strings into private constants

digitalbuddha · 2017-03-01T14:54:50Z

apollo-integration/src/test/java/com/apollographql/android/impl/ResponseNormalizationTest.java

+  }
+
+
+  //  private static String readNormalizerJsonFile(String name) throws IOException {


digitalbuddha · 2017-03-01T14:55:53Z

apollo-runtime/src/main/java/com/apollographql/android/cache/ResponseReaderShadow.java

+
+  void didParseObject(Map<String, Object> objectMap);
+
+  void didParseList(List array);


willParseList not necessary?

iOS only uses willParseList to allocate more space on their "stack" data structure. I'm not aware of any similar pattern in Java (besides setting the size when a collection is first initialized)

digitalbuddha · 2017-03-01T14:57:19Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/ApolloRecord.java

+import java.util.HashMap;
+import java.util.Map;
+
+//Todo: Enhance this class to better support generalized serialization ISSUE____


reminder to create issue :-)

digitalbuddha · 2017-03-01T14:57:36Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/ApolloRecord.java

+
+  public final String key;
+
+  //Todo: provide serialized wrapper?


same, imo every TODO needs issue for tracking.

digitalbuddha · 2017-03-01T14:58:13Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/ApolloRecord.java

+//Todo: Enhance this class to better support generalized serialization ISSUE____
+public class ApolloRecord {
+
+  public final String key;


does key need to be public? any reason not to have key() and keep the field private?

digitalbuddha · 2017-03-01T14:58:53Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/CacheKeyResolver.java

+import javax.annotation.Nullable;
+
+public interface CacheKeyResolver {
+  @Nullable String cacheKeyForJsonObject(Map<String, Object> jsonObject);


@nonnull on param?

digitalbuddha · 2017-03-01T15:01:33Z

...e/src/main/java/com/apollographql/android/cache/normalized/InMemoryNormalizedCacheStore.java

+
+public class InMemoryNormalizedCacheStore extends NormalizedCacheStore {
+
+  private final Map<String, ApolloRecord> store = new HashMap<>();


nitpick and sorta opinion to use LinkedHashMap https://publicobject.com/2016/02/08/linkedhashmap-is-always-better-than-hashmap/ also do we need to worry about threading here?

digitalbuddha · 2017-03-01T15:02:15Z

...e/src/main/java/com/apollographql/android/cache/normalized/InMemoryNormalizedCacheStore.java

+    }
+  }
+
+  public Collection<ApolloRecord> getAllRecords() {


unless there is a setAllRecords I don't think the get prefix is necessary (similar to how its store.values() not store.getValues())

digitalbuddha

nothing major, just did some human linting. Looks good overall! great job!!

sav007 · 2017-03-01T15:39:39Z

apollo-runtime/src/main/java/com/apollographql/android/cache/ResponseReaderShadow.java

+import java.util.List;
+import java.util.Map;
+
+public interface ResponseReaderShadow {


Should we hide this to package protected?

sav007 · 2017-03-01T15:40:14Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/ApolloRecord.java

+import java.util.Map;
+
+//Todo: Enhance this class to better support generalized serialization ISSUE____
+public class ApolloRecord {


package protected, final ?

sav007 · 2017-03-01T15:48:08Z

General comments:

should we drop prefix normalized and just use cache
by default make all classes as closed as possible (make final, package protected etc)

sav007 · 2017-03-01T16:50:14Z

...time/src/main/java/com/apollographql/android/cache/normalized/GraphQLResponseNormalizer.java

+
+public class GraphQLResponseNormalizer implements ResponseReaderShadow {
+
+  private Stack<List<String>> pathStack;


Stack is based on Vector that is synchronized. Do we really need synchronization? Question about performance, should we replace with light weight impl of stack structure?

Don't need synchronization here. Any suggestions to a lighter stack impl?

I was thinking most of custom structure that behaves as stack, but we can use ArrayDeque

Tried ArrayDeque, but it does not support nulls. So rolled our own "SimpleStack"

sav007 · 2017-03-01T16:51:49Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/NormalizedCache.java

+    this.cacheKeyResolver = cacheKeyResolver;
+  }
+
+  public NormalizedCacheStore getCacheStore() {


nit: We are avoiding of get so cacheStore()

sav007 · 2017-03-01T16:52:09Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/NormalizedCache.java

+    return cacheKeyResolver;
+  }
+
+  public GraphQLResponseNormalizer createResponseNormalizer() {


nit: responseNormalizer()

sav007 · 2017-03-01T16:54:58Z

...time/src/main/java/com/apollographql/android/cache/normalized/GraphQLResponseNormalizer.java

+  private List<String> path;
+  private ApolloRecord currentRecord;
+
+  private InMemoryNormalizedCacheStore recordSet;


Shouldn't it be generic NormalizedCacheStore ?

I wasn't sure if I wanted to add allRecords to the generic interface, which is necessary for the NormalizedCacheStore. This is really more of a per-request store that will be thrown out once parsing is done (and the results added to the aggregate store the user supplies)

Replaced with the concept of a RecordSet which is more inline with the iOS implementation

sav007 · 2017-03-01T16:56:03Z

...time/src/main/java/com/apollographql/android/cache/normalized/GraphQLResponseNormalizer.java

+
+    path = new ArrayList<>();
+    currentRecord = new ApolloRecord(rootKeyForOperation(operation));
+    recordSet = new InMemoryNormalizedCacheStore();


Is this intentionally, use InMemoryNormalizedCacheStore shouldn't it be configured in constructor what impl of NormalizedCacheStore will be used?

I'm thinking that using a intermediate store might be valuable as if there is a parsing error in the middle of normalization, we probably don't want to have some of the records from the response stored, and others not.

Also, some store implementations may be more efficient with a batched call to mergeOrInsert(Collection) as opposed to serial calls to mergeOrInsert(Record)

if there is a parsing error

for that we def need to support transactions, so that we can roll back if error happen

with a batched call

well this is totally up to store impl, it can accumulate all merges internally and save it in one short, but for us it should be transparent

I feel like a more scalable solution is to use temporary names within the store. So you start writing to a file/record called temp, if everything succeeds you rename to the actual record filename/key. This way there's no transactions and no duplication of memory allocation.

sav007 · 2017-03-01T16:57:47Z

apollo-runtime/src/main/java/com/apollographql/android/impl/ResponseBodyConverter.java

@@ -17,15 +19,20 @@
  private final Operation operation;
  private final ResponseFieldMapper responseFieldMapper;
  private final Map<ScalarType, CustomTypeAdapter> customTypeAdapters;
+  private final NormalizedCache normalizedCache;
+  private final GraphQLResponseNormalizer resultNormalizer;


nit: responseNormalizer ?

sav007 · 2017-03-01T16:58:27Z

...time/src/main/java/com/apollographql/android/cache/normalized/GraphQLResponseNormalizer.java

+
+import javax.annotation.Nullable;
+
+public class GraphQLResponseNormalizer implements ResponseReaderShadow {


GraphQLResponseNormalizer -> ResponseNormalizer ?

sav007 · 2017-03-01T16:59:50Z

apollo-runtime/src/main/java/com/apollographql/android/impl/ResponseBodyConverter.java

@@ -53,6 +60,7 @@
        }
      }
      jsonReader.endObject();
+      this.normalizedCache.getCacheStore().mergeOrInsert(resultNormalizer.getRecords());


should we expect any Exception here? What if this operation failed?

Open to discussion on what the right behavior is. I was thinking on an Exception we don't normalize anything.

sav007 · 2017-03-01T17:03:38Z

...o-runtime/src/main/java/com/apollographql/android/cache/normalized/NormalizedCacheStore.java

+
+import java.util.Collection;
+
+public abstract class NormalizedCacheStore {


Are all methods safe? I mean do we expect any exceptions while we reading/merging etc?

Curious to your architecture opinion on this. I tend to lean towards requiring implementations to catch their own IO related errors.
Generally, an exception on read can be replaced with null.

I wouldn't be sure what to do with exception on a merge though..

I think that broken cache, should only fail in one case when cache only policy is set.
Other than that if cache is broken for some reason, we should always hit network.
Of course we need to log that as error for user to debug, plus user should understand if response is taken from cache or from network, to proper handle this.

It almost feels like we want to return an absent optional here. Or something similar. So when reading it can return an empty container or something.

stubailo · 2017-03-01T17:48:34Z

Hey - it looks like a lot of these comments are focused on things like variable naming and code style - are there any comments about the overall design or architecture?

digitalbuddha · 2017-03-01T18:34:15Z

Nothing jumped out as an issue from an architectural standpoint.

sav007 · 2017-03-01T19:21:25Z

so far design looks reasonable, no any major concerns

BenSchwab · 2017-03-02T21:13:38Z

apollo-runtime/src/main/java/com/apollographql/android/impl/BufferedResponseReader.java

@@ -64,15 +67,28 @@
        value = readCustomType((Field.CustomTypeField) field);
        break;
      case CONDITIONAL:
-        value = readConditional((Field.ConditionalTypeField) field);
+        value = readConditional((Field.ConditionalTypeField) field, operation.variables());


The parsing for conditional is slightly strange

When we call readConditional, the field is actually __typename and not the conditional field. However, we will need to store the _typename for reading from the cache. Thus, read conditional actually stores two values at once, and can't follow as clean as a pattern as every other type of field.

BenSchwab · 2017-03-02T21:21:04Z

apollo-runtime/src/main/java/com/apollographql/android/impl/ResponseBodyConverter.java

@@ -53,6 +59,7 @@
        }
      }
      jsonReader.endObject();
+      this.cache.cacheStore().merge(responseNormalizer.records());


Resurfacing some comments from earlier:

I still feel like the right thing is to either store all, or none of the records from a response, and not burden cachestore implementations with the concept of transactions, or rollbacks.

I also don't love that the ResponseBodyConverter is saving the records at all. I feel like it's job should be to create the records as the response is parsed, and then return them with the Data.

is to either store all, or none of the records from a response

Completely agree. And that what I actually meant by transaction, if we failed to parse then don't store at all. But at the same time we need to make sure that responseNormalizer won't crash parsing network response

I also don't love that the ResponseBodyConverter is saving the records at all

can we pass the instance of normalizer in constructor and move logic of merging to RealApolloCall?

Good call. The normalizer should be a single use object, so changed it to have RealApolloCall send it ResponseBodyConverter and then save the records.

Also, good point about responseNormalizer crashing. In theory, if it's built right, and we get a well-structured graphql response, it won't throw an error. On a malformed response, it may throw something. Not sure who would error first -- the generated response reading, or the normalizer in that case.

BenSchwab · 2017-03-02T23:24:42Z

@sav007 Looks like you moved the http code to cache/http. should this stuff go in cache/normalized

digitalbuddha · 2017-03-02T23:28:02Z

Sounds reasonable. The other one should be in cache/http maybe?

sav007 · 2017-03-03T01:23:18Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/InMemoryCacheStore.java

+
+public final class InMemoryCacheStore extends CacheStore {
+
+  private final RecordSet recordSet;


Do we need to have RecordSet or we can merge it to this class?

I feel that RecordSet better describes it's purpose (a in memory data structure to store records).

I don't think we will keep InMemoryCacheStore around for very long. I think we will want to ship with some sort CacheStore implementation, but I think it will either be disk based or at least a LRU in-memory cache.

Now that I think about it, let's not include this at all in the runtime. The only thing using it is the tests, so I will move it there. We can cross the bridge of what default to ship with later.

sav007 · 2017-03-03T02:14:14Z

One question do we need really RecordSet or we can work with CacheStore only?

sav007 · 2017-03-03T03:30:20Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloCall.java

-        return responseBodyConverter.convert(response.body());
+        ResponseNormalizer normalizer = cache.responseNormalizer();
+        Response<T> convertedResponse = responseBodyConverter.convert(response.body(), normalizer);
+        cache.cacheStore().merge(normalizer.records());


should we wrap this with own try/catch?

So the question is if someone has a faulty cacheStore, is it better to fail the entire response, or catch the error?

My issue is if we catch the error, how to we communicate that the cacheStore is faulty?

Yeah, we can log but I guess you are right, lets leave it and fail for now

sav007

lgtm

digitalbuddha · 2017-03-03T17:05:54Z

lgtm as well.

BenSchwab added the Status: Revision Needed label Mar 1, 2017

digitalbuddha reviewed Mar 1, 2017

View reviewed changes

sav007 reviewed Mar 1, 2017

View reviewed changes

BenSchwab added 2 commits March 2, 2017 12:37

Add apollo-integration project

7b09e88

Add normalized cache, and normalized cache writing

9a2c213

BenSchwab force-pushed the bschwab/normalized-cache-v1 branch from 169cf27 to 9a2c213 Compare March 2, 2017 20:46

BenSchwab added 2 commits March 2, 2017 12:52

Fix a bunch of new lines, and accidental refactors

e92bc6e

Add issues for TODOs

e1caffe

BenSchwab commented Mar 2, 2017

View reviewed changes

more missing issues

cab8692

BenSchwab commented Mar 2, 2017

View reviewed changes

Delete broken test case

3b0fbb3

BenSchwab added Priority: High and removed Status: Revision Needed labels Mar 2, 2017

BenSchwab changed the title ~~[WIP] Add Normalized Cache structure and Response normalization~~ Add Normalized Cache structure and Response normalization Mar 2, 2017

Move normalized cache classes to cache/normalized

c29b91e

sav007 reviewed Mar 3, 2017

View reviewed changes

Only pass responsenormalizer in actual convert method

a86a13e

sav007 reviewed Mar 3, 2017

View reviewed changes

BenSchwab added 2 commits March 2, 2017 19:37

fix checkstyle

9aece8d

Move InMemoryCacheStore to tests

5085b9c

sav007 approved these changes Mar 3, 2017

View reviewed changes

remove bad import statement

a2e68d5

BenSchwab merged commit ad3f3bb into master Mar 3, 2017

BenSchwab deleted the bschwab/normalized-cache-v1 branch March 3, 2017 18:18

BenSchwab mentioned this pull request Mar 6, 2017

Create GraphQLResultNormalizer to create normalized records from GraphQLResponse #204

Closed

		@@ -0,0 +1,7 @@
		<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.ben_schwab.topshelf.apollointegration">

		<application android:allowBackup="true" android:label="@string/app_name" android:supportsRtl="true">


		private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'", Locale.US);

		ApolloClient apolloClient;

		}


		// private static String readNormalizerJsonFile(String name) throws IOException {


		void didParseObject(Map<String, Object> objectMap);

		void didParseList(List array);


		public class InMemoryNormalizedCacheStore extends NormalizedCacheStore {

		private final Map<String, ApolloRecord> store = new HashMap<>();


		public class GraphQLResponseNormalizer implements ResponseReaderShadow {

		private Stack<List<String>> pathStack;


		import javax.annotation.Nullable;

		public class GraphQLResponseNormalizer implements ResponseReaderShadow {


		import java.util.Collection;

		public abstract class NormalizedCacheStore {


		public final class InMemoryCacheStore extends CacheStore {

		private final RecordSet recordSet;

Add Normalized Cache structure and Response normalization #260

Add Normalized Cache structure and Response normalization #260

Conversation

BenSchwab commented Mar 1, 2017 • edited Loading

BenSchwab commented Mar 1, 2017

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

digitalbuddha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 commented Mar 1, 2017

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

sav007 Mar 1, 2017 • edited 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

sav007 Mar 1, 2017 • edited 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

stubailo commented Mar 1, 2017

digitalbuddha commented Mar 1, 2017

sav007 commented Mar 1, 2017

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

BenSchwab commented Mar 2, 2017

digitalbuddha commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 left a comment

Choose a reason for hiding this comment

digitalbuddha commented Mar 3, 2017

BenSchwab commented Mar 1, 2017 •

edited

Loading

sav007 Mar 1, 2017 •

edited

Loading

sav007 Mar 1, 2017 •

edited

Loading