Skip to content
Open
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
80 changes: 79 additions & 1 deletion core/models/history_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import datetime as base_datetime
from dirtyfields import DirtyFieldsMixin
from django.core.exceptions import ValidationError
from django.db import models
from django.db import models, transaction
from django.db.models import F
from simple_history.models import HistoricalRecords
from core.utils import CachedManager, CachedModelMixin
Expand Down Expand Up @@ -217,6 +217,84 @@ def copy(self, exclude_fields=["id", "uuid"]):

return new_instance

@classmethod
def bulk_save(cls, data_list, user, batch_size=100):
Copy link
Member

Choose a reason for hiding this comment

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

can it be default at None and retrieve it from the get_current_user() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Models shouldn't be aware of current user as that's at the controller layer.

"""
Efficiently update or create multiple instances based on 'id' field.
All operations are atomic - either all succeed or all fail.

Args:
data_list: List of dicts with instance data (with or without 'id')
user: User performing the operation
batch_size: Number of records to process per batch

Returns:
dict with 'created' and 'updated' counts
"""
if not data_list:
return {'created': 0, 'updated': 0}

now = py_datetime.now()

ids_to_update = [d['id'] for d in data_list if d.get('id')]

existing = {obj.id: obj for obj in cls.objects.filter(id__in=ids_to_update, is_deleted=False)}

to_create = []
to_update = []

exclude_fields = {'id', 'uuid', 'date_created', 'user_created', 'date_updated',
'user_updated', 'version', 'is_deleted', 'date_valid_from',
'date_valid_to', 'replacement_uuid'}

for data in data_list:
record_id = data.get('id')

if record_id and record_id in existing:
instance = existing[record_id]
for field, value in data.items():
if field not in exclude_fields:
setattr(instance, field, value)
instance.user_updated = user
instance.date_updated = now
instance.version = F('version') + 1
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

Using F('version') + 1 with bulk_update will not work as expected. The F expression is meant for database-level updates in .update() calls, but bulk_update saves the current Python object values to the database. The F expression will be serialized as-is rather than being evaluated as SQL. This will either fail or set an incorrect value. The version increment logic at lines 288-292 attempts to compensate by re-fetching versions, but the initial assignment is still problematic. Set instance.version to the current value + 1 instead: instance.version = instance.version + 1.

Suggested change
instance.version = F('version') + 1
instance.version = instance.version + 1

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

@weilu can you double check that ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the use of F('version') itself is problematic as proven by passing unit test in openimis/openimis-be-social_protection_py#119, but we can switch to instance.version for clarity

to_update.append(instance)
else:
create_data = {k: v for k, v in data.items() if k not in exclude_fields}
instance = cls(**create_data)
instance.set_pk()
instance.user_created = user
instance.user_updated = user
instance.date_created = now
instance.date_updated = now
instance.version = 1
to_create.append(instance)

with transaction.atomic():
created_count = 0
updated_count = 0

if to_create:
cls.objects.bulk_create(to_create, batch_size=batch_size)
created_count = len(to_create)

if to_update:
update_fields = [f for f in to_update[0].__dict__.keys()
if not f.startswith('_') and f not in exclude_fields]
Comment on lines +282 to +283
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

Accessing to_update[0] without checking if to_update is empty will raise an IndexError. This code is inside the if to_update: block but relies on at least one element existing. While the if to_update: check prevents the block from executing when empty, the logic assumes all instances have identical field sets, which may not hold if the model has fields with null=True or if different subclasses are mixed. Consider extracting field names from the model's _meta.get_fields() instead of relying on instance __dict__.

Suggested change
update_fields = [f for f in to_update[0].__dict__.keys()
if not f.startswith('_') and f not in exclude_fields]
update_fields = [
field.name for field in cls._meta.get_fields()
if (
field.concrete
and not field.auto_created
and not (field.many_to_many or field.one_to_many)
and field.name not in exclude_fields
)
]

Copilot uses AI. Check for mistakes.
update_fields += ['user_updated', 'date_updated', 'version']

cls.objects.bulk_update(to_update, update_fields, batch_size=batch_size)

ids = [obj.id for obj in to_update]
updated_objects = cls.objects.filter(id__in=ids).only('id', 'version')
version_map = {obj.id: obj.version for obj in updated_objects}
for obj in to_update:
obj.version = version_map.get(obj.id, obj.version)

updated_count = len(to_update)

return {'created': created_count, 'updated': updated_count}

@classmethod
def filter_queryset(cls, queryset=None):
if queryset is None:
Expand Down
Loading