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

Add record field serialization #342

Merged
merged 9 commits into from
Mar 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.apollographql.android.cache.normalized.CacheControl;
import com.apollographql.android.cache.normalized.CacheKey;
import com.apollographql.android.cache.normalized.CacheKeyResolver;
import com.apollographql.android.cache.normalized.CacheStore;
import com.apollographql.android.impl.normalizer.EpisodeHeroName;
import com.apollographql.android.impl.normalizer.HeroAndFriendsNamesWithIDs;
import com.apollographql.android.impl.normalizer.type.Episode;
Expand All @@ -33,7 +34,7 @@
public class ApolloWatcherTest {
private ApolloClient apolloClient;
private MockWebServer server;
private InMemoryCacheStore cacheStore;
private CacheStore cacheStore;

@Before public void setUp() {
server = new MockWebServer();
Expand Down
2 changes: 2 additions & 0 deletions apollo-runtime/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ dependencies {
testCompile dep.mockWebServer
testCompile dep.okhttpTestSupport
testCompile dep.mockito
testCompile project(":apollo-api")

androidTestCompile project(":apollo-api")
androidTestCompile dep.truth
androidTestCompile (dep.testRunner) {
exclude module: 'support-annotations'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
import android.support.test.InstrumentationRegistry;
import android.support.test.runner.AndroidJUnit4;

import com.apollographql.android.api.graphql.internal.Optional;
import com.apollographql.android.cache.normalized.FieldsAdapter;
import com.apollographql.android.cache.normalized.Record;
import com.squareup.moshi.Moshi;

import org.junit.Before;
import org.junit.Test;
Expand All @@ -16,15 +17,15 @@
public class SqlStoreTest {

public static final String KEY = "key";
public static final String FIELDS = "fields";
public static final String FIELDS = "{\"fieldKey\": \"value\"}";
public static final String IN_MEMORY_DB = null; //null means db is memory only
private SqlStore sqlStore;

@Before
public void setUp() {
ApolloSqlHelper apolloSqlHelper = ApolloSqlHelper.create(InstrumentationRegistry.getTargetContext(),
IN_MEMORY_DB);
sqlStore = SqlStore.create(apolloSqlHelper, FieldsAdapter.create(new Moshi.Builder().build()));
sqlStore = SqlStore.create(apolloSqlHelper, FieldsAdapter.create());
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for this change? I'd prefer to be able to use 1 moshi instance for whole app apollo or otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be missing something of how Moshi works, but it seems dangerous to register our BigDecimal converter on a global Moshi instance, in case there is an existing converter. (Currently, it seems we do not allow a user to pass in a Moshi instance to Apollo anyway).

We could register these convertors on the Apollo's Moshi instance in ApolloClient. But that Moshi instance seems primarily concerned with serializing variable objects, and initializing cache serializers there would take some thought of how to do it properly without spaghetti dependencies in the ApolloClient builder.

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking was from a performance standup. Moshi caches adapters but it's on a per instance level. So say I have same date converter on app and apollo client, I'd prefer to not have to cache both. Currently our app works with 1 instance of gson. It doesn't have to be solved now. I can do a pr if it becomes an issue

}

@Test
Expand All @@ -42,8 +43,8 @@ public void testRecordDeletion() {
@Test
public void testRecordSelection() {
createRecord();
Record record = sqlStore.selectRecordForKey(KEY);
assertThat(record.key()).isEqualTo(KEY);
Optional<Record> record = sqlStore.selectRecordForKey(KEY);
assertThat(record.get().key()).isEqualTo(KEY);
}

private long createRecord() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package com.apollographql.android.cache.normalized;

//Todo: provide serializable reference to differentiate between a reference and regular string
// (Issue: https://github.com/apollographql/apollo-android/issues/265)
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public final class CacheReference {

private final String key;
private static final Pattern SERIALIZATION_REGEX_PATTERN = Pattern.compile("ApolloCacheReference\\{(.*)\\}");
private static final String SERIALIZATION_TEMPLATE = "ApolloCacheReference{%s}";

public CacheReference(String key) {
this.key = key;
Expand All @@ -31,5 +34,24 @@ public String key() {
@Override public String toString() {
return key;
}

public String serialize() {
return String.format(SERIALIZATION_TEMPLATE, key);
}

public static CacheReference deserialize(String serializedCacheReference) {
Matcher matcher = SERIALIZATION_REGEX_PATTERN.matcher(serializedCacheReference);
if (!matcher.find() || matcher.groupCount() != 1) {
throw new IllegalArgumentException("Not a cache reference: " + serializedCacheReference
+ " Must be of the form:" + SERIALIZATION_TEMPLATE);
}
return new CacheReference(matcher.group(1));
}

public static boolean canDeserialize(String value) {
Matcher matcher = SERIALIZATION_REGEX_PATTERN.matcher(value);
return matcher.matches();
}

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.apollographql.android.cache.normalized;

import com.apollographql.android.impl.CacheJsonStreamReader;
import com.squareup.moshi.JsonAdapter;
import com.squareup.moshi.JsonReader;
import com.squareup.moshi.JsonWriter;
import com.squareup.moshi.Moshi;
import com.squareup.moshi.Types;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Type;
import java.math.BigDecimal;
import java.util.Map;

import okio.BufferedSource;
import okio.Okio;

import static com.apollographql.android.impl.ApolloReader.bufferedSourceJsonReader;
import static com.apollographql.android.impl.ApolloReader.cacheJsonStreamReader;

public final class FieldsAdapter {
private final JsonAdapter<Map<String, Object>> serializationAdapter;

private FieldsAdapter(Moshi moshi) {
Type type = Types.newParameterizedType(Map.class, String.class, Object.class);
serializationAdapter = moshi.adapter(type);
}

public static FieldsAdapter create() {
Moshi moshi = new Moshi.Builder()
.add(CacheReference.class, new CacheReferenceAdapter())
.add(BigDecimal.class, new BigDecimalAdapter())
.build();
return new FieldsAdapter(moshi);
}

public String toJson(Map<String, Object> fields) {
return serializationAdapter.toJson(fields);
}

public Map<String, Object> from(BufferedSource bufferedFieldSource) throws IOException {
final CacheJsonStreamReader cacheJsonStreamReader =
cacheJsonStreamReader(bufferedSourceJsonReader(bufferedFieldSource));
return cacheJsonStreamReader.buffer();
}

public Map<String, Object> from(String jsonFieldSource) throws IOException {
final BufferedSource bufferSource = Okio.buffer(Okio.source(new ByteArrayInputStream(jsonFieldSource.getBytes())));
return from(bufferSource);
}

private static class CacheReferenceAdapter extends JsonAdapter<CacheReference> {

@Override public CacheReference fromJson(JsonReader reader) throws IOException {
throw new IllegalStateException(this.getClass().getName() + " should only be used for serialization.");
}

@Override public void toJson(JsonWriter writer, CacheReference value) throws IOException {
writer.value(value.serialize());
}
}

private static class BigDecimalAdapter extends JsonAdapter<BigDecimal> {

@Override public BigDecimal fromJson(JsonReader reader) throws IOException {
throw new IllegalStateException(this.getClass().getName() + " should only be used for serialization.");
}

@Override public void toJson(JsonWriter writer, BigDecimal value) throws IOException {
writer.value(value);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,46 @@
import java.util.Map;
import java.util.Set;

//Todo: Enhance this class to better support generalized serialization
// https://github.com/apollographql/apollo-android/issues/265
public final class Record {

private final String key;
private final Map<String, Object> fields;

public static class Builder {
private final Map<String, Object> fields;
private final String key;

public Builder(String key) {
this(key, new LinkedHashMap<String, Object>());
}

public Builder(String key, Map<String, Object> fields) {
this.key = key;
this.fields = fields;
}

public Builder addField(String key, Object value) {
fields.put(key, value);
return this;
}

public String key() {
return key;
}

public Record build() {
return new Record(key, fields);
}
}

public static Builder builder(String key) {
return new Builder(key);
}

public Builder toBuilder() {
return new Builder(key(), this.fields);
}

public Record(String cacheKey) {
this.key = cacheKey;
fields = new LinkedHashMap<>();
Expand All @@ -22,10 +55,6 @@ public Record(String key, Map<String, Object> fields) {
this.fields = fields;
}

public void addField(String key, Object value) {
fields.put(key, value);
}

public Object field(String fieldKey) {
return fields.get(fieldKey);
}
Expand All @@ -51,4 +80,5 @@ public Set<String> mergeWith(Record otherRecord) {
public Map<String, Object> fields() {
return fields;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class ResponseNormalizer<R> implements ResponseReaderShadow<R> {
private final CacheKeyResolver<R> cacheKeyResolver;

private List<String> path;
private Record currentRecord;
private Record.Builder currentRecordBuilder;
private RecordSet recordSet;

public ResponseNormalizer(@Nonnull CacheKeyResolver<R> cacheKeyResolver) {
Expand All @@ -45,7 +45,7 @@ public Set<String> dependentKeys() {
dependentKeys = new HashSet<>();

path = new ArrayList<>();
currentRecord = new Record(CacheKeyResolver.rootKeyForOperation(operation).key());
currentRecordBuilder = Record.builder(CacheKeyResolver.rootKeyForOperation(operation).key());
recordSet = new RecordSet();
}

Expand All @@ -58,12 +58,12 @@ public Set<String> dependentKeys() {
path.remove(path.size() - 1);
Object value = valueStack.pop();
String cacheKey = field.cacheKey(variables);
String dependentKey = currentRecord.key() + "." + cacheKey;
String dependentKey = currentRecordBuilder.key() + "." + cacheKey;
dependentKeys.add(dependentKey);
currentRecord.addField(cacheKey, value);
currentRecordBuilder.addField(cacheKey, value);

if (recordStack.isEmpty()) {
recordSet.merge(currentRecord);
recordSet.merge(currentRecordBuilder.build());
}
}

Expand All @@ -82,16 +82,17 @@ public Set<String> dependentKeys() {
path = new ArrayList<>();
path.add(cacheKeyValue);
}
recordStack.push(currentRecord);
currentRecord = new Record(cacheKeyValue);
recordStack.push(currentRecordBuilder.build());
currentRecordBuilder = Record.builder(cacheKeyValue);
}

@Override public void didParseObject(R objectSource) {
path = pathStack.pop();
valueStack.push(new CacheReference(currentRecord.key()));
dependentKeys.add(currentRecord.key());
recordSet.merge(currentRecord);
currentRecord = recordStack.pop();
Record completedRecord = currentRecordBuilder.build();
valueStack.push(new CacheReference(completedRecord.key()));
dependentKeys.add(completedRecord.key());
recordSet.merge(completedRecord);
currentRecordBuilder = recordStack.pop().toBuilder();
}

@Override public void didParseList(List array) {
Expand Down

This file was deleted.

Loading