Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic interface inheritance support (fixes #8) #9

Merged
merged 2 commits into from Dec 2, 2022

Conversation

nettrino
Copy link

@nettrino nettrino commented Dec 1, 2022

This PR partially addresses #8. Currently, most graphql servers (and particularly for our case, neo4j over apollo) don't appropriately support interfaces inheriting from interfaces. Thus, if we have smth like

  interface ResourceEntity {
    id: ID! @id(autogenerate: false)
    type: NodeType!
  }
  
  interface CloudResourceEntity implements ResourceEntity {
    id: ID! @id(autogenerate: false)
    type: NodeType!
    provider: CloudProvider!

then CloudResourceEntity will be a regular interface without implementing the secondary interface in the schema that will be emitted. As we autogenerate types, we want any field or directive (e.g., a @relationship directive) that is of type ResourceEntity, to support CloudResourceEntity. We want to be able to do this in a best effort way for auto-generated code, until things are supported by neo4j or other servers.

For types that implement CloudResourceEntity, they need to declare that they implement ResourceEntity as well, so for python, we just need to fix inheritance for CloudResourceEntity classes, as well as any types that implement the interface (e.g., so as to not have method resolution order problems). Thus, assuming a type

 type AWSCloudPlatform implements CloudPlatform & CloudResourceEntity & ResourceEntity {
...
} 

we want the emitted python code to be

@dataclass(kw_only=True)
class ResourceEntity(DataClassJSONMixin):
...

@dataclass(kw_only=True)
class CloudResourceEntity(ResourceEntity):
...

@dataclass(kw_only=True)
class AWSCloudPlatform(CloudPlatform, CloudResourceEntity):
...

This was not supported as of today as

  • interfaces were not being taken into account wrt to inheritance, and thus never had a parent class
  • we did not resolve multi-inheritance properly for types, thus class AWSCloudPlatform parent class hierarchy would be getting emitted as (CloudPlatform, CloudResourceEntity, ResourceEntity), which messes up MRO.

This still leaves a question open as to how do we deal with accessing said types via graphql when it comes to neo4j. For instance, if we have a field of type ResourceEntity, and we want to pass CloudResourceEntity there, that is allowed by the RFC (see graphql/graphql-spec#373) but this won't be understood by the graphql server for now. Unions of interfaces are also not allowed by the SPEC (graphql/graphql-js#451).

That said, I don't know if we will ever run into this issue, since we would be able to pass all types implementing the interface as inputs (they will be implementing all interfaces in case of intermediate interfaces by default, thus AWSCloudPlatform always has to implement both CloudResourceEntity and ResourceEntity). Thus I think this will be OK for now for most practical scenarios.

Since the GraphQL that will be getting emitted by Apollo+Neo4j as of today will strip away the implements part of any interface, essentially converting

  interface CloudResourceEntity implements ResourceEntity {
    id: ID! @id(autogenerate: false)
    ...

to

  interface CloudResourceEntity  {
    id: ID! @id(autogenerate: false)
 ...

we are forced to pass interface inheritance information via the config file. To do so, we use interfaceInheritance as a magic key in the yaml config, so we would be passing smth like:

scalars:
  CVE: str
  ...

interfaceInheritance:
  CloudResourceEntity: ResourceEntity

Copy link

@miki725 miki725 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 some hacks here and there but we should prioritize getting schema to function. I have a feeling well need more refactors soon so prolly worth seeing all requirements first before cleaning things up further

@@ -18,6 +20,14 @@ class BlockFieldInfo(NamedTuple):
value_type: str


# a dictionary where for each node, we hold its children
inheritanceTree: Dict[str, Set[str]] = {"root": set()}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
inheritanceTree: Dict[str, Set[str]] = {"root": set()}
inheritanceTree: Dict[str, Set[str]] = defaultdict(lambda: set())

https://docs.python.org/3.11/library/collections.html#collections.defaultdict

then whatever you access on the dict is always present automatically

Comment on lines 76 to 78
siblings = inheritanceTree.get(deps[display_name], set())
siblings.add(display_name)
inheritanceTree[deps[display_name]] = siblings
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
siblings = inheritanceTree.get(deps[display_name], set())
siblings.add(display_name)
inheritanceTree[deps[display_name]] = siblings
inheritanceTree.setdefault(deps[display_name], set()).add(display_name)

https://docs.python.org/3.11/library/stdtypes.html#dict.setdefault

or if you do defaultdict as per above:

Suggested change
siblings = inheritanceTree.get(deps[display_name], set())
siblings.add(display_name)
inheritanceTree[deps[display_name]] = siblings
inheritanceTree[deps[display_name]].add(display_name)

Comment on lines 90 to 92
siblings = inheritanceTree.get(p, set())
siblings.add(display_name)
inheritanceTree[p] = siblings
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar pattern as above with either setdefault or defaultdict()[]


def update_interface_dependencies(config_file_content):
global INTERMEDIATE_INTERFACES
if type(config_file_content) is dict:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if type(config_file_content) is dict:
if isinstance(config_file_content, dict)

doing type check with is is not ideal:

>>> class Foo(dict): pass
>>> type(Foo()) is dict
False
>>> isinstance(Foo(), dict)
True

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but I am explicitly followed the style of the rest of the repo here in the thought that we will perhaps refactor everything together

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you do the defaultdict suggestion isinstance will be required. either way its up to you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to make the change - leaving extra bad code in was not great and leaves more work for potential future refactorings

global INTERMEDIATE_INTERFACES
if type(config_file_content) is dict:
data = config_file_content.get("interfaceInheritance")
if type(data) is dict:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Comment on lines 57 to 58
if not intermediate_interfaces:
intermediate_interfaces = INTERMEDIATE_INTERFACES
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not intermediate_interfaces:
intermediate_interfaces = INTERMEDIATE_INTERFACES
intermediate_interfaces = intermediate_interfaces or INTERMEDIATE_INTERFACES

usually simplifies things a bit, especially if you count the mccabe function complexity its nice to reduce any direct if statements to more simple alternatives

@nettrino nettrino merged commit 61f540a into main Dec 2, 2022
@nettrino nettrino deleted the nettrino/interface_inheritance branch December 2, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants