[Sugar-devel] [PATCH sugar-datastore] Ensure we return valid internal / calculated properties

[Simon Schampijer]

Hi Sascha,

thanks for the patch.

On 02/11/11 23:21, Sascha Silbe wrote:
> The copy in the metadata storage can get corrupted, e.g. due to low level
> crashes or running out of battery (see OLPC#11372 [1] for a real-life
> example).
>
> This is especially problematic for the uid property, since without it the
> caller (i.e. the Journal) can't even figure out which entry to delete.
>
> [1] https://dev.laptop.org/ticket/11372

It would be good to add here information what the patch does, that it 
adds the uid and filesize to the metadata but does not write it to disk etc.

> Reported-by: Gary Martin<garycmartin at googlemail.com>
> Signed-off-by: Sascha Silbe<silbe at activitycentral.com>
> ---
>
> Passes the test suite - at least after fixing the latter to accept decimal
> strings instead of just integers for the 'filesize' property. No difference
> in performance noticeable with three runs of the test suite (default
> settings, on my desktop machine).
>
>   src/carquinyol/datastore.py |   20 ++++++++++++++++++++
>   1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/src/carquinyol/datastore.py b/src/carquinyol/datastore.py
> index 4f3faba..eafc8e1 100644
> --- a/src/carquinyol/datastore.py
> +++ b/src/carquinyol/datastore.py
> @@ -333,6 +333,7 @@ class DataStore(dbus.service.Object):
>                   return self._find_all(query, properties)
>
>               metadata = self._metadata_store.retrieve(uid, properties)
> +            self._fill_internal_props(metadata, uid, properties)
>               entries.append(metadata)
>
>           logger.debug('find(): %r', time.time() - t)
> @@ -350,10 +351,28 @@ class DataStore(dbus.service.Object):
>           entries = []
>           for uid in uids:
>               metadata = self._metadata_store.retrieve(uid, properties)
> +            self._fill_internal_props(metadata, uid, properties)
>               entries.append(metadata)
>
>           return entries, count
>
> +    def _fill_internal_props(self, metadata, uid, names=None):
> +        """Fill in internal / computed properties in metadata
> +
> +        Properties are only set if they appear in names or if names is
> +        empty.
> +        """
> +        if not names or 'uid' in names:
> +            metadata['uid'] = uid
> +
> +        if not names or 'filesize' in names:
> +            file_path = self._file_store.get_file_path(uid)
> +            if os.path.exists(file_path):
> +                stat = os.stat(file_path)
> +                metadata['filesize'] = str(stat.st_size)
> +            else:
> +                metadata['filesize'] = '0'

Why do you make the filesize a string here? In all the other cases in 
the code it is an int e.g.:

if os.path.exists(file_path):
     stat = os.stat(file_path)
     props['filesize'] = stat.st_size
else:
     props['filesize'] = 0

I think we should be consistent here.

Regards,
    Simon