Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 3 additions & 5 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,20 @@ Changelog

.. rst-class:: emphasize-children

0.25
0.24
====

0.25.0 (unreleased)
0.24.1 (unreleased)
------
Fixed
^^^^^
- Fixed asyncio "no current event loop" deprecation warning by replacing `asyncio.get_event_loop()` with modern event loop handling using `get_running_loop()` with fallback to `new_event_loop()` (#1865)
- Fix update pk field only raises unfriendly error (#1873)

Changed
^^^^^^^
- add benchmarks for `get_for_dialect` (#1862)

0.24
====

0.24.0
------
Fixed
Expand Down
4 changes: 4 additions & 0 deletions tests/test_model_methods.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import os
from uuid import uuid4

import pytest

from tests.testmodels import (
Dest_null,
Event,
Expand Down Expand Up @@ -121,6 +123,8 @@ async def test_save_partial(self):
n_mdl = await self.cls.get(id=self.mdl.id)
self.assertEqual(n_mdl.name, "Test")
self.assertEqual(n_mdl.desc, "Something")
with pytest.raises(OperationalError):
await self.mdl.save(update_fields=["id"])

async def test_create(self):
mdl = self.cls(name="Test2")
Expand Down
12 changes: 7 additions & 5 deletions tortoise/backends/base/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,15 @@ async def execute_update(
values = []
expressions = {}
for field in update_fields or self.model._meta.fields_db_projection.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

The current logic assumes that if no values and no expressions, then the user must have been passed pk as part of update_fields. That is a bit hard to follow. What do you think about the version below?

for field in update_fields or self.model._meta.fields_db_projection.keys():
  field_obj = self.model._meta.fields_map[field]
  if field_obj.pk:
    if update_fields:
      raise OperationalError(
                f"Can't update pk field, use `{self.model.__name__}.create` instead."
            )
    continue

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 think we should allow .update_fields=['id', 'xxx'] as the Django do. And show warning to make sure user want to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should allow .update_fields=['id', 'xxx'] as the Django do. And show warning to make sure user want to do that.

Why introduce a behavior that triggers a warning? This doesn't seem like a good design. You usually introduce warnings about deprecation or something you want users to stop doing.

if not self.model._meta.fields_map[field].pk:
instance_field = getattr(instance, field)
if isinstance(instance_field, Expression):
if not (field_obj := self.model._meta.fields_map[field]).pk:
if isinstance(instance_field := getattr(instance, field), Expression):
expressions[field] = instance_field
else:
value = self.model._meta.fields_map[field].to_db_value(instance_field, instance)
values.append(value)
values.append(field_obj.to_db_value(instance_field, instance))
if not values and not expressions:
raise OperationalError(
f"Can't update pk field only, use `{self.model.__name__}.create` instead."
)
values.append(self.model._meta.pk.to_db_value(instance.pk, instance))
return (
await self.db.execute_query(self.get_update_sql(update_fields, expressions), values)
Expand Down