Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Allow modifying pep images with same name #1556
Conversation
Currently, 'peps.converters.add_pep_image' does not allow to add an image that already exists in the database for the same name. As a consequence images cannot be updated. This commit modifies 'peps.converters.add_pep_image' to also update the image db record if the corresponding file has changed as per 'filecmp.cmp'. This requires removing the file prior to the db update, because the django 'FileField' does not override existing files. Alternatives to the ad hoc removal of the file in 'add_pep_image' include passing a custom 'OverwriteStorage' to the used 'ImageField', see e.g.: https://code.djangoproject.com/ticket/4339 https://stackoverflow.com/questions/9522759/imagefield-overwrite-image-file-with-same-name Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
|
I just skimmed through the peps git history to see how other image updates were propagated in the past. It generally doesn't seem to have happened often. One case that I found is |
|
@lukpueh I'm not aware of any special thing done for that PEP. |
|
There was at least one point where we accidentally broke PEP image publication in general, so I was wondering if it could be that the earlier versions of the image didn't make it into the website DB. However, I don't think the image commit history backs that up. |
Test a recent change in `add_pep_image` that allows modifying images. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
|
I just pushed a test for my patch. While writing it I found another little issue (see comment inline), which suggests to use my alternative solution (override on upload). What are others' thoughts? It would be nice to fix this. Pep 458 still references an outdated file, although the repo has the correct one. Thanks |
|
|
||
| # We need to remove a prior uploaded file to fix an inconsistency | ||
| # between db and filesystem. | ||
| # TODO: This should be dealt with in page.model or converters module |
lukpueh
Mar 4, 2020
Author
Here's some additional info for this line;
A prior test that calls add_pep_image might have already created the 'pep-3001-1.png' file, without persisting the db object. Now a new call to add_pep_image will check for objects, whose name ends with 'pep-3001-1.png', and, since it does not find any, create it anew along with the file. But since Django does not allow to override files it duplicates it using a random infix, e.g 'pep-3001-1_0u7tlLY.png', which is also used as a name on the object.
Now due to the endswith-check and with the random infix, each subsequent attempt to add 'pep-3001-1.png' with add_pep_image, will also duplicate it the object and file.
Maybe we should just override images on upload as suggested as alternative fix in the PR description?
Fixes #824
Currently,
peps.converters.add_pep_imagedoes not allow to add an image that already exists in the database for the same name. As a consequence images cannot be updated.This PR modifies
peps.converters.add_pep_imageto also update the image db record if the corresponding file has changed as perfilecmp.cmp'This requires removing the file prior to the db update, because the djangoFileFielddoes not override existing files.Alternatives to the ad hoc removal of the file in
add_pep_imageinclude passing a customOverwriteStorageto the usedImageField, see e.g.:https://code.djangoproject.com/ticket/4339
https://stackoverflow.com/questions/9522759/imagefield-overwrite-image-file-with-same-name