Weird behaviour of delete_uploaded_files with update_record

[Leonel Câmara]

Say you have a table like this

db.define_table('picture',
    Field('sort_position', 'integer', readable=False, writable=False, default=0),
    Field('photo', 'upload', label=T('Photo'), uploadseparate=True),
    Field('thumb','upload', uploadseparate=True, readable=False, writable=False, autodelete=True, compute=lambda row: THUMB(row.photo)),
)

THUMB creates a thumbnail and saves it with an appropriate name as is expected.

Now insert a couple of images. See that the thumbnails have been created on disk.

Then pick a record and update it's sort position using update_record.

record = db(db.picture.id > 0).select().first()
record.update_record(sort_position=1)

What you will see is that the file stored in thumb will be deleted even though thumb still has the same value.

Looking at Set.delete_uploaded_files the problem seems to be that

upload_fields and fieldname in upload_fields

Is False so it goes to the else which causes the deletion.

Is this supposed to happen? Or is it a bug caused by making autodelete work for computed fields? Any fix suggestions?

[Richard Vézina]

To me it a bug... I struggle with it for sometimes now... I thought it was my fault at first and fix things on my side, but then bug still coming back... I recall having filling an issue or discuss that with Simone over the list...

Richard

--
-- mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-developers@googlegroups.com
unsubscribe: web2py-developers+unsubscribe@googlegroups.com
details : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official : http://www.web2py.com/
---
You received this message because you are subscribed to the Google Groups "web2py-developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to web2py-developers+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Richard Vézina]

Here, seems a similar report :

https://groups.google.com/forum/#!topic/web2py/odP4yuFm2AY

Saddly I can't find anything back related to the issue I had... Will try later to find things back if I post anything regarding this...

I recall that it has something to do with default web2py callback.

Richard

[Leonel Câmara]

Set.delete_uploaded_files is indeed added to _before_update and _before_delete by default, that's not the problem, the problem is that it is deleting stuff it should not.

[Niphlod]

chiming in without reading...but isn't this due to a computed field that doesn't get what's needed to be properly recalculated ? 

[Leonel Câmara]

It's not, you can easily make THUMB never fail and simply return the old value if it doesn't get what it needs. The file still gets deleted.

[Leonel Câmara]

To be clear, the problem here is that Set.delete_uploaded_files is put both in the default _before_update and _before_delete, and while the behavior is correct for the latter it is wrong in the former. Because when you are doing an update you should not delete files just because they aren't one of the fields being updated, which is exactly what is happening right now.

[Leonel Câmara]

Pull request with the fix

https://github.com/web2py/pydal/pull/429

[Leonel Câmara]

Anyone care to comment, I'm starting to think compute fields the way they are made right now are not sustainable as you don't have any indication of which fields will be used to compute. This makes them inefficient and creates problems like this one right here.

Compute fields would probably be better implemented as a callable class with a required_fields attribute. 

As an alternative the user could declare a function and then add required_fields example:

def full_name_length(row):
    return len(row.first_name + row.last_name)


full_name_length.required_fields = ('first_name', 'last_name')


Field('name_length', 'integer', compute=full_name_length)

As a backward compatibility measure we could assume compute fields that don't have required_fields defined, require all the fields.

This would allow for a much smarter solution to the delete_uploaded_files problem and for more efficiency processing record insertions and updates as the compute fields would only be calculated if one of their required_fields actually changed.

[Richard Vézina]

If we can be warned somehow which fields value is missing instead of just compute nothing when there is something wrong... But with all the callback feature of web2py it kind of complexe task to make it better without breaking anything...

Richard

--

[Giovanni Barillari]

This is interesting but might lead to performance issues. 

If I have a table with 30 columns and 3 computed columns, would you make a for cycle to check the presence of the fields before calling the callbacks? This makes 90 checks if I don't specify any required fields.

I would enjoy having something like this but without performance degradation.

/Giovanni

[Giovanni Barillari]

I would also like to add something more to the topic, with two use cases involving a code of an application in my company.

We have several computed fields in our models, but we usually use callbacks to really compute fields since we had the same troubles regarding the fields checks.

The first example is computing the full name from two different fields (similar to your example), and we do that like this (this is weppy notation but it's easily translatable to vanilla pydal):

    def _build_name(self, fields):
        components = []
        for key in ['first_name', 'last_name']:
            val = fields.get(key)
            if val:
                components.append(val)
        return " ".join(components)


    @before_insert
    def _create_name(self, fields):
        fields['name'] = self._build_name(fields)


    @before_update
    def _store_ids_for_name_update(self, dbset, fields):
        current._nameupd_contacts = dbset.select(self.id).column('id')


    @after_update
    def _update_name(self, dbset, fields):
        if any(key in fields for key in ['first_name', 'last_name']):
            ids = current._nameupd_contacts
            if not all(key in fields for key in ['first_name', 'last_name']):
                for rid in ids:
                    row = self.db(self.id == rid).select(self.first_name, self.last_name).first()
                    if not row:
                        continue
                    self.db(self.id == rid).update_naive(name=self._build_name(row))
            else:
                for rid in ids:
                    self.db(self.id == rid).update_naive(name=self._build_name(fields))
        del current._nameupd_contacts

So here are the problems:

1) the computation should be performed if any of the fields `first_name` and `last_name` are specified (so i would ask for a `depends_on_any` and `depends_on_all` attributes instead of a `required_fields` to distinct the case

2) we need both fields even if the update specifies just one of them; would be nice to have an helper for this too, instead of the strange thing we're doing on the current object

3) all these checks should be made in a more efficient way

Another example is with financial things, for example we have a deals table with several computed columns depending on combination of other columns, eg if you specify a value we also need a discount one to compute the net_value field, but we want to reach an error when the discount is missing, not avoid to compute the net_value. So here are other problems:

1) the `depends_on_all` should take care of raising some exception if needed

2) this lead to the impossibility of handling some operations with the actual computation layer, because in this example, if I want to update several records i can only specify a single tuple of values for the value and the discount, so if I want to update several records with just the value and using the actual stored discount in each row, I need to perform single update operations. Is there any solution for this?

I hope I added some points for the discussion :)

/Giovanni

[Richard Vézina]

Could the check be just a failing callable in case there is missing values passed? This would break backward compatibility, thought we could make the fail silent to keep backward compatibility maybe a flag that allow the callable to fail or not... This is maybe not the respecting the rule of the art though...

Richard

--