fix: update pk field only raises unfriendly error

[waketzheng]

Description

Close #1870

Motivation and Context

Explicit raises OptionalError when executing await model.save(update_fields=['id'])

How Has This Been Tested?

make ci

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added the changelog accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[codspeed-hq]

CodSpeed Performance Report

Merging #1873 will not alter performance

_{Comparing waketzheng:update-pk-field-only (ce75192) with develop (4e47cff)}

Summary

✅ 16 untouched benchmarks

[coveralls]

Pull Request Test Coverage Report for Build 13321805682

Details

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on update-pk-field-only at 94.958%

---

Totals
Change from base Build 13286636838:	95.0%
Covered Lines:	348
Relevant Lines:	355

---

💛 - Coveralls

[henadzit]

@waketzheng I added benchmarks for updates, let's merge them first so we have a baseline for performance and can be sure that this changes doesn't impact it significantly.

[henadzit]

Can you please rebase on top of the latest changes in develop?

[henadzit]

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

[waketzheng]

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

[henadzit]

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.

[henadzit]

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.

[henadzit]

This is quite hard to read. I can see why you would want to use the := operator in a simple if-else like

if a := something:
  ...

but in this case the assignment is hidden within a function call which make it quite hard to read. I would rather have the variable outside of the if check.

[waketzheng]

Updated.

Look at this: https://peps.python.org/pep-0572/#examples

To keep easy to read:

• This is ok:

match1 = pattern1.match(data)
if match1:
   print(match1)

# Change to be 
if match1 := pattern1.match(data):
   print(match1)

• This ok too

group = re.match(data).group(1) if re.match(data) else None

# Change to be
group = m.group(1) if (m := re.match(data)) else None

---

This is not recommended:

if (match1 := pattern1.match(data)) and len(match1.groups()) > 2:
   print(match1)

This not recommended either:

if isinstance(m := get_object(), str):
    return m

[waketzheng]

@henadzit All done.

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.

But I don't quite get the point that why do you say This doesn't seem like a good design.
Deprecation warning is important for compatibility, for example:
If we want to remove the index parameter for Field class, we should triggers a warning for 0.24.x and then delete it at 0.25.0 or later version.

[henadzit]

LGTM!