Opened 16 years ago

Closed 8 years ago

#5619 closed Bug (invalid)

FileField and ImageField return the wrong path/url before calling save_FOO_file()

Reported by: wreese@… Owned by: Leah Culver
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Models with FileField or ImageField attributes do not function properly. The get_FOO_filename() and get_FOO_url() methods fail to insert the upload_to path if they are accessed before a call to save_FOO_image(). This happens because _get_FIELD_filename() and _get_FIELD_url() do not call field.get_filename(). I've attached a patch to resolve the issue.

Attachments (2)

get_filename_fix.diff (933 bytes ) - added by wreese@… 16 years ago.
patch-5619-with-tests.diff (2.7 KB ) - added by Leah Culver 16 years ago.
added tests for previous patch

Download all attachments as: .zip

Change History (27)

by wreese@…, 16 years ago

Attachment: get_filename_fix.diff added

comment:1 by Chris Beaven, 16 years ago

Needs tests: set

comment:2 by Marty Alchin, 16 years ago

Keywords: fs-rf added

comment:3 by Marty Alchin, 16 years ago

Keywords: fs-rf-fixed added; fs-rf removed

comment:4 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Marty Alchin, 16 years ago

milestone: 1.0 beta

comment:6 by Leah Culver, 16 years ago

Owner: changed from nobody to Leah Culver
Status: newassigned

comment:7 by Marty Alchin, 16 years ago

Sorry for not explaining this properly already, but the fs-rf-fixed keyword on this ticket indicates that this fix has already been rolled into #5361.

by Leah Culver, 16 years ago

Attachment: patch-5619-with-tests.diff added

added tests for previous patch

comment:8 by Leah Culver, 16 years ago

Triage Stage: AcceptedReady for checkin

Malcolm reviewed.

comment:9 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [7986]) Fixed #5619 -- Return the same path in get_FOO_filename() before and after a
model is saved (i.e. make sure the upload prefix is prepended in both cases).

Patch from wreese@…. Tests from Leah Culver.

comment:10 by Carl Meyer, 16 years ago

Resolution: fixed
Status: closedreopened

This patch is totally broken, and breaks get_FIELD_url for all files or images uploaded prior to the current day, because field.get_directory_name uses datetime.now() to interpolate the date values. [7986] needs to be reverted as soon as possible.

comment:11 by Karen Tracey <kmtracey@…>, 16 years ago

You do know you can get your own copy reverted back to pre-7986 by doing something like:

svn -r7985 up

right? So you can get your code back to working immediately without the change actually being reverted instantaneously.

comment:12 by Carl Meyer, 16 years ago

Of course, already done - but only after too much time spent looking for the problem. Not trying to be a pain, just trying to save others the same hassle.

comment:13 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: reopenedclosed

Since you've already opened another ticket (#7843) for the new bug, how about now also reopening this one (which does fix another problem)? It only requires one ticket. I'm sorry it's had an unintended side-effect and I'll look at it very shortly, but for now, just use an earlier revision for a little bit.

comment:14 by Carl Meyer, 16 years ago

My bad - guess I assumed that that if the fix here wasn't workable, this ticket would still need attention as well. Sorry for the hassle.

comment:15 by Malcolm Tredinnick, 16 years ago

you could have reopened this. There's just no need for two tickets (the fix here needs changing, not reverting, since it fixes a real problem, as can be seen from the patch). Anyway, we're on the same page now. It'll be fixed soon.

comment:16 by Malcolm Tredinnick, 16 years ago

(In [7998]) Reverted [7986].

It turns out that we need to treat filename creation/display (in
particular, the upload_to path) differently depending upon whether the value is
out of the database or provided by other code and there's no reliable way to
determine that at the moment (although some later proposed changes might alter
that). So calling get_FIELD_filename on an unsaved model with a changed file
attribute will not necessarily return the same result as after the save().

Refs #5619. Fixed #7843.

comment:17 by Malcolm Tredinnick, 16 years ago

(In [7999]) Added a cavaet to the use of get_FOO_filename() and get_FOO_url(). This
constraint has always existed, but it's very hard to fix in the current code,
so better to work around it for now.

Refs #5619

comment:18 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinSomeday/Maybe

As noted in the above commits, there are technical reasons why it's not really possible to fix this in a straightforward fashion. Tracking when an attribute is updated by code (as opposed to a database read) is something we might add in the future, in which case, this problem could be addressed at that point. For now, the simplest and practical solution is "don't do that". The model is going to be saved in any case, so save it first, then read off the filename.

Reopening and moving to Someday/Maybe.

comment:19 by Marty Alchin, 16 years ago

Keywords: fs-rf-fixed removed
milestone: 1.0 beta

Yeah, I'm actually not sure now why I had put fs-rf-fixed on this one. When I add Leah's tests to the #5361 patch, it fails quite reliably, so it's clearly still an issue. I'll keep it in my head and see what happens if/when we get that potential future differentiation between db-load and otherwise. Removing all fs-rf tags, as well as the 1.0 beta milestone, since it clearly won't get taken care of in the immediate future.

The one thing I'd note on this is that the docs probably shouldn't say "*if* you are using upload_to," since it's actually a required argument on all FileFields. That requirement isn't going away after #5361 either, so it's still important to make it clear.

comment:20 by Julien Phalip, 13 years ago

Component: Core frameworkDatabase layer (models, ORM)
Patch needs improvement: set

3 years later, I'm not sure how relevant this is still. At the very least the patch needs to be updated to use unittests.

comment:21 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Bug

comment:22 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:23 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:24 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:25 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed

I think this is obsolete. The methods in the original patch were removed in ef48a3e69c02438db32f20531f5c679e8315d528.

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