use pk instead of id in clear expired tokens#1593
use pk instead of id in clear expired tokens#1593sahama wants to merge 2 commits intodjango-oauth:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for creating this PR. To help me review this can you please describe the bug this fixes and provide a test demonstrating the scenario? Also don't forget to add your self to the Authors and update the changelog. |
|
|
||
| with suppress(access_token_model.DoesNotExist): | ||
| access_token_model.objects.get(id=self.access_token_id).revoke() | ||
| access_token_model.objects.get(pk=self.access_token_id).revoke() |
There was a problem hiding this comment.
I'm not sure if it will work
|
@sahama are you willing to complete the work on this PR or should we close it? |
dopry
force-pushed
the
BUGFIX/pk-instead-of-id
branch
from
4a82f7b to
7217311
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the token clearing functionality fails when custom primary keys are used instead of the default id field. The fix changes hardcoded id references to use Django's pk shortcut, which works with any primary key field name.
Changes:
- Updated
AbstractRefreshToken.revoke()to usepkinstead ofidwhen revoking related access tokens - Updated
clear_expired()batch_delete helper to usepkinstead ofidfor token deletion queries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flat_queryset = queryset.values_list("pk", flat=True)[:CLEAR_EXPIRED_TOKENS_BATCH_SIZE] | ||
| batch_length = flat_queryset.count() | ||
| queryset.model.objects.filter(id__in=list(flat_queryset)).delete() | ||
| queryset.model.objects.filter(pk__in=list(flat_queryset)).delete() |
There was a problem hiding this comment.
The changes from id to pk in the batch_delete function are correct for supporting custom primary keys. However, this functionality lacks test coverage for the specific case of custom primary keys. Consider adding a test that verifies the clear_expired function works correctly when models use custom primary key fields (not named 'id'). The existing tests in tests/test_models.py only test with the default BigAutoField primary key.
|
|
||
| with suppress(access_token_model.DoesNotExist): | ||
| access_token_model.objects.get(id=self.access_token_id).revoke() | ||
| access_token_model.objects.get(pk=self.access_token_id).revoke() |
There was a problem hiding this comment.
The change from id to pk in the revoke method is correct for supporting custom primary keys. However, similar to the clear_expired function, there's no test coverage for the case where AccessToken uses a custom primary key field. Consider adding a test that verifies the refresh token revocation works correctly when the AccessToken model uses a custom primary key.
dopry
force-pushed
the
BUGFIX/pk-instead-of-id
branch
from
7217311 to
071ec3b
Compare
dopry
force-pushed
the
BUGFIX/pk-instead-of-id
branch
from
071ec3b to
7442a1f
Compare
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Fixes #
Fixing error in clearing expired tokens when custom pk used.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS