-
Notifications
You must be signed in to change notification settings - Fork 57
Feat: Adding the option of importing interfaces from strings #423
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
base: main
Are you sure you want to change the base?
Feat: Adding the option of importing interfaces from strings #423
Conversation
|
Thank you. Can you add a test or two? |
|
@zerolab , done :) |
|
@zerolab can we merge this feat? |
579ae8b to
874e3d3
Compare
jams2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Hercilio1, some feedback follows.
If possible, it would be helpful to hear more about your use-case. When you say your interfaces depend on data in models, what exactly are you referring to? Could you share a reproducible example?
I don't have a strong objection to allowing interfaces to be declared as strings (or even callables), but I wonder if the issue might be solved by different architecture or looser coupling between components.
grapple/actions.py
Outdated
| interface_classes = list(getattr(cls, "graphql_interfaces", ())) | ||
| for i, interface_class in enumerate(interface_classes): | ||
| if isinstance(interface_class, str): | ||
| interface_classes[i] = import_string(interface_class) | ||
| interface_classes = tuple(interface_classes) | ||
|
|
||
| interfaces = {interface, *interface_classes} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some unnecessary construction of intermediate containers here:
- cast
interface_classesto a list; - mutate it;
- cast it to a tuple;
- use the tuple elements in a new set.
The desired behaviour might be expressed more simply, something like:
| interface_classes = list(getattr(cls, "graphql_interfaces", ())) | |
| for i, interface_class in enumerate(interface_classes): | |
| if isinstance(interface_class, str): | |
| interface_classes[i] = import_string(interface_class) | |
| interface_classes = tuple(interface_classes) | |
| interfaces = {interface, *interface_classes} | |
| interface_classes = getattr(cls, "graphql_interfaces", ()) | |
| interfaces = { | |
| import_string(i) if isinstance(i, str) else i for i in interface_classes | |
| } | |
| interfaces.add(interface) |
| settings, "WAGTAILDOCS_DOCUMENT_MODEL", "wagtaildocs.Document" | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: rather than amending the existing tests to include the new functionality, could we add new tests that cover new behaviour and its edge cases? E.g. what happens when a string doesn't resolve?
@jams2 , this issue arises because the project I’m integrating with wagtail-grapple has a large number of GraphQL interfaces. Many of these interfaces are not directly tied to a single model but are complementary features that extend the functionality of Wagtail pages. The challenge is that some of these interfaces need to import their related models to implement custom business logic. If I import those interfaces directly inside the models.py file (so I can list them in the graphql_interfaces attribute), Django attempts to load the model before the app registry is ready, which raises the AppRegistryNotReady("Models aren't loaded yet.") exception. By allowing interfaces to be referenced as strings (e.g. "apps.custom.graphql.object_types.CustomInterface"), we avoid the premature import problem. This approach is harmless, keeps the architecture flexible, and provides a clean solution for projects with multiple, interdependent interfaces. I also addressed all your reviews :) |
I want to add new interfaces to a Page model, but the problem is that those interfaces import model files to collect different sets of data from them.
The issue is that
graphql_interfaces = [CustomInterface]requires importingCustomInterfaceinside the model file, which causes the errorAppRegistryNotReady("Models aren't loaded yet.").The
load_type_fieldsruns at the right time, but I can’t definegraphql_interfaceswithout importing the interface in the models file.The fix would be something like
graphql_interfaces = ["apps.custom.graphql.object_types.CustomInterface"], so I implemented it this way.