Skip to content

There doesn't seem to be a way to override is_type_of on objects #25

Closed
@ezyang

Description

@ezyang

In my server, I have the following lines: https://github.com/ezyang/ghstack/blob/973ef7b25a71afa8f813cd8107f227938b3413f1/ghstack/github_fake.py#L288

GITHUB_SCHEMA.get_type('Repository').is_type_of = lambda obj, info: isinstance(obj, Repository)  # type: ignore
GITHUB_SCHEMA.get_type('PullRequest').is_type_of = lambda obj, info: isinstance(obj, PullRequest)  # type: ignore

I don't know how to put these on the Repository/PullRequest classes, in the same way resolvers can be put on the appropriate object class and then automatically called. The obvious things (e.g., adding an is_type_of method) do not work.

Activity

Cito

Cito commented on Mar 6, 2019

@Cito
Member

HI @ezyang, thanks for reporting this issue.

is_type_of is actually a method of the GraphQL type which checks whether the given object is of that type. What you're probably looking for is the __typename attribute which you can set on objects to specify their GraphQL type as a string and which is checked by default_resolve_type_fn. This is the way it's done also in GraphQL.js which graphql-core-next copies.

However, I see that that graphql-core-next currently checks __typename only on objects that are returned as dicts, not on (data)class instances like in your case.

It would certainly be a good idea and very simple to extend graphql-core-next to check for __typename attributes on non-dict objects as well - you would simply set __typename = 'Repository' on your Repository class. Unfortunately, there is an issue here in that Python applies name mangling to attributes starting with two underscores (a feature intended to be used for fake "private" attributes that is frowned upon but unfortunately still exists in Python 3).

So probably we should use the special name __typename__ instead (these names do not underly name mangling). It's not nice, since these special names are actually reserved for Python, and we deviate a bit from the GraphQL API, but it's the only obvious solution that comes to my mind just now. What do you think?

ezyang

ezyang commented on Mar 7, 2019

@ezyang
Author

A special name like __typename__ seems fine to me, as long as it's prominently documented.

Note that it's not impossible to demangle it, if you really wanted to. But I think it's probably better here to just hew to Pythonic conventions.

Cito

Cito commented on Mar 7, 2019

@Cito
Member

I also considered demangling and immediately dismissed the idea. But now after sleeping over it, I am thinking maybe it is not so bad an idea after all. The rule for the mangling - prefixing with the class name- is well documened and not implementation specific, and the class name can be easily inspected. That would allow us to be completely compatible to GraphQL.js and consistent between dicts and class based objects.

Another option would be check both attributes (__typename and __typename__), but that would go against the Zen of Python.

Cito

Cito commented on Mar 7, 2019

@Cito
Member

So I decided to go with the demangling. This will be available in the next version (released soon).

Please reopen if you have any better ideas.

Cito

Cito commented on Mar 10, 2019

@Cito
Member

Version 1.0,2 with this fix has now been released on PyPI.

Hellzed

Hellzed commented on Feb 20, 2020

@Hellzed

Hi @Cito , we might need to reopen this!
Unfortunately, in cases such as using an inherited __typename property, basic demangling is not enough.
Let's imagine I have the following (Relay inspired) Node and subclass:

class Node:
    @property
    def __typename(self):
        return self.__class__.__name__

class A(Node):
    ....

Obviously _A__typename will not be found, and we will ignore _Node__typename.

Something like this (instead of just using a simple getattr()) would help instead:

def get_typename(value):
    for cls in value.__class__.__mro__:
        __typename = getattr(value, f"_{cls.__name__}__typename", None)
        if __typename:
            return __typename
    return None

What do you think?

Cito

Cito commented on Feb 20, 2020

@Cito
Member

Good point @Hellzed. Will add your A(Node) example and maybe and a more complicated one with multiple inheritance to the test suite. Your suggested code will probably solve this.

reopened this on Feb 20, 2020
self-assigned this
on Feb 20, 2020
added
bugSomething isn't working
on Feb 20, 2020
added this to the v3.1 milestone on Feb 20, 2020
Cito

Cito commented on Feb 21, 2020

@Cito
Member

This is implemented now in the master branch and will be available in the next beta release.

2 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions

    There doesn't seem to be a way to override is_type_of on objects · Issue #25 · graphql-python/graphql-core