Support GQL interfaces for polymorphic SQLA models #365
Conversation
|
Thanks for the PR! Will check it out this week 🙂 |
Codecov ReportBase: 96.97% // Head: 97.09% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 96.97% 97.09% +0.11%
==========================================
Files 7 7
Lines 694 722 +28
==========================================
+ Hits 673 701 +28
Misses 21 21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@erikwrede Thanks! And sorry for the delay, been busy over here. I've added a doc, unit tests, and a docstring for |
|
@polgfred thanks for the update! Appreciate the weekend-effort 🙂 There's two things we should discuss before merging this: 1. Should we explicitly exclude the field linked in 2. Removing "message": "Abstract type 'PersonType' must resolve to an Object type at runtime for field 'Query.people'. Either the 'PersonType' type should provide a 'resolve_type' function or each possible type should provide an 'is_type_of' function."If the We need to clarify that the Please let me know what you think! |
|
@erikwrede Yeah, I agree with both those observations. An interface isn't exactly a polymorphic type for the reason you mentioned, but in my experience it's much more common for a polymorphic base class to be abstract, so we should call that out in the docs and examples. And I agree that it makes sense to hide the |
|
@erikwrede Added checks for the stuff you called out. |
- Added a note that polymorphic_on fields are excluded by default. - Added a note that a polymorphic_identity is unsupported for interfaces
…n-abstract Interface models chore: added type name to assertion error for easier debugging during development
|
Awesome, looks great to me! |
| # make sure that the model doesn't have a polymorphic_identity defined | ||
| if hasattr(_meta.model, "__mapper__"): | ||
| polymorphic_identity = _meta.model.__mapper__.polymorphic_identity | ||
| assert ( | ||
| polymorphic_identity is None | ||
| ), '{}: An interface cannot map to a concrete type (polymorphic_identity is "{}")'.format( | ||
| cls.__name__, polymorphic_identity | ||
| ) |
There was a problem hiding this comment.
@polgfred I just realized that this might have been a mistake to add. Sometimes we have a base type that is extended by other types but has a polymorphic identity. In this case, there would be an object type implementing the interface with no additional fields. We might need to revert hat bit, what do you think?
Hi!
Would love any feedback on this.
My background assumption here is that, for most folks using
graphene_sqlalchemybindings, GraphQL interfaces are going to take the form of polymorphic base classes. So it would be great if you could extendSQLAlchemyInterfacethe same way you extendSQLAlchemyObjectTypefor concrete types -- and wire up the attributes using the same reflection mechanism.So, in order to accomplish this, I:
SQLAlchemyObjectTypeup into aSQLAlchemyBasetypeSQLAlchemyObjectTypeand (a new)SQLAlchemyInterfaceextendSQLAlchemyBaseRegistryto accept objects of typeSQLAlchemyBaseI've tested this in a work project that depends heavily on a few polymorphic JTI base types, and this mechanism works perfectly. However, I'm sure I haven't thought through potential limitations and incorrect assumptions, which is why I'm submitting this for review and feedback.
Thanks!