Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions netbox/extras/querysets.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def annotate_config_context_data(self):
_data=EmptyGroupByJSONBAgg('data', ordering=['weight', 'name'])
).values("_data").order_by()
)
).distinct()
)

def _get_config_context_filters(self):
# Construct the set of Q objects for the specific object types
Expand All @@ -117,7 +117,7 @@ def _get_config_context_filters(self):
).values_list(
'tag_id',
flat=True
)
).distinct()
)
) | Q(tags=None),
is_active=True,
Expand Down
94 changes: 69 additions & 25 deletions netbox/extras/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from core.models import DataSource, ObjectType
from dcim.models import Device, DeviceRole, DeviceType, Location, Manufacturer, Platform, Region, Site, SiteGroup
from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, ImageAttachment, Tag
from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, ImageAttachment, Tag, TaggedItem
from tenancy.models import Tenant, TenantGroup
from utilities.exceptions import AbortRequest
from virtualization.models import Cluster, ClusterGroup, ClusterType, VirtualMachine
Expand Down Expand Up @@ -536,6 +536,34 @@ def test_virtualmachine_site_context(self):
vms[1].get_config_context()
)

def test_valid_local_context_data(self):
device = Device.objects.first()
device.local_context_data = None
device.clean()

device.local_context_data = {"foo": "bar"}
device.clean()

def test_invalid_local_context_data(self):
device = Device.objects.first()

device.local_context_data = ""
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = 0
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = False
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = 'foo'
with self.assertRaises(ValidationError):
device.clean()

@tag('regression')
Copy link
Member Author

Choose a reason for hiding this comment

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

Just rearranging the test to keep all of the regression tests together at the end of the test class.

def test_multiple_tags_return_distinct_objects(self):
"""
Tagged items use a generic relationship, which results in duplicate rows being returned when queried.
Expand Down Expand Up @@ -573,6 +601,7 @@ def test_multiple_tags_return_distinct_objects(self):
self.assertEqual(ConfigContext.objects.get_for_object(device).count(), 1)
self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context())

@tag('regression')
def test_multiple_tags_return_distinct_objects_with_separate_config_contexts(self):
"""
Tagged items use a generic relationship, which results in duplicate rows being returned when queried.
Expand Down Expand Up @@ -621,32 +650,47 @@ def test_multiple_tags_return_distinct_objects_with_separate_config_contexts(sel
self.assertEqual(ConfigContext.objects.get_for_object(device).count(), 2)
self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context())

def test_valid_local_context_data(self):
device = Device.objects.first()
device.local_context_data = None
device.clean()

device.local_context_data = {"foo": "bar"}
device.clean()
@tag('performance', 'regression')
def test_config_context_annotation_query_optimization(self):
"""
Regression test for issue #20327: Ensure config context annotation
doesn't use expensive DISTINCT on main query.

def test_invalid_local_context_data(self):
Verifies that DISTINCT is only used in tag subquery where needed,
not on the main device query which is expensive for large datasets.
"""
device = Device.objects.first()

device.local_context_data = ""
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = 0
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = False
with self.assertRaises(ValidationError):
device.clean()

device.local_context_data = 'foo'
with self.assertRaises(ValidationError):
device.clean()
queryset = Device.objects.filter(pk=device.pk).annotate_config_context_data()

# Main device query should NOT use DISTINCT
self.assertFalse(queryset.query.distinct)

# Check that tag subqueries DO use DISTINCT by inspecting the annotation
config_annotation = queryset.query.annotations.get('config_context_data')
self.assertIsNotNone(config_annotation)

def find_tag_subqueries(where_node):
"""Find subqueries in WHERE clause that relate to tag filtering"""
subqueries = []

def traverse(node):
if hasattr(node, 'children'):
for child in node.children:
try:
if child.rhs.query.model is TaggedItem:
subqueries.append(child.rhs.query)
except AttributeError:
traverse(child)
traverse(where_node)
return subqueries

# Find subqueries in the WHERE clause that should have DISTINCT
tag_subqueries = find_tag_subqueries(config_annotation.query.where)
distinct_subqueries = [sq for sq in tag_subqueries if sq.distinct]

# Verify we found at least one DISTINCT subquery for tags
self.assertEqual(len(distinct_subqueries), 1)
self.assertTrue(distinct_subqueries[0].distinct)


class ConfigTemplateTest(TestCase):
Expand Down