Opened 10 years ago

Closed 5 years ago

#1561 closed defect (wontfix)

Most activities and Sugar itself chokes if the type of metadata returned is different from the expected one

Reported by: sayamindu Owned by: erikos
Priority: Normal Milestone: Unspecified
Component: Sugar Version: Git as of bugdate
Severity: Unspecified Keywords: r!
Cc: dsd Distribution/OS: Unspecified
Bug Status: New

Description

Consider the following in Journal code:

keep = int(self.metadata.get('keep', 0))

All is fine and well when the get() method returns an int. However, due to various factors (filesystem corruption, malformed journal bundles, etc), if the metadata type is something else, the code chokes.

1258525147.890608 ERROR root: Exception while displaying entry:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/jarabe/journal/listview.py", line 175, in _refresh_view
    entry.metadata = metadata_list[i]
  File "/usr/lib/python2.6/site-packages/jarabe/journal/collapsedentry.py", line 336, in set_metadata
    BaseCollapsedEntry.set_metadata(self, metadata)
  File "/usr/lib/python2.6/site-packages/jarabe/journal/collapsedentry.py", line 239, in set_metadata
    self.keep_icon.props.keep = self.get_keep()
  File "/usr/lib/python2.6/site-packages/jarabe/journal/collapsedentry.py", line 209, in get_keep
    keep = int(self.metadata.get('keep', 0))
ValueError: invalid literal for int() with base 10: 'o'

A possible workaround is to use the following patch for the Journal code:

-        keep = int(self.metadata.get('keep', 0))
+        try:
+            keep = int(self.metadata.get('keep', 0))
+        except ValueError:
+            keep =  0

However, that would require patching of each and activity out there.
A possible workaround would be to patch sugar-toolkit itself:

diff --git a/src/sugar/datastore/datastore.py b/src/sugar/datastore/datastore.py
index 8d23721..10a4dda 100644
--- a/src/sugar/datastore/datastore.py
+++ b/src/sugar/datastore/datastore.py
@@ -79,9 +79,16 @@ class DSMetadata(gobject.GObject):
 
     def get(self, key, default=None):
         if self._props.has_key(key):
-            return self._props[key]
-        else:
-            return default
+            retvalue = self._props[key]
+            if default is None:
+                return retvalue
+            elif type(retvalue) == type(default):
+                return retvalue
+            else:
+                logging.warning('The default metadata value \
+                    type does not match the type in the datastore.')
+        
+        return default

The patch assumes that the default value supplied would be correct, but I think we can safely consider that activity authors will supply the correct default value in most cases.

Change History (10)

comment:1 Changed 10 years ago by sayamindu

The "proper" way to fix this would be to use methods like get_int(), get_str(), get_float() and so on - but changing get() in all activities is going to be tough.

comment:2 follow-up: Changed 10 years ago by dsd

  • Cc dsd added

I think we should leave it as-is and just fix everything. fixing sugar is the important bit.

comment:3 in reply to: ↑ 2 Changed 10 years ago by tomeu

Replying to dsd:

I think we should leave it as-is and just fix everything. fixing sugar is the important bit.

Can you extend on what you mean by that?

comment:4 Changed 10 years ago by dsd

Find every user of metadata.get() and make sure appropriate checking/exception handling is in place.

comment:5 Changed 10 years ago by tomeu

  • Keywords r! added; r? removed

I think that we still can (and should) make some improvements in the DS so that data cannot get so easily corrupted.

But there's still the problem of an activity setting the keep property to 'blah blah', for example. I think that nor Activities nor Sugar should rely on the metadata having the expected contents and have code to deal with it, which is what Daniel said, AFAIU.

comment:6 Changed 9 years ago by erikos

  • Milestone changed from Unspecified by Release Team to 0.90

Ok, I am moving this to 0.90 as we do not have a new patch which deals with what has been discussed in this ticket.

If someone cares and has patches - could go into 0.88.x if not too invasive.

comment:7 Changed 6 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar

comment:8 Changed 6 years ago by dnarvaez

  • Milestone changed from 0.90 to Unspecified

comment:9 Changed 6 years ago by dnarvaez

  • Bug Status changed from Unconfirmed to New
  • Priority changed from Unspecified by Maintainer to Normal

I'm sort of unconvinced about having sugar/activities deal with it call by call. But I don't really have a proposal.

comment:10 Changed 5 years ago by godiard

  • Resolution set to wontfix
  • Status changed from new to closed

Closing as is not something we have a agreement and do not have a proposal.

Note: See TracTickets for help on using tickets.