Code

Opened 2 years ago

Closed 21 months ago

#17750 closed New feature (wontfix)

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

Reported by: rjalves 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 rjalves 2 years ago.
0001-Documentation-for-API-change-on-User.get_profile.patch (1.5 KB) - added by rjalves 2 years ago.
0001-Fixed-17750-User.get_profile-now-accepts-cached-argu.patch (4.8 KB) - added by rjalves 2 years ago.
Final patch with changes to code, documentation and tests.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 2 years ago by claudep

  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 2 years ago by rjalves

  • 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 2 years ago by claudep

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 2 years ago by rjalves

  • Patch needs improvement unset

Please confirm that patch is acceptable.

comment:5 Changed 2 years ago by claudep

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 2 years ago by rjalves

Final patch with changes to code, documentation and tests.

comment:6 follow-up: Changed 2 years ago by rjalves

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 2 years ago by claudep

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 2 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 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 22 months ago by akaariai

  • Severity changed from Normal to Release 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 21 months ago by ptone

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 21 months ago by claudep

  • Resolution set to wontfix
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.