Skip to content

Closes #18980: Optimize update of object data when adding/removing custom fields #18983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 24, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 31 additions & 16 deletions netbox/extras/models/customfields.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
from django.contrib.postgres.fields import ArrayField
from django.core.validators import RegexValidator, ValidationError
from django.db import models
from django.db.models import F, Func, Value
from django.db.models.expressions import RawSQL
from django.urls import reverse
from django.utils.html import escape
from django.utils.safestring import mark_safe
Expand Down Expand Up @@ -281,12 +283,20 @@ def populate_initial_data(self, content_types):
Populate initial custom field data upon either a) the creation of a new CustomField, or
b) the assignment of an existing CustomField to new object types.
"""
if self.default is None:
# We have to convert None to a JSON null for jsonb_set()
value = RawSQL("'null'::jsonb", [])
else:
value = Value(self.default, models.JSONField())
for ct in content_types:
model = ct.model_class()
instances = model.objects.exclude(**{'custom_field_data__contains': self.name})
for instance in instances:
instance.custom_field_data[self.name] = self.default
model.objects.bulk_update(instances, ['custom_field_data'], batch_size=100)
ct.model_class().objects.update(
custom_field_data=Func(
F('custom_field_data'),
Value([self.name]),
value,
function='jsonb_set'
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we weren't updating instances that already had a value for the self.name path in the custom field data field (I think) and only setting it on those that did not.

But now we're just flat out setting the default value on all records (either adding or updating, per jsonb_set docs), is that right? I think this is safe to do since this is only running as a result of adding new custom field. Is that the reasoning or is there something else I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the exclusion filter was left over from an earlier implementation of custom fields and is no longer needed. An argument could be made for retaining it, however it's cleaner IMO to overwrite any data that happens to exist (somehow) when adding a new custom field on the model, because that's what I'd expect to happen as a user.


def remove_stale_data(self, content_types):
"""
Expand All @@ -295,22 +305,27 @@ def remove_stale_data(self, content_types):
"""
for ct in content_types:
if model := ct.model_class():
instances = model.objects.filter(custom_field_data__has_key=self.name)
for instance in instances:
del instance.custom_field_data[self.name]
model.objects.bulk_update(instances, ['custom_field_data'], batch_size=100)
model.objects.update(
custom_field_data=F('custom_field_data') - self.name
)

def rename_object_data(self, old_name, new_name):
"""
Called when a CustomField has been renamed. Updates all assigned object data.
Called when a CustomField has been renamed. Removes the original key and inserts the new
one, copying the value of the old key.
"""
for ct in self.object_types.all():
model = ct.model_class()
params = {f'custom_field_data__{old_name}__isnull': False}
instances = model.objects.filter(**params)
for instance in instances:
instance.custom_field_data[new_name] = instance.custom_field_data.pop(old_name)
model.objects.bulk_update(instances, ['custom_field_data'], batch_size=100)
ct.model_class().objects.update(
custom_field_data=Func(
F('custom_field_data') - old_name,
Value([new_name]),
Func(
F('custom_field_data'),
function='jsonb_extract_path_text',
template=f"to_jsonb(%(expressions)s -> '{old_name}')"
),
function='jsonb_set')
)

def clean(self):
super().clean()
Expand Down