Address comments on expand_specific_macro #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
obi1kenobi
reviewed
Feb 23, 2019
| @@ -97,6 +99,33 @@ def remove_directives_from_ast(ast, directive_names_to_omit): | |||
| return new_ast | |||
|
|
|||
|
|
|||
| def find_target_and_copy_path_to_it(ast): | |||
| """Return the same ast and the ast at the target, with the path to the target shallow copied.""" | |||
There was a problem hiding this comment.
Mind describing in a bit more detail what this function is intended to accomplish? I think I get what it is doing, but not why it's doing it. "Same AST" is also a bit misleading.
Also, a nit: I'd feel better about it with ast -> AST for consistency.
obi1kenobi
reviewed
Feb 23, 2019
|
|
||
| The order of selections in the resulting SelectionSet has the following properties: | ||
| - property fields are before vertex fields. | ||
| - fields present in selection_set_b are after all other fields. |
There was a problem hiding this comment.
presumably this only applies to vertex fields from B, right?
There was a problem hiding this comment.
Applies to both vertex fields and property fields, but separately. Think of it as "It applies to everything, but it has lower priority than the first bullet point".
obi1kenobi
reviewed
Feb 23, 2019
| merged_field = deepcopy(field_a) | ||
| merged_field.directives += field_b.directives | ||
| merged_field = copy(field_a) | ||
| merged_field.directives = field_a.directives + field_b.directives |
There was a problem hiding this comment.
My personal preference here would be to be explicit about the fact that we expect this to produce a new list copy using list(chain(...)), but I can live with it as-is if you prefer this.
obi1kenobi
reviewed
Feb 23, 2019
| merged_selection_dict[name] | ||
| for name in selection_name_order | ||
| if name in merged_selection_dict and merged_selection_dict[name].selection_set is not None | ||
| ]) | ||
| )))) |
There was a problem hiding this comment.
This is fairly repetitive. We already have a dependency on funcy, maybe funcy.split would help?
There was a problem hiding this comment.
Also, I realize I suggested this approach on the previous iteration -- in retrospect that wasn't a good idea, sorry about that.
obi1kenobi
reviewed
Feb 23, 2019
| # TODO(bojanserafimov): Write test for this failure | ||
| raise GraphQLCompilationError( | ||
| u'Cannot prove type coercion at macro target is valid. Please provide a' | ||
| u'proof that {} subclasses {} using the subclass_sets argument.' |
There was a problem hiding this comment.
Not sure that proof is the word we're looking for here... I think the user could conceivably "prove" something that is actually false.
obi1kenobi
reviewed
Feb 23, 2019
| for selection in selection_ast.selection_set.selections: | ||
| if isinstance(selection, InlineFragment): | ||
| if len(selection_ast.selection_set.selections) != 1: | ||
| raise GraphQLCompilationError(u'Found selections outside type coercion {}' |
There was a problem hiding this comment.
This is a user-facing error, so let's give the user some actionable advice, and let's avoid printing a giant AST selection set in the error message.
obi1kenobi
reviewed
Feb 23, 2019
obi1kenobi
approved these changes
Feb 25, 2019
pmantica1
pushed a commit
that referenced
this pull request
Sep 6, 2019
* Add a skeleton for the macro expansion system. (#181) * Add a skeleton for the macro expansion system. * Add copyright lines. * Add a few helpers for macro validation. (#183) * 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. (#186) * 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. (#187) * Add more validation, together with the new 2-directive macro format. * Delint. * Delint. * Test invalid edge macros (#188) * Add tests for expansion of invalid macros * Rename test class * Update copyright year * Bugfix for macro validation, and more validation checks. (#189) * Bugfix for macro validation. * Add another validation check, and a testcase for it. * Move test_macro_expansion_errors to test_macro_validation (#192) * Subclasses (#185) * 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 * Deduplicate parsing and validation code, fix a schema/type checking bug. (#193) * Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint. * Add partial implementation of macro edge expansion. (#194) * Deduplicate parsing and validation code, fix a schema/type checking bug. * Delint. * Add partial implementation of macro edge expansion. * Delint. * Test macro edge expansion (#190) * 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 * Add subclass_sets argument to macro expansion (#195) This is needed in macro expansion to determine whether merging nested coercions is allowed. * Add and fix macro expansion tests (#196) * 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) If we remove the `@macro_edge_target` directive we permanently lose information on where the user code should be pasted inside the macro. * Expand specific macro (#198) * 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 (#200) * Address comments on expand_specific_macro * Avoid deepcopy * Address comments * Compile macro to ir (#199) * 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 (#203) * Disallow @macro_edge_target on union type * Add todo * Add todo * Type coercion at @macro_edge_target (#202) * 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) * 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 (#206) * 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 * Fix tests (#226) * Merge master into macro_system, resolving conflicts (#227) * 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 * Deepcopy schema * Add schema_without_macros to macro_registry (#230) * Add schema, hints, and subclass_sets to macro_registry * Fix lint * Fix capitalization * WIP * Implement * Raise NotIpmlementedError * Address comments * Make macro edge schema fields point to list types (#233) * Make macro edge schema fields point to list types * Fix lint * Fix docstring in validation (#236) * Fix macro on superclass (#235) * Fix macro definition on superclass bug * Fix lint errors * Revert "Fix lint errors" This reverts commit dab933d. * Add blank line * Fix slow subclass sets computation (#276) * Add test and fix broken tests in macro expansion (#275) * Fix broken tests * Revert tag rename * Rename tag * Remove stale TODOs (#278) * Validate macro edge does not exist on sub/superclass (#277) * Validate macro edge does not exist on sub/superclass * Pick better variable names * Remove unused import * Fix lint * Only allow supported directives at macro expansion * Tidy up * Lint * Add tests for directives on expansion * Add macro edge target validation (#279) * Add validation for the @macro_edge_target directive * Remove implication that we cannot handle target at union type * Remove unwanted file * Add macro system documentation * Add macro registry example * Fix example * Fix link * Address some nits * Add example with macro runtime parameters * Add code example * Edit example * Address nits * Isort * Macro tag names (#281) * Implement tag renaming * Add comment * Fix lint * Fix disambiguation * Address comments * Write better docstrings * Clarify that argument is not mutated * Document that same object is returned * Fix docstrings * Clarify code and error messages * Add format in error * Remove stale test (#307) * Add Macro edge allowed/disallowed/required directive lists to directives.py * Nit * Nit * Address comments, Validator compares directives - Address inconsistent directive lists. - _validate_macro_ast_directives compares directives (incl. args/location) fully * Revert directive comparison including args, location As _validate_macro_ast_directives receives instances of directives being used, args and location differed, causing issues during testing. I reverted this change. * Rename optional directive list to better reflect usage * Address comments, replace OPTIONAL_UNRESTRICTED directive list with just OPTIONAL * Add allowed directive list, remove optional directive list * Schema for macro definitions (#319) * Schema for macro definitions generator * Fix lint issues * Add partial tests, remove precondition in __init__ * Add unimplemented flag to test * Add precondition to get_schema_for_macro_definition * Add precondition to schema_for_macro_definition, add validation testing * Nit * Remove precondition, error reporting for nondefault directives * Update schema_for_macro_definition documentation, and error reporting * Inline include_required and exclude_disallowed directives in macro definitions * Nit * Address comments, import default graphql directives * Nit * Fix nit * Add TODO where get_schema_for_macro_definition should be used * Nit * Merge master to macro system (#352) * Allow list types in RootSchemaQuery (#284) * Allow list types in RootSchemaQuery * Use list types in schema generation * Document common Docker Compose issue (#304) * Document common Docker Compose issue * Combine troubleshooting tips for MySQL/PostgreSQL * Add inheritance structure tuple (#301) * Added inheritance structure tuple * Fixed linter errors * Nit * Nits * Added missing class to comment example: * Move element linking to SchemaGraph (#308) * Add indexes (#312) * Added indexes to the SchemaGraph * Add additional index methods and remove type field * Nits * Nits * More nits * More nits * Changed all_indexes to indexes * Revert "Changed all_indexes to indexes" This reverts commit 44ffd00. * Nit * Addressed code review comments * Prohibit null value ignoring for edge indexes * Fix typo * Stop overriding ignore_nulls for edge indexes to true * Fix typo * Refactor inheritance structure constructor (#314) * Add immediate superclasses dict * Make toposorting not use OrientDB constructs. * Refactored InheritanceStructure constructor * Move inheritance structure class and related methods * Fixed docstring * Added documentation for InheritanceStructure class * Nits * Return validation to its original place * Fix terminology * More nits * More nits * More nits * Re-add list coercion * Add indexes to test SchemaGraph (#315) * Add indexes to test SchemaGraph * Changed sets arguments to lists and remove is None check * Add output info to query metadata (#311) * Add output info to query metadata * Update explain info tests * Add output tests to explain_info * Remove context output * Add docstring metadata outputs property * Use query_metadta_table variable and name * Replace more names with query_metadata_table * Remove fold from OutputInfo * Change set input for list input (#318) * Fix index documentation (#321) * Fix index documentation * Nit * Refactor and clean up IR lowering code. Add common IR lowering module. (#324) * Refactor and clean up IR lowering code. Add common IR lowering operations module. * Delint. * Improve expression comment around null values. * Add defensive strip_non_null_from_type() call. * Find bug in match backend * Add a SchemaGraph section in the README.md (#326) * Add SchemaGraph documentation in the README.md file * Add SchemaGraph documentation in the README.md file * Nits * Nits * Nits * Nits * Fix heading sizes * Added SchemaGraph introduction * Simplify SchemaGraph section * Fix hyperlink to SchemaGraph section * Nits * Add index disclaimer * Nit * Ask question in comment * Improve getting started documentation (#323) * Improving schema autogeneration documentation * Fix README.md example * Nits * Fix typo * Fix step number * Revert testing config changes * Fix end-to-end example * Shorten README.md * Nit * Remove boilerplate database startup code * Fix typo * Communicate assumptions through code * Nits * Remove getting started section * Record the GraphQL type of LocalField expressions. Required to make progress on Cypher backend, since a lowering step there converts LocalField expressions into ContextField expressions. Prior to this commit, ContextField expressions include the GraphQL type of the field, whereas LocalField expressions do not, making that lowering step impossible to implement. * Delint. * Ignore properties with invalid names * Document MySQL installation issue * Update docstring. * Refactor graph_client to snapshot_graph_client for code clarity (#343) * Implement fix * Simplify integration test * Lint * Remove comment * Add comment * Resolve merge conflict * Delete references to non-existent non-graph interfaces * Address comments * Query cardinality estimation (#345) * Add query cost estimation (#317) * Add basic cost estimation * Add basic cost estimation tests * Add basic filter estimation tests * Add TODO for test SchemaGraph * Add more filter tests * Fix lint * Refactor cardinality estimation * Add filter selectivity tests * Add in_collection support to cardinality estimator * Document assumption for in_collection estimation * Add copyright line * Address comments * Delint * Change directory of test_cost_estimation to snapshot_tests * Delint * Revert merge * Reflect graph_client to snapshot_graph_client refactor from merge with master * Use INBOUND_EDGE_DIRECTION instead of EDGE_DESTINATION... * Nit * Merge OSX and Ubuntu MySQL sections * Nit * Address comments * Add not-contains filter operator (#349) * Create not-in-collection tests * Fix typos * Add gremlin query strings * Fix test cases for negation * Implement not-contains filter * Fix MATCH not-contains syntax * Add not-contains snapshot tests * Wrap all MATCH expressions in parentheses * Fix formatting nits * Relax tag filter constraint (#351) * Use TagInfo instead of context['tags'] * Add test for out of order tag and filter * Process tags before filters * Update readme * Add docstring * Fix comment * Remove initialization of context['tags'] * Resolve merge conflict * Make sure the tag is not reordered afer the filter (#322) * Make sure the tag is not reordered afer the filter * Expand macro preserving order of vertex fields * Add sanity check and simplify condition * Add error context * Merge master into macro system (#355) * Allow list types in RootSchemaQuery (#284) * Allow list types in RootSchemaQuery * Use list types in schema generation * Document common Docker Compose issue (#304) * Document common Docker Compose issue * Combine troubleshooting tips for MySQL/PostgreSQL * Add inheritance structure tuple (#301) * Added inheritance structure tuple * Fixed linter errors * Nit * Nits * Added missing class to comment example: * Move element linking to SchemaGraph (#308) * Add indexes (#312) * Added indexes to the SchemaGraph * Add additional index methods and remove type field * Nits * Nits * More nits * More nits * Changed all_indexes to indexes * Revert "Changed all_indexes to indexes" This reverts commit 44ffd00. * Nit * Addressed code review comments * Prohibit null value ignoring for edge indexes * Fix typo * Stop overriding ignore_nulls for edge indexes to true * Fix typo * Refactor inheritance structure constructor (#314) * Add immediate superclasses dict * Make toposorting not use OrientDB constructs. * Refactored InheritanceStructure constructor * Move inheritance structure class and related methods * Fixed docstring * Added documentation for InheritanceStructure class * Nits * Return validation to its original place * Fix terminology * More nits * More nits * More nits * Re-add list coercion * Add indexes to test SchemaGraph (#315) * Add indexes to test SchemaGraph * Changed sets arguments to lists and remove is None check * Add output info to query metadata (#311) * Add output info to query metadata * Update explain info tests * Add output tests to explain_info * Remove context output * Add docstring metadata outputs property * Use query_metadta_table variable and name * Replace more names with query_metadata_table * Remove fold from OutputInfo * Change set input for list input (#318) * Fix index documentation (#321) * Fix index documentation * Nit * Refactor and clean up IR lowering code. Add common IR lowering module. (#324) * Refactor and clean up IR lowering code. Add common IR lowering operations module. * Delint. * Improve expression comment around null values. * Add defensive strip_non_null_from_type() call. * Find bug in match backend * Add a SchemaGraph section in the README.md (#326) * Add SchemaGraph documentation in the README.md file * Add SchemaGraph documentation in the README.md file * Nits * Nits * Nits * Nits * Fix heading sizes * Added SchemaGraph introduction * Simplify SchemaGraph section * Fix hyperlink to SchemaGraph section * Nits * Add index disclaimer * Nit * Ask question in comment * Improve getting started documentation (#323) * Improving schema autogeneration documentation * Fix README.md example * Nits * Fix typo * Fix step number * Revert testing config changes * Fix end-to-end example * Shorten README.md * Nit * Remove boilerplate database startup code * Fix typo * Communicate assumptions through code * Nits * Remove getting started section * Record the GraphQL type of LocalField expressions. Required to make progress on Cypher backend, since a lowering step there converts LocalField expressions into ContextField expressions. Prior to this commit, ContextField expressions include the GraphQL type of the field, whereas LocalField expressions do not, making that lowering step impossible to implement. * Delint. * Ignore properties with invalid names * Document MySQL installation issue * Update docstring. * Refactor graph_client to snapshot_graph_client for code clarity (#343) * Implement fix * Simplify integration test * Lint * Remove comment * Add comment * Resolve merge conflict * Delete references to non-existent non-graph interfaces * Address comments * Query cardinality estimation (#345) * Add query cost estimation (#317) * Add basic cost estimation * Add basic cost estimation tests * Add basic filter estimation tests * Add TODO for test SchemaGraph * Add more filter tests * Fix lint * Refactor cardinality estimation * Add filter selectivity tests * Add in_collection support to cardinality estimator * Document assumption for in_collection estimation * Add copyright line * Address comments * Delint * Change directory of test_cost_estimation to snapshot_tests * Delint * Revert merge * Reflect graph_client to snapshot_graph_client refactor from merge with master * Use INBOUND_EDGE_DIRECTION instead of EDGE_DESTINATION... * Nit * Merge OSX and Ubuntu MySQL sections * Nit * Address comments * Add not-contains filter operator (#349) * Create not-in-collection tests * Fix typos * Add gremlin query strings * Fix test cases for negation * Implement not-contains filter * Fix MATCH not-contains syntax * Add not-contains snapshot tests * Wrap all MATCH expressions in parentheses * Fix formatting nits * Relax tag filter constraint (#351) * Use TagInfo instead of context['tags'] * Add test for out of order tag and filter * Process tags before filters * Update readme * Add docstring * Fix comment * Remove initialization of context['tags'] * Not in collection filter (#350) * Create not-in-collection filter unit tests * Create not-in-collection filter snapshot tests * Implement not-in-collection filter * Make linter happy * nit * Fix bug in macro expansion with pro-forma fields (#415) * Fix bug in macro expansion with pro-forma fields * Address comments * Remove add transitive closure (#443) * Remove _add_transitive_closure * Nit: * Nit * Address comments * Fix typo * Make "macro_edge" usable in SDL field definitions. * Break out README changes into their own branch, for easier review. * Delint. * Integrate AST helpers refactor. * Delint. * Fix cross-module import. Allow deprecated directive. * Refactor validation, adding stricter checks and better error messages. * Disallow `@fold` directives on macro edges. * Disallow `@output_source` directives on macro edges as well. * Only allow filter directives at macro expansion points. * Improve error message for unsupported directive on macro edge. * Refactor macro edge expansion code to move it into its own module. * Remove dead code from helpers file. * Mark tests of unimplemented functionality with pytest xfail mark. * Add validation for schema conflicts of reverse macro edges. Also add new test cases for macro - reverse-macro conflicts. * Add validation error message. * Add more reverse macro edge validation logic. * Add macro edge target class name to macro edge descriptor. * Tweak test cases to ensure all validation error messages are hit. * Delint. * Refactor cross-macro validation code. * Delint. * Explicitly error out on duplicate tags during expansion. * Add macro validity rules description. * Fix typo. * Remove stale TODO. * Refactor and deduplicate validation code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to comments addressed #199 (comment)