Code

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#8593 closed Bug (fixed)

Image upload in Windows changes filename to lowercase

Reported by: robvdl Owned by: SmileyChris
Component: File uploads/storage Version: 1.0
Severity: Normal Keywords: upload image lowercase
Cc: mitsuhiko Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

When using an ImageField and uploading an image via admin, say the filename was Test.jpg, uploading it in Linux will save the file as Test.jpg on the disk, and the reference in the database is also Test.jpg, things work normally as expected here.

However, when uploading the same file in Windows, the filename is saved on the disk as test.jpg (all lower case), but in the database is still referenced as Test.jpg. Since the filesystem in Windows is not case sensitive, this is not really much of an issue immediately. If you however, then copy your Django based site back to a Linux based machine, you will get a missing image, as it is expected to find the file "Test.jpg" in the filesystem, however the file is "test.jpg". In this case, this can become a bit of a problem.

I am not sure if the problem is related to Django or Python on Windows.

Attachments (5)

8593-regression-tests.diff (4.4 KB) - added by ramiro 5 years ago.
File with new tests added to the django test suite that fail
8593-r9832.diff (6.7 KB) - added by ramiro 5 years ago.
Patch that solves the issue, also includes the tests already uploaded in 8593-regression-tests.diff
8593-dont-lowercase-uploaded-filenames-r9961.diff (7.1 KB) - added by ramiro 5 years ago.
Same patch but with two comments edited to be more accurate
8593-dont-lowercase-uploaded-filenames-r10727.diff (7.3 KB) - added by ramiro 5 years ago.
Updated patch so it applies after file_storage tests switch from doctests to unit tests in r10707
8593.diff (8.0 KB) - added by SmileyChris 3 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

As a note, Windows is case sensitive on file creation, but not after that. Django is (correctly) doing to-lower to test for file existence, but I agree with the issue that either:

  • the name in the database should be lowercased to match
  • the file name when actually written should honor the original casing

comment:2 Changed 6 years ago by SmileyChris

Writing with the original casing will be more difficult (this is done by django.utils._os.safe_join) so just normalizing the case seems the easiest course of action.

Easiest solution seems to be using os.path.normcase in the get_directory_name and get_filename methods of FileField.

comment:3 Changed 6 years ago by Soviut

  • Version changed from SVN to 1.0

I can confirm that this issue exists in 1.0 since its the release I'm suffering from it.

comment:4 follow-up: Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by kmtracey

#9442 was a dup, it also has a suggested fix.

comment:6 in reply to: ↑ 4 Changed 6 years ago by dawidjoubert

Replying to SmileyChris:
Where does this put us with a fix?

comment:7 Changed 6 years ago by kmtracey

If you want to move the process along, the next step would be posting a patch that fixes the issue, passes all existing tests, and contains a test that fails on the current code (it's OK if it fails only on Windows -- but it can't rely on a particular configuration like a networked drive) and succeeds with the fix.

Changed 5 years ago by ramiro

File with new tests added to the django test suite that fail

comment:8 Changed 5 years ago by ramiro

I think that, contrarily to what SmileyChris suggests, it is worth going for the real and correct solution to this problem as the reporter of #9442 suggests rather than adding a workaround.

By correct solution I mean making sure Django uses the same path (case-wise) both for:

  • the file written to disk
  • and the path stored in the database

when storing a FileField/ImageField in all supported platforms.

I've attached a patch (8593-regression-tests.diff) that add tests to the test suite that show how currently, when running on Windows (whose file systems are case-insensitive but case-preserving), Django is lowercasing the path used to create the file in the file system.

These new tests shouldn't fail on other platfoms (i've tested also on Linux).

Changed 5 years ago by ramiro

Patch that solves the issue, also includes the tests already uploaded in 8593-regression-tests.diff

comment:9 Changed 5 years ago by ramiro

  • Has patch set

8593-r9832.diff implements a fix for this issue by modifying Django's django.utils._os.safe_join to not call os.path.normcase() under Windows, the modification has been implemented in a way that doesn't break the security safeguards that are the raison d'être of such function.

As safe_join() is also used by the template loaders, the case-insensitive-filesystems-specific template regression tests have been also modified (AFAICS these tests as implemented aren't really useful because they aren't using a template file path as written to the file system when comparing paths but rather two Django maintained in-memory representations of such path.)

Net effect of the path is that now Django doesn't use a path in the file system that differs from the path stored in the DB for uploaded files, irrespective of platform. This should help when moving projects and deployments form platform to platform.

Changed 5 years ago by ramiro

Same patch but with two comments edited to be more accurate

comment:10 Changed 5 years ago by ramiro

  • Owner changed from nobody to ramiro

Changed 5 years ago by ramiro

Updated patch so it applies after file_storage tests switch from doctests to unit tests in r10707

comment:11 Changed 5 years ago by mitsuhiko

  • Cc mitsuhiko added

The case is more complex. On OS X the IO system performs unicode normalization for the filename after the file was stored. This is fine until you mount the volume on a linux machine or access it over the network. I'm not sure what the fix is, OS X normalizes in the IO system somehow, it also affects IO access on NFS mounts and similar volumes. You can't really find out how the normalization works from inside Python.

comment:12 Changed 4 years ago by adamnelson

  • Patch needs improvement set

Reflecting @mitsuhiko's comment by marking that the patch needs improvement.

comment:13 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:14 Changed 3 years ago by SmileyChris

  • Easy pickings unset

For what it's worth, I agree with ramiro that this should be fixed to preserve the case.

I've got a branch with the a new patch here: https://github.com/SmileyChris/django/compare/master...8593-safejoin-lowercase-on-windows

I'm ignoring mitsuhiko's comment, as it has nothing to do with image uploads on windows.

Changed 3 years ago by SmileyChris

comment:15 Changed 3 years ago by SmileyChris

  • milestone set to 1.4
  • Patch needs improvement unset

comment:16 Changed 3 years ago by srj55

  • milestone 1.4 deleted
  • Patch needs improvement set

Successfully tested the patch from @SmileyChris on Win7

comment:17 Changed 3 years ago by srj55

  • milestone set to 1.4
  • Patch needs improvement unset

Successfully tested the patch from @SmileyChris on Win7

comment:18 Changed 3 years ago by ramiro

  • Owner ramiro deleted

comment:19 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:20 Changed 3 years ago by SmileyChris

  • Owner set to SmileyChris
  • Resolution set to fixed
  • Status changed from new to closed

In [16267]:

Fixes #8593 -- better handling of safe_join case sensitivity on windows. Thanks for the initial patch, ramiro.

comment:21 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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.