From c4fba99176e1ddf6ee8381786dc4f558c86dcff3 Mon Sep 17 00:00:00 2001 From: micimize Date: Tue, 28 May 2019 00:52:15 -0500 Subject: [PATCH 1/4] feat(cache): handle full blown cyclical pointers in normalization --- .../lib/src/cache/normalized_in_memory.dart | 50 +++- .../graphql/lib/src/utilities/traverse.dart | 78 +++++-- .../test/normalized_in_memory_test.dart | 55 ++++- .../graphql/test/optimistic_cache_test.dart | 215 ++++++++++++++++++ 4 files changed, 360 insertions(+), 38 deletions(-) create mode 100644 packages/graphql/test/optimistic_cache_test.dart diff --git a/packages/graphql/lib/src/cache/normalized_in_memory.dart b/packages/graphql/lib/src/cache/normalized_in_memory.dart index 32db222bb..642f9b15a 100644 --- a/packages/graphql/lib/src/cache/normalized_in_memory.dart +++ b/packages/graphql/lib/src/cache/normalized_in_memory.dart @@ -65,7 +65,7 @@ class NormalizedInMemoryCache extends InMemoryCache { /// *WARNING* if your system allows cyclical references, this will break dynamic denormalizedRead(String key) { try { - return traverse(super.read(key), _denormalizingDereference); + return Traversal(_denormalizingDereference).traverse(super.read(key)); } catch (error) { if (error is StackOverflowError) { throw NormalizationException( @@ -90,11 +90,11 @@ class NormalizedInMemoryCache extends InMemoryCache { return value is Map ? lazilyDenormalized(value) : value; } + // get a normalizer for a given target map Normalizer _normalizerFor(Map into) { List normalizer(Object node) { final String dataId = dataIdFromObject(node); if (dataId != null) { - writeInto(dataId, node, into, normalizer); return [prefix, dataId]; } return null; @@ -103,10 +103,10 @@ class NormalizedInMemoryCache extends InMemoryCache { return normalizer; } + // [_normalizerFor] for this cache's data List _normalize(Object node) { final String dataId = dataIdFromObject(node); if (dataId != null) { - writeInto(dataId, node, data, _normalize); return [prefix, dataId]; } return null; @@ -120,14 +120,15 @@ class NormalizedInMemoryCache extends InMemoryCache { Map into, [ Normalizer normalizer, ]) { + normalizer ??= _normalizerFor(into); if (value is Map) { - final Object existing = into[key]; - final Map merged = (existing is Map) - ? deeplyMergeLeft(>[existing, value]) - : value; - + final Map merged = _mergedWithExisting(into, key, value); + final Traversal traversal = Traversal( + normalizer, + transformSideEffect: _traversingWriteInto(into), + ); // normalized the merged value - into[key] = traverseValues(merged, normalizer ?? _normalizerFor(into)); + into[key] = traversal.traverseValues(merged); } else { // writing non-map data to the store is allowed, // but there is no merging strategy @@ -151,3 +152,34 @@ String typenameDataIdFromObject(Object object) { } return null; } + +/// Writing side effect for traverse +/// +/// Essentially, we avoid problems with cyclical objects by +/// tracking seen nodes in the [Traversal], +/// and we pass this as a side effect to take advantage of that tracking +SideEffect _traversingWriteInto(Map into) { + void sideEffect(Object ref, Object value, Traversal traversal) { + final String key = (ref as List)[1]; + if (value is Map) { + final Map merged = _mergedWithExisting(into, key, value); + into[key] = traversal.traverseValues(merged); + } else { + // writing non-map data to the store is allowed, + // but there is no merging strategy + into[key] = value; + return; + } + } + + return sideEffect; +} + +/// get the given value merged with any pre-existing map with the same key +Map _mergedWithExisting( + Map into, String key, Map value) { + final Object existing = into[key]; + return (existing is Map) + ? deeplyMergeLeft(>[existing, value]) + : value; +} diff --git a/packages/graphql/lib/src/utilities/traverse.dart b/packages/graphql/lib/src/utilities/traverse.dart index 127c4fa39..7a38c79d4 100644 --- a/packages/graphql/lib/src/utilities/traverse.dart +++ b/packages/graphql/lib/src/utilities/traverse.dart @@ -1,32 +1,62 @@ +import 'dart:collection'; + typedef Transform = Object Function(Object node); +typedef SideEffect = void Function( + Object transformResult, + Object node, + Traversal traversal, +); -Map traverseValues( - Map node, - Transform transform, -) { - return node.map( - (String key, Object value) => MapEntry( - key, - traverse(value, transform), - ), - ); -} +class Traversal { + Traversal( + this.transform, { + this.transformSideEffect, + this.seenObjects, + }) { + seenObjects ??= HashSet(); + } + + Transform transform; -// Attempts to apply the transform to every leaf of the data structure recursively. -// Stops recursing when a node is transformed (returns non-null) -Object traverse(Object node, Transform transform) { - final Object transformed = transform(node); - if (transformed != null) { - return transformed; + /// An optional side effect to call when a node is transformed. + SideEffect transformSideEffect; + HashSet seenObjects; + + bool hasAlreadySeen(Object node) { + final bool wasAdded = seenObjects.add(node.hashCode); + return !wasAdded; } - if (node is List) { - return node - .map((Object node) => traverse(node, transform)) - .toList(); + /// Traverse only the values of the given map + Map traverseValues(Map node) { + return node.map( + (String key, Object value) => MapEntry( + key, + traverse(value), + ), + ); } - if (node is Map) { - return traverseValues(node, transform); + + // Attempts to apply the transform to every leaf of the data structure recursively. + // Stops recursing when a node is transformed (returns non-null) + Object traverse(Object node) { + final Object transformed = transform(node); + if (hasAlreadySeen(node)) { + return transformed ?? node; + } + if (transformed != null) { + if (transformSideEffect != null) { + transformSideEffect(transformed, node, this); + } + return transformed; + } + + if (node is List) { + return node.map((Object node) => traverse(node)).toList(); + } + if (node is Map) { + return traverseValues(node); + } + return node; } - return node; } diff --git a/packages/graphql/test/normalized_in_memory_test.dart b/packages/graphql/test/normalized_in_memory_test.dart index 2126fb402..2e940a313 100644 --- a/packages/graphql/test/normalized_in_memory_test.dart +++ b/packages/graphql/test/normalized_in_memory_test.dart @@ -133,10 +133,12 @@ final Map cyclicalOperationData = { 'b': { '__typename': 'B', 'id': 5, - 'a': { - '__typename': 'A', - 'id': 1, - }, + 'as': [ + { + '__typename': 'A', + 'id': 1, + }, + ] }, }, }; @@ -150,7 +152,39 @@ final Map cyclicalNormalizedA = { final Map cyclicalNormalizedB = { '__typename': 'B', 'id': 5, - 'a': ['@cache/reference', 'A/1'], + 'as': [ + ['@cache/reference', 'A/1'] + ], +}; + +Map get cyclicalObjOperationData { + Map a; + Map b; + a = { + '__typename': 'A', + 'id': 1, + }; + b = { + '__typename': 'B', + 'id': 5, + 'as': [a] + }; + a['b'] = b; + return {'a': a}; +} + +final Map cyclicalObjNormalizedA = { + '__typename': 'A', + 'id': 1, + 'b': ['@cache/reference', 'B/5'], +}; + +final Map cyclicalObjNormalizedB = { + '__typename': 'B', + 'id': 5, + 'as': [ + ['@cache/reference', 'A/1'] + ], }; NormalizedInMemoryCache getTestCache() => NormalizedInMemoryCache( @@ -187,4 +221,15 @@ void main() { expect(b.data, equals(cyclicalNormalizedB)); }); }); + + group('Handles Object/pointer self-references/cycles', () { + final NormalizedInMemoryCache cache = getTestCache(); + test('lazily reads cyclical references', () { + cache.write(rawOperationKey, cyclicalObjOperationData); + final LazyCacheMap a = cache.read('A/1') as LazyCacheMap; + expect(a.data, equals(cyclicalObjNormalizedA)); + final LazyCacheMap b = a['b'] as LazyCacheMap; + expect(b.data, equals(cyclicalObjNormalizedB)); + }); + }); } diff --git a/packages/graphql/test/optimistic_cache_test.dart b/packages/graphql/test/optimistic_cache_test.dart new file mode 100644 index 000000000..6d617b9dd --- /dev/null +++ b/packages/graphql/test/optimistic_cache_test.dart @@ -0,0 +1,215 @@ +import 'dart:io' show Directory; + +import 'package:test/test.dart'; + +import 'package:graphql/src/cache/normalized_in_memory.dart' + show typenameDataIdFromObject; +import 'package:graphql/src/cache/optimistic.dart'; +import 'package:graphql/src/cache/lazy_cache_map.dart'; + +List reference(String key) { + return ['cache/reference', key]; +} + +const String rawOperationKey = 'rawOperationKey'; + +final Map rawOperationData = { + 'a': { + '__typename': 'A', + 'id': 1, + 'list': [ + 1, + 2, + 3, + { + '__typename': 'Item', + 'id': 4, + 'value': 4, + } + ], + 'b': { + '__typename': 'B', + 'id': 5, + 'c': { + '__typename': 'C', + 'id': 6, + 'cField': 'value', + }, + 'bField': {'field': true} + }, + 'd': { + 'id': 9, + 'dField': {'field': true} + }, + }, + 'aField': {'field': false} +}; + +final Map updatedCValue = { + '__typename': 'C', + 'id': 6, + 'new': 'field', + 'cField': 'changed value', +}; + +final Map updatedCOperationData = { + 'a': { + '__typename': 'A', + 'id': 1, + 'list': [ + 1, + 2, + 3, + { + '__typename': 'Item', + 'id': 4, + 'value': 4, + } + ], + 'b': { + '__typename': 'B', + 'id': 5, + 'c': updatedCValue, + 'bField': {'field': true} + }, + 'd': { + 'id': 9, + 'dField': {'field': true} + }, + }, + 'aField': {'field': false} +}; + +final Map subsetAValue = { + 'a': { + '__typename': 'A', + 'id': 1, + 'list': [ + 5, + 6, + 7, + { + '__typename': 'Item', + 'id': 8, + 'value': 8, + } + ], + 'd': { + 'id': 10, + }, + }, +}; + +final Map updatedSubsetOperationData = { + 'a': { + '__typename': 'A', + 'id': 1, + 'list': [ + 5, + 6, + 7, + { + '__typename': 'Item', + 'id': 8, + 'value': 8, + } + ], + 'b': { + '__typename': 'B', + 'id': 5, + 'c': updatedCValue, + 'bField': {'field': true} + }, + 'd': { + 'id': 10, + 'dField': {'field': true} + }, + }, + 'aField': {'field': false} +}; + +Map get cyclicalOperationData { + Map a; + Map b; + a = { + '__typename': 'A', + 'id': 1, + }; + b = { + '__typename': 'B', + 'id': 5, + 'as': [a] + }; + a['b'] = b; + return {'a': a}; +} + +final Map cyclicalNormalizedA = { + '__typename': 'A', + 'id': 1, + 'b': ['@cache/reference', 'B/5'], +}; + +final Map cyclicalNormalizedB = { + '__typename': 'B', + 'id': 5, + 'as': [ + ['@cache/reference', 'A/1'] + ], +}; + +OptimisticCache getTestCache() => OptimisticCache( + dataIdFromObject: typenameDataIdFromObject, + storageProvider: () => Directory.systemTemp.createTempSync('file_test_'), + ); + +void main() { + group('Normalizes writes', () { + final OptimisticCache cache = getTestCache(); + test('lazily reads cyclical references', () { + cache.write(rawOperationKey, cyclicalOperationData); + final LazyCacheMap a = cache.read('A/1') as LazyCacheMap; + expect(a.data, equals(cyclicalNormalizedA)); + final LazyCacheMap b = a['b'] as LazyCacheMap; + expect(b.data, equals(cyclicalNormalizedB)); + }); + }); + + group('Normalizes writes optimistically', () { + final OptimisticCache cache = getTestCache(); + test('lazily reads cyclical references', () { + cache.addOptimisiticPatch(rawOperationKey, + (cache) => cache..write(rawOperationKey, cyclicalOperationData)); + final LazyCacheMap a = cache.read('A/1') as LazyCacheMap; + expect(a.data, equals(cyclicalNormalizedA)); + final LazyCacheMap b = a['b'] as LazyCacheMap; + expect(b.data, equals(cyclicalNormalizedB)); + }); + }); + + group('Optimistic writes', () { + final OptimisticCache cache = getTestCache(); + test('.addOptimisiticPatch .readDenormalize round trip', () { + cache.addOptimisiticPatch( + rawOperationKey, + (cache) => cache..write(rawOperationKey, rawOperationData), + ); + expect(cache.denormalizedRead(rawOperationKey), equals(rawOperationData)); + }); + test('updating nested data changes top level optimistic operation', () { + cache.addOptimisiticPatch( + '$rawOperationKey.C', + (cache) => cache..write('C/6', updatedCValue), + ); + expect( + cache.denormalizedRead(rawOperationKey), + equals(updatedCOperationData), + ); + }); + test('removing optimistic patch clears results', () { + cache.removeOptimisticPatch(rawOperationKey); + expect(cache.read(rawOperationKey), equals(null)); + expect(cache.read('C/6'), equals(null)); + }); + }); +} From 38d3588272e1ebf45508ac606f5a51a92d95f6a2 Mon Sep 17 00:00:00 2001 From: micimize Date: Tue, 28 May 2019 00:56:36 -0500 Subject: [PATCH 2/4] fix(cache): don't use super.read in denormalizedRead so OptimisticCache overrides it's behavior --- packages/graphql/lib/src/cache/normalized_in_memory.dart | 2 +- packages/graphql/lib/src/core/observable_query.dart | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/graphql/lib/src/cache/normalized_in_memory.dart b/packages/graphql/lib/src/cache/normalized_in_memory.dart index 642f9b15a..29813b8a7 100644 --- a/packages/graphql/lib/src/cache/normalized_in_memory.dart +++ b/packages/graphql/lib/src/cache/normalized_in_memory.dart @@ -65,7 +65,7 @@ class NormalizedInMemoryCache extends InMemoryCache { /// *WARNING* if your system allows cyclical references, this will break dynamic denormalizedRead(String key) { try { - return Traversal(_denormalizingDereference).traverse(super.read(key)); + return Traversal(_denormalizingDereference).traverse(read(key)); } catch (error) { if (error is StackOverflowError) { throw NormalizationException( diff --git a/packages/graphql/lib/src/core/observable_query.dart b/packages/graphql/lib/src/core/observable_query.dart index ec3bd99f0..4d450afd6 100644 --- a/packages/graphql/lib/src/core/observable_query.dart +++ b/packages/graphql/lib/src/core/observable_query.dart @@ -17,7 +17,6 @@ enum QueryLifecycle { SIDE_EFFECTS_PENDING, SIDE_EFFECTS_BLOCKING, - // right now only Mutations ever become completed COMPLETED, CLOSED } From c73e105d4f5e3b1ad0d3a552622cf35fafcff6df Mon Sep 17 00:00:00 2001 From: micimize Date: Wed, 29 May 2019 11:27:26 -0500 Subject: [PATCH 3/4] style: remove some unneeded typings --- .../graphql/lib/src/cache/normalized_in_memory.dart | 10 +++++----- packages/graphql/lib/src/cache/optimistic.dart | 2 +- packages/graphql/lib/src/utilities/helpers.dart | 3 +-- packages/graphql/test/normalized_in_memory_test.dart | 4 ++-- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/graphql/lib/src/cache/normalized_in_memory.dart b/packages/graphql/lib/src/cache/normalized_in_memory.dart index 29813b8a7..7b51c9754 100644 --- a/packages/graphql/lib/src/cache/normalized_in_memory.dart +++ b/packages/graphql/lib/src/cache/normalized_in_memory.dart @@ -93,7 +93,7 @@ class NormalizedInMemoryCache extends InMemoryCache { // get a normalizer for a given target map Normalizer _normalizerFor(Map into) { List normalizer(Object node) { - final String dataId = dataIdFromObject(node); + final dataId = dataIdFromObject(node); if (dataId != null) { return [prefix, dataId]; } @@ -122,7 +122,7 @@ class NormalizedInMemoryCache extends InMemoryCache { ]) { normalizer ??= _normalizerFor(into); if (value is Map) { - final Map merged = _mergedWithExisting(into, key, value); + final merged = _mergedWithExisting(into, key, value); final Traversal traversal = Traversal( normalizer, transformSideEffect: _traversingWriteInto(into), @@ -162,7 +162,7 @@ SideEffect _traversingWriteInto(Map into) { void sideEffect(Object ref, Object value, Traversal traversal) { final String key = (ref as List)[1]; if (value is Map) { - final Map merged = _mergedWithExisting(into, key, value); + final merged = _mergedWithExisting(into, key, value); into[key] = traversal.traverseValues(merged); } else { // writing non-map data to the store is allowed, @@ -178,8 +178,8 @@ SideEffect _traversingWriteInto(Map into) { /// get the given value merged with any pre-existing map with the same key Map _mergedWithExisting( Map into, String key, Map value) { - final Object existing = into[key]; + final existing = into[key]; return (existing is Map) - ? deeplyMergeLeft(>[existing, value]) + ? deeplyMergeLeft([existing, value]) : value; } diff --git a/packages/graphql/lib/src/cache/optimistic.dart b/packages/graphql/lib/src/cache/optimistic.dart index 53dfe302e..1677ea769 100644 --- a/packages/graphql/lib/src/cache/optimistic.dart +++ b/packages/graphql/lib/src/cache/optimistic.dart @@ -82,7 +82,7 @@ class OptimisticCache extends NormalizedInMemoryCache { if (patch.data.containsKey(key)) { final Object patchData = patch.data[key]; if (value is Map && patchData is Map) { - value = deeplyMergeLeft(>[ + value = deeplyMergeLeft([ value as Map, patchData, ]); diff --git a/packages/graphql/lib/src/utilities/helpers.dart b/packages/graphql/lib/src/utilities/helpers.dart index c49fa3116..92f16bba3 100644 --- a/packages/graphql/lib/src/utilities/helpers.dart +++ b/packages/graphql/lib/src/utilities/helpers.dart @@ -71,6 +71,5 @@ Map deeplyMergeLeft( Iterable> maps, ) { // prepend an empty literal for functional immutability - return (>[{}]..addAll(maps)) - .reduce(_recursivelyAddAll); + return (>[{}]..addAll(maps)).reduce(_recursivelyAddAll); } diff --git a/packages/graphql/test/normalized_in_memory_test.dart b/packages/graphql/test/normalized_in_memory_test.dart index 2e940a313..f51d416ec 100644 --- a/packages/graphql/test/normalized_in_memory_test.dart +++ b/packages/graphql/test/normalized_in_memory_test.dart @@ -173,13 +173,13 @@ Map get cyclicalObjOperationData { return {'a': a}; } -final Map cyclicalObjNormalizedA = { +final Map cyclicalObjNormalizedA = { '__typename': 'A', 'id': 1, 'b': ['@cache/reference', 'B/5'], }; -final Map cyclicalObjNormalizedB = { +final Map cyclicalObjNormalizedB = { '__typename': 'B', 'id': 5, 'as': [ From c9c5623b3a1da7c41a771b5c849576a9159e14be Mon Sep 17 00:00:00 2001 From: micimize Date: Wed, 29 May 2019 11:34:58 -0500 Subject: [PATCH 4/4] chore: fix relative override breaking ci --- packages/graphql_flutter/pubspec.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/graphql_flutter/pubspec.yaml b/packages/graphql_flutter/pubspec.yaml index 68fd7537d..d0cc995bf 100644 --- a/packages/graphql_flutter/pubspec.yaml +++ b/packages/graphql_flutter/pubspec.yaml @@ -23,6 +23,6 @@ dev_dependencies: test: ^1.5.3 environment: sdk: '>=2.2.0 <3.0.0' -dependency_overrides: - graphql: - path: ../graphql +# dependency_overrides: +# graphql: +# path: ../graphql