Improve handling of polymorphism in the optimizer #720
Conversation
Reviewer's Guide by SourceryThis pull request improves the handling of polymorphic types in the optimizer. It fixes a bug where the optimizer did not honor the Updated class diagram for OptimizerStoreclassDiagram
class OptimizerStore {
only: List[str]
select_related: Set[str]
prefetch_related: Dict[str, Prefetch]
defer: Set[str]
only_load: Set[str]
clear()
add_select_related(field_path: str)
add_prefetch_related(field_path: str, queryset: QuerySet)
add_defer(field_path: str)
add_only_load(field_path: str)
apply(queryset: QuerySet, info: ResolveInfo, config: OptimizerConfig) : QuerySet
}
note for OptimizerStore "The OptimizerStore class stores optimization hints for a Django QuerySet."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @diesieben07 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case for when
disable_optimizationis set toTrueon a Django definition. - It might be worth adding a comment explaining why
_interfacesis being cached.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #720 +/- ##
==========================================
+ Coverage 88.21% 88.27% +0.05%
==========================================
Files 42 42
Lines 3853 3914 +61
==========================================
+ Hits 3399 3455 +56
- Misses 454 459 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Arrived on this PR by chance, currently also exploring polymorphic models with strawberry-django. I'm glad to see there are other folks working on this problem ! ... Except I'm using |
|
@bellini666 Thanks! I've addressed your notes. I am unsure why there were 3 pipeline failures now - the error seems very unrelated. @advl Adapting this for |
|
I have converted this PR to a draft, because I just noticed a flaw with the approach I have taken. It does not work for nested relations (e.g. "get all projects (in a polymorphic way) for a company"). Apologies for not catching that earlier. |
- Properly handle fields in subclasses when using django-polymorphic - Properly handle polymorphism in Connection and OffsetPaginated
diesieben07
force-pushed
the
optimizer-polymorphic
branch
from
35e79fe to
e911379
Compare
for more information, see https://pre-commit.ci
|
I have fixed the problem with relations not being optimized and added even more tests. |
There was a problem hiding this comment.
Hey @diesieben07 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to encapsulate the logic for extracting possible concrete types, as it's used in multiple places.
- The changes look good overall, but it would be helpful to have a more detailed explanation of the changes in the documentation.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if dj_definition is None or dj_definition.disable_optimization: | ||
| return None | ||
|
|
||
| if not issubclass(model, dj_definition.model): |
There was a problem hiding this comment.
question: Clarify the polymorphic model branch condition.
The condition that falls back to optimizing using the parent model when model is not a subclass of dj_definition.model yet may be a polymorphic model is subtle. It would be helpful to double-check that the intended relationship (using issubclass(dj_definition.model, model)) is the correct one and that there is no confusion between the model hierarchy directions.
| @@ -820,16 +825,25 @@ | |||
|
|
|||
| remote_field = model_field.remote_field | |||
| remote_model = remote_field.model | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the logic for iterating over concrete polymorphic types into helper functions to reduce nesting and improve readability, especially in functions like _get_model_hints and optimize
It looks like the added branches for polymorphic and concrete type handling have increased the nesting and interleaved concerns in a few functions. To reduce the complexity without reverting your changes, consider extracting the logic for iterating over concrete polymorphic types into helper functions. For example, you could create a dedicated function to aggregate “concrete stores” so that the loops in functions like `_get_model_hints` are simpler:
```python
def _aggregate_concrete_store(model, schema, concrete_type, parent_type, info, config, cache, level):
store = None
concrete_store = _get_model_hints(
model,
schema,
concrete_type,
parent_type=parent_type,
info=info,
config=config,
cache=cache,
level=level + 1,
)
if concrete_store is not None:
store = concrete_store if store is None else store | concrete_store
return storeThen update your loop as follows:
field_store = None
for concrete_field_type in get_possible_concrete_types(remote_model, schema, f_type):
concrete_store = _aggregate_concrete_store(
remote_model,
schema,
concrete_field_type,
parent_type=_get_gql_definition(schema, concrete_field_type),
info=field_info,
config=config,
cache=cache,
level=level,
)
if concrete_store is not None:
field_store = concrete_store if field_store is None else field_store | concrete_storeThis helps centralize the polymorphic type handling and reduces the nesting in your core functions, making the code easier to follow. Similar extraction techniques can be applied elsewhere (for instance, in the loops within the optimize and is_optimized functions) where concrete type iteration is performed.
…mizer-polymorphic # Conflicts: # docs/guide/optimizer.md
|
As discussed on Discord, I have expanded the scope of this pull request to also include support for |
|
Thank you @diesieben07 |
|
@diesieben07 Also Thank you :) You are resolving a lot of headaches with this PR 👍 |
|
@diesieben07 ultimately, have you tested the optimzer with a polymorphic relation inside a polymorphic subtype (with inheritance manager) ? (i have the case 😅 ) to optimize this type of query: query MyQuery {
communications {
edges {
node {
id
... on MessageDoss {
dossier {
... on DossierE {
id
}
... on DossierV {
id
}
}
}
... on NotiDoss {
id
}
}
}
}
} |
|
@stygmate I have not, but I don't see why it won't work. But I'll add a test-case for it! |
|
@diesieben07 I'm trying your branch. And the specific case i pointed seems to not work. The objects of the relations (foreignkey) are of the main django model type so i encounter a resolution of types error. I suspect select_subclasses to not be executed in this case. |
|
@stygmate I'm looking into it! |
|
Okay, I am not sure how to make this work. Here are the models that I used for testing: https://gist.github.com/diesieben07/7360ebf934d174abcf865c23856bef97 Assume we want to get all notifications (polymorphic) and then for every The top level "Notification" query will use @stygmate Can you let me know how you would write this query manually, without the optimizer? |
|
I looked further into this and there are multiple existing issues in both Django-Polymorphic and with InheritanceManager regarding relations in subclasses. Django PolymorphicNeither InheritanceManager
The issue here is that The following crude hack makes this work in my test: class CustomInheritanceQuerySet(InheritanceQuerySet):
def _prefetch_related_objects(self):
_base_objs = []
for obj in self._result_cache:
path = obj._meta.get_path_to_parent(self.model)
for p in path:
obj = getattr(obj, p.join_field.name)
_base_objs.append(obj)
prefetch_related_objects(_base_objs, *self._prefetch_related_lookups)
self._prefetch_done = True
|
|
@diesieben07 I will check all of that tomorrow. |
|
I'm not sure we're talking about the same thing. Create the virtual environment with Poetry, then run migrations, then the management command What I would like is for the optimizer to add this: This is a different issue, but in this case, the foreign key returns the base model, whereas it should return a subtype so that strawberry-django can select the correct GraphQL type. However, with this approach, everything fits into a single SQL query. The query is displayed on the screen during the execution of the |
|
@stygmate On first glance the first problem is that you're calling select_related and select_subclasses, which overwrite each other unless you do extra work. |
|
@stygmate I looked into it further, and as it stands that will never work with |
|
@diesieben07 I'll try using your model-utils version combined with this PR and run some tests. |
|
@diesieben07 thank you so much for this ❤️ Really well done and properly tested. Just some small things to adjust and it should be good to merge. Btw, regarding your discussion with @stygmate , is there anything else that you want to implement here? Or is it good to merge after the adjustments I'm asking? |
|
@bellini666 Thank you! Not sure if I missed it, but I can't see any requested changes from you. As for the discussion, in my opinion the still present issues with relations in polymorphic models are ones that the optimizer can't fix and that need to be fixed in Django-Model-Utils (see the PR I linked above) and Django-Polymorphic. |
This improves handling of polymorphic types in the optimizer, when using Django Polymorphic, InheritanceManager from django-model-utils or a custom solution.
Description
Following improvements are made:
polymorphic_ctypefield toonly. This field is used by django-polymorphic and would otherwise cause extra queries.prefixwhen adding the primary key toonly. This manifested when using Django Polymorphic.InheritanceManageris added. When used, the optimizer will use it to support polymorphic models, just like with django-polymorphic.Types of Changes
Checklist