Merge lp:~manishsinha/zeitgeist/fix-blacklist-api into lp:zeitgeist/0.1

[Seif Lotfy]

Manish took the initiative to reimplemented the blacklist according to https://bugs.launchpad.net/zeitgeist/+bug/612344
All test cases work and I think we should start reviewing the code here (i know its not the purpose of a merge request).

[Seif Lotfy (seif)]

line 15-19 in the patch is unnecessary it can be replaced at line 15 with
    ---
    self._blacklist = pickle.load(file(CONFIG_FILE))
    ---

line 52 in the patch can be removed and is not needed, since internally we do not need to make event dbus send-able unless we are sending them out.

line 82 in the patch can be removed and is not needed, since internally we do not need to make event dbus send-able unless we are sending them out.

[Seif Lotfy (seif)]

needs fixing

review: Needs Fixing

[Markus Korn (thekorn)]

I'm exited to see some progress on the blacklist API front, thanks Manish and Seif.
But, to be honest, I don't see which direction this API takes. First of all, in the bug Seif linked we discussed many different approaches, and for me it is not clear which one is implemented here.
And secondly, if I interpret Manish's comment at [0] as final decision (which is perfectly fine with me), I see that the implementation in this branch is completely different

* Changed-signal has signature a(E)s and not sa(E)
* GetTemplates is not returning a dictionary a{sa(E)} but a(sa(E))

Also, when getting a Changed Signal, how do I know what the content means? Is it an added or removed Blacklist?

And one last comment: IMHO pickle sucks, have you considered using JSON, as templates are perfect JSON objects.

[0] https://bugs.launchpad.net/zeitgeist/+bug/612344/comments/18

review: Needs Information

[Seif Lotfy (seif)]

> I'm exited to see some progress on the blacklist API front, thanks Manish and
> Seif.
> But, to be honest, I don't see which direction this API takes. First of all,
> in the bug Seif linked we discussed many different approaches, and for me it
> is not clear which one is implemented here.
> And secondly, if I interpret Manish's comment at [0] as final decision (which
> is perfectly fine with me), I see that the implementation in this branch is
> completely different
>
> * Changed-signal has signature a(E)s and not sa(E)
> * GetTemplates is not returning a dictionary a{sa(E)} but a(sa(E))

Yeah I noticed this while trying to build a UI tool for it. This needs to change, since its not what we agreed on and not what I had in mind.

>
> Also, when getting a Changed Signal, how do I know what the content means? Is
> it an added or removed Blacklist?

I think we need 2 signals in that case for both situations

>
> And one last comment: IMHO pickle sucks, have you considered using JSON, as
> templates are perfect JSON objects.

Thank you for bringing this topic up. I agree that JSON is the way to go for us not only here but for the datasource registry since it will allow us to do easier debugging.

>
> [0] https://bugs.launchpad.net/zeitgeist/+bug/612344/comments/18

It is a good momentum generator now so lets keep on this.

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> But, to be honest, I don't see which direction this API takes. First of all,
> in the bug Seif linked we discussed many different approaches, and for me it
> is not clear which one is implemented here.

This is the one Mikkel proposed.

> And secondly, if I interpret Manish's comment at [0] as final decision (which
> is perfectly fine with me), I see that the implementation in this branch is
> completely different
>
> * Changed-signal has signature a(E)s and not sa(E)

Yeah. This is just a placement or parameters or dbus signature. Can be changed anytime.

> * GetTemplates is not returning a dictionary a{sa(E)} but a(sa(E))

Are you sure dictionary is a good way to go? I found the latter to be convenient.

>
> Also, when getting a Changed Signal, how do I know what the content means? Is
> it an added or removed Blacklist?

Same thought from my side. When I was implementing, I was wondering about this. Left this for discussion during the review as the change doesn't involve huge code overhaul.

> And one last comment: IMHO pickle sucks, have you considered using JSON, as
> templates are perfect JSON objects.

I just carry forwarded what already was there. Didn't change this part. Still, it wont be a huge change in the codebase.

[Markus Korn (thekorn)]

> It is a good momentum generator now so lets keep on this.

I hate your momentum generators, they make me thinking about problems like this in an endless loop ;)

Can someone please summarize for me why we want this string identifier 'blacklister' for the client setting a certain blacklist? And why is it not an identifier for the blacklist template itself?
In my understanding it's purely informational, so why do we group the templates internally by this string, instead we could just maintain templates internally as
   blacklists = set([(<template1>, "client1"), (<template2>, "client1"), (<template3>, "client2")])

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> line 15-19 in the patch is unnecessary it can be replaced at line 15 with
> ---
> self._blacklist = pickle.load(file(CONFIG_FILE))
> ---

Will check when I reach home

> line 52 in the patch can be removed and is not needed, since internally we do
> not need to make event dbus send-able unless we are sending them out.

Agree

> line 82 in the patch can be removed and is not needed, since internally we do
> not need to make event dbus send-able unless we are sending them out.

Agree

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> Can someone please summarize for me why we want this string identifier
> 'blacklister' for the client setting a certain blacklist?

Ask Mikkel. IIRC he proposed the idea.

> And why is it not an identifier for the blacklist template itself?

I think, since the blacklist template is an event itself which has an id, then that id can act as an identifier for the template.

> In my understanding it's purely informational, so why do we group the
> templates internally by this string, instead we could just maintain templates
> internally as
> blacklists = set([(<template1>, "client1"), (<template2>, "client1"),
> (<template3>, "client2")])

I still find having a dictionary a better idea. Keys are blacklister and the value contains the blacklist templates set by them.

[Siegfried Gevatter (rainct)]

A quick comment: How's the Changed signal supposed to work, ie. how do you know if the templates are being added or removed? Why not have two separated signals (TemplatesAdded, TemplatesRemoved)?

I also don't buy this "blacklister" stuff. What's the point of it?

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> A quick comment: How's the Changed signal supposed to work, ie. how do you
> know if the templates are being added or removed? Why not have two separated
> signals (TemplatesAdded, TemplatesRemoved)?

+1 for this idea.

> I also don't buy this "blacklister" stuff. What's the point of it?

Well, that is for knowing which application set the blacklist. Kamstrup proposed that idea

Unmerged revisions

1655. By Manish Sinha (मनीष सिन्हा) on 2011-01-02

Fixed unit tests and cleaned the blacklist extension

1654. By Manish Sinha (मनीष सिन्हा) on 2011-01-02

Rebase from trunk

1653. By Manish Sinha (मनीष सिन्हा) on 2011-01-02

Fix the logical error when comparing blacklist templates

1652. By Manish Sinha (मनीष सिन्हा) on 2010-12-28

Fixed blacklist-test.py's 2/4 tests - testClear and testBlacklistUingClientDBusInterface

In testSetOne even though blacklist is being set, it cannot be retreived.
testAplyBlacklist fails becuase testSetOne fails

1651. By Manish Sinha (मनीष सिन्हा) on 2010-12-28

Added RemoveTemplate and fixed all the names and functionality

1650. By Manish Sinha (मनीष सिन्हा) on 2010-12-21

Got the Blacklist extension working. It can query all blacklists, set blacklists and block events which match the blacklists

1649. By Manish Sinha (मनीष सिन्हा) on 2010-12-16

Commit to exhibit that bug 691167 bug is real

=== modified file '_zeitgeist/engine/extensions/blacklist.py'
--- _zeitgeist/engine/extensions/blacklist.py	2010-10-19 13:54:12 +0000
+++ _zeitgeist/engine/extensions/blacklist.py	2011-01-03 00:36:07 +0000
@@ -3,6 +3,7 @@
 # Zeitgeist
 #
 # Copyright © 2009 Mikkel Kamstrup Erlandsen <[e-mail]>
+# Copyright © 2010 Manish Sinha <[e-mail]>
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Lesser General Public License as published by
@@ -56,41 +57,105 @@
 		if os.path.exists(CONFIG_FILE):
 			try:
 				raw_blacklist = pickle.load(file(CONFIG_FILE))
-				self._blacklist = map(Event, raw_blacklist)
+				self._blacklist = {}
+				for key in raw_blacklist:
+					self._blacklist[key] = map(Event, raw_blacklist[key])
 				log.debug("Loaded blacklist config from %s"
 				          % CONFIG_FILE)
 			except Exception, e:
 				log.warn("Failed to load blacklist config file %s: %s"\
 				         % (CONFIG_FILE, e))
-				self._blacklist = []
+				self._blacklist = {}
 		else:
 			log.debug("No existing blacklist config found")
-			self._blacklist = []
-	
+			self._blacklist = {}
+		
 	def pre_insert_event(self, event, sender):
 		for tmpl in self._blacklist:
-			if event.matches_template(tmpl): return None
+			for single_event in self._blacklist[tmpl]:
+				if event.matches_template(single_event): return None
 		return event
 	
 	# PUBLIC
-	def set_blacklist(self, event_templates):
-		self._blacklist = event_templates
-		map(Event._make_dbus_sendable, self._blacklist)
-		
+	def set_blacklist(self, event_templates, blacklister):
+		try:
+			event_list = self._blacklist[blacklister]
+		except KeyError:
+			event_list = []
+			self._blacklist[blacklister] = event_list
+
+		map(Event._make_dbus_sendable, event_templates)
+
+		# Create a list to hold non-duplicate events
+		new_templates = []
+		if len(event_list) > 0:
+			for candidate_temp in event_list:
+				for event in event_templates:
+					# If the event is already in the list
+					if event.matches_template(candidate_temp):
+						log.debug("Duplicate blacklist template found. Skipping")
+						continue
+					# The event is new, insert it to the blacklist`
+					new_templates.append(event)
+		else:
+			new_templates.extend(event_templates)
+
+		event_list.extend(new_templates)
+		
+		# Create plain python objects and pickel it
+		self._create_plain_and_save()
+
+		return new_templates
+
+	def remove_blacklist(self, event_templates, blacklister):
+		try:
+			event_list = self._blacklist[blacklister]
+		except KeyError:
+			log.debug("No such blacklister exists")
+			return []
+
+		map(Event._make_dbus_sendable, event_templates)
+
+		# Create a list to hold the event templates which exist
+		candidate_template = []
+		for event in event_templates:
+			for template in event_list:
+				# If the event is in the list, add it to candidate list
+				if event.matches_template(template):
+					candidate_template.append(template)
+		
+		# Remove all the candidate event templates
+		for tempel in candidate_template:
+			event_list.remove(tempel)
+
+		self._create_plain_and_save()
+
+		return candidate_template
+
+	def _create_plain_and_save(self):
+		blacklist_dict = {}
+		for b_actor in self._blacklist:
+			conv_event = map(Event.get_plain, self._blacklist[b_actor])
+			blacklist_dict[str(b_actor)] = conv_event
+
 		out = file(CONFIG_FILE, "w")
-		pickle.dump(map(Event.get_plain, self._blacklist), out)		
+		pickle.dump(blacklist_dict, out)
 		out.close()
-		log.debug("Blacklist updated: %s" % self._blacklist)
+		log.debug("Blacklist updated: %s" % blacklist_dict)
 	
 	# PUBLIC
 	def get_blacklist(self):
-		return self._blacklist
+		event_list = []
+		for blacklister in self._blacklist:
+			event_list.append([blacklister, self._blacklist[blacklister]])
+
+		return event_list
 	
 	@dbus.service.method(BLACKLIST_DBUS_INTERFACE,
-	                     in_signature="a("+constants.SIG_EVENT+")")
-	def SetBlacklist(self, event_templates):
+	                     in_signature="a("+constants.SIG_EVENT+")s")
+	def AddTemplates(self, event_templates, blacklister):
 		"""
-		Set the blacklist to :const:`event_templates`. Events
+		Add a new blacklist template :const:`event_templates`. Events
 		matching any these templates will be blocked from insertion
 		into the log. It is still possible to find and look up events
 		matching the blacklist which was inserted before the blacklist
@@ -98,14 +163,17 @@
 		
 		:param event_templates: A list of
 		    :class:`Events <zeitgeist.datamodel.Event>`
+        :param blacklister: The unique identifier of the
+              application setting the blacklist
 		"""
 		tmp = map(Event, event_templates)
-		self.set_blacklist(tmp)
+		added_templates = self.set_blacklist(tmp, blacklister)
+		self.Changed(added_templates, blacklister)
 		
 	@dbus.service.method(BLACKLIST_DBUS_INTERFACE,
 	                     in_signature="",
-	                     out_signature="a("+constants.SIG_EVENT+")")
-	def GetBlacklist(self):
+	                     out_signature="a(sa("+constants.SIG_EVENT+"))")
+	def GetTemplates(self):
 		"""
 		Get the current blacklist templates.
 		
@@ -113,3 +181,29 @@
 		    :class:`Events <zeitgeist.datamodel.Event>`
 		"""
 		return self.get_blacklist()
+	
+	@dbus.service.method(BLACKLIST_DBUS_INTERFACE, 
+						in_signature="a("+constants.SIG_EVENT+")s")
+	def RemoveTemplates(self, event_templates, blacklister):
+		"""
+		Remove the blacklist templates :const: `event_templates`.
+		The event templates specified would be removed for the 
+		specific blacklister. When the removal of template is 
+		successful, then event matching these templates can be 
+		inserted.
+
+		:param event_templates: A list of 
+			:class: `Events <zeitgeist.datamodel.Event>`
+		:param blacklister: The unique identiier of the 
+			application setting the blacklist. The event 
+			templates which are to be removed and searched 
+			against this blacklister
+		"""
+		tmp = map(Event, event_templates)
+		removed_templates = self.remove_blacklist(tmp, blacklister)
+		self.Changed(removed_templates, blacklister)
+	
+	@dbus.service.signal(BLACKLIST_DBUS_INTERFACE, 
+								signature="a("+constants.SIG_EVENT+")s")
+	def Changed(self, event_templates, blacklister):
+		pass

=== modified file 'test/blacklist-test.py'
--- test/blacklist-test.py	2010-09-22 19:21:19 +0000
+++ test/blacklist-test.py	2011-01-03 00:36:07 +0000
@@ -29,18 +29,28 @@
 		self.blacklist = dbus.Interface(obj, "org.gnome.zeitgeist.Blacklist")
 	
 	def testClear(self):
-		self.blacklist.SetBlacklist([])
-		empty = self.blacklist.GetBlacklist()
+		blks = self.blacklist.GetTemplates()
+		for entry in blks:
+			blacklists = map(Event.get_plain, entry[1])
+			self.blacklist.RemoveTemplates(str(entry[0]), blacklists)
+		empty = self.blacklist.GetTemplates()
 		self.assertEquals(empty, [])
 		
 	def testSetOne(self):
+		blacklister = "application://zeitgeist.unittest"
 		orig = Event.new_for_values(interpretation=Interpretation.ACCESS_EVENT,
 		                            subject_uri="http://nothingtoseehere.gov")
-		self.blacklist.SetBlacklist([orig])
-		result = map(Event, self.blacklist.GetBlacklist())
-		
-		self.assertEquals(len(result), 1)
-		result = result[0]
+		self.blacklist.AddTemplates([orig,], blacklister)
+		allblks = self.blacklist.GetTemplates()
+		blks = {}
+		for blk in allblks:
+			blks[str(blk[0])] = map(Event.new_for_struct, blk[1])
+	
+		self.assertEquals(len(blks.keys()), 1)
+		self.assertEquals(blks.has_key(blacklister), True)
+		blacklists = blks[blacklister]
+		self.assertEquals(len(blacklists), 1)
+		result = blacklists[0]
 		self.assertEquals(result.manifestation, "")
 		self.assertEquals(result.interpretation, Interpretation.ACCESS_EVENT)
 		self.assertEquals(len(result.subjects), 1)
@@ -73,9 +83,14 @@
 		del self.blacklist
 		iface = ZeitgeistDBusInterface()
 		blacklist = iface.get_extension("Blacklist", "blacklist")
-		blacklist.SetBlacklist([])
-		empty = blacklist.GetBlacklist()
+
+		blks = blacklist.GetTemplates()
+		for entry in blks:
+			blacklists = map(Event.get_plain, entry[1])
+			blacklist.RemoveTemplates(str(entry[0]), blacklists)
+		empty = blacklist.GetTemplates()
 		self.assertEquals(empty, [])
+	
 
 if __name__ == "__main__":
 	unittest.main()