[MERGE/PQM] Add XMLRPC support
Jonathan Lange
jml at mumak.net
Tue Aug 5 23:39:27 BST 2008
On Tue, Aug 5, 2008 at 5:53 PM, Daniel Watkins
<daniel at daniel-watkins.co.uk> wrote:
> The attached adds XMLRPC submission support to PQM. It depends on my
> TestCaseWithQueue work for the tests.
>
> All that this patch provides over email is speedier feedback when
> something goes awry, you don't get anything more detailed than you do
> in the email response. I have further work that builds upon this that
> adds that in.
>
>
Hi Daniel,
I've decided to leap in and review your XMLRPC patch. I'm in a bit of a rush
so I've occasionally been terse -- please forgive me.
I hope this helps,
jml
> === modified file 'bin/pqm'
> --- bin/pqm 2008-07-17 02:03:01 +0000
> +++ bin/pqm 2008-07-31 06:38:30 +0000
> @@ -58,7 +58,7 @@
> from pqm.commandline import parse_command_line
> from pqm.errors import PQMCmdFailure, PQMException
> from pqm.lockfile import LockFile
> -from pqm.script import Command, read_email
> +from pqm.script import Command, get_email_string, read_email_from_file
>
>
> def dir_from_option(configp, option, default):
> @@ -104,7 +104,8 @@
> try:
> logger.info('trying script ' + script.filename)
> logname = os.path.join(logdir, os.path.basename(script.filename) + '.log')
> - (sender, subject, msg, sig) = read_email(logger, open(script.filename))
> + (sender, subject, msg, sig) = \
> + read_email_from_file(logger, open(script.filename))
I'd personally prefer breaking the line at the first paren after
read_email_from_file.
> if options.verify_sigs:
> sigid,siguid = verify_sig(
> script.getSender(), msg, sig, 0, logger, options.keyring)
> @@ -203,19 +204,10 @@
> def do_read_mode(logger, options):
> sender = None
> try:
> - (sender, subject, msg, sig) = read_email(logger)
> - if options.verify_sigs:
> - sigid, siguid = verify_sig(
> - sender, msg, sig, 1, logger, options.keyring)
> - open(transaction_file, 'a').write(sigid + '\n')
> - fname = 'patch.%d' % (time.time())
> - logger.info('new patch ' + fname)
> - f = open('tmp.' + fname, 'w')
> - f.write('From: ' + sender + '\n')
> - f.write('Subject: ' + subject + '\n')
> - f.write(string.join(re.split('\r?\n', msg), '\n')) # canonicalize line endings
> - f.close()
> - os.rename('tmp.' + fname, fname)
> + email = read_email_from_file(logger, sys.stdin)
> + email_string = get_email_string(logger, options, email)
> + queuedir = os.path.abspath('.')
This smells a bit to me.
Unfortunately, I don't really understand PQM well enough, so it may be the
right thing to do.
> + pqm.write_script(queuedir, email_string)
> except:
> if sender and mail_reply:
> server = smtplib.SMTP(mail_server)
>
> === modified file 'pqm/__init__.py'
> --- pqm/__init__.py 2008-07-17 10:09:34 +0000
> +++ pqm/__init__.py 2008-07-31 06:34:10 +0000
> @@ -26,6 +26,7 @@
> import stat
> import string
> import sys
> +import time
>
> from pqm.errors import PQMException, PQMTlaFailure
> from pqm.script import Script
> @@ -54,6 +55,16 @@
> sys.exit(1)
>
>
> +def write_script(queuedir, text):
> + fname = 'patch.%d' % (time.time(),)
> + tmp_name = os.path.join(queuedir, 'tmp.%s' % (fname,))
> + logger.info('new patch ' + fname)
> + f = open(tmp_name, 'w')
> + f.write(text)
> + f.close()
> + os.rename(tmp_name, os.path.join(queuedir, fname))
> +
> +
> def find_patches(queuedir, logger, branch_spec_handler, configp, options):
> patches=[]
> patches_re=re.compile('^patch\.\d+$')
>
> === modified file 'pqm/script.py'
> --- pqm/script.py 2008-07-17 02:03:01 +0000
> +++ pqm/script.py 2008-07-31 06:17:16 +0000
> @@ -36,11 +36,8 @@
> from pqm.errors import PQMCmdFailure, PQMException, PQMTlaFailure
>
>
> -def read_email(logger, file=None):
> +def get_email_structure(logger, msg):
> """Read an email and check it has the required structure for commands."""
> - if not file:
> - file = sys.stdin
> - msg = email.message_from_file(file)
> sender = msg['From']
> logger.info("recieved email from %s", sender)
> subject = msg['Subject']
> @@ -72,6 +69,36 @@
> return (sender, subject, msg.get_payload(), None)
>
>
> +def read_email_from_file(logger, file=None):
Docstring please.
> + if not file:
I'd rather 'if file is None'.
> + file = sys.stdin
> + msg = email.message_from_file(file)
> + return get_email_structure(logger, msg)
> +
> +
> +def read_email_from_string(logger, string):
Docstring please.
> + msg = email.message_from_string(string)
> + return get_email_structure(logger, msg)
> +
> +
> +def get_email_string(logger, options, email):
Docstring please!
> + (sender, subject, msg, sig) = email
> + if options.verify_sigs:
> + # TODO: Daniel Watkins, 2008-07-31
> + # Get rid of the imports from pqm module itself.
> + from pqm import verify_sig
> + sigid, siguid = verify_sig(sender, msg, sig, 1, logger,
> + options.keyring)
> + open(transaction_file, 'a').write(sigid + '\n')
> + # canonicalize line endings
> + body = '\n'.join(re.split('\r?\n', msg))
> + return ("From: %(sender)s\n"
> + "Subject: %(subject)s\n"
> + "%(body)s" % {'sender': sender,
> + 'subject': subject,
> + 'body': body,})
> +
> +
I'm not super clear what this last function is for, but in general I think
this is an improvement to the structure of the mail handling code.
> class Script(object):
> """A command script."""
>
> @@ -103,7 +130,7 @@
> # something I plan on breaking in an ad hoc manner right now. The
> # read_email and email.message_from_file and verify_sig functions
> # need rearranging and consolidating to allow this.
> - details = read_email(self.logger, open(self.filename))
> + details = read_email_from_file(self.logger, open(self.filename))
> (self.sender, self.subject, self.msg, self.sig) = details
> self.signing_email = None
> if self.verify_sigs:
>
> === modified file 'pqm/tests/__init__.py'
> --- pqm/tests/__init__.py 2008-07-22 17:02:03 +0000
> +++ pqm/tests/__init__.py 2008-08-01 00:42:41 +0000
> @@ -27,11 +27,12 @@
> class QueueSetup(object):
> """Setup a queue with mock messages in it."""
>
> - def __init__(self, processing=True, empty=False):
> + def __init__(self, processing=True, empty=False, verify_sigs=True):
> self.configFileName = "Foo"
> self.cwd = os.path.abspath(os.path.curdir)
> self.processing = processing
> self.empty = empty
> + self.verify_sigs = verify_sigs
> if not empty:
> self.message = sample_message
> self.message_bad = sample_message_bad
> @@ -42,10 +43,16 @@
> myFile.write(dedent("""\
> [DEFAULT]
> queuedir=%s/queue
> + """ % (self.cwd,)))
> + if not self.verify_sigs:
> + myFile.write(dedent("""\
> + verify_sigs=False
> + """))
> + myFile.write(dedent("""\
> [http://www.example.com/bar/baz]
> project=project
> [http://www.example.com/fred/waldo]
> - """ % self.cwd))
> + """))
> myFile.close()
> self.queuedir = os.path.join(self.cwd, "queue")
> try:
> @@ -75,7 +82,7 @@
>
> class TestCaseWithQueue(TestCaseWithTransport):
>
> - def getQueue(self, processing=True, empty=False):
> - queue = QueueSetup(processing, empty)
> + def getQueue(self, processing=True, empty=False, verify_sigs=True):
> + queue = QueueSetup(processing, empty, verify_sigs)
> queue.setUp()
> return queue
>
> === added file 'pqm/ui/tests/test_xmlrpc.py'
> --- pqm/ui/tests/test_xmlrpc.py 1970-01-01 00:00:00 +0000
> +++ pqm/ui/tests/test_xmlrpc.py 2008-08-01 01:01:52 +0000
> @@ -0,0 +1,45 @@
> +# Copyright (C) 2008 Canonical Limited
> +# Authors: Daniel Watkins <daniel at daniel-watkins.co.uk>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 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 General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
> +import os
> +
> +from pqm.tests import sample_message, TestCaseWithQueue
> +from pqm.ui.twistd import PQMInfo
> +from pqm.ui.xmlrpc import PQM_XMLRPC
> +
> +
> +class TestPQM_XMLRPC(TestCaseWithQueue):
> +
> + def get_xmlrpc(self, empty=True, verify_sigs=False):
> + queue = self.getQueue(empty=empty, verify_sigs=verify_sigs)
> + pqminfo = PQMInfo([queue.configFileName])
> + return PQM_XMLRPC(pqminfo)
> +
> + def test_empty_submission(self):
> + xmlrpc = self.get_xmlrpc()
> + out = xmlrpc.xmlrpc_submit('')
> + self.assertEqual("Error: 'No From specified'", out)
> +
> + def test_valid_submission(self):
It would be good if you could add a comment explaining the behaviour you want.
This is especially important since the casual reader (that's me) doesn't know
what sample_message is.
Something like:
# Submitting a message like ______ makes PQM do _____
> + xmlrpc = self.get_xmlrpc()
> + out = xmlrpc.xmlrpc_submit(sample_message)
> + self.assertEqual('Success!', out)
> + queuedir = xmlrpc.pqminfo.queuedir
> + patches = [f for f in os.listdir(queuedir) if f.startswith('patch.')]
> + self.assertEqual(1, len(patches)) # We should have only one patch
> + self.assertFileEqual(sample_message,
> + os.path.join(queuedir, patches[0]))
>
I think it would also be good to add a test for a malformed submission.
> === modified file 'pqm/ui/twistd.py'
> --- pqm/ui/twistd.py 2008-04-16 10:23:52 +0000
> +++ pqm/ui/twistd.py 2008-08-01 00:33:29 +0000
> @@ -29,6 +29,7 @@
> from pqm.PQMConfigParser import ConfigParser
> from pqm.commandline import default_pqm_config_files
>
> +
> class QueueResource(resource.Resource):
> """A resource that shows a PQM queue."""
>
> @@ -156,14 +157,16 @@
> def __init__(self, filenames):
> self.filenames = filenames
> self.messages = []
> + self.configp = ConfigParser()
> + self.configp.read(self.filenames)
> + self.queuedir = pqm.get_queuedir(self.configp, logging, [])
>
Two things here.
configp should probably be config_parser. Characters are cheap.
The other thing is that if you have a lot of big files that take a while to
parse, this operation will block, preventing the process from handling any
other incoming requests. I'm not sure if it's an issue now, but it's something
to be aware of.
> def refresh(self):
> - configp = ConfigParser()
> - configp.read(self.filenames)
> - handler = pqm.BranchSpecOptionHandler(configp)
> - queuedir = pqm.get_queuedir(configp, logging, [])
> + self.configp.read(self.filenames)
> + handler = pqm.BranchSpecOptionHandler(self.configp)
> + self.queuedir = pqm.get_queuedir(self.configp, logging, [])
> self.messages = pqm.find_patches(
> - queuedir, logging, handler, configp, FakeOptions())
> + self.queuedir, logging, handler, self.configp, FakeOptions())
> try:
> [message.getSender() for message in self.messages]
> except:
> @@ -175,13 +178,17 @@
>
> application = None
> def main(argv):
> + from pqm.ui.xmlrpc import PQM_XMLRPC
> global application
> application = service.Application('pqm')
> pqminfo = PQMInfo(default_pqm_config_files())
> serviceCollection = service.IServiceCollection(application)
> port_str = os.environ.get('PQM_PORT', '8000')
> - internet.TCPServer(int(port_str), server.Site(QueueResource(pqminfo))
> - ).setServiceParent(serviceCollection)
> + r = resource.Resource()
> + r.putChild('', QueueResource(pqminfo))
> + r.putChild('RPC2', PQM_XMLRPC(pqminfo))
> + internet.TCPServer(int(port_str), server.Site(r)).setServiceParent(
> + serviceCollection)
> internet.TimerService(30, pqminfo.refresh).setServiceParent(serviceCollection)
>
> if __name__ == '__builtin__':
>
> === added file 'pqm/ui/xmlrpc.py'
> --- pqm/ui/xmlrpc.py 1970-01-01 00:00:00 +0000
> +++ pqm/ui/xmlrpc.py 2008-08-01 00:43:22 +0000
> @@ -0,0 +1,61 @@
> +# Copyright (C) 2008 Canonical Limited
> +# Authors: Daniel Watkins <daniel at daniel-watkins.co.uk>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 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 General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
> +import logging
> +import os.path
> +from StringIO import StringIO
> +import time
> +
> +from twisted.web.xmlrpc import XMLRPC
> +
> +import pqm
> +from pqm.errors import PQMException
> +from pqm.script import get_email_string, read_email_from_string
> +from pqm.ui.twistd import FakeOptions
> +
> +
> +class PQM_XMLRPC(XMLRPC):
> +
Docstring please.
> + def __init__(self, pqminfo):
> + XMLRPC.__init__(self)
> + self.pqminfo = pqminfo
> + self.logger = logging.getLogger('pqm')
> +
> + def xmlrpc_submit(self, text):
And here too!
> + try:
> + queuedir = self.pqminfo.queuedir
> + queuedir = os.path.abspath(queuedir)
> + # Set up options for processing
> + options = FakeOptions()
> + options.verify_sigs = self.pqminfo.configp.get_boolean_option(
> + 'DEFAULT', 'verify_sigs', True)
> + if options.verify_sigs:
> + options.keyring = self.pqminfo.configp.get('DEFAULT',
> + 'keyring')
> + # TODO: Eww.
Please say *why* it's eww and what can be done about it.
> + pqm.gpgv_path = self.pqminfo.configp.get_option('DEFAULT',
> + 'gpgv_path', 'gpgv')
> + pqm_subdir = os.path.join(queuedir, 'pqm')
> + pqm.pqm_subdir = pqm_subdir
> + # Process input
This isn't a particularly enlightening comment.
> + email = read_email_from_string(self.logger, text)
> + email_string = get_email_string(self.logger, options, email)
> + # Write script
Neither is this :)
> + pqm.write_script(queuedir, email_string)
> + return "Success!"
I think returning a human-readable string sends mixed messages. Perhaps just
return True instead.
> + except PQMException, e:
> + return "Error: %s" % (e,)
>
Could you please raise a Fault instead?
Thanks for the patch!
jml
More information about the bazaar
mailing list