Landscape Client

Code review comment for lp:~al-maisan/landscape-client/obsolete-results-v

  1. obsolete-results-v
  2. Merge into trunk
Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

On 04/30/2010 04:59 PM, Thomas Herve wrote:
> Review: Approve
> [5] Some pyflakes:
> landscape/broker/tests/test_exchange.py:1: 'os' imported but unused
> landscape/broker/tests/test_exchange.py:8: 'ExchangeStore' imported but unused
>
> [6] get_message_context should retrieve all the data from the database in one query, and pass it to the MessageContext __init__, thus removing the select in there.

Hello Thomas,

thanks for the suggestions above! I have revised the code accordingly.
Please see the attached incremental diff.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

obsolete-results-v-inc-4.diff
1=== modified file 'landscape/broker/exchangestore.py'
2--- landscape/broker/exchangestore.py 2010-04-30 08:36:15 +0000
3+++ landscape/broker/exchangestore.py 2010-04-30 18:11:31 +0000
4@@ -28,27 +28,18 @@
5 @param id: the database key value for this instance.
6 """
7
8- def __init__(self, db, id):
9+ def __init__(self, db, operation_id, secure_id, message_type, timestamp):
10 self._db = db
11- self.id = id
12-
13- cursor = db.cursor()
14- try:
15- cursor.execute(
16- "SELECT operation_id, secure_id, message_type, timestamp "
17- "FROM message_context WHERE id=?", (id,))
18- row = cursor.fetchone()
19- finally:
20- cursor.close()
21-
22- self.operation_id = row[0]
23- self.secure_id = row[1]
24- self.message_type = row[2]
25- self.timestamp = row[3]
26+ self.operation_id = operation_id
27+ self.secure_id = secure_id
28+ self.message_type = message_type
29+ self.timestamp = timestamp
30
31 @with_cursor
32 def remove(self, cursor):
33- cursor.execute("DELETE FROM message_context WHERE id=?", (self.id,))
34+ cursor.execute(
35+ "DELETE FROM message_context WHERE operation_id=?",
36+ (self.operation_id,))
37
38
39 class ExchangeStore(object):
40@@ -72,21 +63,21 @@
41 def add_message_context(
42 self, cursor, operation_id, secure_id, message_type):
43 """Add a L{MessageContext} with the given data."""
44+ params = (operation_id, secure_id, message_type, time.time())
45 cursor.execute(
46 "INSERT INTO message_context "
47 " (operation_id, secure_id, message_type, timestamp) "
48- " VALUES (?,?,?,?)",
49- (operation_id, secure_id, message_type, time.time()))
50- return MessageContext(self._db, cursor.lastrowid)
51+ " VALUES (?,?,?,?)", params)
52+ return MessageContext(self._db, *params)
53
54 @with_cursor
55 def get_message_context(self, cursor, operation_id):
56 """The L{MessageContext} for the given C{operation_id} or C{None}."""
57 cursor.execute(
58- "SELECT id FROM message_context WHERE operation_id=?",
59- (operation_id,))
60- result = cursor.fetchone()
61- return MessageContext(self._db, result[0]) if result else None
62+ "SELECT operation_id, secure_id, message_type, timestamp "
63+ "FROM message_context WHERE operation_id=?", (operation_id,))
64+ row = cursor.fetchone()
65+ return MessageContext(self._db, *row) if row else None
66
67 @with_cursor
68 def all_operation_ids(self, cursor):
69
70=== modified file 'landscape/broker/tests/test_exchange.py'
71--- landscape/broker/tests/test_exchange.py 2010-04-30 10:48:32 +0000
72+++ landscape/broker/tests/test_exchange.py 2010-04-30 15:54:23 +0000
73@@ -1,11 +1,9 @@
74-import os
75 from landscape import SERVER_API, CLIENT_API
76 from landscape.lib.persist import Persist
77 from landscape.lib.hashlib import md5
78 from landscape.lib.fetch import fetch_async
79 from landscape.schema import Message, Int
80 from landscape.broker.exchange import get_accepted_types_diff, MessageExchange
81-from landscape.broker.exchangestore import ExchangeStore
82 from landscape.broker.transport import FakeTransport
83 from landscape.broker.store import MessageStore
84 from landscape.broker.ping import Pinger

« Back to merge proposal