Skip to content

Add 'getDefinitionsMap' to 'ASTValidationContext' #1476

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

Closed
Closed
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
40 changes: 27 additions & 13 deletions src/validation/ValidationContext.js
Original file line number Diff line number Diff line change
@@ -7,11 +7,13 @@
* @flow strict
*/

import keyMap from '../jsutils/keyMap';
import type { ObjMap } from '../jsutils/ObjMap';
import type { GraphQLError } from '../error';
import { visit, visitWithTypeInfo } from '../language/visitor';
import { Kind } from '../language/kinds';
import type {
ASTKindToNode,
DocumentNode,
OperationDefinitionNode,
VariableNode,
@@ -38,6 +40,10 @@ type VariableUsage = {|
+defaultValue: ?mixed,
|};

type KindToNodesMap = $Shape<
$ObjMap<ASTKindToNode, <Node>(Node) => ?Array<Node>>,
>;

/**
* An instance of this class is passed as the "this" context to all validators,
* allowing access to commonly useful contextual information from within a
@@ -46,19 +52,29 @@ type VariableUsage = {|
export class ASTValidationContext {
_ast: DocumentNode;
_errors: Array<GraphQLError>;
_fragments: ?ObjMap<FragmentDefinitionNode>;
_fragments: ObjMap<FragmentDefinitionNode>;
_fragmentSpreads: Map<SelectionSetNode, $ReadOnlyArray<FragmentSpreadNode>>;
_recursivelyReferencedFragments: Map<
OperationDefinitionNode,
$ReadOnlyArray<FragmentDefinitionNode>,
>;
_defsByKind: KindToNodesMap;

constructor(ast: DocumentNode): void {
this._ast = ast;
this._errors = [];
this._fragments = undefined;
this._fragmentSpreads = new Map();
this._recursivelyReferencedFragments = new Map();

const defsByKind = Object.create(null);
for (const node of ast.definitions) {
if (defsByKind[node.kind]) {
defsByKind[node.kind].push(node);
} else {
defsByKind[node.kind] = [node];
}
}
this._defsByKind = defsByKind;
}

reportError(error: GraphQLError): void {
@@ -73,20 +89,18 @@ export class ASTValidationContext {
return this._ast;
}

getDefinitionsMap(): KindToNodesMap {
return this._defsByKind;
}

getFragment(name: string): ?FragmentDefinitionNode {
let fragments = this._fragments;
if (!fragments) {
this._fragments = fragments = this.getDocument().definitions.reduce(
(frags, statement) => {
if (statement.kind === Kind.FRAGMENT_DEFINITION) {
frags[statement.name.value] = statement;
}
return frags;
},
Object.create(null),
if (!this._fragments) {
this._fragments = keyMap(
this.getDefinitionsMap().FragmentDefinition || [],
def => def.name.value,
);
}
return fragments[name];
return this._fragments[name];
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 still move the this._fragments instantiation to here:

if (!this._fragments) {
  const fragments = keyMap(...);
  this._fragments = fragments;
  return fragments[name];
}
return this._fragments[name];

No point incurring the cost of object-creation until someone actually asks for it.

}

getFragmentSpreads(
8 changes: 3 additions & 5 deletions src/validation/rules/KnownDirectives.js
Original file line number Diff line number Diff line change
@@ -47,11 +47,9 @@ export function KnownDirectives(
locationsMap[directive.name] = directive.locations;
}

const astDefinitions = context.getDocument().definitions;
for (const def of astDefinitions) {
if (def.kind === Kind.DIRECTIVE_DEFINITION) {
locationsMap[def.name.value] = def.locations.map(name => name.value);
}
const defNodes = context.getDefinitionsMap().DirectiveDefinition || [];
Copy link
Contributor

@mjmahone mjmahone Aug 24, 2018

Choose a reason for hiding this comment

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

I think it makes more sense to have both contexts have a getDirectiveDefinitions() by having ASTValidationContext define the function. On the other hand, if we can hide getDefinitionsMap somehow from being publicly exposed, I'm OK with the current API.

One nice thing with not exposing it: we could later easily add a getFragmentDefinitions() and getOperationDefinitions() using the same private structure, without ever publicly exposing the somewhat complex KindToNodesMap.

My big worry with getDefinitionsMap is that the type signature for it is a bit strange to me. It's very clever, which makes it hard for a newcomer to immediately grasp. Whereas getDirectiveDefinitions(): $ReadOnlyArray<DirectiveDefinitionNode> obviously returns all the known directives, and the type signature supports that.

for (const node of defNodes) {
locationsMap[node.name.value] = node.locations.map(name => name.value);
}

return {
9 changes: 2 additions & 7 deletions src/validation/rules/LoneAnonymousOperation.js
Original file line number Diff line number Diff line change
@@ -9,7 +9,6 @@

import type { ASTValidationContext } from '../ValidationContext';
import { GraphQLError } from '../../error/GraphQLError';
import { Kind } from '../../language/kinds';
import type { ASTVisitor } from '../../language/visitor';

export function anonOperationNotAloneMessage(): string {
@@ -25,13 +24,9 @@ export function anonOperationNotAloneMessage(): string {
export function LoneAnonymousOperation(
context: ASTValidationContext,
): ASTVisitor {
let operationCount = 0;
const operationDefs = context.getDefinitionsMap().OperationDefinition;
const operationCount = operationDefs ? operationDefs.length : 0;
return {
Document(node) {
operationCount = node.definitions.filter(
definition => definition.kind === Kind.OPERATION_DEFINITION,
).length;
},
OperationDefinition(node) {
if (!node.name && operationCount > 1) {
context.reportError(
43 changes: 15 additions & 28 deletions src/validation/rules/NoUnusedFragments.js
Original file line number Diff line number Diff line change
@@ -22,38 +22,25 @@ export function unusedFragMessage(fragName: string): string {
* within operations, or spread within other fragments spread within operations.
*/
export function NoUnusedFragments(context: ASTValidationContext): ASTVisitor {
const operationDefs = [];
const fragmentDefs = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

In the context of GQL documents getDefinitionsMap is a just reusable version of this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very clever. I like the idea behind it a lot. I'm not sure it makes sense to expose publicly yet, but for the internal rules it makes a lot of sense to use something like getDefinitionsMap directly.

const fragmentNameUsed = Object.create(null);
const operationDefs = context.getDefinitionsMap().OperationDefinition || [];
for (const operation of operationDefs) {
for (const fragment of context.getRecursivelyReferencedFragments(
operation,
)) {
fragmentNameUsed[fragment.name.value] = true;
}
}

return {
OperationDefinition(node) {
operationDefs.push(node);
return false;
},
FragmentDefinition(node) {
fragmentDefs.push(node);
const fragName = node.name.value;
if (fragmentNameUsed[fragName] !== true) {
context.reportError(
new GraphQLError(unusedFragMessage(fragName), [node]),
);
}
return false;
},
Document: {
leave() {
const fragmentNameUsed = Object.create(null);
for (const operation of operationDefs) {
for (const fragment of context.getRecursivelyReferencedFragments(
operation,
)) {
fragmentNameUsed[fragment.name.value] = true;
}
}

for (const fragmentDef of fragmentDefs) {
const fragName = fragmentDef.name.value;
if (fragmentNameUsed[fragName] !== true) {
context.reportError(
new GraphQLError(unusedFragMessage(fragName), [fragmentDef]),
);
}
}
},
},
};
}