Fixes: #17976 DeviceType_Count on Manufacturer #18583
Conversation
|
@renatoalmeidaoliveira it would be ideal to solve this by enabling recursion through nested serializers inside |
|
@jeremystretch that was my first try, i.e. using |
|
About using Using
|
|
About changing Running with this with Debug it executes the following query: And looking at the results inside Django debug, its returning the correct results but as an attribute of
So even the query returning the expected results the JSON output omits the result instead of adding it inside the Nested Object |
netbox/utilities/api.py
Outdated
| @@ -87,6 +88,7 @@ def get_prefetches_for_serializer(serializer_class, fields_to_include=None): | |||
| fields_to_include = serializer_class.Meta.fields | |||
|
|
|||
| prefetch_fields = [] | |||
| annotaded_prefetch = {} | |||
There was a problem hiding this comment.
minor spelling issue, annotated would be correct spelling
netbox/utilities/api.py
Outdated
| # Add an annotation to the annotaded_prefetch | ||
| if isinstance(serializer_field, RelatedObjectCountField) and source_field is not None: | ||
| if model_field_name not in annotaded_prefetch: | ||
| annotaded_prefetch[model_field_name] = Count(serializer_field.relation) |
There was a problem hiding this comment.
Do we need to continue processing at this point? I think we can just do a continue here?
netbox/utilities/api.py
Outdated
| # If the serializer field does not map to a discrete model field, skip it. | ||
| try: | ||
| field = model._meta.get_field(model_field_name) | ||
| if isinstance(field, (RelatedField, ManyToOneRel, GenericForeignKey)): | ||
| if (isinstance(field, (RelatedField, ManyToOneRel, GenericForeignKey)) and |
There was a problem hiding this comment.
This can be moved below the try/except - if the field doesn't exist we are doing a continue so it won't get hit and field will be set.
Also - if it is issubclass(type(serializer_field), Serializer) do we want to continue to line 117? Not sure - just asking, could we just do a check on that condition and continue?
|
@renatoalmeidaoliveira is this ready for re-review? If you do some fixes please click the re-review button as otherwise it doesn't pop up in our queues. |
|
@arthanson it's not ready yet... I tryed to use a different approach based on your comments and @jeremystretch's but it breaks a lot of things for other models, gonna post some code snippet about that sollution |
netbox/utilities/api.py
Outdated
| except FieldDoesNotExist: | ||
| continue | ||
|
|
||
| # Prefetch for tags |
There was a problem hiding this comment.
This code is close but I think this is way too specific checking for a field named 'tags'. There shouldn't be anything special about tags - we should be able to check generically for M2M or FK field here.
There was a problem hiding this comment.
If this is needed can you do something like:
if isinstance(field, models.ManyToManyField):
prefetch_fields.append(field.name)
elif serializer_field and source_field is None:
...
netbox/utilities/api.py
Outdated
| nested_annotations = get_annotations_for_serializer(serializer_class, fields_to_include=fields_to_include) | ||
| if nested_annotations: | ||
| related_prefetch = Prefetch(source_field, queryset=model.objects.all().annotate(**nested_annotations)) | ||
| prefetch_fields.append(related_prefetch) |
There was a problem hiding this comment.
Almost there - I think you can just add a return prefetch_fields here. If it's gotten to here then in the loop below on line 99 the only time it modifies prefetch_fields is on line 125 and that can only happen if source_field is None, which it wouldn't be in this case, so all that code is looping / processing for nothing.
If that is the case the fields_to_include doesn't matter in this case. I'm seeing when it is called with source_field you are only getting one value returned for prefetch_fieds (which makes sense as the for loop doesn't append anything) Therefore it doesn't look like you need to recurse into this at all, the lines above can just be a separate function that is called (without the fields_to_include).
Please let me know if that isn't clear.
|
@arthanson I created a method to deal with subfield annotations, and remove the recursion, I think in most cases it gonna work well because the brief fields ussually doesn't contains serializer fields, the only exception I found was Current Implementation:
New implementation:
But even with that difference with the prefetch field the amount of queries in the list endpoint was only by one query in my environment with demo data |
There was a problem hiding this comment.
I didn't mean take out the recursion, as you noted that removes many of the prefetch's that are needed. At line 19 below I've added a return after the append as the code below it has no effect (since in this case source_field is not None
What I wasn't sure on was if the line get_prefetches_for_serializer(type(serializer_field), subfields, field_name) could be simplified in the case where it is passing in source_field, but looking at it more this might be the easiest way. So I'd suggest reverting to the code I posted below.
For more clarification: You are recursing into a function but it has two completely separate logic paths if source_field is None or not. Wasn't sure if it would be cleaner to call two separate functions (do the recursion if source_field is None, but call another function if source_field isn't None that just does the code you have after if source_field is not None. Again, it might be simpler to just keep it all in one function like you have it.
def get_prefetches_for_serializer(serializer_class, fields_to_include=None, source_field=None):
"""
Compile and return a list of fields which should be prefetched on the queryset for a serializer.
"""
model = serializer_class.Meta.model
# If fields are not specified, default to all
if not fields_to_include:
fields_to_include = serializer_class.Meta.fields
prefetch_fields = []
# If this serializer is nested, get annotations and prefetches for the nested serializer
if source_field is not None:
nested_annotations = get_annotations_for_serializer(serializer_class, fields_to_include=fields_to_include)
if nested_annotations:
related_prefetch = Prefetch(source_field, queryset=model.objects.all().annotate(**nested_annotations))
prefetch_fields.append(related_prefetch)
return prefetch_fields
for field_name in fields_to_include:
serializer_field = serializer_class._declared_fields.get(field_name)
# Determine the name of the model field referenced by the serializer field
model_field_name = field_name
if serializer_field and serializer_field.source:
model_field_name = serializer_field.source
# If the serializer field does not map to a discrete model field, skip it.
try:
field = model._meta.get_field(model_field_name)
except FieldDoesNotExist:
continue
# If this field is represented by a nested serializer, recurse to resolve prefetches
# for the related object.
if serializer_field and source_field is None:
if issubclass(type(serializer_field), Serializer):
# Determine which fields to prefetch for the nested object
subfields = serializer_field.Meta.brief_fields if serializer_field.nested else None
for subfield in get_prefetches_for_serializer(type(serializer_field), subfields, field_name):
if isinstance(subfield, Prefetch):
prefetch_fields.append(subfield)
else:
prefetch_fields.append(f'{field_name}__{subfield}')
elif isinstance(field, (RelatedField, ManyToOneRel, GenericForeignKey)):
prefetch_fields.append(field.name)
return prefetch_fields
|
Closing this issue since I didn't find a good solution to keep the recursion and allow Prefetches with annotations. |
Fixes: #17976 DeviceType_Count on Manufacturer
get_prefetches_for_serializerto support nested serializersRelatedObjectCountFieldfields and create aPrefetchwith the correct annotation