Raise error for unused @tag directives (kensho-technologies#224)

* Add tag usage validator

* Update tests with unused @tags

* Create unused tag test

* Enable FoldScopeLocation filter recording

* Fix lint

* Add dummy filters to tag tests

* Add TagInfo to metadata

* Use only metadata to validate tag usage

* Add validate tag usage docstring

* Move tag and variable check to helpers

* Nits

* Create helper to get argument names

* Improve unused tag error message

* Fix metadata tag methods

* Fix IR generation tests

Author: evantey14

graphql_compiler/compiler/compiler_frontend.py

@@ -85,12 +85,12 @@
 )
 from .filters import process_filter_directive
 from .helpers import (
-    FoldScopeLocation, Location, get_ast_field_name, get_edge_direction_and_name,
-    get_field_type_from_schema, get_uniquely_named_objects_by_name, get_vertex_field_type,
-    invert_dict, is_vertex_field_name, strip_non_null_from_type, validate_output_name,
-    validate_safe_string
+    FoldScopeLocation, Location, get_ast_field_name, get_directive_argument_name,
+    get_edge_direction_and_name, get_field_type_from_schema, get_uniquely_named_objects_by_name,
+    get_vertex_field_type, invert_dict, is_tag_argument, is_vertex_field_name,
+    strip_non_null_from_type, validate_output_name, validate_safe_string
 )
-from .metadata import LocationInfo, QueryMetadataTable, RecurseInfo
+from .metadata import LocationInfo, QueryMetadataTable, RecurseInfo, TagInfo
 
 
 # LocationStackEntry contains the following:
@@ -292,6 +292,7 @@ def _compile_property_ast(schema, current_schema_type, ast, location,
             'optional': is_in_optional_scope(context),
             'type': strip_non_null_from_type(current_schema_type),
         }
+        context['metadata'].record_tag_info(tag_name, TagInfo(location=location))
 
     output_directive = unique_local_directives.get('output', None)
     if output_directive:
@@ -718,6 +719,24 @@ def _compile_ast_node_to_ir(schema, current_schema_type, ast, location, context)
     return basic_blocks
 
 
+def _validate_all_tags_are_used(metadata):
+    """Ensure all tags are used in some filter."""
+    tag_names = set([tag_name for tag_name, _ in metadata.tags])
+    filter_arg_names = set()
+    for location, _ in metadata.registered_locations:
+        for filter_info in metadata.get_filter_infos(location):
+            for filter_arg in filter_info.args:
+                if is_tag_argument(filter_arg):
+                    filter_arg_names.add(get_directive_argument_name(filter_arg))
+
+    unused_tags = tag_names - filter_arg_names
+    if unused_tags:
+        raise GraphQLCompilationError(u'This GraphQL query contains @tag directives whose values '
+                                      u'are not used: {}. This is not allowed. Please either use '
+                                      u'them in a filter or remove them entirely.'
+                                      .format(unused_tags))
+
+
 def _compile_root_ast_to_ir(schema, ast, type_equivalence_hints=None):
     """Compile a full GraphQL abstract syntax tree (AST) to intermediate representation.
 
@@ -819,6 +838,8 @@ def _compile_root_ast_to_ir(schema, ast, type_equivalence_hints=None):
         schema, current_schema_type, base_ast, location, context)
     basic_blocks.extend(new_basic_blocks)
 
+    _validate_all_tags_are_used(context['metadata'])
+
     # All operations after this point affect the global query scope, and are not related to
     # the "current" location in the query produced by the sequence of Traverse/Backtrack blocks.
     basic_blocks.append(blocks.GlobalOperationsStart())

graphql_compiler/compiler/filters.py

@@ -8,8 +8,8 @@
 from . import blocks, expressions
 from ..exceptions import GraphQLCompilationError, GraphQLValidationError
 from .helpers import (
-    get_uniquely_named_objects_by_name, is_vertex_field_name, is_vertex_field_type,
-    strip_non_null_from_type, validate_safe_string
+    get_uniquely_named_objects_by_name, is_tag_argument, is_variable_argument, is_vertex_field_name,
+    is_vertex_field_type, strip_non_null_from_type, validate_safe_string
 )
 from .metadata import FilterInfo
 
@@ -79,16 +79,6 @@ def wrapper(filter_operation_info, location, context, parameters, *args, **kwarg
     return decorator
 
 
-def _is_variable_argument(argument_name):
-    """Return True if the argument is a runtime variable, and False otherwise."""
-    return argument_name.startswith('$')
-
-
-def _is_tag_argument(argument_name):
-    """Return True if the argument is a tagged value, and False otherwise."""
-    return argument_name.startswith('%')
-
-
 def _represent_argument(directive_location, context, argument, inferred_type):
     """Return a two-element tuple that represents the argument to the directive being processed.
 
@@ -112,7 +102,7 @@ def _represent_argument(directive_location, context, argument, inferred_type):
     argument_name = argument[1:]
     validate_safe_string(argument_name)
 
-    if _is_variable_argument(argument):
+    if is_variable_argument(argument):
         existing_type = context['inputs'].get(argument_name, inferred_type)
         if not inferred_type.is_same_type(existing_type):
             raise GraphQLCompilationError(u'Incompatible types inferred for argument {}. '
@@ -122,7 +112,7 @@ def _represent_argument(directive_location, context, argument, inferred_type):
         context['inputs'][argument_name] = inferred_type
 
         return (expressions.Variable(argument, inferred_type), None)
-    elif _is_tag_argument(argument):
+    elif is_tag_argument(argument):
         argument_context = context['tags'].get(argument_name, None)
         if argument_context is None:
             raise GraphQLCompilationError(u'Undeclared argument used: {}'.format(argument))
@@ -246,7 +236,7 @@ def _process_has_edge_degree_filter_directive(filter_operation_info, location, c
                              u'"has_edge_degree" filter: {}'.format(filter_operation_info))
 
     argument = parameters[0]
-    if not _is_variable_argument(argument):
+    if not is_variable_argument(argument):
         raise GraphQLCompilationError(u'The "has_edge_degree" filter only supports runtime '
                                       u'variable arguments. Tagged values are not supported.'
                                       u'Argument name: {}'.format(argument))

graphql_compiler/compiler/helpers.py

@@ -242,6 +242,21 @@ def invert_dict(invertible_dict):
     return inverted
 
 
+def is_variable_argument(argument):
+    """Return True if the directive argument is a runtime variable, and False otherwise."""
+    return argument.startswith('$')
+
+
+def is_tag_argument(argument):
+    """Return True if the directive argument is a tagged value, and False otherwise."""
+    return argument.startswith('%')
+
+
+def get_directive_argument_name(argument):
+    """Return the name of a variable or tag argument without the $ or %."""
+    return argument[1:]
+
+
 @total_ordering
 @six.add_metaclass(ABCMeta)
 class BaseLocation(object):

graphql_compiler/compiler/metadata.py

@@ -4,7 +4,7 @@
 
 import six
 
-from .helpers import FoldScopeLocation, Location
+from .helpers import Location
 
 
 LocationInfo = namedtuple(
@@ -24,6 +24,13 @@
 )
 
 
+TagInfo = namedtuple(
+    'TagInfo',
+    (
+        'location',
+    )
+)
+
 FilterInfo = namedtuple(
     'FilterInfo',
     (
@@ -154,11 +161,27 @@ def get_location_info(self, location):
                                  u'{}'.format(location))
         return location_info
 
+    @property
+    def tags(self):
+        """Return an iterable of (tag_name, tag_info) tuples for all tags in the query."""
+        for tag_name, tag_info in six.iteritems(self._tags):
+            yield tag_name, tag_info
+
+    def record_tag_info(self, tag_name, tag_info):
+        """Record information about the tag."""
+        old_info = self._tags.get(tag_name, None)
+        if old_info is not None:
+            raise AssertionError(u'Attempting to define an already-defined tag {}. '
+                                 u'old info {}, new info {}'
+                                 .format(tag_name, old_info, tag_info))
+        self._tags[tag_name] = tag_info
+
+    def get_tag_info(self, tag_name):
+        """Get information about a tag."""
+        return self._tags.get(tag_name, None)
+
     def record_filter_info(self, location, filter_info):
         """Record filter information about the location."""
-        if isinstance(location, FoldScopeLocation):
-            # NOTE(gurer): ignore filters inside the fold for now
-            return
         record_location = location.at_vertex()
         self._filter_infos.setdefault(record_location, []).append(filter_info)
 

graphql_compiler/tests/test_explain_info.py

@@ -3,7 +3,7 @@
 
 from . import test_input_data
 from ..compiler.compiler_frontend import graphql_to_ir
-from ..compiler.helpers import Location
+from ..compiler.helpers import FoldScopeLocation, Location
 from ..compiler.metadata import FilterInfo, RecurseInfo
 from .test_helpers import get_schema
 
@@ -66,8 +66,14 @@ def test_complex_optional_traversal_variables(self):
                    [])
 
     def test_coercion_filters_and_multiple_outputs_within_fold_scope(self):
+        loc = FoldScopeLocation(Location(('Animal',), None, 1), (('out', 'Entity_Related'),), None)
+        filters = [
+            FilterInfo(fields=('name',), op_name='has_substring', args=('$substring',)),
+            FilterInfo(fields=('birthday',), op_name='<=', args=('$latest',)),
+        ]
+
         self.check(test_input_data.coercion_filters_and_multiple_outputs_within_fold_scope,
-                   [],
+                   [(loc, filters)],
                    [])
 
     def test_multiple_filters(self):

graphql_compiler/tests/test_ir_generation_errors.py

@@ -445,6 +445,9 @@ def test_tag_directive_constraints(self):
                 out_Animal_ParentOf @tag(tag_name: "role") {
                     uuid @output(out_name: "uuid")
                 }
+                in_Animal_ParentOf {
+                    name @filter(op_name: "!=", value: ["%role"])
+                }
             }
         }''')
 
@@ -462,6 +465,9 @@ def test_tag_directive_constraints(self):
                 out_Animal_ParentOf {
                     uuid @tag(tag_name: "uuid")
                 }
+                in_Animal_ParentOf {
+                    uuid @filter(op_name: "!=", value: ["%uuid"])
+                }
             }
         }''')
 
@@ -478,6 +484,9 @@ def test_tag_directive_constraints(self):
                     name @tag(tag_name: "name")
                     uuid @output(out_name: "uuid")
                 }
+                in_Animal_ParentOf {
+                    name @filter(op_name: "!=", value: ["%name"])
+                }
             }
         }''')
 
@@ -487,6 +496,16 @@ def test_tag_directive_constraints(self):
                 out_Animal_ParentOf {
                     _x_count @tag(tag_name: "count")
                 }
+                in_Animal_ParentOf {
+                   _x_count @filter(op_name: "=", value: ["%count"])
+                }
+            }
+        }''')
+
+        unused_tag = (GraphQLCompilationError, '''{
+            Animal {
+                name @tag(tag_name: "name")
+                uuid @output(out_name: "uuid")
             }
         }''')
 
@@ -497,6 +516,7 @@ def test_tag_directive_constraints(self):
             tag_with_illegal_name,
             tag_within_fold_scope,
             tag_on_count_field,
+            unused_tag,
         )
 
         for expected_error, graphql in errors_and_inputs:
@@ -730,6 +750,7 @@ def test_duplicate_directive_on_same_field(self):
             Event {
                 name @tag(tag_name: "name")
                 event_date @output(out_name: "date")
+                description @filter(op_name: "has_substring", value: ["%name"])
             }
         }''')
 
@@ -738,6 +759,8 @@ def test_duplicate_directive_on_same_field(self):
                 Event {
                     name @tag(tag_name: "name") @tag(tag_name: "name2")
                     event_date @output(out_name: "date")
+                    description @filter(op_name: "has_substring", value: ["%name"])
+                                @filter(op_name: "has_substring", value: ["%name2"])
                 }
             }''')
 
@@ -750,6 +773,11 @@ def test_property_field_after_vertex_field(self):
                 in_Animal_FedAt {
                     name @output(out_name: "animal")
                 }
+                in_Event_RelatedEvent {
+                    ... on Event {
+                        event_date @filter(op_name: "=", value: ["%date"])
+                    }
+                }
             }
         }''')
 
@@ -761,6 +789,11 @@ def test_property_field_after_vertex_field(self):
                         name @output(out_name: "animal")
                     }
                     event_date @tag(tag_name: "date")
+                    in_Event_RelatedEvent {
+                        ... on Event {
+                            event_date @filter(op_name: "=", value: ["%date"])
+                        }
+                    }
                 }
             }''', '''{
                 FeedingEvent {
@@ -769,6 +802,11 @@ def test_property_field_after_vertex_field(self):
                     }
                     name @output(out_name: "name")
                     event_date @tag(tag_name: "date")
+                    in_Event_RelatedEvent {
+                        ... on Event {
+                            event_date @filter(op_name: "=", value: ["%date"])
+                        }
+                    }
                 }
             }'''
         )