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