google-code-export / django-photologue

Automatically exported from code.google.com/p/django-photologue
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

deleting and ImageModel concrete subclass instance throws unneccessary and imprecise error if the image attribute is empty #73

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. create a subclass of ImageModel for image storage (class 
Picture(ImageModel): pass)
2. create some photosizes
2. create a Picture instance e.g. p=Picture() - but do not actually populate 
the image attribute
3. delete said picture - p.delete()
4. django will raise a ValueError when photologue tries to find the cache path 
to delete cached 
images

What is the expected output? What do you see instead?
I feel the correct response is something like EITHER
1. No error at all - if I haven't given the Picture instance an image field 
yet, then there is no 
cached data to delete, so trying to find the cache folder is unceccessary and 
shoudl not raise an 
error.
2. alternatively if we do want to throw an error in this case to allow the user 
to clean up some 
cached data from some exotic failure case, then it should not be a ValueError - 
something more 
specific that can be caught would be better (say, PhotologueCacheError?)

What version of the product are you using? On what operating system?
Photologue http://django-photologue.googlecode.com/svn/trunk/photologue@348
Django 1.0
Ubuntu 8.04 (i.e. python 2.5.2)

Please provide any additional information below.

having an empty image field is an important use case for me - some of my images 
are fetched 
from remote URLs and are handled by a queued fetching operation - if we haven't 
yet run that 
queued fetch then the image field will be unpopulated since there is as yet no 
image data. 
Additionally, even without the remote URL fetching, some image fields remain 
unpopulated as we 
create records, use the db backend to check for duplicate records, then delete 
the record again if 
duplicates are found. I can' think of more scenarios where this is likely for 
other users.

I'm not yet across the design philosophy of photologue regarding errors and the 
cache and 
whether this use case is considered important, but I'm happy to roll a patch 
for the ImageModel 
class pending a design decision about the 'right' way to handle failure here.

sample partial traceback -
----
    self.clear_cache()
  File "/usr/local/src/photologue/models.py", line 672, in clear_cache
    obj.remove_size(self)
  File "/usr/local/src/photologue/models.py", line 422, in remove_size
    if not self.size_exists(photosize):
  File "/usr/local/src/photologue/models.py", line 337, in size_exists
    if os.path.isfile(func()):
  File "/usr/local/src/djangoes/django/django/utils/functional.py", line 55, in _curried

  File "/usr/local/src/photologue/models.py", line 316, in _get_SIZE_filename
    return os.path.join(self.cache_path(),
  File "/usr/local/src/photologue/models.py", line 284, in cache_path
    return os.path.join(os.path.dirname(self.image.path), "cache")
  File "/usr/local/src/djangoes/django/django/db/models/fields/files.py", line 48, in _get_path

  File "/usr/local/src/djangoes/django/django/db/models/fields/files.py", line 38, in 
_require_file

ValueError: The 'image' attribute has no file associated with it.

Original issue reported on code.google.com by dan.mack...@gmail.com on 9 Oct 2008 at 1:42

GoogleCodeExporter commented 9 years ago
Hm - in fact this seems to be occuring in a variety of places, not just the 
delete method. Pretty much anything 
that clears the cache. Perhaps wrapping the clear_cache method in a 
try...except is the go here.

Original comment by dan.mack...@gmail.com on 9 Oct 2008 at 2:16

GoogleCodeExporter commented 9 years ago
This is pretty much how the Django ORM works with unsaved models. If a Photo 
(or any other model) has 
not been saved to the database there's no reason to call it's delete method, 
nor is there a primary 
key associated with the model for it to know WHAT to delete. If you try to do 
this with say the User 
model from django.contrib.auth.models you will get the following:

"AssertionError: User object can't be deleted because its id attribute is set 
to None"

You're just not getting to that point because Photologue is assuming that the 
model has been saved if 
you're calling delete on it.

If you need to clear a model from memory that hasn't been saved you can just 
del() it. The following 
snippet handle the problem.

# do stuff
p = Picture()
# do more stuff and now we need to delete "p"
if p._get_pk_val() is None:
    del(p)
else:
    p.delete()

I have put a check in the ImageModel delete method (r349) that raises the same 
assertion error for 
consistency but I personally would still use the method above instead of 
looking to catch the 
AssertionError as I think it's more readable.

If you can be more specific on the "variety of places" that you're triggering 
clear_cache on unsaved 
models that would be helpful.

- Justin

Original comment by justin.d...@gmail.com on 9 Oct 2008 at 1:23

GoogleCodeExporter commented 9 years ago
I'm not convinced this is an issue so I'm going to mark it invalid. I'll reopen 
the 
ticket if you still feel this is an problem and can provide more details.

Original comment by justin.d...@gmail.com on 17 Oct 2008 at 5:16

GoogleCodeExporter commented 9 years ago
So, i've run into this problem, but through different means.  I am trying to 
delete a bunch of pictures (which just plainly subclass imagemodel):

class Picture(ImageModel):
  pass

and when I use picture.delete(), all this does is clear the image attribute, 
but the picture model remains!  this is cluttering things up, and causing 
errors when I create a new photosize or try to delete the picture from the 
admin interface because it errors out as described by Dan.

I think this is worth investigating.

Original comment by quasiyo...@gmail.com on 10 Aug 2010 at 9:43

GoogleCodeExporter commented 9 years ago
Just ran into the same problem .. without using Photologue.

Like the original poster, I create a cache system using a model with an image 
field which contains images fetched from the web with a management script.

At some point, I don't know how, deleting outdated cache stopped working. When 
I ran the fetching script I always got that dreaded ValueError complaining that 
there was no file associated with the field of my model.

Now the funny thing: I did a print on the file path, the file indeed did not 
exists, but if I did a os.path.exists(path) and a os.path.isfile(path) they 
always returned True.

Still scratching my head over this.

Original comment by hainea...@gmail.com on 13 Jul 2011 at 3:58