Merge lp:~flo-fuchs/mailman/restclient into lp:mailman

[Florian Fuchs]

I added a rest client in src/mailmanclient as well as a doctest in src/mailman/rest/docs/restclient.txt.

[Barry Warsaw (barry)]

Thanks for getting this client library started, Florian! Here are some
thoughts on your merge proposal.

As we discussed on IRC, I now think the best place for this code is in
src/mailmanclient. Don't worry about that though; I'll fix that up when I
merge to trunk. I know for now it's difficult getting the test suite to run
with that layout, so I'll take that on. I don't think it will be too
difficult, but there will also be some other useful helpers to share.

Omitting the non-controversial stuff.

=== added file 'src/mailman/rest/docs/restclient.txt'
--- src/mailman/rest/docs/restclient.txt 1970-01-01 00:00:00 +0000
+++ src/mailman/rest/docs/restclient.txt 2010-06-25 16:50:42 +0000
> @@ -0,0 +1,89 @@
> +===================
> +Mailman REST Client
> +===================
> +
> +Domains
> +=======
> +
> + # The test framework starts out with an example domain, so let's delete
> + # that first.
> + >>> from mailman.interfaces.domain import IDomainManager
> + >>> from zope.component import getUtility
> + >>> domain_manager = getUtility(IDomainManager)
> +
> + >>> domain_manager.remove('example.com')
> + <Domain example.com...>
> + >>> transaction.commit()

The test infrastructure currently sets up this sample domain. I wonder
whether the REST client tests will more often want to start from a clean
slate, or want this example domain. I think perhaps the former, so I should
probably add a layer that is just like the ConfigLayer, but has an empty
testSetUp(). (You don't have to worry about that for now. :)

> +In order to add new lists first a new domain has to be added.
> +
> + >>> from mailmanclient.rest import MailmanRESTClient, MailmanRESTClientError
> + >>> c = MailmanRESTClient('localhost:8001')
> + >>> c.create_domain('example.com')
> + True

What do you think about having proxy objects for Domains, Lists, etc.?
Launchpadlib works like this and it provides more of a natural, object
oriented API to clients, rather than an XMLRPC style seen here.

So for example, .create_domain() would return a Domain surrogate, and you'd
call things like .get_lists() and .create_list() on that object.

How much harder do you think it would be to do something like that, and do you
think it would be worth it?

> +
> +
> +Mailing lists
> +=============
> +
> +You can get a lists of all lists by calling get_lists(). If no lists have been created yet, MailmanRESTClientError is raised.

Please wrap narrative to 78 characters.

> +
> + >>> lists = c.get_lists()
> + Traceback (most recent call last):
> + ...
> + MailmanRESTClientError: No mailing lists found

I think it might be better to return an empty list instead of raising an
exception here. Think about application code like so:

    # Display all the current lists.
    try:
        lists = c.get_lists()
    except MailmanRESTClientError:
        # Are we sure we got "No mailing lists found" or did some other
        # generic client error occur?
        lists = []
    for mailing_list in lists:
        # ...

The thing is, having no mailing lists isn't an error condition, so this should
probably return an empty (Python) list rather than raise an exception.
...

[lp:~flo-fuchs/mailman/restclient]

6916. By Florian Fuchs on 2010-07-07

added _Domain and _List classes, fixed code style issues

[Florian Fuchs (flo-fuchs)]

First of all: Thanks a lot for that extensive review - I really, really appreciate getting this kind of detailed comment!

Exceptions
==========
MailmanRESTClientError is only used for email- and domain-validation. Apart from that the original Exceptions don't get catched any more.

If there are no mailing lists yet, an empty list is returned instead of an Exception.

Proxy Objects
=============
> What do you think about having proxy objects for Domains, Lists, etc.?
> Launchpadlib works like this and it provides more of a natural, object
> oriented API to clients, rather than an XMLRPC style seen here.

Great idea! I've added two sub-classes two return as List and Domain objects like:
domain = client.get_domain('example.com')
list = domain.create_list('test')
and so on...
No user object yet: What do you think: Are we talking about a User with n email addresses and n memberships here or more like User = Membership?

Generally I'd say "get_singularterm" and "create_singularterm" (get_list(), create_domain() etc.) should return an object with helpful methods and "get_pluralterm"/"create_pluralterm" (get_lists, get_members) should return lists oder dicts...

Style issues
============
I've fixed all the issues according to the style guide and pep8. At least I hope so... Lesson learned... ;-)

Func name issue
===============

> I'm not sure 'reading' a list is the right phrase here. "Reading" a list
> implies to me reading its archive. Maybe .get_list() here?

I was thinking in CRUD terms where I understand "reading" more like getting a db record. But I agree, it's a little strange here. So I changed it to get_list().

List order
==========
> This is why I added the dump_json() helper in
> src/mailman/tests/test_documentation.py. That helper isn't entirely
> appropriate for your tests because you don't explicitly pass in a url; that's
> basically embedded in .get_lists and the client. But there's enough
> commonality that dump_json() should be refactored into something that can be
> shared.

I'm not sure if a func name like dump_json is a little confusing in that context since the client doesn't return any json. Maybe we could rename the function into dump_rest_output() (or similar) and make url, method optional? So if url is set, the rest server is called; if not, only data is sorted. `data` would then either server as a parm for POST/PUT content or as a dict to sort...

For now I've added the sort logic to the test (only a couple of lines).

So, thanks again for reviewing. On to the next round...? :-)

Florian

[lp:~flo-fuchs/mailman/restclient]

6917. By Florian Fuchs on 2010-07-11

added some line breaks in the restclient doctest file; changed confusing function names (http helper functions) in rest client

6918. By Florian Fuchs on 2010-07-15

fixed email validation in restclient to work with email addresses containing subdomains

[Barry Warsaw (barry)]

Thanks for all the great updates. Things look pretty good, and I have only a
few minor issues to comment on. I think we're almost ready to merge it!
Great work.

On the dump_json() issue, i just thought of something: since you're only
displaying dictionaries, perhaps pprint will do the trick. In Python 2.6,
pprint.pprint() sorts the dictionary elements I believe.

    >>> import os
    >>> from pprint import pprint
    >>> pprint(dict(os.environ))
    {'COLUMNS': '79',
     ...
     'DISPLAY': ':0.0',
     'EMACS': 't',
     ...

You asked:

>No user object yet: What do you think: Are we talking about a User with n
>email addresses and n memberships here or more like User = Membership?

I think we should stick fairly close to the internal model here. So 'Users'
represent people, 'Addresses' represent their email addresses, which are
usually associated with a user, and 'Member' joins an address to a mailing
list with a given role. Only indirectly can you get at the user for a member
(i.e. through its address).

I agree about singular/plural terms.

-B

=== added file 'src/mailman/rest/docs/restclient.txt'
--- src/mailman/rest/docs/restclient.txt 1970-01-01 00:00:00 +0000
+++ src/mailman/rest/docs/restclient.txt 2010-07-15 14:02:55 +0000
> @@ -0,0 +1,129 @@
> +===================
> +Mailman REST Client
> +===================
> +
> + # The test framework starts out with an example domain, so let's delete
> + # that first.
> + >>> from mailman.interfaces.domain import IDomainManager
> + >>> from zope.component import getUtility
> + >>> domain_manager = getUtility(IDomainManager)
> +
> + >>> domain_manager.remove('example.com')
> + <Domain example.com...>
> + >>> transaction.commit()
> +
> +First let's get an instance of MailmanRESTClient.
> +
> + >>> from mailmanclient.rest import MailmanRESTClient, MailmanRESTClientError
> + >>> client = MailmanRESTClient('localhost:8001')
> +
> +So far there are no lists.
> +
> + >>> lists = client.get_lists()
> + >>> print lists
> + []

I think you can just do

    >>> client.get_lists()
    []

> +
> +
> +Domains
> +=======
> +
> +In order to add new lists first a new domain has to be added.
> +
> + >>> new_domain = client.create_domain('example.com')
> + >>> new_domaininfo = new_domain.get_domaininfo()
> + >>> for key in sorted(new_domaininfo):
> + ... print '{0}: {1}'.format(key, new_domaininfo[key])
> + base_url: http://example.com
> + ...
> +
> +Later the domain object can be instanciated using get_domain()
> +
> + >>> my_domain = client.get_domain('example.com')
> +
> +
> +Mailing lists
> +=============
> +
> +Now let's add some mailing lists.
> +
> + >>> new_list = my_domain.create_list('test-one')
> +
> +Lets add another list and get some information on the list.

s/Lets/let's/

> +
> + >>> another_list = my_domain.create_list('test-two')
> + >>> another_listinfo = another_list.get_listinfo()
> + >>> for key in sorted(another_listinfo):
> + ... print '{0}: {1}'.format(key, another_listinfo[key])
> + fqdn_listname: <email address hidden>
> + ...
> +
> +Later the new list can be instanciated using get_list():

s/i...

review: Needs Fixing

[lp:~flo-fuchs/mailman/restclient]

6919. By Florian Fuchs on 2010-07-20

changes to the restclient lib:
  * use httplib2 instead of httplib
  * some style, typo and best-practice fixes
  * use pprint in test file
  * refactored http helper methods (only one used now instead of one for each HTTP method)
  * removed host and email address validation (not necessary)
  * use operator.itemgetter instead of lambdas to sort dicts

[Florian Fuchs (flo-fuchs)]

I did some fixes and improvements like suggested in the last review...

> > + entry 0:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
> > + entry 1:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
> > + entry 2:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
>
> The client is returning json here, right?

Nope, the client never returns json. Either HTTP status codes or lists/dicts are returned.

> Should we be using httplib2 and urllib2 here? See the implementation of
> dump_json().

Done.

> > + def _delete_request(self, path):
> > + """Send an HTTP DELETE request.
> > +
> > + :param path: the URL to send the DELETE request to
> > + :type path: string
> > + :return: request status code
> > + :rtype: string
> > + """
> > + try:
> > + self.c.request('DELETE', path, None, self.headers)
> > + r = self.c.getresponse()
> > + return r.status
> > + finally:
> > + self.c.close()
>
> I wonder if this duplication can be refactored?

There's only one http request method now.

> > + def _validate_email_host(self, email_host):
> > + """Validates a domain name.
> > +
> > + :param email_host: the domain str to validate
> > + :type email_host: string
> > + """
> > + pat = re.compile('^[-a-z0-9\.]+\.[-a-z]{2,4}$', re.IGNORECASE)
> > + if not pat.match(email_host):
> > + raise MailmanRESTClientError('%s is not a valid domain name' %
> email_host)
>
> Won't the Mailman core refuse to create a domain if it's not valid? It might
> still be worth doing client-side validation, but I would expect that more in
> some webui JavaScript. What's the advantage of doing this extra check (which
> might be different than what happens in the core)?

I didn't know if the core does email validation. Also, the django app does some validation. So I removed it.

> I wonder if this method is necessary. In general, attributes are preferred
> over accessors, and you've already got a public one right here! So clients
> can do:
>
> >>> my_domain = client.get_domain('example.com')
> >>> my_domain.domain_info
> ...
>
> directly. In fact, for polymorphism, maybe the attribute should just be
> called 'info'?

Done.

review: Needs Resubmitting

[Barry Warsaw (barry)]

I finally managed to figure out how to deploy this. See http://launchpad.net/mailman.client and the trunk branch of that. Thanks so much for the contribution!

review: Approve

=== added file 'src/mailman/rest/docs/restclient.txt'
--- src/mailman/rest/docs/restclient.txt	1970-01-01 00:00:00 +0000
+++ src/mailman/rest/docs/restclient.txt	2010-07-20 21:14:46 +0000
@@ -0,0 +1,124 @@
+===================
+Mailman REST Client
+===================
+
+    >>> from pprint import pprint
+
+    # The test framework starts out with an example domain, so let's delete
+    # that first.
+    >>> from mailman.interfaces.domain import IDomainManager
+    >>> from zope.component import getUtility
+    >>> domain_manager = getUtility(IDomainManager)
+
+    >>> domain_manager.remove('example.com')
+    <Domain example.com...>
+    >>> transaction.commit()
+
+First let's get an instance of MailmanRESTClient.
+
+    >>> from mailmanclient.rest import MailmanRESTClient, MailmanRESTClientError
+    >>> client = MailmanRESTClient('localhost:8001')
+
+So far there are no lists.
+
+    >>> client.get_lists()
+    []
+
+
+Domains
+=======
+
+In order to add new lists first a new domain has to be added.
+
+    >>> new_domain = client.create_domain('example.com')
+    >>> pprint(new_domain.info)
+    {u'base_url': u'http://example.com',
+     u'contact_address': u'[e-mail]',
+     u'description': None,
+     u'email_host': u'example.com',
+     u'http_etag': u'"6b1ccf042e8f76138a0bd37e8509f364da92a5c5"',
+     u'self_link': u'http://localhost:8001/3.0/domains/example.com',
+     u'url_host': u'example.com'}
+
+Later the domain object can be instantiated using get_domain()
+
+    >>> my_domain = client.get_domain('example.com')
+
+
+Mailing lists
+=============
+
+Now let's add s mailing list called 'test-one'.
+
+    >>> new_list = my_domain.create_list('test-one')
+
+Let's add another list and get some information on the list.
+
+    >>> another_list = my_domain.create_list('test-two')
+    >>> pprint(another_list.info)
+    {u'fqdn_listname': u'[e-mail]',
+     u'host_name': u'example.com',
+     u'http_etag': u'"a05542c9faa07cbe2b8fdf8a1655a2361ab365f2"',
+     u'list_name': u'test-two',
+     u'real_name': u'Test-two',
+     u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]'}
+
+Later the new list can be instantiated using get_list():
+
+    >>> some_list = client.get_list('[e-mail]')
+
+The lists have been added and get_lists() returns a list of dicts, sorted
+by fqdn_listname.
+
+    >>> pprint(client.get_lists())
+    [{u'fqdn_listname': u'[e-mail]',
+      u'host_name': u'example.com',
+      u'http_etag': u'"5e99519ef1b823a52254b77e89bec54fbd17bef0"',
+      u'list_name': u'test-one',
+      u'real_name': u'Test-one',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]'},
+     {u'fqdn_listname': u'[e-mail]',
+      u'host_name': u'example.com',
+      u'http_etag': u'"a05542c9faa07cbe2b8fdf8a1655a2361ab365f2"',
+      u'list_name': u'test-two',
+      u'real_name': u'Test-two',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]'}]
+
+
+Membership
+==========
+
+Since we now have a list we should add some members to it (.subscribe()
+returns an HTTP status code, ideally 201)
+
+    >>> new_list.subscribe('[e-mail]', 'Jack')
+    201
+    >>> new_list.subscribe('[e-mail]', 'Meg')
+    201
+    >>> another_list.subscribe('[e-mail]', 'Jack')
+    201
+
+We can get a list of all members:
+
+    >>> pprint(client.get_members())
+    [{u'http_etag': u'"320f9e380322cafbbf531c11eab1ec9d38b3bb99"',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]/member/[e-mail]'},
+     {u'http_etag': u'"cd75b7e93216a022573534d948511edfbfea06cd"',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]/member/[e-mail]'},
+     {u'http_etag': u'"13399f5ebbab8c474926a7ad0ccfda28d717e398"',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]/member/[e-mail]'}]
+
+Or just the members of a specific list:
+
+    >>> pprint(new_list.get_members())
+    [{u'http_etag': u'"320f9e380322cafbbf531c11eab1ec9d38b3bb99"',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]/member/[e-mail]'},
+     {u'http_etag': u'"cd75b7e93216a022573534d948511edfbfea06cd"',
+      u'self_link': u'http://localhost:8001/3.0/lists/[e-mail]/member/[e-mail]'}]
+
+After a while Meg decides to unsubscribe from the mailing list (like
+.subscribe() .unsubscribe() returns an HTTP status code, ideally 200).
+
+    >>> new_list.unsubscribe('[e-mail]')
+    200
+
 
=== added directory 'src/mailmanclient'
=== added file 'src/mailmanclient/__init__.py'
=== added file 'src/mailmanclient/rest.py'
--- src/mailmanclient/rest.py	1970-01-01 00:00:00 +0000
+++ src/mailmanclient/rest.py	2010-07-20 21:14:46 +0000
@@ -0,0 +1,261 @@
+# Copyright (C) 2010 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman 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 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman 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
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""A client library for the Mailman REST API."""
+
+
+from __future__ import absolute_import, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'MailmanRESTClient',
+    'MailmanRESTClientError',
+    ]
+
+
+import re
+import json
+
+from httplib2 import Http
+from operator import itemgetter
+from urllib import urlencode
+from urllib2 import HTTPError
+
+
+class MailmanRESTClientError(Exception):
+    """An exception thrown by the Mailman REST API client."""
+
+
+class MailmanRESTClient():
+    """A wrapper for the Mailman REST API."""
+    
+    def __init__(self, host):
+        """Check and modify the host name.
+
+        :param host: the host name of the REST API 
+        :type host: string
+        :return: a MailmanRESTClient object
+        :rtype: objectFirst line should
+        """
+        self.host = host
+        # If there is a trailing slash remove it
+        if self.host[-1] == '/':
+            self.host = self.host[:-1]
+        # If there is no protocol, fall back to http://
+        if self.host[0:4] != 'http':
+            self.host = 'http://' + self.host
+
+    def __repr__(self):
+        return '<MailmanRESTClient: %s>' % self.host
+
+    def _http_request(self, path, data=None, method=None):
+        """Send an HTTP request.
+        
+        :param path: the path to send the request to
+        :type path: string
+        :param data: POST oder PUT data to send
+        :type data: dict
+        :param method: the HTTP method; defaults to GET or POST (if
+            data is not None)
+        :type method: string
+        :return: the request content or a status code, depending on the
+            method and if the request was successful
+        :rtype: int, list or dict
+        """
+        url = self.host + path
+        # Include general header information
+        headers = {
+            'User-Agent': 'MailmanRESTClient',
+            'Accept': 'text/plain', 
+            }
+        if data is not None:
+            data = urlencode(data)
+        if method is None:
+            if data is None:
+                method = 'GET'
+            else:
+                method = 'POST'
+        method = method.upper()
+        if method == 'POST':
+            headers['Content-type'] = "application/x-www-form-urlencoded"
+        response, content = Http().request(url, method, data, headers)
+        if method == 'GET':
+            if response.status // 100 != 2:
+                return response.status
+            else:
+                return json.loads(content)
+        else:
+            return response.status
+
+    def create_domain(self, email_host):
+        """Create a domain and return a domain object.
+        
+        :param email_host: The host domain to create
+        :type email_host: string
+        :return: A domain object or a status code (if the create
+            request failed)
+        :rtype int or object
+        """
+        data = {
+            'email_host': email_host,
+            }
+        response = self._http_request('/3.0/domains', data, 'POST')
+        if response == 201:
+            return _Domain(self.host, email_host)
+        else:
+            return response
+
+    def get_domain(self, email_host):
+        """Return a domain object.
+        
+        :param email_host: host domain
+        :type email_host: string
+        :rtype object
+        """
+        return _Domain(self.host, email_host)
+
+    def get_lists(self):
+        """Get a list of all mailing list.
+
+        :return: a list of dicts with all mailing lists
+        :rtype: list
+        """
+        response = self._http_request('/3.0/lists')
+        if 'entries' not in response:
+            return []
+        else:
+            # Return a dict with entries sorted by fqdn_listname
+            return sorted(response['entries'],
+                key=itemgetter('fqdn_listname'))
+
+    def get_list(self, fqdn_listname):
+        """Find and return a list object.
+
+        :param fqdn_listname: the mailing list address
+        :type fqdn_listname: string
+        :rtype: object
+        """
+        return _List(self.host, fqdn_listname)
+
+    def get_members(self):
+        """Get a list of all list members.
+
+        :return: a list of dicts with the members of all lists
+        :rtype: list
+        """
+        response = self._http_request('/3.0/members')
+        if 'entries' not in response:
+            return []
+        else:
+            return sorted(response['entries'],
+                key=itemgetter('self_link'))
+
+
+class _Domain(MailmanRESTClient):
+    """A domain wrapper for the MailmanRESTClient."""
+
+    def __init__(self, host, email_host):
+        """Connect to host and get list information.
+
+        :param host: the host name of the REST API 
+        :type host: string
+        :param email_host: host domain
+        :type email_host: string
+        :rtype: object
+        """
+        super(_Domain, self).__init__(host)
+        self.info = self._http_request('/3.0/domains/' + email_host)
+
+    def create_list(self, list_name):
+        """Create a mailing list and return a list object.
+        
+        :param list_name: the name of the list to be created
+        :type list_name: string
+        :rtype: object
+        """
+        fqdn_listname = list_name + '@' + self.info['email_host']
+        data = {
+            'fqdn_listname': fqdn_listname
+            }
+        response = self._http_request('/3.0/lists', data, 'POST')
+        return _List(self.host, fqdn_listname)
+
+    def delete_list(self, list_name):
+        fqdn_listname = list_name + '@' + self.info['email_host']
+        return self._http_request('/3.0/lists/' + fqdn_listname, None, 'DELETE')
+
+
+class _List(MailmanRESTClient):
+    """A mailing list wrapper for the MailmanRESTClient."""
+
+    def __init__(self, host, fqdn_listname):
+        """Connect to host and get list information.
+
+        :param host: the host name of the REST API 
+        :type host: string
+        :param fqdn_listname: the mailing list address
+        :type fqdn_listname: string
+        :rtype: object
+        """
+        super(_List, self).__init__(host)
+        self.info = self._http_request('/3.0/lists/' + fqdn_listname)
+
+    def subscribe(self, address, real_name=None):
+        """Add an address to a list.
+
+        :param address: email address to add to the list.
+        :type address: string
+        :param real_name: the real name of the new member
+        :type real_name: string
+        """
+        data = {
+            'fqdn_listname': self.info['fqdn_listname'],
+            'address': address,
+            'real_name': real_name
+            }
+        return self._http_request('/3.0/members', data, 'POST')
+
+    def unsubscribe(self, address):
+        """Unsubscribe an address to a list.
+
+        :param address: email address to add to the list.
+        :type address: string
+        :param real_name: the real name of the new member
+        :type real_name: string
+        """
+        return self._http_request('/3.0/lists/' +
+                             self.info['fqdn_listname'] +
+                             '/member/' +
+                             address,
+                             None, 
+                             'DELETE')
+
+    def get_members(self):
+        """Get a list of all list members.
+
+        :return: a list of dicts with all members
+        :rtype: list
+        """
+        response = self._http_request('/3.0/lists/' +
+                                 self.info['fqdn_listname'] +
+                                 '/roster/members')
+        if 'entries' not in response:
+            return []
+        else:
+            return sorted(response['entries'],
+                key=itemgetter('self_link'))
+