Skip to content
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.

Sign up for GitHub

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

feat(entity): optimize entity save method and add bulk save functionality #663

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat(entity): optimize entity save method and add bulk save functionality #663

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
127bae2
feat(entity): optimize entity save method and add bulk save functiona…
Oct 14, 2024
6a80a3a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 14, 2024
2fddeab
feat(eav): add bulk save functionality to Entity model
Oct 14, 2024
29ceb87
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 14, 2024
e3ae5b5
style(entity): improve code style and consistency in Entity class
Oct 14, 2024
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
57 changes: 54 additions & 3 deletions eav/models/entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ def _getattr(self, attribute_slug):
"""
return self.__dict__[attribute_slug]

def save(self):
def save(self, *, commit=True):
"""Saves all the EAV values that have been set on this entity."""
for attribute in self.get_all_attributes():
values = {}
attributes = self.get_all_attributes()
for attribute in attributes:
if self._hasattr(attribute.slug):
attribute_value = self._getattr(attribute.slug)
if (
Expand All @@ -113,7 +115,12 @@ def save(self):
and attribute_value is not None
):
attribute_value = EnumValue.objects.get(value=attribute_value)
attribute.save_value(self.instance, attribute_value)
if commit:
attribute.save_value(self.instance, attribute_value)
values[attribute.slug] = attribute_value
if not commit:
attributes_value = self.save_bulk(values, attributes)
Value.objects.bulk_create(attributes_value)
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

If commit=False, I would expect nothing to be saved to the database, which is line with Django behavior. Instead, what do you think about returning a list[Value] back? This would work for any number of attributes that are updated during an eav.save() call.


def validate_attributes(self):
"""
Expand Down Expand Up @@ -200,6 +207,50 @@ def __iter__(self):
"""
return iter(self.get_values())

def save_bulk(
self,
eav_values,
attributes,
):
Comment on lines +210 to +214
Copy link
Contributor

Choose a reason for hiding this comment

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

In line with the other comment, should bulk_save() take a list[Entity]? Or is the use case for this something different than the two scenarios I identified earlier?

Thinking of how Django's bulks_create() works, you pass in a list of objects created in memory that then get written with an optimized database query. In this scenario, I would think that when wanting to update a number of entities, you would do something in your app like:

for patient in patients:
  patient.eav.age = 10
  patient.eav.height = 2.3
  
eav.bulk_save(patients, 10)
Patient.objects.bulk_create(patients)

Note I did a generic eav.bulk_save() as I'm not sure this should be on the Entity manager as inserted into a specific Entity instance versus being on a Model Manager. Maybe that's a future thing, having a manager for a model that has EAV methods on it, but for now, I think a utility function would work just fine too.

Also, thinking about it, we should use the same language as Django with "bulk_update" and their params to not have confusion around things like expecting signals to be called like a typical save()

What do you think?

"""
Prepare a list of EAV Value objects for bulk creation.

This method takes a dictionary of EAV attribute values and a list of Attribute
objects, and returns a list of Value objects that can be bulk created to update
the EAV data for the current Entity instance.

Args:
eav_values: A dictionary mapping attribute slugs to their new values.
attributes: A list of Attribute objects associated with the current Entity.

Returns:
A list of Value objects that can be bulk created.
"""
eav_values_to_create = []
if not eav_values:
return eav_values_to_create

ct = ContentType.objects.get_for_model(self.instance)
attribute_slugs = list(eav_values.keys())

for attr_slug in attribute_slugs:
entity_data = {
"entity_ct": ct,
"attribute": next(
(
attribute
for attribute in attributes
if attribute.slug == attr_slug
),
None,
),
get_entity_pk_type(self): self.pk,
"value": eav_values[attr_slug],
}
eav_values_to_create.append(Value(**entity_data))

return eav_values_to_create


class EAVModelMeta(ModelBase):
def __new__(cls, name, bases, namespace, **kwds):
Expand Down