Merge lp:~kamstrup/zeitgeist/blacklist into lp:zeitgeist/0.1

[Mikkel Kamstrup Erlandsen]

[Mikkel Kamstrup Erlandsen (kamstrup)]

I think the blacklisting framework is good to go in now. Some thing worth mentioning:

 * I added a DEFAULT_EXTENSIONS var in _zeitgeist.engine with a list of class names to load by default. This names are string encoded in order to be able to pull them from an environment var in the future

 * I use a pickled format to store the blacklist in and not a human readable format like fx. JSON, for a reason. Before we completely seal off the API I don't want an on disk format people can tinker with

 * I disable all extensions in the resonance unit tests because extensions can hold state we can not reset a priori (extensions can do anything mind you). Fx. there seems to be a bug in Python-DBus making it impossible to unexport the DBus object the Blacklist extension exports...

 * I probably forgot something I should have mentioned here. Forgive me :-)

[lp:~kamstrup/zeitgeist/blacklist]

1244. By Mikkel Kamstrup Erlandsen on 2009-12-29

Add a test to the blacklist extension to check that events are indeed filtered/let through correctly

[Markus Korn (thekorn)]

Great to have this blacklist framework in a mergable state, however some first comments (I did not run the code yet)

* docstring in load_class(): we are working on zeitgeist and not on zeitgesit ;)
* statement to unload() all extension: list(self.__extensions.iterkeys()) is looking strange, why not self.__extensions.keys() ?
* the popo code made me smile ;) but it is somehow not looking right,
  ** creating a list containing three None objects is looking strange, but ok. I think
  popo = list()
  popo.append(map(unicode, ev[0]))
  [...]
     is looking much nicer
  ** the for loop looks wrong, I think the body must popo[1][i] = map(unicode, subj)

  I think something like this is more readable:

>>> popo = list()
>>> popo.append(map(unicode, ev[0]))
>>> popo.append([map(unicode, subj) for subj in ev[1]])
>>> popo.append(str(ev[2]))

review: Needs Fixing (diff)

[Mikkel Kamstrup Erlandsen (kamstrup)]

> Great to have this blacklist framework in a mergable state, however some first
> comments (I did not run the code yet)
>
> * docstring in load_class(): we are working on zeitgeist and not on zeitgesit

Fixed

> * statement to unload() all extension: list(self.__extensions.iterkeys()) is
> looking strange, why not self.__extensions.keys() ?

This is to avoid a concurrent modification of the key set while we are iterating over it - maybe there is a nicer way..?

> * the popo code made me smile ;) but it is somehow not looking right,
> ** creating a list containing three None objects is looking strange, but ok.
> I think
> popo = list()
> popo.append(map(unicode, ev[0]))
> [...]
> is looking much nicer
> ** the for loop looks wrong, I think the body must popo[1][i] =
> map(unicode, subj)
>
> I think something like this is more readable:
>
> >>> popo = list()
> >>> popo.append(map(unicode, ev[0]))
> >>> popo.append([map(unicode, subj) for subj in ev[1]])
> >>> popo.append(str(ev[2]))

I changed it to use your proposal here. I think you are also right there was a bug in the for loop. I'll add an extra unit test for this later.

Thanks for the review so far

[lp:~kamstrup/zeitgeist/blacklist]

1245. By Mikkel Kamstrup Erlandsen on 2009-12-29

Updates reflecting Markus' comments on LP review

1246. By Mikkel Kamstrup Erlandsen on 2009-12-31

Add needed autofoo magic to include the new blacklist extension files in the distribution tarball

[Siegfried Gevatter (rainct)]

Committed.

------------------------------------------------------------------
revision <email address hidden> (1251)
Author: Siegfried-Angel Gevatter Pujals <email address hidden>
Date: Mon 2010-01-04 20:10:40 +0100
Branch: zeitgeist-trunk
Bugs: https://launchpad.net/bugs/447417 fixed

    Merge with lp:~kamstrup/zeitgeist/blacklist

     - Add Mikkel Kamstrup's implementation of a blacklist,
       together with the needed changes to the extensions system.

     Good work Mikkel!

    Some changes from my side:

     - Add a .constants namespace to __init__.py and move several
       constants into there. They can be used both by Zeitgeist
       itself and by extensions (eg. DATA_PATH).

     - Fix test/engine-extension-test.py so that it doesn't break
       now that there are extensions loaded by default.

     - Some other little tweaks.
------------------------------------------------------------------

I have some concerns but we can discuss further changes as bugs.

Good work!

review: Approve

=== modified file '_zeitgeist/engine/Makefile.am'
--- _zeitgeist/engine/Makefile.am	2009-11-30 23:22:36 +0000
+++ _zeitgeist/engine/Makefile.am	2009-12-31 09:11:14 +0000
@@ -1,3 +1,5 @@
+SUBDIRS = extensions
+
 appdir = $(prefix)/share/zeitgeist/_zeitgeist/engine/
 
 app_PYTHON = \

=== modified file '_zeitgeist/engine/__init__.py'
--- _zeitgeist/engine/__init__.py	2009-11-27 13:11:52 +0000
+++ _zeitgeist/engine/__init__.py	2009-12-31 09:11:14 +0000
@@ -32,6 +32,8 @@
 AVAILABLE_ENGINES = ["resonance"]
 ENGINE_FALLBACK = AVAILABLE_ENGINES[0]
 
+DEFAULT_EXTENSIONS = ["_zeitgeist.engine.extensions.blacklist.Blacklist"]
+
 def get_engine_type():
 	""" Returns the value of the $ZEITGEIST_ENGINE environment variable or,
 	if it isn't defined, the default engine."""
@@ -60,7 +62,7 @@
 	
 	# See if we can reuse _engine
 	if _engine and not _engine.is_closed():
-		running_type = _engine.__module__.split(".").pop().lower()
+		running_type = _engine.__module__.split(".").pop().lower().rpartition("_")[0]
 		if not running_type == engine_type:
 			raise RuntimeError(
 				("There is already a zeitgeist engine running. But this "

=== modified file '_zeitgeist/engine/extension.py'
--- _zeitgeist/engine/extension.py	2009-12-30 19:03:24 +0000
+++ _zeitgeist/engine/extension.py	2009-12-31 09:11:14 +0000
@@ -17,6 +17,10 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import logging
+logging.basicConfig(level=logging.DEBUG)
+log = logging.getLogger("zeitgeist.extension")
+
 class Extension(object):
 	""" Base class for all extensions
 	
@@ -72,7 +76,29 @@
 		    should see it
 		"""
 		return event
-	
+
+def load_class(path):
+	"""
+	Load and return a class from a fully qualified string.
+	Fx. "_zeitgeist.engine.extensions.myext.MyClass"
+	"""
+	module, dot, cls_name = path.rpartition(".")
+	parts = module.split(".")
+	module = __import__(module)
+	for part in parts[1:]:
+		try:
+			module = getattr(module, part)
+		except AttributeError:
+			raise ImportError(
+			  "No such submodule '%s' when loading %s" % (part, path))
+	
+	try:
+		cls = getattr(module, cls_name)
+	except AttributeError:
+		raise ImportError("No such class '%s' in module %s" % (cls_name,path))
+	
+	return cls
+
 class ExtensionsCollection(object):
 	""" Collection to manage all extensions """
 	
@@ -88,6 +114,7 @@
 		return "%s(%r)" %(self.__class__.__name__, sorted(self.__methods.keys()))
 			
 	def load(self, extension):
+		log.debug("Loading extensions '%s'" % extension.__name__)
 		if not issubclass(extension, Extension):
 			raise TypeError(
 				"Unable to load %r, all extensions have to be subclasses of %r" %(extension, Extension)
@@ -99,11 +126,25 @@
 			self._register_method(method, getattr(obj, method))
 		self.__extensions[obj.__class__.__name__] = obj
 		
-	def unload(self, extension):
-		obj = self.__extensions[extension.__name__]
-		for method in obj.PUBLIC_METHODS:
-			del self.methods[method]
-		del self.__extensions[extension.__name__]
+	def unload(self, extension=None):
+		"""
+		Unload a specified extension or unload all extensions if
+		no extension is given
+		"""
+		if extension is None:
+			log.debug("Unloading all extensions")
+			
+			# We need to clone the key list to avoid concurrent
+			# modification of the extension dict
+			for ext_name in list(self.__extensions.iterkeys()):
+				self.unload(self.__extensions[ext_name])
+		else:
+			log.debug("Unloading extension '%s'" \
+			          % extension.__class__.__name__)
+			obj = self.__extensions[extension.__class__.__name__]
+			for method in obj.PUBLIC_METHODS:
+				del self.methods[method]
+			del self.__extensions[extension.__class__.__name__]
 	
 	def apply_get_hooks(self, event):
 		# Apply extension filters if we have an event

=== added directory '_zeitgeist/engine/extensions'
=== added file '_zeitgeist/engine/extensions/Makefile.am'
--- _zeitgeist/engine/extensions/Makefile.am	1970-01-01 00:00:00 +0000
+++ _zeitgeist/engine/extensions/Makefile.am	2009-12-31 09:11:14 +0000
@@ -0,0 +1,5 @@
+appdir = $(prefix)/share/zeitgeist/_zeitgeist/engine/extensions
+
+app_PYTHON = \
+	__init__.py \
+	blacklist.py

=== added file '_zeitgeist/engine/extensions/blacklist.py'
--- _zeitgeist/engine/extensions/blacklist.py	1970-01-01 00:00:00 +0000
+++ _zeitgeist/engine/extensions/blacklist.py	2009-12-31 09:11:14 +0000
@@ -0,0 +1,100 @@
+# -.- coding: utf-8 -.-
+
+# Zeitgeist
+#
+# Copyright © 2009 Mikkel Kamstrup Erlandsen <[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
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import pickle
+import dbus
+import dbus.service
+from xdg import BaseDirectory
+from zeitgeist.datamodel import Event
+from _zeitgeist.engine.extension import Extension
+
+import logging
+logging.basicConfig(level=logging.DEBUG)
+log = logging.getLogger("zeitgeist.blacklist")
+
+CONFIG_FILE = os.path.join(BaseDirectory.save_config_path("zeitgeist"),
+                           "blacklist.pickle")
+
+DBUS_OBJECT_PATH = "/org/gnome/zeitgeist/blacklist"
+DBUS_INTERFACE = "org.gnome.zeitgeist.Blacklist"
+SIG_EVENT="asaasay"
+
+def _event2popo(ev):
+	"""
+	Ensure that an Event instance is a Plain Old Python Object (popo)
+	without DBus wrappings etc.
+	"""
+	popo = list()
+	popo.append(map(unicode, ev[0]))
+	popo.append([map(unicode, subj) for subj in ev[1]])
+	popo.append(str(ev[2]))
+	return popo
+
+class Blacklist(Extension, dbus.service.Object):
+	PUBLIC_METHODS = ["set_blacklist", "get_blacklist"]
+	
+	def __init__ (self, engine):
+		Extension.__init__(self, engine)
+		dbus.service.Object.__init__(self, dbus.SessionBus(),
+		                             DBUS_OBJECT_PATH)
+		if os.path.exists(CONFIG_FILE):
+			try:
+				raw_blacklist = pickle.load(file(CONFIG_FILE))
+				self._blacklist = map(Event, raw_blacklist)
+				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 = []
+		else:
+			log.debug("No existing blacklist config found")
+			self._blacklist = []
+	
+	def insert_event_hook(self, event):
+		for tmpl in self._blacklist:
+			if event.matches_template(tmpl) : return None
+		return event
+	
+	# PUBLIC
+	def set_blacklist(self, event_templates):
+		self._blacklist = event_templates
+		map(Event._make_dbus_sendable, self._blacklist)
+		
+		out = file(CONFIG_FILE, "w")
+		pickle.dump(map(_event2popo, self._blacklist), out)		
+		out.close()
+		log.debug("Blacklist updated: %s" % self._blacklist)
+	
+	# PUBLIC
+	def get_blacklist(self):
+		return self._blacklist
+	
+	@dbus.service.method(DBUS_INTERFACE,
+	                     in_signature="a("+SIG_EVENT+")")
+	def SetBlacklist(self, event_templates):
+		tmp = map(Event, event_templates)
+		self.set_blacklist(tmp)
+		
+	@dbus.service.method(DBUS_INTERFACE,
+	                     in_signature="", out_signature="a("+SIG_EVENT+")")
+	def GetBlacklist(self):
+		return self.get_blacklist()
+	

=== modified file '_zeitgeist/engine/resonance_engine.py'
--- _zeitgeist/engine/resonance_engine.py	2009-12-30 18:24:15 +0000
+++ _zeitgeist/engine/resonance_engine.py	2009-12-31 09:11:14 +0000
@@ -28,7 +28,7 @@
 import gettext
 import logging
 
-from extension import ExtensionsCollection
+from extension import ExtensionsCollection, load_class
 
 from zeitgeist.datamodel import Subject, Event, StorageState, TimeRange, \
 	ResultType, get_timestamp_for_now
@@ -290,8 +290,10 @@
 			raise RuntimeError("old database version")
 		
 		# Load extensions
-		# Right now we don't load any default extension
-		self.__extensions = ExtensionsCollection(self)
+		default_extensions = \
+		           map(load_class, _zeitgeist.engine.DEFAULT_EXTENSIONS)
+		self.__extensions = ExtensionsCollection(
+		                              self, defaults=default_extensions)
 		
 		self._interpretation = TableLookup(cursor, "interpretation")
 		self._manifestation = TableLookup(cursor, "manifestation")
@@ -304,6 +306,7 @@
 	
 	def close(self):
 		global _cursor
+		self.extensions.unload()
 		self._cursor.connection.close()
 		self._cursor = _cursor = None
 	

=== modified file 'configure.ac'
--- configure.ac	2009-11-27 20:32:54 +0000
+++ configure.ac	2009-12-31 09:11:14 +0000
@@ -23,5 +23,6 @@
 	_zeitgeist/loggers/Makefile
 	_zeitgeist/loggers/datasources/Makefile
 	_zeitgeist/engine/Makefile
+	_zeitgeist/engine/extensions/Makefile
 ])
 AC_OUTPUT

=== added file 'test/blacklist-test.py'
--- test/blacklist-test.py	1970-01-01 00:00:00 +0000
+++ test/blacklist-test.py	2009-12-31 09:11:14 +0000
@@ -0,0 +1,65 @@
+#!/usr/bin/python
+# -.- coding: utf-8 -.-
+
+# Update python path to use local zeitgeist module
+import sys
+import os
+import unittest
+import dbus
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
+from zeitgeist.datamodel import *
+from testutils import RemoteTestCase
+
+class BlacklistTest(RemoteTestCase):
+	
+	def __init__(self, methodName):
+		super(BlacklistTest, self).__init__(methodName)
+		self.blacklist = None
+	
+	def setUp(self):
+		# We set up the connection lazily in order to wait for the
+		# engine to come up
+		super(BlacklistTest, self).setUp()
+		obj = dbus.SessionBus().get_object("org.gnome.zeitgeist.Engine",
+		                                              "/org/gnome/zeitgeist/blacklist")
+		self.blacklist = dbus.Interface(obj, "org.gnome.zeitgeist.Blacklist")
+	
+	def testClear(self):
+		self.blacklist.SetBlacklist([])
+		empty = self.blacklist.GetBlacklist()
+		self.assertEquals(empty, [])
+		
+	def testSetOne(self):
+		orig = Event.new_for_values(interpretation=Interpretation.OPEN_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.assertEquals(result.manifestation, "")
+		self.assertEquals(result.interpretation, Interpretation.OPEN_EVENT)
+		self.assertEquals(len(result.subjects), 1)
+		self.assertEquals(result.subjects[0].uri, "http://nothingtoseehere.gov")
+		self.assertEquals(result.subjects[0].interpretation, "")
+	
+	def testApplyBlacklist(self):
+		self.testSetOne()
+		ev = Event()
+		ev.interpretation = Interpretation.OPEN_EVENT
+		ev.manifestation = Manifestation.USER_ACTIVITY
+		ev.actor = "app.//foo.desktop"
+		subj = ev.append_subject()
+		subj.uri = "http://nothingtoseehere.gov"
+		inserted_ids = self.insertEventsAndWait([ev])
+		self.assertEquals(1, len(inserted_ids))
+		self.assertEquals(0, inserted_ids[0])
+		
+		# Now change the event to pass the blacklist
+		subj.uri = "htpp://totallyvaliduri.com"
+		inserted_ids = self.insertEventsAndWait([ev])
+		self.assertEquals(1, len(inserted_ids))
+		self.assertTrue(0 != inserted_ids[0])
+if __name__ == "__main__":
+	unittest.main()

=== modified file 'test/resonance-engine-test.py'
--- test/resonance-engine-test.py	2009-12-30 17:34:24 +0000
+++ test/resonance-engine-test.py	2009-12-31 09:11:14 +0000
@@ -39,7 +39,14 @@
 	def setUp (self):
 		global test_event_1
 		test_event_1 = create_test_event_1()
+		
+		# Some extensions keep state around that interferes
+		# with the tests, so we disable all extensions
+		_zeitgeist.engine.DEFAULT_EXTENSIONS = []
+		
+		# Memory backed tmp DB
 		_zeitgeist.engine.DB_PATH = ":memory:"
+		
 		self.engine = create_engine()
 		
 	def tearDown (self):