Skip to content

Commit

Permalink
Fix issue #25 -- validate the parameters/args used in directives agai…
Browse files Browse the repository at this point in the history
…nst the variables defined in the operation (and tests)
  • Loading branch information
jcward committed Jul 23, 2018
1 parent 00eb2f6 commit f929e48
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 20 deletions.
54 changes: 40 additions & 14 deletions proj/hxgen/src/graphql/HaxeGenerator.hx
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ class HaxeGenerator

// Fragments can have fragments... Do the order of these calls need to be determinant?
generate_type_based_on_selection_set(tname,
tname,
{ debug_name:tname, is_operation:false },
def.selectionSet,
on_type);
}
Expand Down Expand Up @@ -416,11 +416,11 @@ class HaxeGenerator
case TAnon(def): def;
}

function resolve_field(path:Array<String>, ?op_name:String):GQLFieldType
function resolve_field(path:Array<String>, ?gt_info:GenTypeInfo):GQLFieldType
{
var ptr:GQLTypeDefinition = null;

var err_prefix = op_name!=null ? 'Error processing operation ${ op_name }: ' : "";
var err_prefix = gt_info!=null ? 'Error processing operation ${ gt_info.debug_name }: ' : "";

var orig_path = path.join('.');
while (path.length>0) {
Expand Down Expand Up @@ -579,12 +579,12 @@ class HaxeGenerator
}

function generate_type_based_on_selection_set(type_name:String,
op_name:String,
gt_info:GenTypeInfo,
sel_set:{ selections:Array<SelectionNode> },
base_type:String,
depth:Int=0):GQLTypeDefinition
{
if (_basic_types.indexOf(base_type)>=0) throw 'Cannot create a fragment or operation ${ op_name } on a basic type, ${ base_type }';
if (_basic_types.indexOf(base_type)>=0) throw 'Cannot create a fragment or operation ${ gt_info.debug_name } on a basic type, ${ base_type }';

var possible_object_types = get_possible_object_types_from(base_type);

Expand Down Expand Up @@ -619,15 +619,15 @@ class HaxeGenerator
if (is_union(base_type)) {
field_nodes_per_object_type[obj_type] = [];
} else {
error('Error: fragment or op ${ op_name } selected no fields for possible object type ${ obj_type }');
error('Error: fragment or op ${ gt_info.debug_name } selected no fields for possible object type ${ obj_type }');
}
}
}

// Error on unused field pecifications
// Error on unused field specifications
for (cf in constrained_fields) {
if (cf.usage==0) {
error('Error: fragment or op ${ op_name } specified field ${ (cast cf.field_node).name.value } that didn\'t get used in possible types [${ possible_object_types.join(", ") }] via constraints [${ cf.constraints.join(", ") }]');
error('Error: fragment or op ${ gt_info.debug_name } specified field ${ (cast cf.field_node).name.value } that didn\'t get used in possible types [${ possible_object_types.join(", ") }] via constraints [${ cf.constraints.join(", ") }]');
}
}

Expand Down Expand Up @@ -655,25 +655,27 @@ class HaxeGenerator
var name:String = field_node.name.value;
var alias:String = field_node.alias==null ? name : field_node.alias.value;

if (gt_info.is_operation) validate_directives(field_node, gt_info);

// Ignore fields specified more than once (with the same alias)
var dup_key = '$name -> $alias';
if (ignore_duplicates.indexOf(dup_key)>=0) continue;
ignore_duplicates.push(dup_key);

var next_type_path = type_path.slice(0);
next_type_path.push(name);
var resolved = resolve_field(next_type_path, op_name);
var resolved = resolve_field(next_type_path, gt_info);
var type = resolve_type(resolved.type);

switch type {
case TBasic(tname) | TScalar(tname) | TEnum(tname, _):
if (has_sub_selections) throw 'Cannot specify sub-fields of ${ tname } at ${ type_path.join(".") } of operation ${ op_name }';
if (has_sub_selections) throw 'Cannot specify sub-fields of ${ tname } at ${ type_path.join(".") } of operation ${ gt_info.debug_name }';
fields[alias] = resolved;
case TStruct(tname, _) | TUnion(tname, _):
if (!has_sub_selections) throw 'Must specify sub-fields of ${ tname } at ${ type_path.join(".") } of operation ${ op_name }';
if (!has_sub_selections) throw 'Must specify sub-fields of ${ tname } at ${ type_path.join(".") } of operation ${ gt_info.debug_name }';
if (is_union(tname)) {
if (field_node.selectionSet.selections.find(function(sn) return sn.kind==Kind.FIELD)!=null) {
throw 'Can only specify fragment selections of union ${ tname } at ${ type_path.join(".") } of operation ${ op_name }';
throw 'Can only specify fragment selections of union ${ tname } at ${ type_path.join(".") } of operation ${ gt_info.debug_name }';
}
// Should probably check this elsewhere...
// for (tv in values) if (!is_object_type(tv)) throw 'Union $tname must only consist of Object types per GraphQL spec.';
Expand All @@ -682,7 +684,7 @@ class HaxeGenerator
var sub_type_name = (depth==0 && StringTools.endsWith(type_name, OP_OUTER_SUFFIX)) ?
StringTools.replace(type_name, OP_OUTER_SUFFIX, OP_INNER_SUFFIX) : type_name+DEFAULT_SEPARATOR+name;

var sub_type = generate_type_based_on_selection_set(sub_type_name, op_name, field_node.selectionSet, tname, depth+1);
var sub_type = generate_type_based_on_selection_set(sub_type_name, gt_info, field_node.selectionSet, tname, depth+1);
var f = { type:null, is_array:resolved.is_array, is_optional:resolved.is_optional };
fields[alias] = f;
if (is_union(sub_type_name)) {
Expand Down Expand Up @@ -710,6 +712,25 @@ class HaxeGenerator

}

// Per the discussion at https://github.com/facebook/graphql/issues/204, the operation
// variables are global, and any variables used herein (both in fragments or regular fields)
// must be defined in the operation variables.
function validate_directives(field_node:FieldNode, gt_info:GenTypeInfo)
{
if (field_node.directives!=null) {
for (dir in field_node.directives) {
if (dir.arguments!=null) for (arg in dir.arguments) {
// here, arg.name.value is 'if', and arg.value.name.value is 'my_param'
var param = (cast arg.value).name.value;
//trace('Found param: ${ param }');
var valid = false;
for (vrr in gt_info.op_variables) if (vrr.variable.name.value==param) valid = true;
if (!valid) throw 'Error: operation ${ gt_info.debug_name } is expecting parameter ${ param } which hasn\'t been defined in the operation variables!';
}
}
}
}

function write_operation_def_result(root_schema:SchemaMap,
root:ASTDefs.DocumentNode,
def:ASTDefs.OperationDefinitionNode):OpVariables
Expand All @@ -729,7 +750,7 @@ class HaxeGenerator

// gen type based on selection set
generate_type_based_on_selection_set('${ OP_PREFIX }${ op_name }${ OP_OUTER_SUFFIX }',
op_name,
{ debug_name:op_name, is_operation:true, op_variables:def.variableDefinitions },
def.selectionSet,
op_root_type);

Expand Down Expand Up @@ -771,6 +792,11 @@ abstract ID(String) to String from String {\n // Relaxed safety -- allow implic
}
}

typedef GenTypeInfo = {
debug_name:String,
is_operation:Bool, // when false, it's a fragment
?op_variables:Array<VariableDefinitionNode>
}

class StringWriter
{
Expand Down
1 change: 1 addition & 0 deletions test/buddy/Main.hx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class Main {
new tests.operations.QueryTypeGeneration(),
new tests.operations.MutationTypeGeneration(),
new tests.operations.UnnamedQuery(),
new tests.operations.VerifyDirectives(),
new tests.star_wars.StarWarsTest(),

new tests.fragments.FragmentTest(),
Expand Down
144 changes: 144 additions & 0 deletions test/buddy/tests/operations/VerifyDirectives.hx
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package tests.operations;

import buddy.*;
using buddy.Should;

class VerifyDirectives extends BuddySuite
{

public static inline var common_gql = '

# Creates typedefs for all schema types

type FooData {
foo_id:ID!
}

type BarData {
bar_id:ID!
}

union SomeData = FooData | BarData

type OuterData {
id:ID!
title:String!
description:String
foo_or_bar_data: SomeData!
}

type Query {
get_content_by_id: OuterData
}

';

public function new() {
describe("VerifyDirectives:", {

it('should validate these directives without error', {
var parser = new graphql.parser.Parser(common_gql+'

fragment InnerFrag on OuterData {
foo_or_bar_data @include(if: $$with_data) {
...on FooData {
common_id: foo_id
}
...on BarData {
common_id: bar_id
}
}
}

query GetContentsByID($$id: ID, $$with_data: Boolean=false) {
get_content_by_id(id: $$id) {
title
description
foo_or_bar_data @include(if: $$with_data) {
...on FooData {
foo_id
}
}
...InnerFrag
}
}

');

var result = graphql.HaxeGenerator.parse(parser.document).stdout;
result.should.contain("typedef OP_GetContentsByID_Result");
});



it('should throw on a query directive typo', {
var parser = new graphql.parser.Parser(common_gql+'

fragment InnerFrag on OuterData {
foo_or_bar_data @include(if: $$with_data) {
...on FooData {
common_id: foo_id
}
...on BarData {
common_id: bar_id
}
}
}

query GetContentsByID($$id: ID, $$with_data: Boolean=false) {
get_content_by_id(id: $$id) {
title
description
foo_or_bar_data @include(if: $$with_a_typo) {
...on FooData {
foo_id
}
}
...InnerFrag
}
}

');

var err = graphql.HaxeGenerator.parse.bind(parser.document).should.throwType(String);
err.should.contain("operation GetContentsByID is expecting parameter with_a_typo");
});


it('should throw on a fragment directive typo', {
var parser = new graphql.parser.Parser(common_gql+'

fragment InnerFrag on OuterData {
foo_or_bar_data @include(if: $$with_frag_typo) {
...on FooData {
common_id: foo_id
}
...on BarData {
common_id: bar_id
}
}
}

query GetContentsByID($$id: ID, $$with_data: Boolean=false) {
get_content_by_id(id: $$id) {
title
description
foo_or_bar_data @include(if: $$with_data) {
...on FooData {
foo_id
}
}
...InnerFrag
}
}

');

var err = graphql.HaxeGenerator.parse.bind(parser.document).should.throwType(String);
err.should.contain("operation GetContentsByID is expecting parameter with_frag_typo");
});

});
}

}
24 changes: 18 additions & 6 deletions test/manual_debug/Test.hx
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,40 @@ type BarContent {
bar_id:ID!
}

union ExtContentData = FooContent | BarContent
union SomeData = FooContent | BarContent

type ContentData {
type OuterData {
id:ID!
title:String!
description:String
ext_content_data: ExtContentData!
foo_or_bar_data: SomeData!
}

type Query {
get_content_by_id: ContentData
get_content_by_id: OuterData
}

query GetContentsByID($$id: ID) {
fragment InnerFrag on OuterData {
foo_or_bar_data @include(if: $$with_data) {
...on FooContent {
common_id: foo_id
}
...on BarContent {
common_id: bar_id
}
}
}

query GetContentsByID($$id: ID, $$with_data: Boolean=false) {
get_content_by_id(id: $$id) {
title
description
ext_content_data {
foo_or_bar_data @include(if: $$with_data) {
...on FooContent {
foo_id
}
}
...InnerFrag
}
}

Expand Down

0 comments on commit f929e48

Please sign in to comment.