Add more reverse macro edge validation logic.

graphql_compiler/macros/__init__.py

@@ -23,6 +23,7 @@
     DIRECTIVES_ALLOWED_IN_MACRO_EDGE_DEFINITION, DIRECTIVES_REQUIRED_IN_MACRO_EDGE_DEFINITION,
     MacroEdgeDirective
 )
+from .macro_edge.reversal import make_reverse_macro_edge_name
 from .macro_expansion import expand_macros_in_query_ast
 
 
@@ -80,7 +81,25 @@ def create_macro_registry(schema, type_equivalence_hints=None, subclass_sets=Non
         macro_edges_at_class=dict())
 
 
-def _check_macro_edge_for_definition_conflicts(macro_registry, base_class_name, macro_edge_name):
+def _find_macro_edge_name_at_subclass(macro_registry, class_name, macro_edge_name):
+    """Return the descriptor for a given macro edge defined on a subclass, if it exists."""
+    subclasses = macro_registry.subclass_sets[class_name]
+    if class_name not in subclasses:
+        raise AssertionError(u'Found a class that is not a subclass of itself, this means that the '
+                             u'subclass_sets value is incorrectly constructed: {} {} {}'
+                             .format(class_name, subclasses, macro_registry.subclass_sets))
+
+    for subclass_name in subclasses:
+        existing_descriptor = macro_registry.macro_edges_at_class.get(
+            subclass_name, dict()).get(macro_edge_name, None)
+
+        if existing_descriptor is not None:
+            return existing_descriptor
+
+    return None
+
+
+def _check_macro_edge_for_definition_conflicts(macro_registry, macro_edge_descriptor):
     """Ensure that the macro edge on the specified class does not cause any definition conflicts."""
     # There are two kinds of conflicts that we check for:
     # - defining this macro edge would not conflict with any macro edges that already exist
@@ -90,51 +109,86 @@ def _check_macro_edge_for_definition_conflicts(macro_registry, base_class_name,
     #   macro edge definitions.
     # We check for both of them simultaneously, by ensuring that none of the subclasses of the
     # base class name have a macro edge by the specified name.
-    subclasses = macro_registry.subclass_sets[base_class_name]
-    if base_class_name not in subclasses:
-        raise AssertionError(u'Found a class that is not a subclass of itself, this means that the '
-                             u'subclass_sets value is incorrectly constructed: {} {} {}'
-                             .format(base_class_name, subclasses, macro_registry.subclass_sets))
-
-    for subclass_name in subclasses:
-        existing_descriptor = macro_registry.macro_edges_at_class.get(
-            subclass_name, dict()).get(macro_edge_name, None)
-        if existing_descriptor is not None:
-            extra_error_text = u''
-            conflict_on_class_name = existing_descriptor.base_class_name
-            if conflict_on_class_name != base_class_name:
-                # The existing descriptor is defined elsewhere. Let's figure out if it's a subclass
-                # or a superclass conflict.
-                if base_class_name in macro_registry.subclass_sets[conflict_on_class_name]:
-                    relationship = 'supertype'
-                elif conflict_on_class_name in macro_registry.subclass_sets[base_class_name]:
-                    relationship = 'subtype'
-                else:
-                    raise AssertionError(u'Conflict between two macro edges defined on types that '
-                                         u'are not each other\'s supertype: {} {} {}'
-                                         .format(base_class_name, macro_edge_name, macro_registry))
-
-                extra_error_text = (
-                    u' (a {relationship} of {current_type})'
-                ).format(
-                    relationship=relationship,
+    base_class_name = macro_edge_descriptor.base_class_name
+    macro_edge_name = macro_edge_descriptor.macro_edge_name
+
+    existing_descriptor = _find_macro_edge_name_at_subclass(
+        macro_registry, base_class_name, macro_edge_name)
+
+    if existing_descriptor is not None:
+        extra_error_text = u''
+        conflict_on_class_name = existing_descriptor.base_class_name
+        if conflict_on_class_name != base_class_name:
+            # The existing descriptor is defined elsewhere. Let's figure out if it's a subclass
+            # or a superclass conflict.
+            if base_class_name in macro_registry.subclass_sets[conflict_on_class_name]:
+                relationship = 'supertype'
+            elif conflict_on_class_name in macro_registry.subclass_sets[base_class_name]:
+                relationship = 'subtype'
+            else:
+                raise AssertionError(u'Conflict between two macro edges defined on types that '
+                                     u'are not each other\'s supertype: {} {} {}'
+                                     .format(base_class_name, macro_edge_name, macro_registry))
+
+            extra_error_text = (
+                u' (a {relationship} of {current_type})'
+            ).format(
+                relationship=relationship,
+                current_type=base_class_name,
+            )
+
+        raise GraphQLInvalidMacroError(
+            u'A macro edge with name {edge_name} cannot be defined on type {current_type} due '
+            u'to a conflict with another macro edge with the same name defined '
+            u'on type {original_type}{extra_error_text}.'
+            u'Cannot define this conflicting macro, please verify '
+            u'if the existing macro edge does what you want, or rename your macro '
+            u'edge to avoid the conflict. Existing macro definition and args: '
+            u'{macro_graphql} {macro_args}'
+            .format(edge_name=macro_edge_name,
                     current_type=base_class_name,
-                )
-
-            raise GraphQLInvalidMacroError(
-                u'A macro edge with name {edge_name} cannot be defined on type {current_type} due '
-                u'to a conflict with another macro edge with the same name defined '
-                u'on type {original_type}{extra_error_text}.'
-                u'Cannot define this conflicting macro, please verify '
-                u'if the existing macro edge does what you want, or rename your macro '
-                u'edge to avoid the conflict. Existing macro definition and args: '
-                u'{macro_graphql} {macro_args}'
-                .format(edge_name=macro_edge_name,
-                        current_type=base_class_name,
-                        original_type=conflict_on_class_name,
-                        extra_error_text=extra_error_text,
-                        macro_graphql=print_ast(existing_descriptor.expansion_ast),
-                        macro_args=existing_descriptor.macro_args))
+                    original_type=conflict_on_class_name,
+                    extra_error_text=extra_error_text,
+                    macro_graphql=print_ast(existing_descriptor.expansion_ast),
+                    macro_args=existing_descriptor.macro_args))
+
+
+def _check_macro_edge_for_reversal_definition_conflicts(macro_registry, macro_descriptor):
+    """Ensure that the macro edge, when reversed, does not conflict with any existing macro edges.
+
+    This function ensures that for any macro edge being defined, if a corresponding macro edge were
+    to be later defined in the opposite direction (whether manually or automatically), this new
+    reversed macro edge would not conflict with any existing macro edges. To check this, we generate
+    the name of the reversed macro edge, and then check the types the macro edge connects. If a
+    macro edge by the same name exists on either of those types, or any of their subtypes, then
+    the reversed macro edge is deemed in conflict, and the original macro edge definition is
+    considered invalid.
+
+    Args:
+        macro_registry: MacroRegistry object containing macro descriptors, where the new
+                        macro edge descriptor would be added.
+        macro_descriptor: MacroEdgeDescriptor describing the macro edge being added
+    """
+    reverse_macro_edge_name = make_reverse_macro_edge_name(macro_descriptor.macro_edge_name)
+
+    reverse_base_class_name = get_type_at_macro_edge_target(
+        macro_registry.schema_without_macros, macro_descriptor.expansion_ast).name
+
+    reverse_target_class_name = macro_descriptor.base_class_name
+
+    existing_descriptor = _find_macro_edge_name_at_subclass(
+        macro_registry, reverse_base_class_name, reverse_macro_edge_name)
+
+    if existing_descriptor is not None:
+        # There is already a reverse macro edge. Let's make sure its endpoint types are an exact
+        # match compared to the endpoint types of the macro edge being defined.
+        if reverse_base_class_name != existing_descriptor.base_class_name:
+            raise GraphQLInvalidMacroError()
+
+        existing_target_class_name = get_type_at_macro_edge_target(
+            macro_registry.schema_without_macros, existing_descriptor.expansion_ast).name
+        if reverse_target_class_name != existing_target_class_name:
+            raise GraphQLInvalidMacroError()
 
 
 def register_macro_edge(macro_registry, macro_edge_graphql, macro_edge_args):
@@ -157,8 +211,11 @@ def register_macro_edge(macro_registry, macro_edge_graphql, macro_edge_args):
         type_equivalence_hints=macro_registry.type_equivalence_hints)
 
     # Ensure there's no conflict with macro edges defined on subclasses and superclasses.
-    _check_macro_edge_for_definition_conflicts(
-        macro_registry, macro_descriptor.base_class_name, macro_descriptor.macro_edge_name)
+    _check_macro_edge_for_definition_conflicts(macro_registry, macro_descriptor)
+
+    # Ensure there's no conflict between existing macro edges and the (hypothetical) reversed
+    # macro edge of the one being defined.
+    _check_macro_edge_for_reversal_definition_conflicts(macro_registry, macro_descriptor)
 
     for subclass_name in macro_registry.subclass_sets[macro_descriptor.base_class_name]:
         macro_registry.macro_edges_at_class.setdefault(

graphql_compiler/macros/macro_edge/reversal.py

@@ -0,0 +1,20 @@
+from ...schema import INBOUND_EDGE_FIELD_PREFIX, OUTBOUND_EDGE_FIELD_PREFIX
+
+
+# ############
+# Public API #
+# ############
+
+def make_reverse_macro_edge_name(macro_edge_name):
+    if macro_edge_name.startswith(INBOUND_EDGE_FIELD_PREFIX):
+        raw_edge_name = macro_edge_name[len(INBOUND_EDGE_FIELD_PREFIX):]
+        prefix = OUTBOUND_EDGE_FIELD_PREFIX
+    elif macro_edge_name.startswith(OUTBOUND_EDGE_FIELD_PREFIX):
+        raw_edge_name = macro_edge_name[len(OUTBOUND_EDGE_FIELD_PREFIX):]
+        prefix = INBOUND_EDGE_FIELD_PREFIX
+    else:
+        raise AssertionError(u'Unreachable condition reached: {}'.format(macro_edge_name))
+
+    reversed_macro_edge_name = prefix + raw_edge_name
+
+    return reversed_macro_edge_name

graphql_compiler/macros/macro_edge/validation.py

@@ -25,6 +25,7 @@
     DIRECTIVES_ALLOWED_IN_MACRO_EDGE_DEFINITION, DIRECTIVES_REQUIRED_IN_MACRO_EDGE_DEFINITION,
     MacroEdgeDefinitionDirective, MacroEdgeTargetDirective, get_schema_for_macro_edge_definitions
 )
+from .reversal import make_reverse_macro_edge_name
 
 
 def _validate_query_definition(ast):
@@ -190,23 +191,15 @@ def _validate_macro_edge_name_for_class_name(schema, subclass_sets, class_name,
 
 def _validate_reversed_macro_edge(schema, subclass_sets, reverse_start_class_name, macro_edge_name):
     """Ensure that the provided macro does not conflict when its direction is reversed."""
-    if macro_edge_name.startswith(INBOUND_EDGE_FIELD_PREFIX):
-        raw_edge_name = macro_edge_name[len(INBOUND_EDGE_FIELD_PREFIX):]
-        prefix = OUTBOUND_EDGE_FIELD_PREFIX
-    elif macro_edge_name.startswith(OUTBOUND_EDGE_FIELD_PREFIX):
-        raw_edge_name = macro_edge_name[len(OUTBOUND_EDGE_FIELD_PREFIX):]
-        prefix = INBOUND_EDGE_FIELD_PREFIX
-    else:
-        raise AssertionError(u'Unreachable condition reached: {}'.format(macro_edge_name))
-
-    reversed_macro_edge_name = prefix + raw_edge_name
+    reversed_macro_edge_name = make_reverse_macro_edge_name(macro_edge_name)
 
     # The reversed macro edge must not have the same name as an existing edge on the target class
     # it points to, or any of its subclasses. If such an edge exists, then the macro edge is not
     # reversible since the reversed macro edge would conflict with the existing edge on that class.
     conflicting_subclass_name = _find_subclass_with_field_name(
         schema, subclass_sets, reverse_start_class_name, reversed_macro_edge_name)
     if conflicting_subclass_name is not None:
+        extra_error_text = u''
         if conflicting_subclass_name != reverse_start_class_name:
             extra_error_text = (
                 u'{} is a subclass of {}, which is where your macro edge definition points to. '

graphql_compiler/tests/test_macro_validation.py

@@ -526,7 +526,6 @@ def test_macro_edge_reversal_validation_rules_origin_incompatible_conflict(self)
         with self.assertRaises(GraphQLInvalidMacroError):
             register_macro_edge(macro_registry, macro_edge_definition, args)
 
-    @pytest.mark.xfail(strict=True, reason='not implemented')
     def test_macro_edge_reversal_validation_rules_origin_subclass_conflict(self):
         # Reversing a macro edge must not conflict with an existing macro edge
         # defined between different types. The first one produces a macro edge from Animal to Animal
@@ -569,7 +568,6 @@ def test_macro_edge_reversal_validation_rules_origin_subclass_conflict(self):
         with self.assertRaises(GraphQLInvalidMacroError):
             register_macro_edge(macro_registry, macro_edge_definition, args)
 
-    @pytest.mark.xfail(strict=True, reason='not implemented')
     def test_macro_edge_reversal_validation_rules_origin_superclass_conflict(self):
         # Reversing a macro edge must not conflict with an existing macro edge
         # defined between different types. The first one produces a macro edge from Entity to Entity