From 42e5282283484a20a0bde9ac860abb2579657b00 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Fri, 8 Jul 2022 11:44:28 -0400 Subject: [PATCH] Improve filtering cables by termination device/rack/site --- netbox/dcim/filtersets.py | 38 +++++---- netbox/dcim/forms/filtersets.py | 17 +++- .../migrations/0157_new_cabling_models.py | 4 + .../0158_populate_cable_terminations.py | 55 +++++++++---- .../dcim/migrations/0161_cabling_cleanup.py | 8 ++ netbox/dcim/models/cables.py | 78 +++++++++++++------ netbox/dcim/models/device_components.py | 7 ++ netbox/dcim/tests/test_filtersets.py | 41 ++++++---- 8 files changed, 177 insertions(+), 71 deletions(-) diff --git a/netbox/dcim/filtersets.py b/netbox/dcim/filtersets.py index e30ab4f04..f55b3301e 100644 --- a/netbox/dcim/filtersets.py +++ b/netbox/dcim/filtersets.py @@ -1537,27 +1537,35 @@ class CableFilterSet(TenancyFilterSet, NetBoxModelFilterSet): choices=ColorChoices ) device_id = MultiValueNumberFilter( - method='filter_device' + method='filter_by_termination' ) device = MultiValueCharFilter( - method='filter_device', + method='filter_by_termination', field_name='device__name' ) rack_id = MultiValueNumberFilter( - method='filter_device', - field_name='device__rack_id' + method='filter_by_termination', + field_name='rack_id' ) rack = MultiValueCharFilter( - method='filter_device', - field_name='device__rack__name' + method='filter_by_termination', + field_name='rack__name' + ) + location_id = MultiValueNumberFilter( + method='filter_by_termination', + field_name='location_id' + ) + location = MultiValueCharFilter( + method='filter_by_termination', + field_name='location__name' ) site_id = MultiValueNumberFilter( - method='filter_device', - field_name='device__site_id' + method='filter_by_termination', + field_name='site_id' ) site = MultiValueCharFilter( - method='filter_device', - field_name='device__site__slug' + method='filter_by_termination', + field_name='site__slug' ) class Meta: @@ -1569,12 +1577,10 @@ class CableFilterSet(TenancyFilterSet, NetBoxModelFilterSet): return queryset return queryset.filter(label__icontains=value) - def filter_device(self, queryset, name, value): - queryset = queryset.filter( - Q(**{f'_termination_a_{name}__in': value}) | - Q(**{f'_termination_b_{name}__in': value}) - ) - return queryset + def filter_by_termination(self, queryset, name, value): + # Filter by a related object cached on CableTermination. Note the underscore preceding the field name. + # Supported objects: device, rack, location, site + return queryset.filter(**{f'terminations___{name}__in': value}).distinct() class CableTerminationFilterSet(BaseFilterSet): diff --git a/netbox/dcim/forms/filtersets.py b/netbox/dcim/forms/filtersets.py index d9bc79fb5..bd64e02b4 100644 --- a/netbox/dcim/forms/filtersets.py +++ b/netbox/dcim/forms/filtersets.py @@ -730,7 +730,7 @@ class CableFilterForm(TenancyFilterForm, NetBoxModelFilterSetForm): model = Cable fieldsets = ( (None, ('q', 'tag')), - ('Location', ('site_id', 'rack_id', 'device_id')), + ('Location', ('site_id', 'location_id', 'rack_id', 'device_id')), ('Attributes', ('type', 'status', 'color', 'length', 'length_unit')), ('Tenant', ('tenant_group_id', 'tenant_id')), ) @@ -747,13 +747,23 @@ class CableFilterForm(TenancyFilterForm, NetBoxModelFilterSetForm): }, label=_('Site') ) + location_id = DynamicModelMultipleChoiceField( + queryset=Location.objects.all(), + required=False, + label=_('Location'), + null_option='None', + query_params={ + 'site_id': '$site_id' + } + ) rack_id = DynamicModelMultipleChoiceField( queryset=Rack.objects.all(), required=False, label=_('Rack'), null_option='None', query_params={ - 'site_id': '$site_id' + 'site_id': '$site_id', + 'location_id': '$location_id', } ) device_id = DynamicModelMultipleChoiceField( @@ -761,8 +771,9 @@ class CableFilterForm(TenancyFilterForm, NetBoxModelFilterSetForm): required=False, query_params={ 'site_id': '$site_id', - 'tenant_id': '$tenant_id', + 'location_id': '$location_id', 'rack_id': '$rack_id', + 'tenant_id': '$tenant_id', }, label=_('Device') ) diff --git a/netbox/dcim/migrations/0157_new_cabling_models.py b/netbox/dcim/migrations/0157_new_cabling_models.py index e7c55997c..a3a650086 100644 --- a/netbox/dcim/migrations/0157_new_cabling_models.py +++ b/netbox/dcim/migrations/0157_new_cabling_models.py @@ -20,6 +20,10 @@ class Migration(migrations.Migration): ('termination_id', models.PositiveBigIntegerField()), ('cable', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='terminations', to='dcim.cable')), ('termination_type', models.ForeignKey(limit_choices_to=models.Q(models.Q(models.Q(('app_label', 'circuits'), ('model__in', ('circuittermination',))), models.Q(('app_label', 'dcim'), ('model__in', ('consoleport', 'consoleserverport', 'frontport', 'interface', 'powerfeed', 'poweroutlet', 'powerport', 'rearport'))), _connector='OR')), on_delete=django.db.models.deletion.PROTECT, related_name='+', to='contenttypes.contenttype')), + ('_device', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='dcim.device')), + ('_rack', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='dcim.rack')), + ('_location', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='dcim.location')), + ('_site', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='dcim.site')), ], options={ 'ordering': ('cable', 'cable_end', 'pk'), diff --git a/netbox/dcim/migrations/0158_populate_cable_terminations.py b/netbox/dcim/migrations/0158_populate_cable_terminations.py index c35c39a40..82f8cd359 100644 --- a/netbox/dcim/migrations/0158_populate_cable_terminations.py +++ b/netbox/dcim/migrations/0158_populate_cable_terminations.py @@ -1,10 +1,37 @@ from django.db import migrations +def cache_related_objects(termination): + """ + Replicate caching logic from CableTermination.cache_related_objects() + """ + attrs = {} + + # Device components + if getattr(termination, 'device', None): + attrs['_device'] = termination.device + attrs['_rack'] = termination.device.rack + attrs['_location'] = termination.device.location + attrs['_site'] = termination.device.site + + # Power feeds + elif getattr(termination, 'rack', None): + attrs['_rack'] = termination.rack + attrs['_location'] = termination.rack.location + attrs['_site'] = termination.rack.site + + # Circuit terminations + elif getattr(termination, 'site', None): + attrs['_site'] = termination.site + + return attrs + + def populate_cable_terminations(apps, schema_editor): """ Replicate terminations from the Cable model into CableTermination instances. """ + ContentType = apps.get_model('contenttypes', 'ContentType') Cable = apps.get_model('dcim', 'Cable') CableTermination = apps.get_model('dcim', 'CableTermination') @@ -16,22 +43,20 @@ def populate_cable_terminations(apps, schema_editor): # Queue CableTerminations to be created cable_terminations = [] for i, cable in enumerate(cables, start=1): - cable_terminations.append( - CableTermination( + for cable_end in ('a', 'b'): + # We must manually instantiate the termination object, because GFK fields are not + # supported within migrations. + termination_ct = ContentType.objects.get(pk=cable[f'termination_{cable_end}_type']) + termination_model = apps.get_model(termination_ct.app_label, termination_ct.model) + termination = termination_model.objects.get(pk=cable[f'termination_{cable_end}_id']) + + cable_terminations.append(CableTermination( cable_id=cable['id'], - cable_end='A', - termination_type_id=cable['termination_a_type'], - termination_id=cable['termination_a_id'] - ) - ) - cable_terminations.append( - CableTermination( - cable_id=cable['id'], - cable_end='B', - termination_type_id=cable['termination_b_type'], - termination_id=cable['termination_b_id'] - ) - ) + cable_end=cable_end.upper(), + termination_type_id=cable[f'termination_{cable_end}_type'], + termination_id=cable[f'termination_{cable_end}_id'], + **cache_related_objects(termination) + )) # Bulk create the termination objects CableTermination.objects.bulk_create(cable_terminations, batch_size=100) diff --git a/netbox/dcim/migrations/0161_cabling_cleanup.py b/netbox/dcim/migrations/0161_cabling_cleanup.py index db2a9b73e..8a1b7a09e 100644 --- a/netbox/dcim/migrations/0161_cabling_cleanup.py +++ b/netbox/dcim/migrations/0161_cabling_cleanup.py @@ -34,6 +34,14 @@ class Migration(migrations.Migration): model_name='cable', name='termination_b_type', ), + migrations.RemoveField( + model_name='cable', + name='_termination_a_device', + ), + migrations.RemoveField( + model_name='cable', + name='_termination_b_device', + ), # Remove old fields from CablePath migrations.AlterUniqueTogether( diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index cf9f6064d..551521c26 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -19,7 +19,6 @@ from utilities.querysets import RestrictedQuerySet from utilities.utils import to_meters from wireless.models import WirelessLink from .device_components import FrontPort, RearPort -from .devices import Device __all__ = ( 'Cable', @@ -81,22 +80,6 @@ class Cable(NetBoxModel): blank=True, null=True ) - # Cache the associated device (where applicable) for the A and B terminations. This enables filtering of Cables by - # their associated Devices. - _termination_a_device = models.ForeignKey( - to=Device, - on_delete=models.CASCADE, - related_name='+', - blank=True, - null=True - ) - _termination_b_device = models.ForeignKey( - to=Device, - on_delete=models.CASCADE, - related_name='+', - blank=True, - null=True - ) class Meta: ordering = ('pk',) @@ -167,13 +150,6 @@ class Cable(NetBoxModel): else: self._abs_length = None - # TODO: Need to come with a proper solution for filtering by termination parent - # Store the parent Device for the A and B terminations (if applicable) to enable filtering - if hasattr(self, 'a_terminations'): - self._termination_a_device = getattr(self.a_terminations[0], 'device', None) - if hasattr(self, 'b_terminations'): - self._termination_b_device = getattr(self.b_terminations[0], 'device', None) - super().save(*args, **kwargs) # Update the private pk used in __str__ in case this is a new object (i.e. just got its pk) @@ -247,6 +223,32 @@ class CableTermination(models.Model): fk_field='termination_id' ) + # Cached associations to enable efficient filtering + _device = models.ForeignKey( + to='dcim.Device', + on_delete=models.CASCADE, + blank=True, + null=True + ) + _rack = models.ForeignKey( + to='dcim.Rack', + on_delete=models.CASCADE, + blank=True, + null=True + ) + _location = models.ForeignKey( + to='dcim.Location', + on_delete=models.CASCADE, + blank=True, + null=True + ) + _site = models.ForeignKey( + to='dcim.Site', + on_delete=models.CASCADE, + blank=True, + null=True + ) + objects = RestrictedQuerySet.as_manager() class Meta: @@ -277,6 +279,10 @@ class CableTermination(models.Model): }) def save(self, *args, **kwargs): + + # Cache objects associated with the terminating object (for filtering) + self.cache_related_objects() + super().save(*args, **kwargs) # Set the cable on the terminating object @@ -297,6 +303,30 @@ class CableTermination(models.Model): super().delete(*args, **kwargs) + def cache_related_objects(self): + """ + Cache objects related to the termination (e.g. device, rack, site) directly on the object to + enable efficient filtering. + """ + assert self.termination is not None + + # Device components + if getattr(self.termination, 'device', None): + self._device = self.termination.device + self._rack = self.termination.device.rack + self._location = self.termination.device.location + self._site = self.termination.device.site + + # Power feeds + elif getattr(self.termination, 'rack', None): + self._rack = self.termination.rack + self._location = self.termination.rack.location + self._site = self.termination.rack.site + + # Circuit terminations + elif getattr(self.termination, 'site', None): + self._site = self.termination.site + class CablePath(models.Model): """ diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index 203b4469e..82dacbff6 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -126,6 +126,13 @@ class CabledObjectModel(models.Model): help_text="Treat as if a cable is connected" ) + cable_terminations = GenericRelation( + to='dcim.CableTermination', + content_type_field='termination_type', + object_id_field='termination_id', + related_query_name='%(class)s', + ) + class Meta: abstract = True diff --git a/netbox/dcim/tests/test_filtersets.py b/netbox/dcim/tests/test_filtersets.py index 2d3b57d01..24c45dd7f 100644 --- a/netbox/dcim/tests/test_filtersets.py +++ b/netbox/dcim/tests/test_filtersets.py @@ -3663,6 +3663,21 @@ class CableTestCase(TestCase, ChangeLoggedFilterSetTests): ) Site.objects.bulk_create(sites) + locations = ( + Location(name='Location 1', site=sites[0], slug='location-1'), + Location(name='Location 2', site=sites[1], slug='location-1'), + Location(name='Location 3', site=sites[2], slug='location-1'), + ) + for location in locations: + location.save() + + racks = ( + Rack(name='Rack 1', site=sites[0], location=locations[0]), + Rack(name='Rack 2', site=sites[1], location=locations[1]), + Rack(name='Rack 3', site=sites[2], location=locations[2]), + ) + Rack.objects.bulk_create(racks) + tenants = ( Tenant(name='Tenant 1', slug='tenant-1'), Tenant(name='Tenant 2', slug='tenant-2'), @@ -3670,24 +3685,17 @@ class CableTestCase(TestCase, ChangeLoggedFilterSetTests): ) Tenant.objects.bulk_create(tenants) - racks = ( - Rack(name='Rack 1', site=sites[0]), - Rack(name='Rack 2', site=sites[1]), - Rack(name='Rack 3', site=sites[2]), - ) - Rack.objects.bulk_create(racks) - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Model 1', slug='model-1') device_role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') devices = ( - Device(name='Device 1', device_type=device_type, device_role=device_role, site=sites[0], rack=racks[0], position=1), - Device(name='Device 2', device_type=device_type, device_role=device_role, site=sites[0], rack=racks[0], position=2), - Device(name='Device 3', device_type=device_type, device_role=device_role, site=sites[1], rack=racks[1], position=1), - Device(name='Device 4', device_type=device_type, device_role=device_role, site=sites[1], rack=racks[1], position=2), - Device(name='Device 5', device_type=device_type, device_role=device_role, site=sites[2], rack=racks[2], position=1), - Device(name='Device 6', device_type=device_type, device_role=device_role, site=sites[2], rack=racks[2], position=2), + Device(name='Device 1', device_type=device_type, device_role=device_role, site=sites[0], rack=racks[0], location=locations[0], position=1), + Device(name='Device 2', device_type=device_type, device_role=device_role, site=sites[0], rack=racks[0], location=locations[0], position=2), + Device(name='Device 3', device_type=device_type, device_role=device_role, site=sites[1], rack=racks[1], location=locations[1], position=1), + Device(name='Device 4', device_type=device_type, device_role=device_role, site=sites[1], rack=racks[1], location=locations[1], position=2), + Device(name='Device 5', device_type=device_type, device_role=device_role, site=sites[2], rack=racks[2], location=locations[2], position=1), + Device(name='Device 6', device_type=device_type, device_role=device_role, site=sites[2], rack=racks[2], location=locations[2], position=2), ) Device.objects.bulk_create(devices) @@ -3759,6 +3767,13 @@ class CableTestCase(TestCase, ChangeLoggedFilterSetTests): params = {'rack': [racks[0].name, racks[1].name]} self.assertEqual(self.filterset(params, self.queryset).qs.count(), 6) + def test_location(self): + locations = Location.objects.all()[:2] + params = {'location_id': [locations[0].pk, locations[1].pk]} + self.assertEqual(self.filterset(params, self.queryset).qs.count(), 6) + params = {'location': [locations[0].name, locations[1].name]} + self.assertEqual(self.filterset(params, self.queryset).qs.count(), 6) + def test_site(self): site = Site.objects.all()[:2] params = {'site_id': [site[0].pk, site[1].pk]}