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

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

Closed
ezyang opened this issue Mar 6, 2019 · 9 comments
Closed

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

ezyang opened this issue Mar 6, 2019 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ezyang
Copy link

ezyang commented Mar 6, 2019

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.

@Cito
Copy link
Member

Cito commented Mar 6, 2019

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
Copy link
Author

ezyang commented Mar 7, 2019

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
Copy link
Member

Cito commented Mar 7, 2019

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 added a commit that referenced this issue Mar 7, 2019
@Cito
Copy link
Member

Cito commented Mar 7, 2019

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 closed this as completed Mar 7, 2019
@Cito
Copy link
Member

Cito commented Mar 10, 2019

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

@Hellzed
Copy link

Hellzed commented Feb 20, 2020

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
Copy link
Member

Cito commented Feb 20, 2020

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.

@Cito Cito reopened this Feb 20, 2020
@Cito Cito self-assigned this Feb 20, 2020
@Cito Cito added the bug Something isn't working label Feb 20, 2020
@Cito Cito added this to the v3.1 milestone Feb 20, 2020
@Cito
Copy link
Member

Cito commented Feb 21, 2020

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

@Cito
Copy link
Member

Cito commented Mar 4, 2020

Version 3.1.0b1 with this fix has been released now.

@Cito Cito closed this as completed Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants