From d5f37d7a87eb3d276daceff6daeecb231d1435b7 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 09:07:15 -0400 Subject: [PATCH] Use add/remove fields only when assignment count is 100+ --- netbox/circuits/forms/model_forms.py | 14 +++++- netbox/circuits/tests/test_views.py | 2 +- netbox/dcim/forms/model_forms.py | 14 +++++- netbox/dcim/tests/test_views.py | 2 +- netbox/ipam/forms/model_forms.py | 14 +++++- netbox/netbox/forms/model_forms.py | 43 ++++++++++++++----- netbox/netbox/views/generic/object_views.py | 11 ----- netbox/utilities/forms/rendering.py | 18 +++++--- netbox/utilities/templatetags/form_helpers.py | 17 +++++--- 9 files changed, 95 insertions(+), 40 deletions(-) diff --git a/netbox/circuits/forms/model_forms.py b/netbox/circuits/forms/model_forms.py index a84906ffb..dfc41f590 100644 --- a/netbox/circuits/forms/model_forms.py +++ b/netbox/circuits/forms/model_forms.py @@ -43,6 +43,11 @@ __all__ = ( class ProviderForm(PrimaryModelForm): slug = SlugField() + asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('ASNs'), + required=False + ) add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), label=_('Add ASNs'), @@ -66,11 +71,16 @@ class ProviderForm(PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('provider_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_asns') self.fields.pop('remove_asns') - self.fields['add_asns'].label = _('ASNs') + if self.instance.pk: + self.initial['asns'] = list(self.instance.asns.values_list('pk', flat=True)) class ProviderAccountForm(PrimaryModelForm): diff --git a/netbox/circuits/tests/test_views.py b/netbox/circuits/tests/test_views.py index 3a3e2a50c..6ced9a958 100644 --- a/netbox/circuits/tests/test_views.py +++ b/netbox/circuits/tests/test_views.py @@ -42,7 +42,7 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): cls.form_data = { 'name': 'Provider X', 'slug': 'provider-x', - 'add_asns': [asns[6].pk, asns[7].pk], + 'asns': [asns[6].pk, asns[7].pk], 'comments': 'Another provider', 'tags': [t.pk for t in tags], } diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index 1507bdfe3..ee428d241 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -137,6 +137,11 @@ class SiteForm(TenancyForm, PrimaryModelForm): required=False, quick_add=True ) + asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('ASNs'), + required=False + ) add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), label=_('Add ASNs'), @@ -185,11 +190,16 @@ class SiteForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('site_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_asns') self.fields.pop('remove_asns') - self.fields['add_asns'].label = _('ASNs') + if self.instance.pk: + self.initial['asns'] = list(self.instance.asns.values_list('pk', flat=True)) class LocationForm(TenancyForm, NestedGroupModelForm): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 3d7426013..1aac72875 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -160,7 +160,7 @@ class SiteTestCase(ViewTestCases.PrimaryObjectViewTestCase): 'group': groups[1].pk, 'tenant': None, 'facility': 'Facility X', - 'add_asns': [asns[6].pk, asns[7].pk], + 'asns': [asns[6].pk, asns[7].pk], 'time_zone': ZoneInfo('UTC'), 'description': 'Site description', 'physical_address': '742 Evergreen Terrace, Springfield, USA', diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index ff3cac503..2900a1ff3 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -152,6 +152,11 @@ class ASNForm(TenancyForm, PrimaryModelForm): label=_('RIR'), quick_add=True ) + sites = DynamicModelMultipleChoiceField( + queryset=Site.objects.all(), + label=_('Sites'), + required=False + ) add_sites = DynamicModelMultipleChoiceField( queryset=Site.objects.all(), label=_('Add sites'), @@ -179,11 +184,16 @@ class ASNForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.sites.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('sites') self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_sites') self.fields.pop('remove_sites') - self.fields['add_sites'].label = _('Sites') + if self.instance.pk: + self.initial['sites'] = list(self.instance.sites.values_list('pk', flat=True)) class RoleForm(OrganizationalModelForm): diff --git a/netbox/netbox/forms/model_forms.py b/netbox/netbox/forms/model_forms.py index 97215d55a..45948e174 100644 --- a/netbox/netbox/forms/model_forms.py +++ b/netbox/netbox/forms/model_forms.py @@ -2,6 +2,8 @@ import json from django import forms from django.contrib.contenttypes.models import ContentType +from django.db import models +from django.db.models.fields.related import ManyToManyRel from extras.choices import * from utilities.forms.fields import CommentField, SlugField @@ -71,26 +73,47 @@ class NetBoxModelForm( def _post_clean(self): """ Override BaseModelForm's _post_clean() to store many-to-many field values on the model instance. + Handles both forward and reverse M2M relationships, and supports both simple (single field) + and add/remove (dual field) modes. """ self.instance._m2m_values = {} - for field in self.instance._meta.local_many_to_many: - if field.name in self.cleaned_data: - # Standard M2M field (set-based) - self.instance._m2m_values[field.name] = list(self.cleaned_data[field.name]) - elif f'add_{field.name}' in self.cleaned_data or f'remove_{field.name}' in self.cleaned_data: - # Add/remove M2M field pair: compute the effective set - current = set(getattr(self.instance, field.name).values_list('pk', flat=True)) \ + for field in self.instance._meta.get_fields(): + # Determine the accessor name for this M2M relationship + if isinstance(field, models.ManyToManyField): + name = field.name + elif isinstance(field, ManyToManyRel): + name = field.get_accessor_name() + else: + continue + + if name in self.cleaned_data: + # Simple mode: single multi-select field + self.instance._m2m_values[name] = list(self.cleaned_data[name]) + elif f'add_{name}' in self.cleaned_data or f'remove_{name}' in self.cleaned_data: + # Add/remove mode: compute the effective set + current = set(getattr(self.instance, name).values_list('pk', flat=True)) \ if self.instance.pk else set() add_values = set( - v.pk for v in self.cleaned_data.get(f'add_{field.name}', []) + v.pk for v in self.cleaned_data.get(f'add_{name}', []) ) remove_values = set( - v.pk for v in self.cleaned_data.get(f'remove_{field.name}', []) + v.pk for v in self.cleaned_data.get(f'remove_{name}', []) ) - self.instance._m2m_values[field.name] = list((current | add_values) - remove_values) + self.instance._m2m_values[name] = list((current | add_values) - remove_values) return super()._post_clean() + def _save_m2m(self): + """ + Save many-to-many field values that were computed in _post_clean(). This handles M2M fields + not included in Meta.fields (e.g. those managed via M2MAddRemoveFields). + """ + super()._save_m2m() + meta_fields = self._meta.fields + for field_name, values in self.instance._m2m_values.items(): + if not meta_fields or field_name not in meta_fields: + getattr(self.instance, field_name).set(values) + class PrimaryModelForm(OwnerMixin, NetBoxModelForm): """ diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 2aaa92830..c6630d341 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -299,17 +299,6 @@ class ObjectEditView(GetReturnURLMixin, BaseObjectView): object_created = form.instance.pk is None obj = form.save() - # Process any add/remove M2M field pairs - for field in obj._meta.local_many_to_many: - add_key = f'add_{field.name}' - remove_key = f'remove_{field.name}' - if add_key in form.cleaned_data or remove_key in form.cleaned_data: - m2m_manager = getattr(obj, field.name) - if add_values := form.cleaned_data.get(add_key): - m2m_manager.add(*add_values) - if remove_values := form.cleaned_data.get(remove_key): - m2m_manager.remove(*remove_values) - # Check that the new object conforms with any assigned object-level permissions if not self.queryset.filter(pk=obj.pk).exists(): raise PermissionsViolation() diff --git a/netbox/utilities/forms/rendering.py b/netbox/utilities/forms/rendering.py index 17235e107..c7bb324d8 100644 --- a/netbox/utilities/forms/rendering.py +++ b/netbox/utilities/forms/rendering.py @@ -76,15 +76,21 @@ class TabbedGroups: class M2MAddRemoveFields: """ - Represents an add/remove field pair for a many-to-many relationship. Rather than rendering - a single multi-select pre-populated with all current values (which can crash the browser for - large datasets), this renders two fields: one for adding new relations and one for removing - existing relations. + Represents a many-to-many relationship field on a form. It supports two rendering modes: + + 1. Simple mode: A single multi-select field pre-populated with current values. This is used + for new objects or existing objects with fewer than THRESHOLD current assignments. + 2. Add/remove mode: Two separate fields for adding and removing relations. This avoids + crashing the browser when an object has a very large number of current assignments. + + The form must define three fields: '{name}', 'add_{name}', and 'remove_{name}'. The form's + __init__() method determines the mode and removes the unused fields. Parameters: - name: The name of the M2M field on the model (e.g. 'asns'). The form must define - corresponding 'add_{name}' and 'remove_{name}' fields. + name: The name of the M2M field on the model (e.g. 'asns'). """ + THRESHOLD = 100 + def __init__(self, name): self.name = name diff --git a/netbox/utilities/templatetags/form_helpers.py b/netbox/utilities/templatetags/form_helpers.py index 974fcc633..156bd72c2 100644 --- a/netbox/utilities/templatetags/form_helpers.py +++ b/netbox/utilities/templatetags/form_helpers.py @@ -81,11 +81,18 @@ def render_fieldset(form, fieldset): ) elif type(item) is M2MAddRemoveFields: - for field_name in (f'add_{item.name}', f'remove_{item.name}'): - if field_name in form.fields: - rows.append( - ('field', None, [form[field_name]]) - ) + if item.name in form.fields: + # Simple mode: render a single multi-select field + rows.append( + ('field', None, [form[item.name]]) + ) + else: + # Add/remove mode: render separate add and remove fields + for field_name in (f'add_{item.name}', f'remove_{item.name}'): + if field_name in form.fields: + rows.append( + ('field', None, [form[field_name]]) + ) elif type(item) is ObjectAttribute: value = getattr(form.instance, item.name)