strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

Return union of Django objects

Open edouardruiz opened this issue 2 years ago • 10 comments

Hi,

Return an union of Django models isn't supported at the moment (strawberry-graphql==0.74.1 and strawberry-graphql-django==0.2.5).

This example will fail:

Contributor = strawberry.union("Contributor", types=(Tenant, ShippingCompany))
    @strawberry_django.field
    def contributor(self) -> Optional[Contributor]:
        return getattr(self.contributor, "shippingcompany", self.contributor)

with this error:

The type "<class 'users.models.ShippingCompany'>" cannot be resolved for the field "contributor" , are you using a strawberry.field?

One workaround is to set _type_definition on the retuned type like this:

    @strawberry_django.field
    def contributor(self) -> Optional[Contributor]:
        if hasattr(self.contributor, "shippingcompany"):
            self.contributor.shippingcompany._type_definition = ShippingCompany._type_definition

            return self.contributor.shippingcompany

        self.contributor._type_definition = Tenant._type_definition  # type: ignore [attr-defined]

        return self.contributor

It would be great if we could return the Django objects directly.

edouardruiz avatar Aug 31 '21 14:08 edouardruiz

Thanks @edouardruiz! Just wanted to add that this is issue is not just for Django but also for any other integration.

At the moment when using unions you need to return a Strawberry Type. We should allow to return any type, by either allowing to pass a function to unions that finds the correct type or by finding the right type automatically (not sure I like this option though)

patrick91 avatar Aug 31 '21 14:08 patrick91

It feels like it should be possible to solve this in strawberry-Django. @la4de is there a way to find the strawberry type from a Django model?

jkimbo avatar Sep 01 '21 06:09 jkimbo

@jkimbo are you suggesting that create a resolve_type for unions in strawberry django?

patrick91 avatar Sep 01 '21 07:09 patrick91

@patrick91 no, I was thinking that you could implement some logic in the strawberry-django resolver to look up the right StrawberryType from the Django instance. Something like this:

class StrawberryDjangoField(StrawberryField):
	def get_result(self, source, info, arguments):
		if self.resolved_type is Union:
			model_instance = super.get_result(source, info, arguments)
			strawberry_type = get_type_from_instance(model_instance, options=self.resolved_type.types)  # this function will need to be implemented
			return strawberry_type.from_instance(model_instance)
		...

What do you think?

jkimbo avatar Sep 01 '21 08:09 jkimbo

@jkimbo that sounds a like a good plan, we would still need to do a generic solution in future I think (for when you return dicts or other instances) :)

patrick91 avatar Sep 01 '21 08:09 patrick91

Is a method like strawberry_type.from_instance(o) actually available? I'm trying to figure out how to deal with this in my code right now.

Normally if I have a query or mutation that returns an object I can say:

@strawberry.field
def foo() -> GQLFoo:
    return Foo()

and strawberry seems to deal with generating a GQLFoo from my Foo so that its fields can work. Now, when I introduce a union:

@strawberry.field
def foo() -> Union[GQLFoo, OtherThing]:
    return Foo()

it blows up with that error in the issue description. Is there a way I could write GQLFoo.bind(Foo()) to explicitly invoke the logic that strawberry must have somewhere to do this conversion?

Edit: In other words, even if I don't want "implicit" detection of the type, how can I do it explicitly? (without having to write out GQLFoo(a=foo.a, b=foo.b, c=foo.c), which doesn't work in my case anyway because I need the self in resolvers inside of GQLFoo to actually be my Foo instance)

radix avatar Aug 03 '22 20:08 radix

I have been spelunking the code and I guess the issue with my assumption is that a GQLFoo is never actually instantiated when I return a Foo instance even without a union. So a method like GQLFoo.bind or strawberry_type.from_instance can't be implemented, at least not trivially.

I guess some sort of concept of a "bound instance" would need to be created? so that the resolving machinery can know which graphql type to look up resolvers on for a given concrete type?

I am pretty sure that Graphene solved this somehow, so there's probably some inspiration there

radix avatar Aug 03 '22 20:08 radix

I have been spelunking the code and I guess the issue with my assumption is that a GQLFoo is never actually instantiated when I return a Foo instance even without a union. So a method like GQLFoo.bind or strawberry_type.from_instance can't be implemented, at least not trivially.

I guess some sort of concept of a "bound instance" would need to be created? so that the resolving machinery can know which graphql type to look up resolvers on for a given concrete type?

I am pretty sure that Graphene solved this somehow, so there's probably some inspiration there

@radix I think this can be used by using resolve type on the union: https://github.com/graphql-python/graphql-core/blob/c214c1d93a78d7d1a1a0643b1f92a8bf29f4ad14/src/graphql/type/definition.py#L1004-L1020

/cc @bellini666 as you might have done something similar in strawberry django plus

patrick91 avatar Aug 03 '22 20:08 patrick91

Hey @patrick91 and @radix ,

If I'm understanding the issue correctly, it is on how to tell which type of a union the object is if it isn't actually an instance of it (e.g. a django model)

In both strawberry-graphql-django and strawberry-django-plus this issue was solved by implementing a custom is_type_of which basically checks for isinstance(obj, (cls, model)), the cls being the type class itself and the model the django model. Since the strawberry union implementation checks for that for non strawberry objects, it resolves to the correct type.

Here are both implementations for reference (both are injected when processing the type):

  • https://github.com/strawberry-graphql/strawberry-graphql-django/blob/main/strawberry_django/type.py#L174
  • https://github.com/blb-ventures/strawberry-django-plus/blob/main/strawberry_django_plus/type.py#L294

ps. The implementation of checking for cls.__dict__ is better because otherwise inheritances of that given type would not override that check.

I am pretty sure that Graphene solved this somehow, so there's probably some inspiration there

It actually solved in a way that also causes other issues =P

Graphene django has a register in which it basically created a map of model => type, which gets populated automatically when you create a model. This is fine for 99% of the cases, but what happens if you have more than one type created for a given model?

E.g. I faced this in a graphene project where I had 2 types for the User model, a UserType and a PublicUserType. The public one was the one I used to return in public places, which only displayed public and insensitive user information. What happened is that the last created one was the one graphene was going to map always. Also, the relay interface became sensitive to only one type, since it only know the model for one of the types (it could be fixed if the map was a model => [type]), so the other one did not exist based on that map.

bellini666 avatar Aug 03 '22 22:08 bellini666

I'd just like to suggest that I think having a supported, explicit mechanism to choose the variant, unrelated to any Django support, would be useful as the first solution to this problem (I assume _type_definition is a private member). If we can return an object that includes both the value and the chosen variant, then an automatic variant inference could be built on top of that.

radix avatar Aug 12 '22 18:08 radix