Macro system #356

bojanserafimov · 2019-06-14T17:40:16Z

No description provided.

* Add a skeleton for the macro expansion system. * Add copyright lines.

* Add a few helpers for macro validation. Minor refactor of AST manipulation functions. * Add copyright lines. * Add more helpers, rename the file. * Fix imports. * Refactor and deduplicate code in get_only_selection_from_ast(). * Rebalance lines so they are under 100 chars.

* Add a few core macro edge validation steps. More to come. * Thanks, linter. * Update further validation list. * Applied code review suggestions.

…187) * Add more validation, together with the new 2-directive macro format. * Delint. * Delint.

* Add tests for expansion of invalid macros * Rename test class * Update copyright year

* Bugfix for macro validation. * Add another validation check, and a testcase for it.

* Implement computing subclass_sets * Add tests for subclass * Add docs and fix lint * Address comments * Address comments * Remove unnecessary note * Restrict the definition of subclass * Remove union types from subclass logic * Avoid positional kwargs * Avoid passing kwargs positionally

…ug. (#193) * Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint.

* Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint. * Add partial implementation of macro edge expansion. * Delint.

@output

* Add tests * Separate macro expansion tests that expect errors * Fix broken @output directives * Add docstring for test_helpers method * Add tests * Separate macro expansion tests that expect errors * Fix broken @output directives * Add docstring for test_helpers method * Fix linter errors * Fix test * Add tests, fix tests * Add tests * Add tests * Add test docstrin * Add tests * Add tests * Add expected answers * Add test with inline fragment directive merging * More tests

This is needed in macro expansion to determine whether merging nested coercions is allowed.

* Add subclass_sets argument to macro expansion * Fix invalid tests, add more macro expansion tests

If we remove the `@macro_edge_target` directive we permanently lose information on where the user code should be pasted inside the macro.

* Fix broken tests * Add basic merging, passing basic test * Make progress, pass second test * Pass third test * Implement some target type coercion * Pass some more tests * WIP * Subclass sets plumbing * Fix lint errors * Unskip test * Add todos * Unskip macro expansion errors tests * Add test * Merge extra selections at the beginning * Order selections deterministically when merging

* Address comments on expand_specific_macro * Avoid deepcopy * Address comments

* Add ast_to_ir method * Try compiling macros to IR as validation * Fix and unskip macro validation tests * Fix lint errors * Check argument names * Address comments * Address comment * Copy selections * Third attempt at copying the right things * Restructure for readability * Fix comment

* Disallow @macro_edge_target on union type * Add todo * Add todo

* Implement get_type_at_target * Implement macro expansion with coercion on user side only * Fix lint * Address comments * Address comments

* Add validation for use of directives in macros * Add tests * Check macro argument types * Revert "Check macro argument types" This reverts commit 7b2280e. * Address comments * Address comments * Address nits

* Check macro argument types * Address comments * Address comments, add tests * Make bool not a decimal, and datetime not a date * Address comment * Address comments * Fix python 2 bytes validation * Update docstring * Remove test for bytes because it can't work in py2 * Fix comment

* Adding SQL Support documentation * Indicating that traversing edges isn't yet a thing * Fixing SQLite typo * Flavor -> Flavors * Adding E2E SQL example * Addressing PR comments, fixing CompilerMetadata references * Fixing MSFT reference * Fixing typo * Addressing Micheala's CR * Recommit to trigger the final coverage check * Added schema geneneration (#201) * Added schema gen Fixed import order Fixed Pipfile Fixing imports Fixed nits and style changes Removed SchemaGraph documentation Removed even more documentation * Fixed nits * Added copyright message * Nits * Fixed imports * Fixed lint issues * Fixing pipfile issue * Removed frozendict * Removed frozen dict from schema * Run through non graph classes in sorted order (#207) * Change the schema from our manually written one to the autogenerated one (#211) * Reordered the schema string * Added x_count * Added union birthevent * Added union of FoodOrSpecies * Sorted manually written schema * Changed FedAt edge type to BirthEvent * Fixed missing test change from Event to BirthEvent * Removed test on Location that was invalid after field changes * Added fully generated schema * Linter fixes * Fixed nits * Changed BirthEvent to FeedingEvent * Changed SQL schema * Changed SQL schema * Updated SQL schema to account for change of Events to FeeedingEvents * Nits * Nits * Add schema generation from orientdb records (#204) * Added function to get GraphQL schema from OrientDB records * Fixed documentation and style changes * Hide the base vertex if it is not representable * Fixed bug that failed to include certain abstract classes in schema * Fully fixed bug * Nits * More nits * Raise exception when schema is empty * Improved docummentation for get_graphql_schema_from_schema_graph * Cleaned up duplicated OrientDB generated string * Deleted class fields from SchemaElements since they are not used in the code * Loosened strict edge validation * Updated docummentation * Removed redundant documentation * Improved documentation and moved get_graphql_schema_from_orientdb_schema_data * Removed hidden classes parameter, updated documentation and performed refactors for get_graphql_from_schema_graph * Fixed style issues and added class fields again * Nits * Nits and style changes * Add graphql syntax highlighting (#218) * Update schema generation docs * Added syntax highlighting for GraphQL * Removed changes that are not relevant to this branch * Added two sanity tests to prevent someone from updating the current schema with an invalid one (#219) * Added two sanity tests to prevent someone from updating the current schema with an invalid one * Nits * Fixed linter errors * Nits * Update SQL schema types and removed type override no-ops. (#223) * Remove Animal's Birthday field type override to Date since it is inferred by the compiler * Updated SQL schema to have event_date as a DateTime object * Fixed linter issues * Trigger linter check * Trigger linter check * Fixed linter errors * Added debug info to contributing.md (#216) * Added debug info to contributing.md * Nits * Mentioned that command to stop mysql only works in Ubuntu * Added mysql stop command for Mac * Remove typo * Add separate lockfiles for py2/py3, and new flake8 plugins for better linting (#225) * Add linters for proper use of quotes, and for catching print statements in code. * Make a lockfile via py2 so it still works in py2. * Allow different python versions to use different lockfiles. * Verbose lockfile change script. * Fix for python 2 printing version info to stderr instead of stdout. * Add a script for auto-generating both sets of lockfiles. * Add script for making py2 venv. Add info to CONTRIBUTING.md * Add TROUBLESHOOTING.md file. * Echo that we are deleting the old lockfiles. * Fix usage of EventOrBirthEvent in tests * Fix lint errors

* Add schema, hints, and subclass_sets to macro_registry * Fix lint * Fix capitalization

Also add new test cases for macro - reverse-macro conflicts.

bojanserafimov

The name validation logic seems correct and satisfies the desired properties:

The inverse of a macro edge from A to B is a macro edge from B to A.
All macro edges either have an inverse or the inverse can be created with no conflict.

I just have comments on the error messages.

bojanserafimov · 2019-09-05T14:38:19Z

graphql_compiler/macros/__init__.py

+
+
+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."""


This implies there's at most one, which is not the case.

Would rephrasing to say Return any descriptor resolve the concern, or would you prefer something else?

bojanserafimov · 2019-09-05T14:44:33Z

graphql_compiler/macros/__init__.py

+
+    # 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)


Maybe returning all conflicts is more user friendly. Imagine fixing a naming conflict only to find out there's another naming conflict. I won't block for this though, it's definitely a "nice to have".

I agree, it's a nice to have. In the interest of getting the rest of this out, though, I think I'll save it for another time – and hopefully having multiple conflicts doesn't happen often enough in anyone's workflow for this to really matter in the end.

bojanserafimov

The name validation logic seems correct and satisfies the desired properties:

There's at most macro with a given name at a given class and its subclasses.
The inverse of a macro edge from A to B is a macro edge from B to A.
All macro edges either have an inverse or the inverse can be created with no conflict.

The additional checks derived from these desired properties don't seem too conservative.

I just have comments on the error messages.

obi1kenobi

Ready for review!

obi1kenobi · 2019-09-06T04:35:07Z

graphql_compiler/macros/macro_edge/ast_rewriting.py

@@ -238,51 +238,6 @@ def remove_directives_from_ast(ast, directive_names_to_omit):
    return new_ast


-def omit_ast_from_ast_selections(ast, ast_to_omit):


This function was never used, so I deleted it.

obi1kenobi · 2019-09-06T04:48:11Z

graphql_compiler/macros/__init__.py

+def register_macro_edge(macro_registry, macro_edge_graphql, macro_edge_args):
+    """Add the new macro edge descriptor to the provided MacroRegistry object, mutating it.
+
+    In order to register a new macro edge, the following properties must be true:


I added the explicit set of macro edge validity rules here. Let me know if I missed any.

obi1kenobi · 2019-09-06T04:49:34Z

graphql_compiler/macros/macro_edge/ast_rewriting.py

@@ -374,6 +329,20 @@ def merge_selection_sets(selection_set_a, selection_set_b):
                                          u'same edge {} twice, which is disallowed.'
                                          .format(field_name))

+        # TODO(predrag): Find a way to avoid this situation by making the rewriting smarter.


This is caught by tests marked xfail. That annotation means that the tests are expected to fail (with a specific error), and if they fail differently or not at all, that should be treated as a test error and reported. When we address this TODO, we can remove the xfail annotation on those tests and everything should work fine.

obi1kenobi · 2019-09-06T04:50:20Z

graphql_compiler/macros/macro_edge/descriptor.py

-        'macro_args',       # Dict[str, Any] containing any arguments required by the macro
+        'base_class_name',    # str, name of GraphQL type where the macro edge is defined.
+                              # The macro edge exists at this type and all of its subtypes.
+        'target_class_name',  # str, the name of the GraphQL type that the macro edge points to.


Storing the target class name made a lot of validation code simpler, and means that we don't have to keep recomputing this by walking the macro definition ASTs in situations like generating the schema with macro edges included.

obi1kenobi · 2019-09-06T04:51:49Z

graphql_compiler/macros/macro_edge/expansion.py

+    new_ast, prefix_selections, suffix_selections = _expand_specific_macro_edge(
+        macro_registry.subclass_sets, macro_edge_descriptor.target_class_name,
+        sanitized_macro_ast, ast)
+    # TODO(predrag): Write a test that makes sure we've chosen names for filter arguments that


Hoping to address this during our tech debt pass later this month.

obi1kenobi · 2019-09-06T04:53:29Z

graphql_compiler/macros/macro_expansion.py

+
+                # Recurse on the new_selection_ast, to expand any macros
+                # that exist at a deeper level.
+                # TODO(predrag): Move get_vertex_field_type() to the top-level schema.py file,


Also a good candidate for tech debt week.

obi1kenobi · 2019-09-06T04:55:25Z

graphql_compiler/macros/validation.py

+        # There is already a reverse macro edge of the same name that starts at the same type.
+        # Let's make sure its endpoint types are an exact match compared to the endpoint types
+        # of the macro edge being defined.
+        errors = []


Here and below, I wanted to avoid the situation where there are obviously two things wrong with the macro edge definition, and we print both definitions in the error message, but only complain about one of the two wrong things. The code is definitely a bit tricky, but I think the improvement in error message quality is worth it.

bojanserafimov

Looks ready for release. I have one comment. I can't approve this because I'm the author.

bojanserafimov · 2019-09-06T13:04:27Z

graphql_compiler/macros/macro_edge/expansion.py

+    return replacement_selection_ast, sibling_prefix_selections, sibling_suffix_selections
+
+
+def _merge_non_overlapping_dicts(merge_target, new_data):


tech debt week: move to top-level utils

obi1kenobi and others added 30 commits February 6, 2019 15:24

Add a skeleton for the macro expansion system. (#181)

e74ab48

* Add a skeleton for the macro expansion system. * Add copyright lines.

Add a few core macro edge validation steps. (#186)

11c6705

* Add a few core macro edge validation steps. More to come. * Thanks, linter. * Update further validation list. * Applied code review suggestions.

Add more validation, together with the new 2-directive macro format. (#…

3259797

…187) * Add more validation, together with the new 2-directive macro format. * Delint. * Delint.

Test invalid edge macros (#188)

690b435

* Add tests for expansion of invalid macros * Rename test class * Update copyright year

Bugfix for macro validation, and more validation checks. (#189)

b1480b5

* Bugfix for macro validation. * Add another validation check, and a testcase for it.

Move test_macro_expansion_errors to test_macro_validation (#192)

03f7d9a

Deduplicate parsing and validation code, fix a schema/type checking b…

304a086

…ug. (#193) * Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint.

Add partial implementation of macro edge expansion. (#194)

15c5c49

* Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint. * Add partial implementation of macro edge expansion. * Delint.

Add subclass_sets argument to macro expansion (#195)

8b04e66

This is needed in macro expansion to determine whether merging nested coercions is allowed.

Add and fix macro expansion tests (#196)

d853488

* Add subclass_sets argument to macro expansion * Fix invalid tests, add more macro expansion tests

Keep the @macro_edge_target in the macro descriptor (#197)

30d5460

If we remove the `@macro_edge_target` directive we permanently lose information on where the user code should be pasted inside the macro.

Address comments on expand_specific_macro (#200)

6a71a00

* Address comments on expand_specific_macro * Avoid deepcopy * Address comments

Disallow @macro_edge_target on union type (#203)

cc9343c

* Disallow @macro_edge_target on union type * Add todo * Add todo

Type coercion at @macro_edge_target (#202)

3281eed

* Implement get_type_at_target * Implement macro expansion with coercion on user side only * Fix lint * Address comments * Address comments

Macro edge validation for use of directives (#205)

f9f5bb9

* Add validation for use of directives in macros * Add tests * Check macro argument types * Revert "Check macro argument types" This reverts commit 7b2280e. * Address comments * Address comments * Address nits

Fix tests (#226)

a2c66a9

Add schema, hints, and subclass_sets to macro_registry

44bb12d

Fix lint

eb32f95

Deepcopy schema

a565504

Add schema_without_macros to macro_registry (#230)

190c6cd

* Add schema, hints, and subclass_sets to macro_registry * Fix lint * Fix capitalization

WIP

0652e2d

Implement

2219f51

Merge branch 'macro_system' into implement_schema_with_macros

9c2aef4

obi1kenobi added 14 commits August 27, 2019 13:45

Only allow filter directives at macro expansion points.

bdb24bf

Improve error message for unsupported directive on macro edge.

88193ba

Refactor macro edge expansion code to move it into its own module.

860d6c0

Merge branch 'master' into macro_system

1c75da0

Merge branch 'master' into macro_system

1316dff

Remove dead code from helpers file.

587e20a

Mark tests of unimplemented functionality with pytest xfail mark.

fdc6dc8

Add validation for schema conflicts of reverse macro edges.

d75dbba

Also add new test cases for macro - reverse-macro conflicts.

Add validation error message.

d52614a

Add more reverse macro edge validation logic.

784d469

Add macro edge target class name to macro edge descriptor.

353e4f9

Tweak test cases to ensure all validation error messages are hit.

da2e97c

Delint.

dca96f9

Merge branch 'master' into macro_system

33708bb

bojanserafimov commented Sep 5, 2019

View reviewed changes

obi1kenobi added 8 commits September 6, 2019 00:10

Refactor cross-macro validation code.

5ff443b

Merge branch 'master' into macro_system

14ac93f

Delint.

7b914e5

Explicitly error out on duplicate tags during expansion.

9fb1e1b

Add macro validity rules description.

b558389

Fix typo.

0f677b5

Remove stale TODO.

4f05624

Refactor and deduplicate validation code.

5384e33

obi1kenobi reviewed Sep 6, 2019

View reviewed changes

bojanserafimov commented Sep 6, 2019

View reviewed changes

pmantica1 approved these changes Sep 6, 2019

View reviewed changes

pmantica1 merged commit ca72257 into master Sep 6, 2019

pmantica1 deleted the macro_system branch September 6, 2019 16:31

obi1kenobi mentioned this pull request Oct 2, 2019

Allow "virtual" edges to be defined, and expanded using an AST-based macro system #180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Macro system #356

Macro system #356

bojanserafimov commented Jun 14, 2019

bojanserafimov left a comment

bojanserafimov Sep 5, 2019

obi1kenobi Sep 5, 2019

bojanserafimov Sep 5, 2019

obi1kenobi Sep 5, 2019

bojanserafimov left a comment

obi1kenobi left a comment

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

obi1kenobi Sep 6, 2019

bojanserafimov left a comment •

edited

bojanserafimov Sep 6, 2019



		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."""

		@@ -238,51 +238,6 @@ def remove_directives_from_ast(ast, directive_names_to_omit):
		return new_ast


		def omit_ast_from_ast_selections(ast, ast_to_omit):

		return replacement_selection_ast, sibling_prefix_selections, sibling_suffix_selections


		def _merge_non_overlapping_dicts(merge_target, new_data):

Macro system #356

Macro system #356

Conversation

bojanserafimov commented Jun 14, 2019

bojanserafimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojanserafimov left a comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojanserafimov left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojanserafimov left a comment •

edited