Opened 5 years ago

Closed 4 years ago

#17750 closed New feature (wontfix)

User.get_profile() returns cached object when database object has changed

Reported by: Renato Alves Owned by: nobody
Component: contrib.auth Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently there is no way to bypass the cache if we know that the cached object is going to be outdated.

For instance when using tests one may want to retrieve the user profile data in order to prepopulate a form and after posting the form data one may want to check if the alteration was successful. In this case the last call to get_profile() will return the object as it was before the post.

I therefore suggest that the attribute cached=True is added to .get_profile() such that by default the cache is used but if needed one can request .get_profile(False) or .get_profile(cached=False) to hit the database again.

Attached patch with code changes and updated unittests.

Attachments (3)

0001-User.get_profile-now-accepts-cached-argument-to-allo.patch (2.4 KB) - added by Renato Alves 5 years ago.
0001-Documentation-for-API-change-on-User.get_profile.patch (1.5 KB) - added by Renato Alves 5 years ago.
0001-Fixed-17750-User.get_profile-now-accepts-cached-argu.patch (4.8 KB) - added by Renato Alves 5 years ago.
Final patch with changes to code, documentation and tests.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by Claude Paroz

Needs documentation: set
Needs tests: unset
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The tests should be moved to django/contrib/auth/tests/models.py.
This needs a documentation update also, as get_profile is public API (https://docs.djangoproject.com/en/dev/topics/auth/#django.contrib.auth.models.User.get_profile)

comment:2 Changed 5 years ago by Renato Alves

Needs documentation: unset

I tried moving the tests to django/contrib/auth/tests/models.py as indicated but UserProfile module is required for the tests which is not available as part of contrib/auth.
Creating it on django/contrib/auth/models.py seemed like a bad idea and creating it on django/contrib/auth/tests/models.py doesn't work as the app is not in INSTALLED_APPS and one cannot refer to it with AUTH_PROFILE_MODULE="auth.tests.UserProfile"

comment:3 Changed 5 years ago by Claude Paroz

Thanks for the doc part. But please, merge both patches into one.

You are right in that we cannot create new models for contrib tests, so I think we should create a new test directory in tests/regressiontests/user_profile/...

In the documentation, you should also add the ".. versionchanged:: 1.5" mark to indicate this was a new addition (as a new feature, it will be too late to be included in 1.4).

comment:4 Changed 5 years ago by Renato Alves

Patch needs improvement: unset

Please confirm that patch is acceptable.

comment:5 Changed 5 years ago by Claude Paroz

For me, it looks rather fine now.
Did you try to use the more convenient override_settings decorator for the AUTH_PROFILE_MODULE in tests (you can find examples in regressiontests/cache/tests.py)?

Changed 5 years ago by Renato Alves

Final patch with changes to code, documentation and tests.

comment:6 Changed 5 years ago by Renato Alves

Included your suggestion in the patch.

PS: I couldn't figure out how to make old patches obsolete (or remove them).

comment:7 in reply to:  6 Changed 5 years ago by Claude Paroz

Thanks for working on this. Now let's just wait for 1.4 to be released before pushing this forward.

Replying to rjalves:

PS: I couldn't figure out how to make old patches obsolete (or remove them).

Don't bother, letting older patches doesn't hurt.

comment:8 Changed 4 years ago by anonymous

I can't believe a bug like this would even exist in a so-called "mature" application framework.

Anyhow ... 1.4 has been out for over a month and this is still broken.

comment:9 Changed 4 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:10 Changed 4 years ago by Anssi Kääriäinen

Severity: NormalRelease blocker

I was going to commit this and did some final polish, available from here: https://github.com/akaariai/django/tree/ticket_17750

The reason I decided to wait is that the custom user model stuff is likely going to land into Django soon. get_profile() is going to be deprecated, so I am wondering if it is a good idea to do changes to already deprecated features. In addition, I want to avoid messing in the same code areas to make merging easier (even if this isn't a big problem in Git age).

I am marking this as Release Blocker - the reasoning is that I want us to decide if we want this into 1.5 or not, and without this marker this is likely going to be forgotten into the depths of Trac...

comment:11 Changed 4 years ago by Preston Holmes

I'd lean strongly toward a wontfix for this. If one has specific needs for this in testing during deprecation, they can just manipulate the _profile_cache attr directly.

I'm starting to rework the auth docs substantially and get_profile will have a much smaller presence in the 1.5 docs.

comment:12 Changed 4 years ago by Claude Paroz

Resolution: wontfix
Status: newclosed

Jannis leidel has also expressed his opinion on IRC in favour of a "won't fix" for this. Therefore, I'll close it. Renato, thanks for working on this and sorry not to have fixed this sooner.

Note: See TracTickets for help on using tickets.
Back to Top