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

Structural data validation on cache writes and more #754

Merged
merged 18 commits into from
Nov 6, 2020
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
1 change: 0 additions & 1 deletion examples/flutter_bloc/ios/Flutter/.last_build_id

This file was deleted.

8 changes: 4 additions & 4 deletions examples/starwars/lib/episode/hero_query.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ class HeroForEpisode extends StatelessWidget {
// NOTE: a loading message is always sent, but if you're developing locally,
// the network result might be returned so fast that
// flutter rebuilds again too quickly for you don't see the loading result on the stream
print([
result.source,
if (result.data != null) result.data['hero']['name']
]);
// print([
// result.source,
// if (result.data != null) result.data['hero']['name']
// ]);
if (result.hasException) {
return Text(result.exception.toString());
}
Expand Down
16 changes: 16 additions & 0 deletions examples/starwars/lib/generated_plugin_registrant.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//
// Generated file. Do not edit.
//

// ignore: unused_import
import 'dart:ui';

import 'package:connectivity_for_web/connectivity_for_web.dart';

import 'package:flutter_web_plugins/flutter_web_plugins.dart';

// ignore: public_member_api_docs
void registerPlugins(PluginRegistry registry) {
ConnectivityPlugin.registerWith(registry.registrarFor(ConnectivityPlugin));
registry.registerMessageHandler();
}
4 changes: 3 additions & 1 deletion examples/starwars/lib/reviews/review_page_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ class PagingReviews extends StatelessWidget {
Widget build(BuildContext context) {
return Query(
options: QueryOptions(
// if we have cached results, don't clobber them
fetchPolicy: FetchPolicy.cacheFirst,
document: gql(r'''
query Reviews($page: Int!) {
reviews(page: $page) {
Expand Down Expand Up @@ -59,7 +61,7 @@ class PagingReviews extends StatelessWidget {
: RaisedButton(
onPressed: () {
fetchMore(
FetchMoreOptions(
FetchMoreOptions.partial(
variables: {'page': nextPage},
updateQuery: (existing, newReviews) => ({
'reviews': {
Expand Down
23 changes: 23 additions & 0 deletions packages/graphql/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ As of `v4`, it is built on foundational libraries from the [gql-dart project], i
- [Direct Cache Access API](#direct-cache-access-api)
- [`Request`, `readQuery`, and `writeQuery`](#request-readquery-and-writequery)
- [`FragmentRequest`, `readFragment`, and `writeFragment`](#fragmentrequest-readfragment-and-writefragment)
- [Other Cache Considerations](#other-cache-considerations)
- [Write strictness and `partialDataPolicy`](#write-strictness-and-partialdatapolicy)
- [Possible cache write exceptions](#possible-cache-write-exceptions)
- [Policies](#policies)
- [Exceptions](#exceptions)
- [Links](#links)
Expand Down Expand Up @@ -440,6 +443,26 @@ client.writeFragment(fragmentRequest, data);

> **NB** You likely want to call the cache access API from your `client` for automatic broadcasting support.

## Other Cache Considerations

### Write strictness and `partialDataPolicy`

As of [#754](https://github.com/zino-app/graphql-flutter/pull/754) we can now enforce strict structural constraints on data written to the cache. This means that if the client receives structurally invalid data from the network or on `client.writeQuery`, it will throw an exception.

By default, optimistic data is excluded from these constraints for ease of use via `PartialDataCachePolicy.acceptForOptimisticData`, as it is easy to miss `__typename`, etc.
This behavior is configurable via `GraphQLCache.partialDataPolicy`, which can be set to `accept` for no constraints or `reject` for full constraints.

### Possible cache write exceptions

At link execution time, one of the following exceptions can be thrown:

* `CacheMisconfigurationException` if the structure seems like it should write properly, and is perhaps failing due to a `typePolicy`
* `UnexpectedResponseStructureException` if the server response looks malformed.
* `MismatchedDataStructureException` in the event of a malformed optimistic result (and `PartialDataCachePolicy.reject`).
* `CacheMissException` if write succeeds but `readQuery` then returns `null` (though **data will not be overwritten**)

</details>

## Policies

Policies are used to configure execution and error behavior for a given request.
Expand Down
110 changes: 78 additions & 32 deletions packages/graphql/lib/src/cache/_normalizing_data_proxy.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'package:graphql/src/cache/fragment.dart';
import 'package:graphql/src/exceptions/exceptions_next.dart';
import "package:meta/meta.dart";

import 'package:gql_exec/gql_exec.dart' show Request;
Expand Down Expand Up @@ -29,10 +30,24 @@ abstract class NormalizingDataProxy extends GraphQLDataProxy {
/// [returnPartialData] is `true`
bool addTypename = false;

/// Used for testing
/// Used for testing.
///
/// Passed through to normalize. When [denormalizeOperation] isn't passed [returnPartialData],
/// It will simply return `null` if any part of the query can't be constructed.
///
/// **NOTE**: This is not exposed as a configuration for a reason.
/// If enabled, it would be easy to eagerly return an unexpected partial result from the cache,
/// resulting in mangled and hard-to-reason-about app state.
@protected
bool get returnPartialData => false;

/// Whether it is permissible to write partial data to the this proxy.
/// Determined by [PartialDataCachePolicy]
///
/// Passed through to normalize. When [normalizeOperation] isn't passed [acceptPartialData],
/// It will set missing fields to `null` if any part of a structurally valid query result is missing.
bool get acceptPartialData;

/// Flag used to request a (re)broadcast from the [QueryManager].
///
/// This is set on every [writeQuery] and [writeFragment] by default.
Expand Down Expand Up @@ -68,8 +83,11 @@ abstract class NormalizingDataProxy extends GraphQLDataProxy {
// provided from cache
read: (dataId) => readNormalized(dataId, optimistic: optimistic),
typePolicies: typePolicies,
//dataIdFromObject: dataIdFromObject,
returnPartialData: returnPartialData,
addTypename: addTypename ?? false,
// if there is partial data, we cannot read and return null
handleException: true,
// provided from request
document: request.operation.document,
operationName: request.operation.operationName,
Expand All @@ -87,6 +105,8 @@ abstract class NormalizingDataProxy extends GraphQLDataProxy {
dataIdFromObject: dataIdFromObject,
returnPartialData: returnPartialData,
addTypename: addTypename ?? false,
// if there is partial data, we cannot read and return null
handleException: true,
// provided from request
document: fragmentRequest.fragment.document,
idFields: fragmentRequest.idFields,
Expand All @@ -99,21 +119,34 @@ abstract class NormalizingDataProxy extends GraphQLDataProxy {
Map<String, dynamic> data,
bool broadcast = true,
}) {
normalizeOperation(
// provided from cache
write: (dataId, value) => writeNormalized(dataId, value),
read: (dataId) => readNormalized(dataId),
typePolicies: typePolicies,
dataIdFromObject: dataIdFromObject,
// provided from request
document: request.operation.document,
operationName: request.operation.operationName,
variables: sanitizeVariables(request.variables),
// data
data: data,
);
if (broadcast ?? true) {
broadcastRequested = true;
try {
normalizeOperation(
// provided from cache
write: (dataId, value) => writeNormalized(dataId, value),
read: (dataId) => readNormalized(dataId),
typePolicies: typePolicies,
dataIdFromObject: dataIdFromObject,
acceptPartialData: acceptPartialData,
addTypename: addTypename ?? false,
// provided from request
document: request.operation.document,
operationName: request.operation.operationName,
variables: sanitizeVariables(request.variables),
// data
data: data,
);
if (broadcast ?? true) {
broadcastRequested = true;
}
} on PartialDataException catch (e) {
if (request.validatesStructureOf(data)) {
throw CacheMisconfigurationException(
e,
request: request,
data: data,
);
}
rethrow;
}
}

Expand All @@ -122,22 +155,35 @@ abstract class NormalizingDataProxy extends GraphQLDataProxy {
@required Map<String, dynamic> data,
bool broadcast = true,
}) {
normalizeFragment(
// provided from cache
write: (dataId, value) => writeNormalized(dataId, value),
read: (dataId) => readNormalized(dataId),
typePolicies: typePolicies,
dataIdFromObject: dataIdFromObject,
// provided from request
document: request.fragment.document,
idFields: request.idFields,
fragmentName: request.fragment.fragmentName,
variables: sanitizeVariables(request.variables),
// data
data: data,
);
if (broadcast ?? true) {
broadcastRequested = true;
try {
normalizeFragment(
// provided from cache
write: (dataId, value) => writeNormalized(dataId, value),
read: (dataId) => readNormalized(dataId),
typePolicies: typePolicies,
dataIdFromObject: dataIdFromObject,
acceptPartialData: acceptPartialData,
addTypename: addTypename ?? false,
// provided from request
document: request.fragment.document,
idFields: request.idFields,
fragmentName: request.fragment.fragmentName,
variables: sanitizeVariables(request.variables),
// data
data: data,
);
if (broadcast ?? true) {
broadcastRequested = true;
}
} on PartialDataException catch (e) {
if (request.validatesStructureOf(data)) {
throw CacheMisconfigurationException(
e,
fragmentRequest: request,
data: data,
);
}
rethrow;
}
}
}
11 changes: 10 additions & 1 deletion packages/graphql/lib/src/cache/_optimistic_transactions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import 'package:meta/meta.dart';
import 'package:graphql/src/cache/_normalizing_data_proxy.dart';
import 'package:graphql/src/cache/data_proxy.dart';

import 'package:graphql/src/cache/cache.dart' show GraphQLCache;
import 'package:graphql/src/cache/cache.dart'
show GraphQLCache, PartialDataCachePolicy;

/// API for users to provide cache updates through
typedef CacheTransaction = GraphQLDataProxy Function(GraphQLDataProxy proxy);
Expand All @@ -32,6 +33,14 @@ class OptimisticProxy extends NormalizingDataProxy {

GraphQLCache cache;

/// Whether it is permissible to write partial data, as determined by [partialDataPolicy].
///
/// Passed through to normalize. When [normalizeOperation] isn't passed [acceptPartialData],
/// It will set missing fields to `null` if any part of a structurally valid query result is missing.
@override
bool get acceptPartialData =>
cache.partialDataPolicy != PartialDataCachePolicy.reject;

/// `typePolicies` to pass down to `normalize` (proxied from [cache])
get typePolicies => cache.typePolicies;

Expand Down
42 changes: 36 additions & 6 deletions packages/graphql/lib/src/cache/cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ class GraphQLCache extends NormalizingDataProxy {
Store store,
this.dataIdFromObject,
this.typePolicies = const {},

/// Input variable sanitizer for referencing custom scalar types in cache keys.
///
/// Defaults to [sanitizeFilesForCache]. Can be set to `null` to disable sanitization.
/// If present, a sanitizer will be built with [variableSanitizer]
this.partialDataPolicy = PartialDataCachePolicy.acceptForOptimisticData,
Object Function(Object) sanitizeVariables = sanitizeFilesForCache,
}) : sanitizeVariables = variableSanitizer(sanitizeVariables),
store = store ?? InMemoryStore();
Expand All @@ -46,12 +42,30 @@ class GraphQLCache extends NormalizingDataProxy {
/// rebroadcast operations.
final Store store;

/// Determines how partial data should be handled when written to the cache
///
/// [acceptForOptimisticData] is the default and recommended, as it provides
/// the best balance between safety and usability.
final PartialDataCachePolicy partialDataPolicy;

/// Whether it is permissible to write partial data, as determined by [partialDataPolicy].
///
/// Passed through to normalize. When [normalizeOperation] isn't passed [acceptPartialData],
/// It will simply return `null` if any part of the query can't be constructed.
@override
bool get acceptPartialData =>
partialDataPolicy == PartialDataCachePolicy.accept;

/// `typePolicies` to pass down to [normalize]
final Map<String, TypePolicy> typePolicies;

/// Optional `dataIdFromObject` function to pass through to [normalize]
final DataIdResolver dataIdFromObject;

/// Input variable sanitizer for referencing custom scalar types in cache keys.
///
/// Defaults to [sanitizeFilesForCache]. Can be set to `null` to disable sanitization.
/// If present, a sanitizer will be built with [variableSanitizer]
@override
final SanitizeVariables sanitizeVariables;

Expand Down Expand Up @@ -116,7 +130,7 @@ class GraphQLCache extends NormalizingDataProxy {
/// Write normalized data into the cache,
/// deeply merging maps with existing values
///
/// Called from [writeQuery] and [writeFragment].
/// Called from [witeQuery] and [writeFragment].
void writeNormalized(String dataId, dynamic value) {
if (value is Map<String, Object>) {
final existing = store.get(dataId);
Expand Down Expand Up @@ -190,3 +204,19 @@ class GraphQLCache extends NormalizingDataProxy {
broadcastRequested = true;
}
}

/// Determines how partial data should be handled when written to the cache
///
/// [acceptForOptimisticData] is the default and recommended, as it provides
/// the best balance between safety and usability.
enum PartialDataCachePolicy {
/// Accept partial data in all cases (default).
accept,

/// Accept partial data, but only for transient optimistic writes made with
/// [GraphQLCache.recordOptimisticTransaction]
acceptForOptimisticData,

/// Reject partial data in all cases.
reject,
}
10 changes: 10 additions & 0 deletions packages/graphql/lib/src/cache/data_proxy.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import 'package:gql_exec/gql_exec.dart' show Request;
import 'package:graphql/src/cache/_normalizing_data_proxy.dart';
import 'package:graphql/src/exceptions/exceptions_next.dart';

import './fragment.dart';

Expand Down Expand Up @@ -118,6 +120,10 @@ abstract class GraphQLDataProxy {
///
/// For complex `normalize` type policies that involve custom reads,
/// `optimistic` will be the default.
///
/// Will throw a [PartialDataException] if the [data] structure
/// doesn't match that of the [request] `operation.document`,
/// or a [CacheMisconfigurationException] if the write fails for some other reason.
void writeQuery(
Request request, {
Map<String, dynamic> data,
Expand All @@ -133,6 +139,10 @@ abstract class GraphQLDataProxy {
///
/// For complex `normalize` type policies that involve custom reads,
/// `optimistic` will be the default.
///
/// Will throw a [PartialDataException] if the [data] structure
/// doesn't match that of the [fragmentRequest] `fragment.document`,
/// or a [CacheMisconfigurationException] if the write fails for some other reason.
void writeFragment(
FragmentRequest fragmentRequest, {
Map<String, dynamic> data,
Expand Down
Loading