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

Proposed by Seif Lotfy on 2011-01-03
Status: Rejected
Rejected by: Siegfried Gevatter on 2011-04-22
Proposed branch: lp:~manishsinha/zeitgeist/fix-blacklist-api
Merge into: lp:zeitgeist/0.1
Diff against target: 240 lines (+136/-27)
2 files modified
_zeitgeist/engine/extensions/blacklist.py (+112/-18)
test/blacklist-test.py (+24/-9)
To merge this branch: bzr merge lp:~manishsinha/zeitgeist/fix-blacklist-api
Reviewer Review Type Date Requested Status
Markus Korn Needs Information on 2011-01-03
Seif Lotfy Needs Fixing on 2011-01-03
Review via email: mp+45013@code.launchpad.net

Description of the change

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).

To post a comment you must log in.
Seif Lotfy (seif) wrote :

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) wrote :

needs fixing

review: Needs Fixing
Markus Korn (thekorn) wrote :

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) wrote :

> 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.

> 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) wrote :

> 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")])

> 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

> 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) wrote :

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?

> 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '_zeitgeist/engine/extensions/blacklist.py'
2--- _zeitgeist/engine/extensions/blacklist.py 2010-10-19 13:54:12 +0000
3+++ _zeitgeist/engine/extensions/blacklist.py 2011-01-03 00:36:07 +0000
4@@ -3,6 +3,7 @@
5 # Zeitgeist
6 #
7 # Copyright © 2009 Mikkel Kamstrup Erlandsen <mikkel.kamstrup@gmail.com>
8+# Copyright © 2010 Manish Sinha <manishsinha@ubuntu.com>
9 #
10 # This program is free software: you can redistribute it and/or modify
11 # it under the terms of the GNU Lesser General Public License as published by
12@@ -56,41 +57,105 @@
13 if os.path.exists(CONFIG_FILE):
14 try:
15 raw_blacklist = pickle.load(file(CONFIG_FILE))
16- self._blacklist = map(Event, raw_blacklist)
17+ self._blacklist = {}
18+ for key in raw_blacklist:
19+ self._blacklist[key] = map(Event, raw_blacklist[key])
20 log.debug("Loaded blacklist config from %s"
21 % CONFIG_FILE)
22 except Exception, e:
23 log.warn("Failed to load blacklist config file %s: %s"\
24 % (CONFIG_FILE, e))
25- self._blacklist = []
26+ self._blacklist = {}
27 else:
28 log.debug("No existing blacklist config found")
29- self._blacklist = []
30-
31+ self._blacklist = {}
32+
33 def pre_insert_event(self, event, sender):
34 for tmpl in self._blacklist:
35- if event.matches_template(tmpl): return None
36+ for single_event in self._blacklist[tmpl]:
37+ if event.matches_template(single_event): return None
38 return event
39
40 # PUBLIC
41- def set_blacklist(self, event_templates):
42- self._blacklist = event_templates
43- map(Event._make_dbus_sendable, self._blacklist)
44-
45+ def set_blacklist(self, event_templates, blacklister):
46+ try:
47+ event_list = self._blacklist[blacklister]
48+ except KeyError:
49+ event_list = []
50+ self._blacklist[blacklister] = event_list
51+
52+ map(Event._make_dbus_sendable, event_templates)
53+
54+ # Create a list to hold non-duplicate events
55+ new_templates = []
56+ if len(event_list) > 0:
57+ for candidate_temp in event_list:
58+ for event in event_templates:
59+ # If the event is already in the list
60+ if event.matches_template(candidate_temp):
61+ log.debug("Duplicate blacklist template found. Skipping")
62+ continue
63+ # The event is new, insert it to the blacklist`
64+ new_templates.append(event)
65+ else:
66+ new_templates.extend(event_templates)
67+
68+ event_list.extend(new_templates)
69+
70+ # Create plain python objects and pickel it
71+ self._create_plain_and_save()
72+
73+ return new_templates
74+
75+ def remove_blacklist(self, event_templates, blacklister):
76+ try:
77+ event_list = self._blacklist[blacklister]
78+ except KeyError:
79+ log.debug("No such blacklister exists")
80+ return []
81+
82+ map(Event._make_dbus_sendable, event_templates)
83+
84+ # Create a list to hold the event templates which exist
85+ candidate_template = []
86+ for event in event_templates:
87+ for template in event_list:
88+ # If the event is in the list, add it to candidate list
89+ if event.matches_template(template):
90+ candidate_template.append(template)
91+
92+ # Remove all the candidate event templates
93+ for tempel in candidate_template:
94+ event_list.remove(tempel)
95+
96+ self._create_plain_and_save()
97+
98+ return candidate_template
99+
100+ def _create_plain_and_save(self):
101+ blacklist_dict = {}
102+ for b_actor in self._blacklist:
103+ conv_event = map(Event.get_plain, self._blacklist[b_actor])
104+ blacklist_dict[str(b_actor)] = conv_event
105+
106 out = file(CONFIG_FILE, "w")
107- pickle.dump(map(Event.get_plain, self._blacklist), out)
108+ pickle.dump(blacklist_dict, out)
109 out.close()
110- log.debug("Blacklist updated: %s" % self._blacklist)
111+ log.debug("Blacklist updated: %s" % blacklist_dict)
112
113 # PUBLIC
114 def get_blacklist(self):
115- return self._blacklist
116+ event_list = []
117+ for blacklister in self._blacklist:
118+ event_list.append([blacklister, self._blacklist[blacklister]])
119+
120+ return event_list
121
122 @dbus.service.method(BLACKLIST_DBUS_INTERFACE,
123- in_signature="a("+constants.SIG_EVENT+")")
124- def SetBlacklist(self, event_templates):
125+ in_signature="a("+constants.SIG_EVENT+")s")
126+ def AddTemplates(self, event_templates, blacklister):
127 """
128- Set the blacklist to :const:`event_templates`. Events
129+ Add a new blacklist template :const:`event_templates`. Events
130 matching any these templates will be blocked from insertion
131 into the log. It is still possible to find and look up events
132 matching the blacklist which was inserted before the blacklist
133@@ -98,14 +163,17 @@
134
135 :param event_templates: A list of
136 :class:`Events <zeitgeist.datamodel.Event>`
137+ :param blacklister: The unique identifier of the
138+ application setting the blacklist
139 """
140 tmp = map(Event, event_templates)
141- self.set_blacklist(tmp)
142+ added_templates = self.set_blacklist(tmp, blacklister)
143+ self.Changed(added_templates, blacklister)
144
145 @dbus.service.method(BLACKLIST_DBUS_INTERFACE,
146 in_signature="",
147- out_signature="a("+constants.SIG_EVENT+")")
148- def GetBlacklist(self):
149+ out_signature="a(sa("+constants.SIG_EVENT+"))")
150+ def GetTemplates(self):
151 """
152 Get the current blacklist templates.
153
154@@ -113,3 +181,29 @@
155 :class:`Events <zeitgeist.datamodel.Event>`
156 """
157 return self.get_blacklist()
158+
159+ @dbus.service.method(BLACKLIST_DBUS_INTERFACE,
160+ in_signature="a("+constants.SIG_EVENT+")s")
161+ def RemoveTemplates(self, event_templates, blacklister):
162+ """
163+ Remove the blacklist templates :const: `event_templates`.
164+ The event templates specified would be removed for the
165+ specific blacklister. When the removal of template is
166+ successful, then event matching these templates can be
167+ inserted.
168+
169+ :param event_templates: A list of
170+ :class: `Events <zeitgeist.datamodel.Event>`
171+ :param blacklister: The unique identiier of the
172+ application setting the blacklist. The event
173+ templates which are to be removed and searched
174+ against this blacklister
175+ """
176+ tmp = map(Event, event_templates)
177+ removed_templates = self.remove_blacklist(tmp, blacklister)
178+ self.Changed(removed_templates, blacklister)
179+
180+ @dbus.service.signal(BLACKLIST_DBUS_INTERFACE,
181+ signature="a("+constants.SIG_EVENT+")s")
182+ def Changed(self, event_templates, blacklister):
183+ pass
184
185=== modified file 'test/blacklist-test.py'
186--- test/blacklist-test.py 2010-09-22 19:21:19 +0000
187+++ test/blacklist-test.py 2011-01-03 00:36:07 +0000
188@@ -29,18 +29,28 @@
189 self.blacklist = dbus.Interface(obj, "org.gnome.zeitgeist.Blacklist")
190
191 def testClear(self):
192- self.blacklist.SetBlacklist([])
193- empty = self.blacklist.GetBlacklist()
194+ blks = self.blacklist.GetTemplates()
195+ for entry in blks:
196+ blacklists = map(Event.get_plain, entry[1])
197+ self.blacklist.RemoveTemplates(str(entry[0]), blacklists)
198+ empty = self.blacklist.GetTemplates()
199 self.assertEquals(empty, [])
200
201 def testSetOne(self):
202+ blacklister = "application://zeitgeist.unittest"
203 orig = Event.new_for_values(interpretation=Interpretation.ACCESS_EVENT,
204 subject_uri="http://nothingtoseehere.gov")
205- self.blacklist.SetBlacklist([orig])
206- result = map(Event, self.blacklist.GetBlacklist())
207-
208- self.assertEquals(len(result), 1)
209- result = result[0]
210+ self.blacklist.AddTemplates([orig,], blacklister)
211+ allblks = self.blacklist.GetTemplates()
212+ blks = {}
213+ for blk in allblks:
214+ blks[str(blk[0])] = map(Event.new_for_struct, blk[1])
215+
216+ self.assertEquals(len(blks.keys()), 1)
217+ self.assertEquals(blks.has_key(blacklister), True)
218+ blacklists = blks[blacklister]
219+ self.assertEquals(len(blacklists), 1)
220+ result = blacklists[0]
221 self.assertEquals(result.manifestation, "")
222 self.assertEquals(result.interpretation, Interpretation.ACCESS_EVENT)
223 self.assertEquals(len(result.subjects), 1)
224@@ -73,9 +83,14 @@
225 del self.blacklist
226 iface = ZeitgeistDBusInterface()
227 blacklist = iface.get_extension("Blacklist", "blacklist")
228- blacklist.SetBlacklist([])
229- empty = blacklist.GetBlacklist()
230+
231+ blks = blacklist.GetTemplates()
232+ for entry in blks:
233+ blacklists = map(Event.get_plain, entry[1])
234+ blacklist.RemoveTemplates(str(entry[0]), blacklists)
235+ empty = blacklist.GetTemplates()
236 self.assertEquals(empty, [])
237+
238
239 if __name__ == "__main__":
240 unittest.main()

Subscribers

People subscribed via source and target branches