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.

Sign up for GitHub

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

Refactor query reading to use graphql-anywhere #747

Merged
merged 14 commits into from
Oct 6, 2016
Merged

Refactor query reading to use graphql-anywhere #747

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
09d58f0
Some tests pass
Oct 5, 2016
80a0ff2
Generate fragment definitions in doc
Oct 5, 2016
69727a7
Fix a whole host of tests
Oct 5, 2016
a52f443
Lol how did I screw that one up
Oct 5, 2016
23afba0
Properly record missing fields
Oct 5, 2016
8dda7fe
Improvements
Oct 5, 2016
0aa563f
Import as graphqlAnywhere
Oct 5, 2016
1c59ca0
Remove unused function
Oct 5, 2016
d8a4879
Remove SelectionSet interface
Oct 5, 2016
82dc4b4
Fix tests
Oct 5, 2016
cce085b
Remove readFragmentFromStore function
Oct 6, 2016
fe7f013
Remove selection set API from read
Oct 6, 2016
f6ac722
Clarify change
Oct 6, 2016
b6f10c9
Update changelog
Oct 6, 2016
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: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"license": "MIT",
"dependencies": {
"es6-promise": "^4.0.3",
"graphql-anywhere": "^0.1.11",
"graphql-tag": "^0.1.13",
"lodash.assign": "^4.0.8",
"lodash.clonedeep": "^4.3.2",
Expand Down
1 change: 0 additions & 1 deletion src/data/debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import isObject = require('lodash.isobject');
import omit = require('lodash.omit');
import mapValues = require('lodash.mapvalues');


export function stripLoc(obj: Object) {
if (isArray(obj)) {
return obj.map(stripLoc);
Expand Down
317 changes: 83 additions & 234 deletions src/data/diffAgainstStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
import isArray = require('lodash.isarray');
import isNull = require('lodash.isnull');
import isObject = require('lodash.isobject');
import has = require('lodash.has');
import merge = require('lodash.merge');

import {
storeKeyNameFromField,
resultKeyNameFromField,
isField,
isInlineFragment,
storeKeyNameFromFieldNameAndArgs,
} from './storeUtils';

import {
Expand All @@ -19,8 +10,9 @@ import {

import {
SelectionSet,
Field,
Document,
OperationDefinition,
FragmentDefinition,
} from 'graphql';

import {
Expand All @@ -29,14 +21,15 @@ import {
FragmentMap,
} from '../queries/getFromAST';

import {
shouldInclude,
} from '../queries/directives';

import {
ApolloError,
} from '../errors/ApolloError';

import graphql, {
Copy link
Contributor

@helfer helfer Oct 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So many things called graphql now... I guess that's life.

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'll change to graphql as graphqlAnywhere just to keep it classy.

Resolver,
ResultMapper,
} from 'graphql-anywhere';

export interface DiffResult {
result?: any;
isMissing?: boolean;
Expand Down Expand Up @@ -104,6 +97,52 @@ export function handleFragmentErrors(fragmentErrors: { [typename: string]: Error
}
}

type ReadStoreContext = {
store: NormalizedCache;
throwOnMissingField: boolean;
hasMissingField: boolean;
}

const readStoreResolver: Resolver = (
fieldName: string,
objId: string,
args: any,
context: ReadStoreContext
Copy link
Contributor

Choose a reason for hiding this comment

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

are these the standard arguments to graphql-anywhere resolvers? Can you walk me through graphql-anywhere execution some time, so I understand why fieldName and objectId are there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would have expected to somehow get the parent somehow, but apparently that's not how it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In graphql-anywhere, it's fieldName, rootValue, args, context. In this case, the rootValue just happens to be the object ID because that's what the parent resolver returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. I would have expected the parent to be the first argument. Maybe fieldName could be the last, since it's kind of like the info field in graphql?

Copy link
Contributor Author

@stubailo stubailo Oct 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it's the first argument because in regular GraphQL it's the first thing you write:

Query: {
  myFieldName: (root, args, context) => { ... }
}

In this case it's: (myFieldName, root, args, context) => { ... }

) => {
const obj = context.store[objId];
const storeKeyName = storeKeyNameFromFieldNameAndArgs(fieldName, args);
const fieldValue = (obj || {})[storeKeyName];

if (typeof fieldValue === 'undefined') {
if (context.throwOnMissingField) {
throw new ApolloError({
errorMessage: `Can't find field ${storeKeyName} on object (${objId}) ${JSON.stringify(obj, null, 2)}.
Perhaps you want to use the \`returnPartialData\` option?`,
extraInfo: {
isFieldError: true,
},
});
}

context.hasMissingField = true;

return fieldValue;
}

if (isJsonValue(fieldValue)) {
// if this is an object scalar, it must be a json blob and we have to unescape it
return fieldValue.json;
}

if (isIdValue(fieldValue)) {
return fieldValue.id;
}

return fieldValue;
};

const mapper: ResultMapper = (childValues, rootValue) => childValues;

/**
* Given a store, a root ID, and a selection set, return as much of the result as possible and
* identify which selection sets and root IDs need to be fetched to get the rest of the requested
Expand All @@ -130,238 +169,48 @@ export function diffSelectionSetAgainstStore({
variables: Object,
fragmentMap?: FragmentMap,
}): DiffResult {
if (selectionSet.kind !== 'SelectionSet') {
throw new Error('Must be a selection set.');
}
const doc = makeDocument(selectionSet, rootId, fragmentMap);

if (!fragmentMap) {
fragmentMap = {};
}
const context: ReadStoreContext = {
store,
throwOnMissingField,

const result = {};
let hasMissingFields = false;

// A map going from a typename to missing field errors thrown on that
// typename. This data structure is needed to support union types. For example, if we have
// a union type (Apple | Orange) and we only receive fields for fragments on
// "Apple", that should not result in an error. But, if at least one of the fragments
// for each of "Apple" and "Orange" is missing a field, that should return an error.
// (i.e. with this approach, we manage to handle missing fields correctly even for
// union types without any knowledge of the GraphQL schema).
let fragmentErrors: { [typename: string]: Error } = {};

selectionSet.selections.forEach((selection) => {
// Don't push more than one missing field per field in the query
let fieldResult: any;

const included = shouldInclude(selection, variables);

if (isField(selection)) {
const diffResult = diffFieldAgainstStore({
field: selection,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
included,
});
hasMissingFields = hasMissingFields || diffResult.isMissing;
fieldResult = diffResult.result;

const resultFieldKey = resultKeyNameFromField(selection);
if (included && fieldResult !== undefined) {
(result as any)[resultFieldKey] = fieldResult;
}
} else if (isInlineFragment(selection)) {
const typename = selection.typeCondition.name.value;

if (included) {
try {
const diffResult = diffSelectionSetAgainstStore({
selectionSet: selection.selectionSet,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
});

hasMissingFields = hasMissingFields || diffResult.isMissing;
fieldResult = diffResult.result;

if (isObject(fieldResult)) {
merge(result, fieldResult);
}

if (!fragmentErrors[typename]) {
fragmentErrors[typename] = null;
}
} catch (e) {
if (e.extraInfo && e.extraInfo.isFieldError) {
fragmentErrors[typename] = e;
} else {
throw e;
}
}
}
} else {
const fragment = fragmentMap[selection.name.value];

if (!fragment) {
throw new Error(`No fragment named ${selection.name.value}`);
}

const typename = fragment.typeCondition.name.value;

if (included) {
try {
const diffResult = diffSelectionSetAgainstStore({
selectionSet: fragment.selectionSet,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
});
hasMissingFields = hasMissingFields || diffResult.isMissing;
fieldResult = diffResult.result;

if (isObject(fieldResult)) {
merge(result, fieldResult);
}

if (!fragmentErrors[typename]) {
fragmentErrors[typename] = null;
}
} catch (e) {
if (e.extraInfo && e.extraInfo.isFieldError) {
fragmentErrors[typename] = e;
} else {
throw e;
}
}
}
}
});
// Filled in during execution
hasMissingField: false,
};

if (throwOnMissingField) {
handleFragmentErrors(fragmentErrors);
}
const result = graphql(readStoreResolver, doc, 'ROOT_QUERY', context, variables, mapper);
Copy link
Contributor

Choose a reason for hiding this comment

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

we could one-up graphql here and make the arguments named. (i know that's technically in graphql-anywhere, but I'm just putting all my thoughts here).

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the role of ROOT_QUERY here? It's kind of like a rootValue, but the resolvers don't return field names, so I'm unsure of how it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation has the resolvers return either the ID of the related object or a scalar leaf field. So the initial root value is the ID of the root object, which is ROOT_QUERY.


return {
result,
isMissing: hasMissingFields,
isMissing: context.hasMissingField,
};
}

function diffFieldAgainstStore({
field,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
included = true,
}: {
field: Field,
throwOnMissingField: boolean,
variables: Object,
// Shim to use graphql-anywhere, to be removed
function makeDocument(
selectionSet: SelectionSet,
rootId: string,
store: NormalizedCache,
fragmentMap?: FragmentMap,
included?: boolean,
}): DiffResult {
const storeObj = store[rootId] || {};
const storeFieldKey = storeKeyNameFromField(field, variables);

if (! has(storeObj, storeFieldKey)) {
if (throwOnMissingField && included) {
throw new ApolloError({
errorMessage: `Can't find field ${storeFieldKey} on object (${rootId}) ${JSON.stringify(storeObj, null, 2)}.
Perhaps you want to use the \`returnPartialData\` option?`,
extraInfo: {
isFieldError: true,
},
});
}

return {
isMissing: true,
};
}

const storeValue = storeObj[storeFieldKey];

// Handle all scalar types here
if (! field.selectionSet) {
if (isJsonValue(storeValue)) {
// if this is an object scalar, it must be a json blob and we have to unescape it
return {
result: storeValue.json,
};
} else {
// if this is a non-object scalar, we can return it immediately
return {
result: storeValue,
};
}
}

// From here down, the field has a selection set, which means it's trying to
// query a GraphQLObjectType
if (isNull(storeValue)) {
// Basically any field in a GraphQL response can be null
return {
result: null,
};
fragmentMap: FragmentMap
): Document {
if (rootId !== 'ROOT_QUERY') {
throw new Error('only supports query');
}

if (isArray(storeValue)) {
let isMissing: any;

const result = (storeValue as string[]).map((id) => {
// null value in array
if (isNull(id)) {
return null;
}

const itemDiffResult = diffSelectionSetAgainstStore({
store,
throwOnMissingField,
rootId: id,
selectionSet: field.selectionSet,
variables,
fragmentMap,
});

if (itemDiffResult.isMissing) {
// XXX merge all of the missing selections from the children to get a more minimal result
isMissing = 'true';
}
const op: OperationDefinition = {
kind: 'OperationDefinition',
operation: 'query',
selectionSet,
};

return itemDiffResult.result;
});
const frags: FragmentDefinition[] = fragmentMap ?
Object.keys(fragmentMap).map((name) => fragmentMap[name]) :
[];

return {
result,
isMissing,
};
}

// If the store value is an object and it has a selection set, it must be
// an escaped id.
if (isIdValue(storeValue)) {
const unescapedId = storeValue.id;
return diffSelectionSetAgainstStore({
store,
throwOnMissingField,
rootId: unescapedId,
selectionSet: field.selectionSet,
variables,
fragmentMap,
});
}
const doc: Document = {
kind: 'Document',
definitions: [op, ...frags],
};

throw new Error('Unexpected value in the store where the query had a subselection.');
return doc;
}
8 changes: 6 additions & 2 deletions src/data/storeUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,13 @@ export function storeKeyNameFromField(field: Field, variables?: Object): string
}

export function storeKeyNameFromFieldNameAndArgs(fieldName: string, args?: Object): string {
const stringifiedArgs: string = JSON.stringify(args);
if (args) {
const stringifiedArgs: string = JSON.stringify(args);
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, did we never do anything about alphabetical ordering etc? Because we'll definitely get a cache miss if the order isn't the same, even if the values are semantically equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we might want to do that. Perhaps something to open a new issue about, since it doesn't necessarily need to be part of the refactor.


return `${fieldName}(${stringifiedArgs})`;
return `${fieldName}(${stringifiedArgs})`;
}

return fieldName;
}

export function resultKeyNameFromField(field: Field): string {
Expand Down
Loading