Implement explicit interface type resolution #1406

AlecRosenbaum · 2021-11-05T13:09:49Z

Description

The behavioral changes included in this PR are:
a) duck typing on fields that return interfaces or types implementing an interface now requires an explicit is_type_of opt-in
b) adding an explicit is_type_of opt-in is now implemented with a is_type_of classmethod

This seems like it might require a change to the documentation mentioning the classmethod, which I can make assuming this approach looks good.

Also, what release type should this be? Technically it is a breaking change, though it's quite minor.

Types of Changes

Issues Fixed or Closed by This PR

No way to resolve type of non-strawberry types returned for an interface #1405

Checklist

I did dev work within a codespace based on your template (which was very convenient), so at the very least all the pre-commit scripts ran correctly. I also ran the test suite, which passes.

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

botberry · 2021-11-05T13:12:33Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This releases fixes an issue where you were not allowed
to return a non-strawberry type for fields that return
an interface. Now this works as long as each type
implementing the interface implements an is_type_of
classmethod. Previous automatic duck typing on types
that implement an interface now requires explicit
resolution using this classmethod.

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Alec Rosenbaum for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

AlecRosenbaum · 2021-11-18T20:38:06Z

I've just rebased/force pushed to resolve some conflicts with main. None of the content should have changed, it was just imports.

codecov · 2021-11-18T20:52:17Z

Codecov Report

Merging #1406 (5a12b9c) into main (ab4a6a6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1406   +/-   ##
=======================================
  Coverage   98.08%   98.09%           
=======================================
  Files         119      119           
  Lines        4129     4143   +14     
  Branches      599      604    +5     
=======================================
+ Hits         4050     4064   +14     
  Misses         42       42           
  Partials       37       37

patrick91 · 2021-11-18T20:53:05Z

@AlecRosenbaum thanks! does this work for unions too?

I'll take a proper look during the weekend 😊

AlecRosenbaum · 2021-11-18T21:21:15Z

It does now 🙂

patrick91

This looks great! ✨ I've left a few comments. Before merging this I'd like to get @BryceBeagle and @jkimbo's opinions on the API:

with this PR we allow to pass a classmethod is_type_of like so:

@strawberry.type
 class Anime(Node):
     name: str

     @classmethod
     def is_type_of(cls, obj, _info) -> bool:
         return isinstance(obj, AnimeORM)

which is a bit different that what we do at the moment, I mean, at the moment
we pass all the configuration options to @strawberry.type.

I'm fine with having is_type_of being a classmethod, but I want to see what you all think :)

@AlecRosenbaum we should add some documentation too :)

RELEASE.md

strawberry/object_type.py

strawberry/schema/schema_converter.py

patrick91 · 2021-11-23T19:43:52Z

strawberry/schema/schema_converter.py

@@ -295,11 +279,23 @@ def get_graphql_fields() -> Dict[str, GraphQLField]:

            return graphql_fields

+        is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]]


GraphQLResolveInfo <- we should use StrawberryInfo, not sure we can do this easily here though, maybe we can do it after we do this issue: #1425

strawberry/union.py

tests/schema/test_interface.py

Co-authored-by: Patrick Arminio <[email protected]>

strawberry/experimental/pydantic/object_type.py

BryceBeagle · 2021-11-28T10:48:20Z

If we're having users implement a function to indicate duck typing is possible... why not just have them implement a function that does the cast/conversion instead? I'm not sure we want to go this deep into supporting duck-typing

patrick91 · 2021-11-28T11:59:40Z

If we're having users implement a function to indicate duck typing is possible... why not just have them implement a function that does the cast/conversion instead? I'm not sure we want to go this deep into supporting duck-typing

This is something I used to do at my previous job, conversion requires a lot of typing and I don't think it's worth the trouble in all cases, if you think a strawberry object as a protocol it would make sense being allowed to return any type that conform the protocol :)

Casting might be an option, but I don't really know how we can do that (other than using some hacks). The other issue with it is that you'd require to do it in every resolver.

We already support duck typing by the way, it's just not well supported when using interfaces/unions (since GraphQL needs to know the actual return type)

jkimbo · 2021-11-28T12:59:21Z

conversion requires a lot of typing and I don't think it's worth the trouble in all cases

@patrick91 Can you give an example of this? In my experience conversion doesn't require any extra typing.

patrick91 · 2021-11-28T14:08:31Z

conversion requires a lot of typing and I don't think it's worth the trouble in all cases

@patrick91 Can you give an example of this? In my experience conversion doesn't require any extra typing.

Oh I meant writing code, not type hints :D

@strawberry.type
class User: 
    name: str

    @classmethod
    def from_model(cls, model) -> User:
        return User(name=model.name)

imagine this with 10 fields :)

jkimbo · 2021-11-28T14:19:01Z

Oh I meant writing code, not type hints :D

@strawberry.type
class User: 
    name: str

    @classmethod
    def from_model(cls, model) -> User:
        return User(name=model.name)

imagine this with 10 fields :)

Ok yeah that does make sense. IMO I still prefer that to duck typing but I can appreciate why it's painful.

Separately I've just discovered that graphql-core will check for a __typename attribute on a value to map an abstract type (union or interface) to a concrete type: https://github.com/graphql-python/graphql-core/blob/v3.1.6/src/graphql/execution/execute.py#L1254-L1272

Could that be a better way of dealing with this?

patrick91 · 2021-11-28T17:31:46Z

Ok yeah that does make sense. IMO I still prefer that to duck typing but I can appreciate why it's painful.

I feel like there might be cases where you're dealing with dictionaries coming from something like mongodb or an API. I don't think we should force people to convert them to instances of Strawberry objects.

In addition to that there's also other usecases, like getting partial objects. For example:

@strawberry.type
class User:
    name: str 
    age: int

@strawberry.type
class Query:
    @strawberry.field
    def user(self, info: Info) -> User:
        selected_fields = fields_from_info(info)
        return models.User.first().only(selected_fields)

This can't be done at all with plain Strawberry Objects, at least for now 😊

That's why I think we should allow duck typing, especially to people that know what they are doing :)

Separately I've just discovered that graphql-core will check for a __typename attribute on a value to map an abstract type (union or interface) to a concrete type: graphql-python/[email protected]/src/graphql/execution/execute.py#L1254-L1272

This is neat! Didn't know that 😊 not sure it would be the solution, as it still requires doing some work in the resolvers, see this ugly example here:

@strawberry.type
class Query:
    @strawberry.field
    def user(self, info: Info) -> SomeInterface:
        items = SomePolymorficModel.all()[:10]
        return add_type_name_to_each_item(items)

I think having a is_type_of function is still preferable to this, especially in context of using other frameworks with Strawberry (django, pydantic, etc)

Sorry if it looks like I'm insisting on this, I'm not 100% liking the idea of having to defined a classmethod on object types, but I'm trying to see it as a potential user of strawberry, and I wouldn't be happy to force to convert compatibile types to other types 😊

BryceBeagle · 2021-11-28T18:58:59Z

Dictionaries are one thing, but I don't think we should be encouraging duck typing between objects between separate libraries (e.g. an ORM and Strawberry). This seems incredibly fragile and prone to breakage/unexpected failure. Feels way too javascript-y to me. At minimum, I think we should have a "strict" mode toggle that disables/enables this loose (and imo dangerous) behavior.

patrick91 · 2021-11-28T20:01:37Z

Dictionaries are one thing

what makes a dictionary special? I'd argue it is better to support duck typing rather than supporting dictionaries :)

but I don't think we should be encouraging duck typing between objects between separate libraries (e.g. an ORM and Strawberry).

We don't encourage, but we shouldn't disallow it. People will want to be able to do that, and again I think we should treat GraphQL types are protocols more than strict classes.

This seems incredibly fragile and prone to breakage/unexpected failure.

We should make errors clearer then, type checkers already help with this :) I've personally used duck typing without any issue ☺️

Feels way too javascript-y to me. At minimum, I think we should have a "strict" mode toggle that disables/enables this loose (and imo dangerous) behavior.

Would this run isinstance on all returned values?

AlecRosenbaum · 2021-12-01T20:40:44Z

What are the next steps for this PR then?

I'm happy to implement an alternative solution as long as it would address the issue of resolving interfaces on duck-typed objects. At the very least, not supporting this would be a big pain point in our gradual transition away from graphene, where we already have a relatively large schema that is 100% reliant on this pattern. We currently have grahpene/strawberry type definitions intermixed but all resolved using duck typing.

jkimbo · 2021-12-02T09:59:38Z

I'm happy for this to be merged. I still have reservations about the api but I also don't have a better solution so we should merge it since it's blocking @AlecRosenbaum

patrick91 · 2021-12-02T10:08:35Z

~~I think we only need to prevent from having to always pass is_type_of now :) #1406 (comment)~~

Edit: I did misunderstand the code :)

patrick91

Thank you for doing this PR, and sorry it took a while to get it merged 😊

botberry · 2021-12-04T15:01:36Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

botberry added bot:has-release-file labels Nov 5, 2021

AlecRosenbaum mentioned this pull request Nov 8, 2021

Migration from Graphene to Strawberry #1042

Closed

patrick91 self-requested a review November 15, 2021 19:02

AlecRosenbaum added 4 commits November 18, 2021 20:30

add failing test

fb16930

implement explicit is_type_of override

c24da6b

add RELEASE.md

a26301d

fix typo within release.md

a7c96fa

AlecRosenbaum force-pushed the implement-interface-type-resolution branch from 4b06434 to a7c96fa Compare November 18, 2021 20:34

AlecRosenbaum added 2 commits November 18, 2021 21:20

add failing test

e9faa62

add union support for is_type_of resolution

3c9744b

patrick91 reviewed Nov 23, 2021

View reviewed changes

Apply suggestions from code review

f69f244

Co-authored-by: Patrick Arminio <[email protected]>

botberry added bot:release-type-minor and removed bot:release-type-patch labels Nov 23, 2021

AlecRosenbaum added 3 commits November 23, 2021 20:09

reformat with black

2a48748

implicitly define is_type_of on pydantic-based object types

fdc290a

fix type checking on is_type_of

5a12b9c

AlecRosenbaum commented Nov 23, 2021

View reviewed changes

strawberry/experimental/pydantic/object_type.py Show resolved Hide resolved

jkimbo approved these changes Dec 2, 2021

View reviewed changes

patrick91 approved these changes Dec 4, 2021

View reviewed changes

patrick91 merged commit eae612e into strawberry-graphql:main Dec 4, 2021

AlecRosenbaum deleted the implement-interface-type-resolution branch December 6, 2021 07:16

pheki mentioned this pull request Aug 1, 2024

No way to resolve type of non-strawberry types returned for an interface #1405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement explicit interface type resolution #1406

Implement explicit interface type resolution #1406

AlecRosenbaum commented Nov 5, 2021 •

edited

Loading

botberry commented Nov 5, 2021 •

edited

Loading

AlecRosenbaum commented Nov 18, 2021

codecov bot commented Nov 18, 2021 •

edited

Loading

patrick91 commented Nov 18, 2021

AlecRosenbaum commented Nov 18, 2021

patrick91 left a comment

patrick91 Nov 23, 2021

BryceBeagle commented Nov 28, 2021

patrick91 commented Nov 28, 2021

jkimbo commented Nov 28, 2021

patrick91 commented Nov 28, 2021

jkimbo commented Nov 28, 2021

patrick91 commented Nov 28, 2021

BryceBeagle commented Nov 28, 2021

patrick91 commented Nov 28, 2021 •

edited by jkimbo

Loading

AlecRosenbaum commented Dec 1, 2021

jkimbo commented Dec 2, 2021

patrick91 commented Dec 2, 2021 •

edited

Loading

patrick91 left a comment

botberry commented Dec 4, 2021

		@@ -295,11 +279,23 @@ def get_graphql_fields() -> Dict[str, GraphQLField]:

		return graphql_fields

		is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]]

Implement explicit interface type resolution #1406

Implement explicit interface type resolution #1406

Conversation

AlecRosenbaum commented Nov 5, 2021 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

botberry commented Nov 5, 2021 • edited Loading

AlecRosenbaum commented Nov 18, 2021

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

patrick91 commented Nov 18, 2021

AlecRosenbaum commented Nov 18, 2021

patrick91 left a comment

Choose a reason for hiding this comment

patrick91 Nov 23, 2021

Choose a reason for hiding this comment

BryceBeagle commented Nov 28, 2021

patrick91 commented Nov 28, 2021

jkimbo commented Nov 28, 2021

patrick91 commented Nov 28, 2021

jkimbo commented Nov 28, 2021

patrick91 commented Nov 28, 2021

BryceBeagle commented Nov 28, 2021

patrick91 commented Nov 28, 2021 • edited by jkimbo Loading

AlecRosenbaum commented Dec 1, 2021

jkimbo commented Dec 2, 2021

patrick91 commented Dec 2, 2021 • edited Loading

patrick91 left a comment

Choose a reason for hiding this comment

botberry commented Dec 4, 2021

AlecRosenbaum commented Nov 5, 2021 •

edited

Loading

botberry commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

patrick91 commented Nov 28, 2021 •

edited by jkimbo

Loading

patrick91 commented Dec 2, 2021 •

edited

Loading