Skip to content

Fixes: #17976 DeviceType_Count on Manufacturer #18583

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Fixes: #17976 DeviceType_Count on Manufacturer #18583

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
00073af
Change get_prefetches_for_serializer to annotate nested serializers
renatoalmeidaoliveira Feb 6, 2025
5ea85b4
fix typo
renatoalmeidaoliveira Feb 25, 2025
53e8da6
refactor get_prefetches_for_serializer
renatoalmeidaoliveira Feb 26, 2025
603253d
Removed prefetch for model fields, since its a Serializer and add for…
renatoalmeidaoliveira Feb 27, 2025
68d9c39
Generalize tags use case
renatoalmeidaoliveira Mar 7, 2025
603c269
Add get_annotations_for_subfields and remove recursion
renatoalmeidaoliveira Mar 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions netbox/utilities/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
from django.contrib.contenttypes.fields import GenericForeignKey
from django.core.exceptions import (
FieldDoesNotExist, FieldError, MultipleObjectsReturned, ObjectDoesNotExist, ValidationError,
)
from django.db.models.fields.related import ManyToOneRel, RelatedField
from django.db.models import Prefetch
from django.urls import reverse
from django.utils.module_loading import import_string
from django.utils.translation import gettext_lazy as _
Expand Down Expand Up @@ -76,7 +75,7 @@ def get_view_name(view):
return drf_get_view_name(view)


def get_prefetches_for_serializer(serializer_class, fields_to_include=None):
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.
"""
Expand All @@ -87,6 +86,14 @@ def get_prefetches_for_serializer(serializer_class, fields_to_include=None):
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for field_name in fields_to_include:
serializer_field = serializer_class._declared_fields.get(field_name)

Expand All @@ -98,20 +105,24 @@ def get_prefetches_for_serializer(serializer_class, fields_to_include=None):
# 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)):
prefetch_fields.append(field.name)
except FieldDoesNotExist:
continue

# Prefetch for tags
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
   ...

if field.name == 'tags':
prefetch_fields.append('tags')

# If this field is represented by a nested serializer, recurse to resolve prefetches
# for the related object.
if serializer_field:
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):
prefetch_fields.append(f'{field_name}__{subfield}')

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}')
return prefetch_fields


Expand Down