From 3ba18633de14522e48dd9ac50c27ad308a18bb7c Mon Sep 17 00:00:00 2001 From: John Anderson Date: Tue, 20 Oct 2020 01:07:22 -0400 Subject: [PATCH 1/8] initial work on config context performance improvements --- netbox/dcim/api/views.py | 13 ++++++++- netbox/dcim/models/devices.py | 3 +- netbox/extras/models/models.py | 7 +++-- netbox/extras/querysets.py | 51 +++++++++++++++++++++++++++++++++- 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index f5b37021d0d..1967b5a3ec3 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -340,7 +340,18 @@ class DeviceViewSet(CustomFieldModelViewSet): queryset = Device.objects.prefetch_related( 'device_type__manufacturer', 'device_role', 'tenant', 'platform', 'site', 'rack', 'parent_bay', 'virtual_chassis__master', 'primary_ip4__nat_outside', 'primary_ip6__nat_outside', 'tags', - ) + ).add_config_context_annotation() + + #queryset = Device.objects.annotate( + # config_contexts=Subquery( + # ConfigContext.objects.filter( + # Q(sites=OuterRef('site')) | Q(sites=None) + # ).annotate( + # _data=EmptyGroupByJSONBAgg('data') + # ).values("_data") + # ) + #) + filterset_class = filters.DeviceFilterSet def get_serializer_class(self): diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index e96becadf9b..413ba73c08e 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -15,6 +15,7 @@ from dcim.choices import * from dcim.constants import * from extras.models import ChangeLoggedModel, ConfigContextModel, CustomFieldModel, TaggedItem +from extras.querysets import ConfigContextQuerySetMixin from extras.utils import extras_features from utilities.choices import ColorChoices from utilities.fields import ColorField, NaturalOrderingField @@ -594,7 +595,7 @@ class Device(ChangeLoggedModel, ConfigContextModel, CustomFieldModel): ) tags = TaggableManager(through=TaggedItem) - objects = RestrictedQuerySet.as_manager() + objects = ConfigContextQuerySetMixin.as_manager() csv_headers = [ 'name', 'device_role', 'tenant', 'manufacturer', 'device_type', 'platform', 'serial', 'asset_tag', 'status', diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index e57caf091aa..e5577b5a965 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -542,8 +542,11 @@ def get_config_context(self): # Compile all config data, overwriting lower-weight values with higher-weight values where a collision occurs data = OrderedDict() - for context in ConfigContext.objects.get_for_object(self): - data = deepmerge(data, context.data) + #for context in ConfigContext.objects.get_for_object(self): + # data = deepmerge(data, context.data) + + for context in self.config_contexts: + data = deepmerge(data, context) # If the object has local config context data defined, merge it last if self.local_context_data: diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index 9d9b55778dc..c8b15f5a23f 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,6 +1,7 @@ from collections import OrderedDict -from django.db.models import Q, QuerySet +from django.contrib.postgres.aggregates import JSONBAgg +from django.db.models import OuterRef, Subquery, Q, QuerySet from utilities.querysets import RestrictedQuerySet @@ -57,3 +58,51 @@ def get_for_object(self, obj): Q(tags__slug__in=obj.tags.slugs()) | Q(tags=None), is_active=True, ).order_by('weight', 'name') + + +class EmptyGroupByJSONBAgg(JSONBAgg): + contains_aggregate = False + + +class ConfigContextQuerySetMixin(RestrictedQuerySet): + + def add_config_context_annotation(self): + from extras.models import ConfigContext + return self.annotate( + config_contexts=Subquery( + ConfigContext.objects.filter( + self._add_config_context_filters() + ).order_by( + 'weight', + 'name' + ).annotate( + _data=EmptyGroupByJSONBAgg('data') + ).values("_data") + ) + ) + + def _add_config_context_filters(self): + + + if self.model._meta.model_name == 'device': + return Q( + Q(sites=OuterRef('site')) | Q(sites=None), + Q(roles=OuterRef('device_role')) | Q(roles=None), + Q(platforms=OuterRef('platform')) | Q(platforms=None), + Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), + Q(tenants=OuterRef('tenant')) | Q(tenants=None), + Q(tags=OuterRef('tags')) | Q(tags=None), + is_active=True, + ) + else: + return Q( + Q(sites=OuterRef('site')) | Q(sites=None), + Q(roles=OuterRef('role')) | Q(roles=None), + Q(platforms=OuterRef('platform')) | Q(platforms=None), + Q(cluster_groups=OuterRef('cluster__group')) | Q(cluster_groups=None), + Q(clusters=OuterRef('cluster')) | Q(clusters=None), + Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), + Q(tenants=OuterRef('tenant')) | Q(tenants=None), + Q(tags=OuterRef('tags')) | Q(tags=None), + is_active=True, + ) From 22d2289ed2b809f9e676d8966fa99aa6268b150e Mon Sep 17 00:00:00 2001 From: John Anderson Date: Fri, 23 Oct 2020 01:18:04 -0400 Subject: [PATCH 2/8] add support for regions and vms --- netbox/dcim/api/views.py | 28 +++++++----- netbox/dcim/models/devices.py | 4 +- netbox/dcim/views.py | 2 +- netbox/extras/models/models.py | 4 +- netbox/extras/querysets.py | 68 +++++++++++++++++------------ netbox/utilities/query_functions.py | 10 +++++ netbox/virtualization/api/views.py | 15 +++++++ netbox/virtualization/models.py | 3 +- netbox/virtualization/views.py | 2 +- 9 files changed, 87 insertions(+), 49 deletions(-) diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index 1967b5a3ec3..956b0cbffaa 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -340,20 +340,24 @@ class DeviceViewSet(CustomFieldModelViewSet): queryset = Device.objects.prefetch_related( 'device_type__manufacturer', 'device_role', 'tenant', 'platform', 'site', 'rack', 'parent_bay', 'virtual_chassis__master', 'primary_ip4__nat_outside', 'primary_ip6__nat_outside', 'tags', - ).add_config_context_annotation() - - #queryset = Device.objects.annotate( - # config_contexts=Subquery( - # ConfigContext.objects.filter( - # Q(sites=OuterRef('site')) | Q(sites=None) - # ).annotate( - # _data=EmptyGroupByJSONBAgg('data') - # ).values("_data") - # ) - #) - + ) filterset_class = filters.DeviceFilterSet + def get_queryset(self): + """ + Build the proper queryset based on the request context + + If the `brief` query param equates to True or the `exclude` query param + includes `config_context` as a value, return the base queryset. + + Else, return the queryset annotated with config context data + """ + + request = self.get_serializer_context()['request'] + if request.query_params.get('brief') or 'config_context' in request.query_params.get('exclude', []): + return self.queryset + return self.queryset.annotate_config_context_data() + def get_serializer_class(self): """ Select the specific serializer based on the request context. diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index 413ba73c08e..6fe2cea2f8d 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -15,7 +15,7 @@ from dcim.choices import * from dcim.constants import * from extras.models import ChangeLoggedModel, ConfigContextModel, CustomFieldModel, TaggedItem -from extras.querysets import ConfigContextQuerySetMixin +from extras.querysets import ConfigContextModelQuerySet from extras.utils import extras_features from utilities.choices import ColorChoices from utilities.fields import ColorField, NaturalOrderingField @@ -595,7 +595,7 @@ class Device(ChangeLoggedModel, ConfigContextModel, CustomFieldModel): ) tags = TaggableManager(through=TaggedItem) - objects = ConfigContextQuerySetMixin.as_manager() + objects = ConfigContextModelQuerySet.as_manager() csv_headers = [ 'name', 'device_role', 'tenant', 'manufacturer', 'device_type', 'platform', 'serial', 'asset_tag', 'status', diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index c6c0ced97ca..886f3e70255 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1163,7 +1163,7 @@ def get(self, request, pk): class DeviceConfigContextView(ObjectConfigContextView): - queryset = Device.objects.all() + queryset = Device.objects.annotate_config_context_data() base_template = 'dcim/device.html' diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index e5577b5a965..efb165a0662 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -542,10 +542,8 @@ def get_config_context(self): # Compile all config data, overwriting lower-weight values with higher-weight values where a collision occurs data = OrderedDict() - #for context in ConfigContext.objects.get_for_object(self): - # data = deepmerge(data, context.data) - for context in self.config_contexts: + for context in self.config_context_data: data = deepmerge(data, context) # If the object has local config context data defined, merge it last diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index c8b15f5a23f..dea03bccd1f 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,8 +1,8 @@ from collections import OrderedDict -from django.contrib.postgres.aggregates import JSONBAgg from django.db.models import OuterRef, Subquery, Q, QuerySet +from utilities.query_functions import EmptyGroupByJSONBAgg from utilities.querysets import RestrictedQuerySet @@ -60,18 +60,14 @@ def get_for_object(self, obj): ).order_by('weight', 'name') -class EmptyGroupByJSONBAgg(JSONBAgg): - contains_aggregate = False +class ConfigContextModelQuerySet(RestrictedQuerySet): - -class ConfigContextQuerySetMixin(RestrictedQuerySet): - - def add_config_context_annotation(self): + def annotate_config_context_data(self): from extras.models import ConfigContext return self.annotate( - config_contexts=Subquery( + config_context_data=Subquery( ConfigContext.objects.filter( - self._add_config_context_filters() + self._get_config_context_filters() ).order_by( 'weight', 'name' @@ -81,28 +77,42 @@ def add_config_context_annotation(self): ) ) - def _add_config_context_filters(self): + def _get_config_context_filters(self): + base_query = Q( + Q(platforms=OuterRef('platform')) | Q(platforms=None), + Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), + Q(tenants=OuterRef('tenant')) | Q(tenants=None), + Q(tags=OuterRef('tags')) | Q(tags=None), + is_active=True, + ) if self.model._meta.model_name == 'device': - return Q( - Q(sites=OuterRef('site')) | Q(sites=None), - Q(roles=OuterRef('device_role')) | Q(roles=None), - Q(platforms=OuterRef('platform')) | Q(platforms=None), - Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), - Q(tenants=OuterRef('tenant')) | Q(tenants=None), - Q(tags=OuterRef('tags')) | Q(tags=None), - is_active=True, + base_query.add((Q(roles=OuterRef('device_role')) | Q(roles=None)), Q.AND) + base_query.add( + (Q( + regions__tree_id=OuterRef('site__region__tree_id'), + regions__level__lte=OuterRef('site__region__level'), + regions__lft__lte=OuterRef('site__region__lft'), + regions__rght__gte=OuterRef('site__region__rght'), + ) | Q(regions=None)), + Q.AND ) - else: - return Q( - Q(sites=OuterRef('site')) | Q(sites=None), - Q(roles=OuterRef('role')) | Q(roles=None), - Q(platforms=OuterRef('platform')) | Q(platforms=None), - Q(cluster_groups=OuterRef('cluster__group')) | Q(cluster_groups=None), - Q(clusters=OuterRef('cluster')) | Q(clusters=None), - Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), - Q(tenants=OuterRef('tenant')) | Q(tenants=None), - Q(tags=OuterRef('tags')) | Q(tags=None), - is_active=True, + base_query.add((Q(sites=OuterRef('site')) | Q(sites=None)), Q.AND) + + elif self.model._meta.model_name == 'virtualmachine': + base_query.add((Q(roles=OuterRef('role')) | Q(roles=None)), Q.AND) + base_query.add((Q(cluster_groups=OuterRef('cluster__group')) | Q(cluster_groups=None)), Q.AND) + base_query.add((Q(clusters=OuterRef('cluster')) | Q(clusters=None)), Q.AND) + base_query.add( + (Q( + regions__tree_id=OuterRef('cluster__site__region__tree_id'), + regions__level__lte=OuterRef('cluster__site__region__level'), + regions__lft__lte=OuterRef('cluster__site__region__lft'), + regions__rght__gte=OuterRef('cluster__site__region__rght'), + ) | Q(regions=None)), + Q.AND ) + base_query.add((Q(sites=OuterRef('cluster__site')) | Q(sites=None)), Q.AND) + + return base_query diff --git a/netbox/utilities/query_functions.py b/netbox/utilities/query_functions.py index ee4310ea7f6..8ad7ceeade9 100644 --- a/netbox/utilities/query_functions.py +++ b/netbox/utilities/query_functions.py @@ -1,3 +1,4 @@ +from django.contrib.postgres.aggregates import JSONBAgg from django.db.models import F, Func @@ -7,3 +8,12 @@ class CollateAsChar(Func): """ function = 'C' template = '(%(expressions)s) COLLATE "%(function)s"' + + +class EmptyGroupByJSONBAgg(JSONBAgg): + """ + JSONBAgg is a builtin aggregation function which means it includes the use of a GROUP BY clause. + When used as an annotation for collecting config context data objects, the GROUP BY is + incorrect. This subclass overrides the Django ORM aggregation control to remove the GROUP BY. + """ + contains_aggregate = False diff --git a/netbox/virtualization/api/views.py b/netbox/virtualization/api/views.py index 1bf41c2b798..d19f0f9fa41 100644 --- a/netbox/virtualization/api/views.py +++ b/netbox/virtualization/api/views.py @@ -64,6 +64,21 @@ class VirtualMachineViewSet(CustomFieldModelViewSet): ) filterset_class = filters.VirtualMachineFilterSet + def get_queryset(self): + """ + Build the proper queryset based on the request context + + If the `brief` query param equates to True or the `exclude` query param + includes `config_context` as a value, return the base queryset. + + Else, return the queryset annotated with config context data + """ + + request = self.get_serializer_context()['request'] + if request.query_params.get('brief') or 'config_context' in request.query_params.get('exclude', []): + return self.queryset + return self.queryset.annotate_config_context_data() + def get_serializer_class(self): """ Select the specific serializer based on the request context. diff --git a/netbox/virtualization/models.py b/netbox/virtualization/models.py index 7d0b9987216..39f579e3091 100644 --- a/netbox/virtualization/models.py +++ b/netbox/virtualization/models.py @@ -8,6 +8,7 @@ from dcim.choices import InterfaceModeChoices from dcim.models import BaseInterface, Device from extras.models import ChangeLoggedModel, ConfigContextModel, CustomFieldModel, ObjectChange, TaggedItem +from extras.querysets import ConfigContextModelQuerySet from extras.utils import extras_features from utilities.fields import NaturalOrderingField from utilities.ordering import naturalize_interface @@ -282,7 +283,7 @@ class VirtualMachine(ChangeLoggedModel, ConfigContextModel, CustomFieldModel): ) tags = TaggableManager(through=TaggedItem) - objects = RestrictedQuerySet.as_manager() + objects = ConfigContextModelQuerySet.as_manager() csv_headers = [ 'name', 'status', 'role', 'cluster', 'tenant', 'platform', 'vcpus', 'memory', 'disk', 'comments', diff --git a/netbox/virtualization/views.py b/netbox/virtualization/views.py index a06a2e5ff3e..4cabfedeb1b 100644 --- a/netbox/virtualization/views.py +++ b/netbox/virtualization/views.py @@ -261,7 +261,7 @@ def get(self, request, pk): class VirtualMachineConfigContextView(ObjectConfigContextView): - queryset = VirtualMachine.objects.all() + queryset = VirtualMachine.objects.annotate_config_context_data() base_template = 'virtualization/virtualmachine.html' From 82f5d0070e2d97672b7b27937da094b5c90fa811 Mon Sep 17 00:00:00 2001 From: John Anderson Date: Fri, 23 Oct 2020 10:56:02 -0400 Subject: [PATCH 3/8] account for null value annotations --- netbox/extras/models/models.py | 9 ++++++++- netbox/extras/querysets.py | 28 +++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index efb165a0662..fe4531b51f5 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -543,7 +543,14 @@ def get_config_context(self): # Compile all config data, overwriting lower-weight values with higher-weight values where a collision occurs data = OrderedDict() - for context in self.config_context_data: + if not hasattr(self, 'config_context_data'): + # The annotation is not available, so we fall back to manually querying for the config context objects + config_context_data = ConfigContext.objects.get_for_object(self, aggregate_data=True) + else: + # The attribute may exist, but the annotated value could be None if there is no config context data + config_context_data = self.config_context_data or [] + + for context in config_context_data: data = deepmerge(data, context) # If the object has local config context data defined, merge it last diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index dea03bccd1f..c5b3de35bc2 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,5 +1,6 @@ from collections import OrderedDict +from django.contrib.postgres.aggregates import JSONBAgg from django.db.models import OuterRef, Subquery, Q, QuerySet from utilities.query_functions import EmptyGroupByJSONBAgg @@ -24,9 +25,12 @@ def __iter__(self): class ConfigContextQuerySet(RestrictedQuerySet): - def get_for_object(self, obj): + def get_for_object(self, obj, aggregate_data=False): """ Return all applicable ConfigContexts for a given object. Only active ConfigContexts will be included. + + Args: + aggregate_data: If True, use the JSONBAgg aggregate function to return only the list of JSON data objects """ # `device_role` for Device; `role` for VirtualMachine @@ -46,7 +50,7 @@ def get_for_object(self, obj): else: regions = [] - return self.filter( + queryset = self.filter( Q(regions__in=regions) | Q(regions=None), Q(sites=obj.site) | Q(sites=None), Q(roles=role) | Q(roles=None), @@ -59,10 +63,28 @@ def get_for_object(self, obj): is_active=True, ).order_by('weight', 'name') + if aggregate_data: + queryset = queryset.aggregate(config_context_data=JSONBAgg('data'))['config_context_data'] + + return queryset + class ConfigContextModelQuerySet(RestrictedQuerySet): + """ + QuerySet manager used by models which support ConfigContext (device and virtual machine). + + Includes a method which appends an annotation of aggregated config context JSON data objects. This is + implemented as a subquery which performs all the joins necessary to filter relevant config context objects. + This offers a substantial performance gain over ConfigContextQuerySet.get_for_object() when dealing with + multiple objects. + + This allows the annotation to be entirely optional. + """ def annotate_config_context_data(self): + """ + Attach the subquery annotation to the base queryset + """ from extras.models import ConfigContext return self.annotate( config_context_data=Subquery( @@ -78,7 +100,7 @@ def annotate_config_context_data(self): ) def _get_config_context_filters(self): - + # Construct the set of Q objects for the specific object types base_query = Q( Q(platforms=OuterRef('platform')) | Q(platforms=None), Q(tenant_groups=OuterRef('tenant__group')) | Q(tenant_groups=None), From 9e84e3b83bddc796f5031aa3888bcd4bab585291 Mon Sep 17 00:00:00 2001 From: John Anderson Date: Sun, 25 Oct 2020 16:49:18 -0400 Subject: [PATCH 4/8] added tests --- netbox/extras/querysets.py | 13 +- netbox/extras/tests/test_models.py | 280 +++++++++++++++++++++++++++- netbox/utilities/query_functions.py | 12 +- 3 files changed, 295 insertions(+), 10 deletions(-) diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index c5b3de35bc2..b48d82287bd 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,9 +1,9 @@ from collections import OrderedDict from django.contrib.postgres.aggregates import JSONBAgg -from django.db.models import OuterRef, Subquery, Q, QuerySet +from django.db.models import OuterRef, Subquery, Q, QuerySet, OrderBy, F, ExpressionWrapper -from utilities.query_functions import EmptyGroupByJSONBAgg +from utilities.query_functions import EmptyGroupByJSONBAgg, OrderableJSONBAgg from utilities.querysets import RestrictedQuerySet @@ -64,7 +64,9 @@ def get_for_object(self, obj, aggregate_data=False): ).order_by('weight', 'name') if aggregate_data: - queryset = queryset.aggregate(config_context_data=JSONBAgg('data'))['config_context_data'] + return queryset.aggregate( + config_context_data=OrderableJSONBAgg('data', ordering=['weight', 'name']) + )['config_context_data'] return queryset @@ -90,11 +92,8 @@ def annotate_config_context_data(self): config_context_data=Subquery( ConfigContext.objects.filter( self._get_config_context_filters() - ).order_by( - 'weight', - 'name' ).annotate( - _data=EmptyGroupByJSONBAgg('data') + _data=EmptyGroupByJSONBAgg('data', ordering=['weight', 'name']) ).values("_data") ) ) diff --git a/netbox/extras/tests/test_models.py b/netbox/extras/tests/test_models.py index 22e6e1f8fed..b0c5fb4025d 100644 --- a/netbox/extras/tests/test_models.py +++ b/netbox/extras/tests/test_models.py @@ -1,9 +1,11 @@ from django.contrib.contenttypes.models import ContentType from django.test import TestCase -from dcim.models import Site +from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Platform, Site, Region from extras.choices import TemplateLanguageChoices -from extras.models import Graph, Tag +from extras.models import ConfigContext, Graph, Tag +from tenancy.models import Tenant, TenantGroup +from virtualization.models import Cluster, ClusterGroup, ClusterType, VirtualMachine class GraphTest(TestCase): @@ -53,3 +55,277 @@ def test_create_tag_unicode(self): tag.save() self.assertEqual(tag.slug, 'testing-unicode-台灣') + + +class ConfigContextTest(TestCase): + """ + These test cases deal with the weighting, ordering, and deep merge logic of config context data. + + It also ensures the various config context querysets are consistent. + """ + + def setUp(self): + + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + self.devicetype = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1') + self.devicerole = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') + self.region = Region.objects.create(name="Region") + self.site = Site.objects.create(name='Site-1', slug='site-1', region=self.region) + self.platform = Platform.objects.create(name="Platform") + self.tenantgroup = TenantGroup.objects.create(name="Tenant Group") + self.tenant = Tenant.objects.create(name="Tenant", group=self.tenantgroup) + self.tag = Tag.objects.create(name="Tag", slug="tag") + + self.device = Device.objects.create( + name='Device 1', + device_type=self.devicetype, + device_role=self.devicerole, + site=self.site + ) + + def test_higher_weight_wins(self): + + context1 = ConfigContext( + name="context 1", + weight=101, + data={ + "a": 123, + "b": 456, + "c": 777 + } + ) + context2 = ConfigContext( + name="context 2", + weight=100, + data={ + "a": 123, + "b": 456, + "c": 789 + } + ) + ConfigContext.objects.bulk_create([context1, context2]) + + expected_data = { + "a": 123, + "b": 456, + "c": 777 + } + self.assertEqual(self.device.get_config_context(), expected_data) + + def test_name_ordering_after_weight(self): + + context1 = ConfigContext( + name="context 1", + weight=100, + data={ + "a": 123, + "b": 456, + "c": 777 + } + ) + context2 = ConfigContext( + name="context 2", + weight=100, + data={ + "a": 123, + "b": 456, + "c": 789 + } + ) + ConfigContext.objects.bulk_create([context1, context2]) + + expected_data = { + "a": 123, + "b": 456, + "c": 789 + } + self.assertEqual(self.device.get_config_context(), expected_data) + + def test_annotation_same_as_get_for_object(self): + """ + This test incorperates features from all of the above tests cases to ensure + the annotate_config_context_data() and get_for_object() queryset methods are the same. + """ + context1 = ConfigContext( + name="context 1", + weight=101, + data={ + "a": 123, + "b": 456, + "c": 777 + } + ) + context2 = ConfigContext( + name="context 2", + weight=100, + data={ + "a": 123, + "b": 456, + "c": 789 + } + ) + context3 = ConfigContext( + name="context 3", + weight=99, + data={ + "d": 1 + } + ) + context4 = ConfigContext( + name="context 4", + weight=99, + data={ + "d": 2 + } + ) + ConfigContext.objects.bulk_create([context1, context2, context3, context4]) + + annotated_queryset = Device.objects.filter(name=self.device.name).annotate_config_context_data() + self.assertEqual(self.device.get_config_context(), annotated_queryset[0].get_config_context()) + + def test_annotation_same_as_get_for_object_device_relations(self): + + site_context = ConfigContext.objects.create( + name="site", + weight=100, + data={ + "site": 1 + } + ) + site_context.sites.add(self.site) + region_context = ConfigContext.objects.create( + name="region", + weight=100, + data={ + "region": 1 + } + ) + region_context.regions.add(self.region) + platform_context = ConfigContext.objects.create( + name="platform", + weight=100, + data={ + "platform": 1 + } + ) + platform_context.platforms.add(self.platform) + tenant_group_context = ConfigContext.objects.create( + name="tenant group", + weight=100, + data={ + "tenant_group": 1 + } + ) + tenant_group_context.tenant_groups.add(self.tenantgroup) + tenant_context = ConfigContext.objects.create( + name="tenant", + weight=100, + data={ + "tenant": 1 + } + ) + tenant_context.tenants.add(self.tenant) + tag_context = ConfigContext.objects.create( + name="tag", + weight=100, + data={ + "tag": 1 + } + ) + tag_context.tags.add(self.tag) + + device = Device.objects.create( + name="Device 2", + site=self.site, + tenant=self.tenant, + platform=self.platform, + device_role=self.devicerole, + device_type=self.devicetype + ) + device.tags.add(self.tag) + + annotated_queryset = Device.objects.filter(name=device.name).annotate_config_context_data() + self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context()) + + def test_annotation_same_as_get_for_object_virtualmachine_relations(self): + + site_context = ConfigContext.objects.create( + name="site", + weight=100, + data={ + "site": 1 + } + ) + site_context.sites.add(self.site) + region_context = ConfigContext.objects.create( + name="region", + weight=100, + data={ + "region": 1 + } + ) + region_context.regions.add(self.region) + platform_context = ConfigContext.objects.create( + name="platform", + weight=100, + data={ + "platform": 1 + } + ) + platform_context.platforms.add(self.platform) + tenant_group_context = ConfigContext.objects.create( + name="tenant group", + weight=100, + data={ + "tenant_group": 1 + } + ) + tenant_group_context.tenant_groups.add(self.tenantgroup) + tenant_context = ConfigContext.objects.create( + name="tenant", + weight=100, + data={ + "tenant": 1 + } + ) + tenant_context.tenants.add(self.tenant) + tag_context = ConfigContext.objects.create( + name="tag", + weight=100, + data={ + "tag": 1 + } + ) + tag_context.tags.add(self.tag) + cluster_group = ClusterGroup.objects.create(name="Cluster Group") + cluster_group_context = ConfigContext.objects.create( + name="cluster group", + weight=100, + data={ + "cluster_group": 1 + } + ) + cluster_group_context.cluster_groups.add(cluster_group) + cluster_type = ClusterType.objects.create(name="Cluster Type 1") + cluster = Cluster.objects.create(name="Cluster", group=cluster_group, type=cluster_type) + cluster_context = ConfigContext.objects.create( + name="cluster", + weight=100, + data={ + "cluster": 1 + } + ) + cluster_context.clusters.add(cluster) + + + virtual_machine = VirtualMachine.objects.create( + name="VM 1", + cluster=cluster, + tenant=self.tenant, + platform=self.platform, + role=self.devicerole + ) + virtual_machine.tags.add(self.tag) + + annotated_queryset = VirtualMachine.objects.filter(name=virtual_machine.name).annotate_config_context_data() + self.assertEqual(virtual_machine.get_config_context(), annotated_queryset[0].get_config_context()) \ No newline at end of file diff --git a/netbox/utilities/query_functions.py b/netbox/utilities/query_functions.py index 8ad7ceeade9..abdc61b6b39 100644 --- a/netbox/utilities/query_functions.py +++ b/netbox/utilities/query_functions.py @@ -1,4 +1,5 @@ from django.contrib.postgres.aggregates import JSONBAgg +from django.contrib.postgres.aggregates.mixins import OrderableAggMixin from django.db.models import F, Func @@ -10,10 +11,19 @@ class CollateAsChar(Func): template = '(%(expressions)s) COLLATE "%(function)s"' -class EmptyGroupByJSONBAgg(JSONBAgg): +class OrderableJSONBAgg(OrderableAggMixin, JSONBAgg): + """ + TODO in Django 3.2 ordering is supported natively on JSONBAgg so this is no longer needed. + """ + template = '%(function)s(%(distinct)s%(expressions)s %(ordering)s)' + + +class EmptyGroupByJSONBAgg(OrderableJSONBAgg): """ JSONBAgg is a builtin aggregation function which means it includes the use of a GROUP BY clause. When used as an annotation for collecting config context data objects, the GROUP BY is incorrect. This subclass overrides the Django ORM aggregation control to remove the GROUP BY. + + TODO in Django 3.2 ordering is supported natively on JSONBAgg so we only need to inherit from JSONBAgg. """ contains_aggregate = False From 26ff33c41a228db42f42d091c13d7159f039d56d Mon Sep 17 00:00:00 2001 From: John Anderson Date: Sun, 25 Oct 2020 17:56:42 -0400 Subject: [PATCH 5/8] pep8 --- netbox/extras/tests/test_models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/netbox/extras/tests/test_models.py b/netbox/extras/tests/test_models.py index b0c5fb4025d..280da75b674 100644 --- a/netbox/extras/tests/test_models.py +++ b/netbox/extras/tests/test_models.py @@ -317,7 +317,6 @@ def test_annotation_same_as_get_for_object_virtualmachine_relations(self): ) cluster_context.clusters.add(cluster) - virtual_machine = VirtualMachine.objects.create( name="VM 1", cluster=cluster, @@ -328,4 +327,4 @@ def test_annotation_same_as_get_for_object_virtualmachine_relations(self): virtual_machine.tags.add(self.tag) annotated_queryset = VirtualMachine.objects.filter(name=virtual_machine.name).annotate_config_context_data() - self.assertEqual(virtual_machine.get_config_context(), annotated_queryset[0].get_config_context()) \ No newline at end of file + self.assertEqual(virtual_machine.get_config_context(), annotated_queryset[0].get_config_context()) From 047f03a58c085b736f243e4a1d801dec5864dc7a Mon Sep 17 00:00:00 2001 From: John Anderson Date: Sun, 25 Oct 2020 19:00:56 -0400 Subject: [PATCH 6/8] clean up imports --- netbox/extras/querysets.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index b48d82287bd..5e7884a0d13 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,7 +1,6 @@ from collections import OrderedDict -from django.contrib.postgres.aggregates import JSONBAgg -from django.db.models import OuterRef, Subquery, Q, QuerySet, OrderBy, F, ExpressionWrapper +from django.db.models import OuterRef, Subquery, Q from utilities.query_functions import EmptyGroupByJSONBAgg, OrderableJSONBAgg from utilities.querysets import RestrictedQuerySet From db87a694884dca4eca214018b6ca1211ea3f1c76 Mon Sep 17 00:00:00 2001 From: John Anderson Date: Fri, 30 Oct 2020 02:56:26 -0400 Subject: [PATCH 7/8] convert region fields to f-string --- netbox/extras/querysets.py | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index 5e7884a0d13..9bfa5da8377 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -109,30 +109,24 @@ def _get_config_context_filters(self): if self.model._meta.model_name == 'device': base_query.add((Q(roles=OuterRef('device_role')) | Q(roles=None)), Q.AND) - base_query.add( - (Q( - regions__tree_id=OuterRef('site__region__tree_id'), - regions__level__lte=OuterRef('site__region__level'), - regions__lft__lte=OuterRef('site__region__lft'), - regions__rght__gte=OuterRef('site__region__rght'), - ) | Q(regions=None)), - Q.AND - ) base_query.add((Q(sites=OuterRef('site')) | Q(sites=None)), Q.AND) + region_field = 'site__region' elif self.model._meta.model_name == 'virtualmachine': base_query.add((Q(roles=OuterRef('role')) | Q(roles=None)), Q.AND) base_query.add((Q(cluster_groups=OuterRef('cluster__group')) | Q(cluster_groups=None)), Q.AND) base_query.add((Q(clusters=OuterRef('cluster')) | Q(clusters=None)), Q.AND) - base_query.add( - (Q( - regions__tree_id=OuterRef('cluster__site__region__tree_id'), - regions__level__lte=OuterRef('cluster__site__region__level'), - regions__lft__lte=OuterRef('cluster__site__region__lft'), - regions__rght__gte=OuterRef('cluster__site__region__rght'), - ) | Q(regions=None)), - Q.AND - ) base_query.add((Q(sites=OuterRef('cluster__site')) | Q(sites=None)), Q.AND) + region_field = 'cluster__site__region' + + base_query.add( + (Q( + regions__tree_id=OuterRef(f'{region_field}__tree_id'), + regions__level__lte=OuterRef(f'{region_field}__level'), + regions__lft__lte=OuterRef(f'{region_field}__lft'), + regions__rght__gte=OuterRef(f'{region_field}__rght'), + ) | Q(regions=None)), + Q.AND + ) return base_query From 28c17f33ab398f20f6c62644ccf9a3ac98cad5d1 Mon Sep 17 00:00:00 2001 From: John Anderson Date: Fri, 30 Oct 2020 02:56:43 -0400 Subject: [PATCH 8/8] move get_queryset() to common mixin --- netbox/dcim/api/views.py | 19 ++----------------- netbox/extras/api/views.py | 23 +++++++++++++++++++++++ netbox/virtualization/api/views.py | 19 ++----------------- 3 files changed, 27 insertions(+), 34 deletions(-) diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index 956b0cbffaa..427aecd5f5b 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -24,7 +24,7 @@ VirtualChassis, ) from extras.api.serializers import RenderedGraphSerializer -from extras.api.views import CustomFieldModelViewSet +from extras.api.views import ConfigContextQuerySetMixin, CustomFieldModelViewSet from extras.models import Graph from ipam.models import Prefix, VLAN from utilities.api import ( @@ -336,28 +336,13 @@ class PlatformViewSet(ModelViewSet): # Devices # -class DeviceViewSet(CustomFieldModelViewSet): +class DeviceViewSet(CustomFieldModelViewSet, ConfigContextQuerySetMixin): queryset = Device.objects.prefetch_related( 'device_type__manufacturer', 'device_role', 'tenant', 'platform', 'site', 'rack', 'parent_bay', 'virtual_chassis__master', 'primary_ip4__nat_outside', 'primary_ip6__nat_outside', 'tags', ) filterset_class = filters.DeviceFilterSet - def get_queryset(self): - """ - Build the proper queryset based on the request context - - If the `brief` query param equates to True or the `exclude` query param - includes `config_context` as a value, return the base queryset. - - Else, return the queryset annotated with config context data - """ - - request = self.get_serializer_context()['request'] - if request.query_params.get('brief') or 'config_context' in request.query_params.get('exclude', []): - return self.queryset - return self.queryset.annotate_config_context_data() - def get_serializer_class(self): """ Select the specific serializer based on the request context. diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index a63dbe44d12..74c9ea889fe 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -26,6 +26,29 @@ from . import serializers +class ConfigContextQuerySetMixin: + """ + Used by views that work with config context models (device and virtual machine). + Provides a get_queryset() method which deals with adding the config context + data annotation or not. + """ + + def get_queryset(self): + """ + Build the proper queryset based on the request context + + If the `brief` query param equates to True or the `exclude` query param + includes `config_context` as a value, return the base queryset. + + Else, return the queryset annotated with config context data + """ + + request = self.get_serializer_context()['request'] + if request.query_params.get('brief') or 'config_context' in request.query_params.get('exclude', []): + return self.queryset + return self.queryset.annotate_config_context_data() + + class ExtrasRootView(APIRootView): """ Extras API root view diff --git a/netbox/virtualization/api/views.py b/netbox/virtualization/api/views.py index d19f0f9fa41..55393b1108d 100644 --- a/netbox/virtualization/api/views.py +++ b/netbox/virtualization/api/views.py @@ -6,7 +6,7 @@ from dcim.models import Device from extras.api.serializers import RenderedGraphSerializer -from extras.api.views import CustomFieldModelViewSet +from extras.api.views import ConfigContextQuerySetMixin, CustomFieldModelViewSet from extras.models import Graph from utilities.api import ModelViewSet from utilities.utils import get_subquery @@ -58,27 +58,12 @@ class ClusterViewSet(CustomFieldModelViewSet): # Virtual machines # -class VirtualMachineViewSet(CustomFieldModelViewSet): +class VirtualMachineViewSet(CustomFieldModelViewSet, ConfigContextQuerySetMixin): queryset = VirtualMachine.objects.prefetch_related( 'cluster__site', 'role', 'tenant', 'platform', 'primary_ip4', 'primary_ip6', 'tags' ) filterset_class = filters.VirtualMachineFilterSet - def get_queryset(self): - """ - Build the proper queryset based on the request context - - If the `brief` query param equates to True or the `exclude` query param - includes `config_context` as a value, return the base queryset. - - Else, return the queryset annotated with config context data - """ - - request = self.get_serializer_context()['request'] - if request.query_params.get('brief') or 'config_context' in request.query_params.get('exclude', []): - return self.queryset - return self.queryset.annotate_config_context_data() - def get_serializer_class(self): """ Select the specific serializer based on the request context.