Merge lp:~seif/zeitgeist/catch_cache_error into lp:zeitgeist/0.1

[Seif Lotfy]

So I just added a little temporary fix that I would like to see with the next release.
Instead of zeitgeist returning nothing when requesting a time-period with a broken event, zeitgeist now can ignore the broken event and return a list with healthy events. The broken events can be seen in the log under Error

[lp:~seif/zeitgeist/catch_cache_error]

1659. By Seif Lotfy on 2011-01-13

add comments to change

[Markus Korn (thekorn)]

I like the general idea of this workaround: hiding the KeyError to the clients, and log an error server-side instead.
However your solution gives me a bad feeling: It ignores our own policy that all events must have at least one subject. So in your workaround it is possible to get events with no subject back. Also, as we don't know what's going wrong I think it makes no sense to limit this workaround to subjects, are we really sure that this kind of broken db entries can not happen to Event interpretations for example too?

My suggestion is: in case of *any* error in the process of creating Event and Subject objects from a db row let's just block the whole event from being returned, let's return None. And log an error server-side, of course.

review: Needs Fixing

[Seif Lotfy (seif)]

My work around is not limited to subjects.
if the subject is broken then we ignore returning an event. Thus its valid.
Thus its exactly as you are suggesting here.
You can see here that i try to extract the subject for the event. If I cant
extract the subject I ignore the whole event

subject = self._get_subject_from_row(row)
# Check if subject has a proper value. If none than something went
# wrong while trying to fetch the subject from the row. So instead
# of failing and raising an error. We silently skip the event.
* if subject:*
event.append_subject(subject)
event = self.extensions.apply_get_hooks(event, sender)
if event is not None:
for n in id_hash[event.id]:
# insert the event into all necessary spots (LP: #673916)
sorted_events[n] = event

[lp:~seif/zeitgeist/catch_cache_error]

1660. By Seif Lotfy on 2011-01-13

update from trunk

[Markus Korn (thekorn)]

Ok, I think I misread the diff. Everything seems to be ok, good work, +1 from me.

review: Approve

=== modified file '_zeitgeist/engine/main.py'
--- _zeitgeist/engine/main.py	2010-12-28 22:32:47 +0000
+++ _zeitgeist/engine/main.py	2011-01-13 22:23:07 +0000
@@ -142,7 +142,15 @@
 		event[0][Event.Id] = row["id"] # Id property is read-only in the public API
 		event.timestamp = row["timestamp"]
 		for field in ("interpretation", "manifestation", "actor"):
-			setattr(event, field, getattr(self, "_" + field).value(row[field]))
+			# Try to get event attributes from row using the attributed field id
+			# If attribute does not exist we break the attribute fetching and return
+			# None instead of of crashing
+			try:
+				setattr(event, field, getattr(self, "_" + field).value(row[field]))
+			except KeyError, e:
+				log.error("Event %i broken: Table %s has no id %i" \
+						%(row["id"], field, row[field]))
+				return None
 		event.payload = row["payload"] or "" # default payload: empty string
 		return event
 	
@@ -152,8 +160,16 @@
 			setattr(subject, field, row["subj_" + field])
 		setattr(subject, "origin", row["subj_origin_uri"])
 		for field in ("interpretation", "manifestation", "mimetype"):
-			setattr(subject, field,
-				getattr(self, "_" + field).value(row["subj_" + field]))
+			# Try to get subject attributes from row using the attributed field id
+			# If attribute does not exist we break the attribute fetching and return
+			# None instead of of crashing
+			try:
+				setattr(subject, field,
+					getattr(self, "_" + field).value(row["subj_" + field]))
+			except KeyError, e:
+				log.error("Event %i broken: Table %s has no id %i" \
+						%(row["id"], field, row["subj_" + field]))
+				return None
 		return subject
 	
 	def get_events(self, ids, sender=None):
@@ -194,12 +210,17 @@
 					events[event.id] = event
 				else:
 					event = events[event.id]
-				event.append_subject(self._get_subject_from_row(row))
-				event = self.extensions.apply_get_hooks(event, sender)
-				if event is not None:
-					for n in id_hash[event.id]:
-						# insert the event into all necessary spots (LP: #673916)
-						sorted_events[n] = event
+				subject = self._get_subject_from_row(row)
+				# Check if subject has a proper value. If none than something went
+				# wrong while trying to fetch the subject from the row. So instead
+				# of failing and raising an error. We silently skip the event.
+				if subject:
+					event.append_subject(subject)
+					event = self.extensions.apply_get_hooks(event, sender)
+					if event is not None:
+						for n in id_hash[event.id]:
+							# insert the event into all necessary spots (LP: #673916)
+							sorted_events[n] = event
 				
 		log.debug("Got %d events in %fs" % (len(sorted_events), time.time()-t))
 		return sorted_events