Schema renaming: Interface implementation suppression #1002

LWprogramming · 2021-03-17T22:50:04Z

Interfaces present a challenge for schema renaming that don't show up with regular object types uninvolved with interfaces.

This PR implements suppressing object types that implement interfaces. The main issue is that suppress an object type Foo that implements an interface Bar can cause problems if, later on, someone queries for all objects that implement Bar and ends up getting objects of suppressed type Foo. If this happens, we remove the interface (as well as all its ancestor interfaces*) from RootSchemaQuery to make it unqueryable.

*For the directed graph consisting of all types in the schema as nodes and the edges are from an interface to a type that implements it (for every such interface and type pair), an interface is an ancestor of another type iff there's a path from the interface's node to that type's node.

codecov · 2021-03-18T20:35:03Z

Codecov Report

Merging #1002 (ee3048c) into main (44c63aa) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head ee3048c differs from pull request most recent head 2d997ee. Consider uploading reports for the commit 2d997ee to get more accurate results

@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
+ Coverage   94.38%   94.40%   +0.02%     
==========================================
  Files         113      113              
  Lines        9080     9133      +53     
==========================================
+ Hits         8570     8622      +52     
- Misses        510      511       +1

Impacted Files	Coverage Δ
...ql_compiler/schema_transformation/rename_schema.py	`99.66% <100.00%> (-0.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44c63aa...2d997ee. Read the comment docs.

graphql_compiler/schema_transformation/rename_schema.py

LWprogramming · 2021-03-19T15:34:20Z

graphql_compiler/schema_transformation/rename_schema.py

    renamed_schema_ast = visit(schema_ast, visitor)
    return renamed_schema_ast


+def _recursively_get_ancestor_interface_names(


Necessary to define this function outside the RenameSchemaTypesVisitor object because we need access to the schema's definitions field in order to get the interface definition nodes.

This depends on some other code being merged into main, such as the code for interface implementation suppression in PR #1002, so this is just a rough draft at the moment and just contains the rough outline for what this code would look like.

graphql_compiler/schema_transformation/rename_schema.py

chewselene · 2021-03-31T15:42:11Z

graphql_compiler/schema_transformation/rename_schema.py

+- If you suppress a type Foo implementing an interface Bar, then Bar will remain in the schema but
+  not in the root query type, making it unqueryable. This is to prevent situations where a scope in
+  a query is of type Bar without also being of some more specific type, which may yield vertices of
+  type Foo even though it was suppressed. For this reason, all interfaces that Bar implements will
+  also be made unqueryable in the same way, and this removal from the root query type will happen
+  recursively until all interfaces that Foo implements (either directly or indirectly) are
+  unqueryable.


Looks like this comment could be updated to mention that field interface suppression is not yet implemented i.e. what this test is addressing.

Hm, so this is a bit tricky-- the plan for field renamings in PR 1005 is more like allowing something that should already exist, specifically if we have a schema like

# Exists interface named Foo with a single field named id type Bar implements Foo { name: String id: String }

then it should be legal to rename the name field, just like if Bar implemented no interfaces. In other words, it seems like it's less of a large functionality change that's coming soon and more like allowing something that should be allowed without adding more special cases to the field renaming API, had the NotImplementedError checks not been so strict.

Is the case in the test case I linked above going to be implemented eventually or will it always be a CascadingSuppressionError? If it is going to be implemented eventually, maybe in the Operations that are not yet supported but will be implemented: section there should be something like automatic suppression of fields when the field is of a suppressed interface type?

The current implementation will always be a CascadingSuppressionError-- the problem in that test case is that the Giraffe type's friend field is of type Character, and Character is to be made unqueryable. However, there's no way to suppress any fields in Giraffe right now because Giraffe implements an interface, so until the changes in #1005 relax this restriction, there's no way to implement the part of the test that shows how we'd want to do such a thing.

Right, I'm not proposing you implement in this PR, but I think it should be documented somewhere that you can't suppress an object that implements an interface if that interfaces is a field type on another object. Is that already documented somewhere that I missed?

graphql_compiler/schema_transformation/rename_schema.py

Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com>

graphql_compiler/schema_transformation/rename_schema.py

chewselene · 2021-04-14T15:33:33Z

graphql_compiler/schema_transformation/rename_schema.py

+        type_not_suppressed = type_renamings[type_name] is not None
+        if type_not_suppressed:
+            continue
+        if type_name in interface_and_object_type_name_to_definition_node_map:


Suggested change

type_not_suppressed = type_renamings[type_name] is not None

if type_not_suppressed:

continue

if type_name in interface_and_object_type_name_to_definition_node_map:

# <comment>

if type_renamings[type_name] is None and type_name in interface_and_object_type_name_to_definition_node_map:

nit: this seems like it could all be done in 1 conditional. What does it mean if type_name in interface_and_object_type_name_to_definition_node_map is False?

This is a bit unfortunate-- so, as it is, we need to compute which interfaces should be made unqueryable before doing any sort of renaming. However, all the error handling for unused type renamings happens after all the renaming is done. If type_name, an entry in type_renamings, is not in the map, then type_name doesn't actually correspond to a type in the schema, and it should have no effect. Now that I look at this again, there might be a better way to organize this but I'm not quite sure how might work best-- open to suggestions!

(I suppose this also affects the first part, where we might do this in a single conditional.)

When type_name not in interface_and_object_type_name_to_definition_node_map, the user is attempting to rename something that is not in the schema, right? It seems like that should be an error to me because renaming something that doesn't exist does not make sense. Am I understanding the meaning correctly, and, if so, any reasons this shouldn't be an error?

It does end up as an error, but the error handling happens later on. The structure of the code, before this PR, is along the lines of:

Go through the schema, applying renamings via the visitor pattern

If there are any unused renamings, then raise an error saying which renamings were unused.

Now, we're in a bit of a catch-22 situation: the point of the visitor is that we don't have access to the entire schema-- only individual nodes at any one time. However, we can't find out which interface nodes to be made unqueryable without schema_ast.definitions, which needs to be done outside a visitor object (and therefore before any detection of unused renamings).

The workaround that this PR currently uses is basically ignoring unused type renamings because they'll be handled later on in the helper functions that use visitor objects. Hacky, but I'm not sure if we have other options here.

I'm assuming you want to hold off on raising an error message until later because it will be a more complete error message with all the issues in the schema renaming, right? This could definitely benefit from a comment explaining that and I still think it can just be 1 conditional.

chewselene · 2021-04-14T16:11:56Z

graphql_compiler/schema_transformation/rename_schema.py

+- If you suppress a type Foo implementing an interface Bar, then Bar will remain in the schema but
+  not in the root query type, making it unqueryable. This is to prevent situations where a scope in
+  a query is of type Bar without also being of some more specific type, which may yield vertices of
+  type Foo even though it was suppressed. For this reason, all interfaces that Bar implements will
+  also be made unqueryable in the same way, and this removal from the root query type will happen
+  recursively until all interfaces that Foo implements (either directly or indirectly) are
+  unqueryable.


Is the case in the test case I linked above going to be implemented eventually or will it always be a CascadingSuppressionError? If it is going to be implemented eventually, maybe in the Operations that are not yet supported but will be implemented: section there should be something like automatic suppression of fields when the field is of a suppressed interface type?

LWprogramming added 5 commits March 16, 2021 17:13

Move interfaces test schema into various types schema

425dc79

Write interface & implementation suppression tests

70ccf6d

Implement interface implementation suppression (with hack)

0c21b8c

Update interface implementation suppression comment

bd62568

Keep only interface implementation suppression tests to cut scope

e06a5e6

LWprogramming added 3 commits March 19, 2021 11:07

Replace hack with proper code

b1c18cc

lint

ae662c1

Clean up comments

9a321bf

LWprogramming commented Mar 19, 2021

View reviewed changes

graphql_compiler/schema_transformation/rename_schema.py Outdated Show resolved Hide resolved

LWprogramming commented Mar 19, 2021

View reviewed changes

LWprogramming changed the title ~~Schema renaming: Interface and implementation suppression~~ Schema renaming: Interface implementation suppression Mar 19, 2021

LWprogramming marked this pull request as ready for review March 19, 2021 15:42

LWprogramming mentioned this pull request Mar 23, 2021

Permit field renaming for interface implementations #1005

Draft

chewselene reviewed Mar 24, 2021

View reviewed changes

LWprogramming added 6 commits March 24, 2021 16:09

Address comments

cffdb7b

lint

499d4b5

typo

269ab07

Implement cascading suppression error for fields of unqueryable types

70e02ce

lint

16b120f

clean up test for this particular PR

80284bd

chewselene reviewed Mar 31, 2021

View reviewed changes

LWprogramming and others added 7 commits April 5, 2021 12:11

Update graphql_compiler/schema_transformation/rename_schema.py

154c93b

Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com>

Update graphql_compiler/schema_transformation/rename_schema.py

3e53924

Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com>

Update graphql_compiler/schema_transformation/rename_schema.py

79f99f4

Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com>

remove trailing space

658a2c5

address comments

ee3048c

Merge branch 'main' into interface_type_suppression

c985dea

lint

2d997ee

LWprogramming requested a review from chewselene April 12, 2021 19:12

chewselene reviewed Apr 14, 2021

View reviewed changes

Address comments

22bd925

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema renaming: Interface implementation suppression #1002

Schema renaming: Interface implementation suppression #1002

LWprogramming commented Mar 17, 2021 •

edited

codecov bot commented Mar 18, 2021 •

edited

LWprogramming Mar 19, 2021

chewselene Mar 31, 2021

LWprogramming Apr 6, 2021

chewselene Apr 14, 2021

LWprogramming Apr 28, 2021

chewselene Apr 28, 2021

chewselene Apr 14, 2021

LWprogramming Apr 20, 2021

chewselene Apr 26, 2021

LWprogramming Apr 28, 2021

chewselene Apr 28, 2021

chewselene Apr 14, 2021

Schema renaming: Interface implementation suppression #1002

Are you sure you want to change the base?

Schema renaming: Interface implementation suppression #1002

Conversation

LWprogramming commented Mar 17, 2021 • edited

codecov bot commented Mar 18, 2021 • edited

Codecov Report

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

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

LWprogramming commented Mar 17, 2021 •

edited

codecov bot commented Mar 18, 2021 •

edited