From cdd1948721df69306d2a72fe8d25b5b4bbf67031 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 f5b37021d..1967b5a3e 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 e96becadf..413ba73c0 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -15,6 +15,7 @@ from taggit.managers import TaggableManager 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 e57caf091..e5577b5a9 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -542,8 +542,11 @@ class ConfigContextModel(models.Model): # 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 9d9b55778..c8b15f5a2 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 @@ class ConfigContextQuerySet(RestrictedQuerySet): 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 152fe800c770be99f3e80d5461847b835e5c474e 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 1967b5a3e..956b0cbff 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 413ba73c0..6fe2cea2f 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -15,7 +15,7 @@ from taggit.managers import TaggableManager 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 c6c0ced97..886f3e702 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1163,7 +1163,7 @@ class DeviceConfigView(ObjectView): 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 e5577b5a9..efb165a06 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -542,10 +542,8 @@ class ConfigContextModel(models.Model): # 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 c8b15f5a2..dea03bccd 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 @@ class ConfigContextQuerySet(RestrictedQuerySet): ).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 @@ class ConfigContextQuerySetMixin(RestrictedQuerySet): ) ) - 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 ee4310ea7..8ad7ceead 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 1bf41c2b7..d19f0f9fa 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 7d0b99872..39f579e30 100644 --- a/netbox/virtualization/models.py +++ b/netbox/virtualization/models.py @@ -8,6 +8,7 @@ from taggit.managers import TaggableManager 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 a06a2e5ff..4cabfedeb 100644 --- a/netbox/virtualization/views.py +++ b/netbox/virtualization/views.py @@ -261,7 +261,7 @@ class VirtualMachineView(ObjectView): class VirtualMachineConfigContextView(ObjectConfigContextView): - queryset = VirtualMachine.objects.all() + queryset = VirtualMachine.objects.annotate_config_context_data() base_template = 'virtualization/virtualmachine.html' From f96b5c67a06c421f00c7399f39981f65f46874fc 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 efb165a06..fe4531b51 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -543,7 +543,14 @@ class ConfigContextModel(models.Model): # 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 dea03bccd..c5b3de35b 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 @@ class CustomFieldQueryset: 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 @@ class ConfigContextQuerySet(RestrictedQuerySet): 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 @@ class ConfigContextQuerySet(RestrictedQuerySet): 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 @@ class ConfigContextModelQuerySet(RestrictedQuerySet): ) 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 4f546f2dd21d3d518445c3b5ad7bd07e4508c34b 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 c5b3de35b..b48d82287 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 @@ class ConfigContextQuerySet(RestrictedQuerySet): ).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 @@ class ConfigContextModelQuerySet(RestrictedQuerySet): 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 22e6e1f8f..b0c5fb402 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 @@ class TagTest(TestCase): 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 8ad7ceead..abdc61b6b 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 56946254e482352e1a5c7b7568d0b4a2a9bbf0f9 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 b0c5fb402..280da75b6 100644 --- a/netbox/extras/tests/test_models.py +++ b/netbox/extras/tests/test_models.py @@ -317,7 +317,6 @@ class ConfigContextTest(TestCase): ) cluster_context.clusters.add(cluster) - virtual_machine = VirtualMachine.objects.create( name="VM 1", cluster=cluster, @@ -328,4 +327,4 @@ class ConfigContextTest(TestCase): 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 cde90ae8bd2e7b308184cb0eb8b93ac47a4f91ad 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 b48d82287..5e7884a0d 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 f3d5de6e563a4ed9f840355124c5bef80faf2954 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 5e7884a0d..9bfa5da83 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -109,30 +109,24 @@ class ConfigContextModelQuerySet(RestrictedQuerySet): 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 ffe01cb42b775873bde21a29f926cec54acc33c6 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 956b0cbff..427aecd5f 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -24,7 +24,7 @@ from dcim.models import ( 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 a63dbe44d..74c9ea889 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -26,6 +26,29 @@ from utilities.utils import copy_safe_request 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 d19f0f9fa..55393b110 100644 --- a/netbox/virtualization/api/views.py +++ b/netbox/virtualization/api/views.py @@ -6,7 +6,7 @@ from rest_framework.routers import APIRootView 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.