Use attname to get pk value

[evanandrews-xrd]

If a model's primary key is another model, the current implementation of LogEntryManager._get_pk_value will needlessly pull in the entire object only to extract its primary key and discard it. This change gets the primary key directly from the first object.

The attname field isn't particularly well documented in Django, but it is briefly mentioned here.

On the base Field class, it is the same as name:

https://github.com/django/django/blob/adae619426b6f50046b3daaa744db52989c9d6db/django/db/models/fields/__init__.py#L973-L974

Whereas for ForeignKey, it is {name}_id:

https://github.com/django/django/blob/adae619426b6f50046b3daaa744db52989c9d6db/django/db/models/fields/related.py#L1117-L1118

Happy to remove the assert if you'd prefer not to use it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (5289482) to head (e848b75).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   95.21%   95.29%   +0.07%     
==========================================
  Files          31       31              
  Lines        1025     1041      +16     
==========================================
+ Hits          976      992      +16     
  Misses         49       49              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hramezani]

Thanks @evanandrews-xrd for this PR.

It would be great to add a test for the change. it can be a small test only for LogEntryManager._get_pk_value

[evanandrews-xrd]

I've added some tests, although looking at this further it might not be necessary to have LogEntryManager._get_pk_value at all. Django provides the Model.pk property which gives you the value of the primary key:

https://docs.djangoproject.com/en/5.0/ref/models/instances/#the-pk-property

https://github.com/django/django/blob/adae619426b6f50046b3daaa744db52989c9d6db/django/db/models/base.py#L653-L663

The only difference now between LogEntryManager._get_pk_value(instance) and instance.pk is that the former will return None if the primary key attribute is (somehow) missing, while the latter will raise AttributeError.

If you'd prefer, we can probably just delete the method and use instance.pk where it's called.

[hramezani]

Thanks @evanandrews-xrd for the update and providing more information.

The only difference now between LogEntryManager._get_pk_value(instance) and instance.pk is that the former will return None if the primary key attribute is (somehow) missing, while the latter will raise AttributeError.

If you'd prefer, we can probably just delete the method and use instance.pk where it's called.

Then we need to catch AttributeError everywhere we use _get_pk_value to prevent breaking change.
I would prefer to keep the method

[hramezani]

Let me know if you are done with this PR. then we can merge it

[evanandrews-xrd]

I've just added a couple of extra comments to include what I touched on above. Happy for you to merge when ready. Thanks!

[hramezani]

Thanks @evanandrews-xrd