GNU Mailman

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

restclient
Merge into 3.0

Proposed by Florian Fuchs on 2010-06-25

Status:	Merged
Merge reported by:	Barry Warsaw
Merged at revision:	not available
Proposed branch:	lp:~flo-fuchs/mailman/restclient
Merge into:	lp:mailman
Diff against target:	396 lines (+385/-0) 2 files modified src/mailman/rest/docs/restclient.txt (+124/-0) src/mailmanclient/rest.py (+261/-0)
To merge this branch:	bzr merge lp:~flo-fuchs/mailman/restclient
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Barry Warsaw	2010-07-07	Approve on 2010-12-26
Florian Fuchs		Needs Resubmitting on 2010-07-28
Mailman Coders	2010-06-25	Pending
Review via email: mp+28522@code.launchpad.net

Description of the change

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

Revision history for this message

Barry Warsaw (barry) wrote on 2010-06-29:

Download full text (19.5 KiB)

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.

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

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

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:

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.

> +
> +Lists can be created by calling create_list('listname@domain.tld').
> +
> +    >>> c.create_list('test-one@example.com')
> +    True
> +    >>> c.create_list('test-two@example.com')
> +    True
> +
> +If there are any mailing lists a list of dicts is returned.
> +
> +    >>> lists = c.get_lists()
> +    >>> lists
> +    [{u'real_name': u'Test-one', u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com', u'http_etag': u'"5e99519ef1b823a52254b77e89bec54fbd17bef0"', u'host_name': u'example.com', u'fqdn_listname': u'test-one@example.com', u'list_name': u'test-one'}, {u'real_name': u'Test-two', u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com', u'http_etag': u'"a05542c9faa07cbe2b8fdf8a1655a2361ab365f2"', u'host_name': u'example.com', u'fqdn_listname': u'test-two@example.com', u'list_name': u'test-two'}]

Unfortunately, this is too fragile because it depends on dictionary sort
order, which isn't defined in Python, and has definitely changed between
Python versions!

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.

Do you want to take a crack at that?

> +
> +Information about a specific list can be read (returns a single dict).
> +
> +    >>> list = c.read_list('test-one@example.com')
> +    >>> list
> +    {u'real_name': u'Test-one', u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com', u'http_etag': u'"5e99519ef1b823a52254b77e89bec54fbd17bef0"', u'host_name': u'example.com', u'fqdn_listname': u'test-one@example.com', u'list_name': u'test-one'}

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?

> +
> +
> +Membership
> +==========
> +
> +Users can subscribe to a mailing list using the list's name, their email address, as well as their name.
> +
> +    >>> c.subscribe_list('test-one@example.com', 'aperson@example.com', 'Garp')
> +    True
> +    >>> c.subscribe_list('test-one@example.com', 'bperson@example.com', 'Jenny')
> +    True
> +    >>> c.subscribe_list('test-two@example.com', 'bperson@example.com', 'Jenny')
> +    True

Here's a place where a more object-oriented client might work better.  For
example, if we had both a List and User object, then depending on which one
you had, you could do a simpler subscription.  E.g. if .read_list/.get_list
returned a List object, you'd do something like this:

>>> test_one = c.get_list('test-one@example.com')
    >>> member = test_one.subscribe('aperson@example.com', 'Garp')

Conversely, if you had a User object, you could do:

>>> aperson = c.get_user('aperson@example.com')
    >>> aperson.subscribe('test-one@example.com')

> +Members of all mailings list can be listed (returns a list of dicts).
> +
> +    >>> all_members = c.get_members()
> +    >>> all_members
> +    [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example.com', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com/member/bperson@example.com', u'http_etag': u'"a5c962186200af483f6508dfbabca7a4a78ef309"'}]

Here's that dict ordering problem again.

> +
> +As well as the members of a specific lists:
> +
> +    >>> list_members = c.get_members('test-one@example.com')
> +    >>> list_members
> +    [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example.com', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}]
> +
> +Lists can be unsubscribed by using list name and the member's email address
> +
> +    >>> c.leave_list('test-one@example.com', 'bperson@example.com')
> +    True
> +
> +After leaving the list only the remaining members are shown:
> +    
> +    >>> new_members = c.get_members('test-one@example.com')
> +    >>> new_members
> +    [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}]

The same API principles apply here too.

=== 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-06-25 16:50:42 +0000
> @@ -0,0 +1,248 @@
> +# Copyright (C) 2009-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/>.

I hate to be so nitpicky on style, but I'm gonna anyway. :)  A few things will
help consistency with other code, so I'll be kind of anal about it for now.

template.py in the top level directory is a good place to start with a new
Python file.  It contains the license header, a template module docstring,
and some important __future__ imports.  It also forces all unadorned classes
to be new style (through the __metaclass__ assignment), and adds an empty
__all__ for publicly exported symbols.

> +
> +import json
> +from httplib import HTTPConnection, HTTPException
> +from urllib import urlencode

See src/mailman/docs/STYLEGUIDE.txt

-----snip snip-----
import json

from httplib import HTTPConnection, HTTPException
from urllib import urlencode
-----snip snip-----

(two blank lines between top level constructs)

> +
> +class MailmanRESTClientError(Exception):
> +    """An exception thrown by the Mailman REST API client."""
> +    pass
> +

(blank line needed)

> +class MailmanRESTClient(object):
> +    """A thin client wrapper for the Mailman REST API."""
> +
> +    def __init__(self, host):
> +        self.host = host
> +        if self.host[-1] == '/':
> +            self.host = self.host[:-1]
> +
> +        # general header information

This should be a complete sentence, e.g.:

# Include general header information.

> +        self.headers = {
> +            "User-Agent": "MailmanRESTClient",
> +            "Accept": "text/plain", 
> +        }

I usually prefer single quotes when possible (one less shift key to hold down
:), and the closing brace should be indented 4 spaces.

> +
> +        try: 
> +            self.c = HTTPConnection(self.host)
> +        except:
> +            raise MailmanRESTClientError('Could not connect to server')

The bare except is problematic; it's generally not a good idea to use them.
In this case, it's probably better to just let the HTTPConnection exception
flow up uncaught.

> +
> +    def __repr__(self):
> +        return '<MailmanRESTClient: %s>' % self.host
> +
> +    def _get_url(self, path):
> +        try:
> +            self.c.request('GET', path, None, self.headers)
> +        except:
> +            raise MailmanRESTClientError('Error sending request')

Similarly here.  I don't think we need to turn all exceptions into
MailmanRESTClientErrors.  Let's reserve that exception hierarchy for
exceptions specifically in our library.

> +        
> +        try:
> +            r = self.c.getresponse()
> +            raw_data = r.read()
> +            if len(raw_data) == 0:
> +                raise None

You're actually not allowed to raise None (I had to try it myself to see :).
This might be another case of just letting the original exception percolate
up:

>>> json.loads('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/json/__init__.py", line 307, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.6/json/decoder.py", line 319, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.6/json/decoder.py", line 338, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

I'm a little less sure about this one, but that probably calls for just
letting the json.loads() fail until there's an overriding reason to mutate
it.

> +            data = json.loads(raw_data)
> +        except:
> +			raise MailmanRESTClientError('Error sending request')
> +        finally:
> +            self.c.close()
> +        
> +        return data
> +
> +    def _post_url(self, path, data):
> +        self.headers['Content-type'] = "application/x-www-form-urlencoded"
> +        
> +        try:
> +            self.c.request('POST', path, urlencode(data), self.headers)
> +        except:
> +            raise MailmanRESTClientError('Could not send request')

Again, probably don't need to try/except this.

> +            
> +        try:
> +            r = self.c.getresponse()
> +            
> +            if r.status == 201:
> +                return True
> +            else:
> +                return r.status

Interesting.  Why not always just return the status code?  It seems like it
would be better to always return an integer and let the client decide what to
do with the status code, rather than transform just 201 into True.

> +        finally:
> +            self.c.close()
> +
> +    def _delete_url(self, path):
> +        try:
> +            self.c.request('DELETE', path, None, self.headers)
> +        except:
> +            raise MailmanRESTClientError('Could not send request')
> +        finally:
> +            self.c.close()

Similar.

> +        
> +    def _put_url(self, path):
> +        """To be implemented...
> +        """

You can use an XXX comment like so:

"""XXX 2010-06-29 florian: to be implemented."""

That slightly suggestive XXX is an old Guido tradition, but there are lots of
good tools that search for them and remind you about them.  E.g. pylint.
Including the date and your Launchpad id is a good idea to identify who wrote
the comment and how old it is, without having to reconstruct it from the vcs
history.

> +        pass
> +        
> +    def get_lists(self):
> +        """Get a list of mail lists.
> +
> +        returns a list of dicts

You can use epydoc rest markup for describing the parameters and return values
for a method.

http://epydoc.sourceforge.net/manual-fields.html#epydoc-fields

There are lots of examples sprinkled about in the current Mailman code.

> +        """
> +        try:
> +            r = self._get_url('/3.0/lists')
> +        except:
> +            raise MailmanRESTClientError('Could not send request')
> +        
> +        if 'entries' not in r:
> +            raise MailmanRESTClientError('No mailing lists found')
> +
> +        return r['entries']
> +
> +    def read_list(self, list_name):
> +        """Get information about list.
> +
> +        :param list_name: name of the list to get information about
> +        :type list_name: string
> +        :rtype: dict

Yep, like that!

> +        """
> +        try:
> +            r = self._get_url('/3.0/lists/' + list_name)
> +        except:
> +            raise MailmanRESTClientError('Unable to get info about list')
> +
> +        return r
> +
> +    def create_list(self, fqdn_listname, **kwargs):
> +        """Create a new list.
> +
> +        :param fqdn_listname: the name of the list including the @ and the domain name.  eg.  test@example.com

This needs to be line wrapped to 78 characters.  You can indent continuation
lines.

> +        :type fqdn_listname: string
> +        :rtype: None
> +        """
> +        data = {
> +                'fqdn_listname': fqdn_listname

Dedent 4 spaces and end in a comma.

> +        }

Indent 4 spaces.

> +        data.update(**kwargs)
> +        try:
> +            return self._post_url('/3.0/lists', data)
> +        except MailmanRESTClientError, e:
> +            raise MailmanRESTClientError(e)

Similar, no need to catch this exception.

> +            
> +
> +    def subscribe_list(self, fqdn_listname, address, real_name, **kwargs):
> +        """Add an address to a list.
> +
> +        :param fqdn_listname: the name of the list .
> +        :type fqdn_listname: string
> +        :param address: email address to add to the list.
> +        :type address: string
> +        :param real_name: the "real" name for the address to be addded
> +        :type real_name: string
> +        """
> +
> +        data = {
> +                'fqdn_listname': fqdn_listname,
> +                'address': address,
> +                'real_name': real_name

Dedent all lines 4 spaces and end the last one in a comma.

> +        }

Indent 4 spaces.

> +        data.update(**kwargs)
> +        try:
> +            r = self._post_url('/3.0/members', data)
> +        except:
> +            raise MailmanRESTClientError('unable to join list')

Don't catch.

> +
> +        return r
> +
> +    def leave_list(self, fqdn_listname, address):
> +        """Remove an address from a list.
> +
> +        :param fqdn_listname: the name of the list.
> +        :type fqdn_listname: string
> +        :param address: email address that is leaving the list
> +        :type address: string
> +        :rtype: None
> +        """
> +        try:
> +            r = self._delete_url('/3.0/lists/' + fqdn_listname + '/member/' + address )

Line length.  You can probably just write this like:

return self._delete_url(
                '/3.0/lists/' + fqdn_listname + '/member/' + address)

(Note, no space before the close paren.)

> +        except:
> +            raise MailmanRESTClientError('unable to leave list')
> +
> +        return True
> +
> +    def get_members(self, fqdn_listname = None):

No spaces around the = sign.

> +        """Get a list of all members for all lists.
> +
> +        :rtype: list of dicts
> +        :type fqdn_listname: string
> +        """
> +
> +        if fqdn_listname != None:
> +            url = '/3.0/lists/' + fqdn_listname + '/roster/members'
> +        else:
> +            url = '/3.0/members'
> +        try:
> +            r = self._get_url(url)
> +        except:
> +            raise MailmanRESTClientError('Could not complete request')
> +
> +        if 'entries' not in r:
> +            raise MailmanRESTClientError('Could not find any members')

Maybe just return the empty list here?

> +
> +        return r['entries']
> +
> +    def get_domains(self):
> +        """Get a list of domains.
> +
> +        :rtype: list of dicts
> +        """
> +        try:
> +            r = self._get_url('/3.0/domains')
> +        except:
> +            raise MailmanRESTClientError('Could not complete request')
> +        if 'entries' not in r:
> +            raise MailmanRESTClientError('Could not find any domains')

And here, an empty list would be fine.

> +
> +        return r['entries']
> +
> +    def read_domain(self, domain_name):
> +        """Get information about a specific domain.
> +
> +        :param domain_name: the name of the domain.
> +        :rtype: dict
> +        """
> +        try:
> +            r = self._get_url('/3.0/domains/' + domain_name)
> +        except:
> +            raise MailmanRESTClientError('Unable to read domain')
> +
> +        return r
> +
> +    def create_domain(self, email_host, **kwargs):
> +        """Create a new domain name.
> +
> +        :param email_host: domain name to create
> +        :type email_host: string
> +        """
> +        data = {
> +            'email_host': email_host,
> +        }
> +        data.update(**kwargs)
> +        try:
> +            r = self._post_url('/3.0/domains', data)
> +        except:
> +            raise MailmanRESTClientError('Unable to create domain')
> +
> +        return r
> +

Similar comments here.

This is a great first draft.  Please let me know if any of my comments don't
make sense or you have questions about them.  Thanks!

-Barry

lp:~flo-fuchs/mailman/restclient updated on 2010-07-07

6916. By Florian Fuchs on 2010-07-07: added _Domain and _List classes, fixed code style issues

Revision history for this message

Florian Fuchs (flo-fuchs) wrote on 2010-07-07:

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

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.

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?

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.

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 updated on 2010-07-15

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

Revision history for this message

Barry Warsaw (barry) wrote on 2010-07-16:

Download full text (18.8 KiB)

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

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: test-two@example.com
> +    ...
> +
> +Later the new list can be instanciated using get_list():

s/instanciated/instantiated/

> +
> +    >>> some_list = client.get_list('test-one@example.com')
> +
> +The lists have been added and get_lists() returns a list of dicts, sorted
> +by fqdn_listname.
> +
> +    >>> lists = client.get_lists()
> +    >>> for i, list in enumerate(lists):
> +    ...     print 'entry %d:' % i 
> +    ...     for key in sorted(list):
> +    ...         print '    {0}: {1}'.format(key, list[key])
> +    entry 0:
> +        fqdn_listname: test-one@example.com
> +        ...
> +    entry 1:
> +        fqdn_listname: test-two@example.com
> +        ...
> +
> +
> +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('jack@example.com', 'Jack')
> +    201
> +    >>> new_list.subscribe('meg@example.com', 'Meg')
> +    201
> +    >>> another_list.subscribe('jack@example.com', 'Jack')
> +    201
> +
> +We can get a list of all members:
> +
> +    >>> members = client.get_members()
> +    >>> for i, member in enumerate(members):
> +    ...     print 'entry %d:' % i 
> +    ...     for key in sorted(member):
> +    ...         print '    {0}: {1}'.format(key, member[key])

The other thing to think about is, if you repeat similar code in the doctest,
you can always refactor them into a function defined in the doctest.

> +    entry 0:
> +        ...
> +        self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/jack@example.com
> +    entry 1:
> +        ...
> +        self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com
> +    entry 2:
> +        ...
> +        self_link: http://localhost:8001/3.0/lists/test-two@example.com/member/jack@example.com

The client is returning json here, right?

> +
> +Or just the members of a specific list:
> +
> +    >>> new_list_members = new_list.get_members()
> +    >>> for i, member in enumerate(new_list_members):
> +    ...     print 'entry %d:' % i 
> +    ...     for key in sorted(member):
> +    ...         print '    {0}: {1}'.format(key, member[key])
> +    entry 0:
> +        http_etag: ...
> +        self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/jack@example.com
> +    entry 1:
> +        http_etag: ...
> +        self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com
> +
> +After a while Meg decides to unsubscribe from the mailing list (like
> +.subscribe() .unsubscribe() returns an HTTP status code, ideally 200).
> +
> +    >>> new_list.unsubscribe('meg@example.com')
> +    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-15 14:02:55 +0000
> @@ -0,0 +1,321 @@
> +# 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 json
> +import re

Believe it or not, these lines should be reversed.  I know it's weird but non
from-import-* imports get sorted in increasing length.  Just a little quirk I
inherited from Guido. :)

import re
import json

> +
> +from httplib import HTTPConnection, HTTPException
> +from urllib import urlencode

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

> +
> +
> +class MailmanRESTClientError(Exception):
> +    """An exception thrown by the Mailman REST API client."""
> +

You don't need that extra blank line.  Strictly speaking, you don't need the
following 'pass' line either!

> +    pass
> +
> +
> +class MailmanRESTClient(object):

The base class isn't necessary here, because the '__metaclass__ = type' at the
top of the module already makes all unadorned classes default to new style.

> +    """A wrapper for the Mailman REST API."""
> +    
> +    def __init__(self, host):
> +        """Connect to host

First line should end in a period, and there should be a blank line
following.  Also, I try to make :param: descriptions full sentences too
(capitalized and ending in a period).  Not the :type: description though.
Apply these rules to all the following docstrings.

> +        :param host: the host name of the REST API 
> +        :type host: string
> +        """
> +        self.host = host
> +        # If there is a trailing slash remove it
> +        if self.host[-1] == '/':
> +            self.host = self.host[:-1]
> +        # Include general header information
> +        self.headers = {
> +            'User-Agent': 'MailmanRESTClient',
> +            'Accept': 'text/plain', 
> +            }
> +        self.c = HTTPConnection(self.host)

Abbreviations == bad! :)  Well, usually. ;)

You can use self.connection or self.conn here to make it a little more
readable.   OTOH, in dump_json(), we create a new HTTP() instance for every
request.  I wonder if that wouldn't be a better way to go.  IOW, what's the
advantage of caching this HTTPConnection instance in this attribute?

> +
> +    def __repr__(self):
> +        return '<MailmanRESTClient: %s>' % self.host
> +
> +    def _get_request(self, path):
> +        """Send an HTTP GET request.
> +        
> +        :param path: the URL to send the GET request to
> +        :type path: string
> +        :rtype: list

Add an :return: to describe what gets returned.

> +        """
> +        try:
> +            self.c.request('GET', path, None, self.headers)
> +            r = self.c.getresponse()
> +            raw_data = r.read()
> +            data = json.loads(raw_data)
> +            return data
> +        finally:
> +            self.c.close()
> +
> +    def _post_request(self, path, data):
> +        """Send an HTTP POST request.
> +        
> +        :param path: the URL to send POST request to
> +        :type path: string
> +        :param data: the post data to send
> +        :type data: dict
> +        :return: request status code
> +        :rtype: string
> +        """
> +        self.headers['Content-type'] = "application/x-www-form-urlencoded"
> +        try:
> +            self.c.request('POST', path, urlencode(data), self.headers)
> +            r = self.c.getresponse()
> +            return r.status
> +        finally:
> +            self.c.close()
> +
> +    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?

> +        
> +    def _put_request(self, path, data):
> +        """Send an HTTP PUT request.
> +
> +        :param path: the URL to send the PUT request to
> +        :type path: string
> +        :param data: the put data to send
> +        :type data: dict
> +        :return: request status code
> +        :rtype: string
> +        """
> +        self.headers['Content-type'] = "application/x-www-form-urlencoded"
> +        try:
> +            self.c.request('PUT', path, urlencode(data), self.headers)
> +            r = self.c.getresponse()
> +            return r.status
> +        finally:
> +            self.c.close()
> +
> +    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)?

> +
> +    def _validate_email_address(self, email_address):
> +        """Validates an email address.
> +        
> +        :param email_address: the email string to validate
> +        :type email_address: string
> +        """
> +        pat = re.compile('^[-a-z0-9_.]+@[-a-z0-9\.]+\.[a-z]{2,6}$',re.IGNORECASE)
> +        if not pat.match(email_address):
> +            raise MailmanRESTClientError('%s is not a valid email_address' % email_address)

Same question here.

> +
> +    def create_domain(self, email_host):
> +        """Create a domain and return a domain object.
> +        
> +        :param email_host: host domain to create
> +        :type email_host: string
> +        :rtype object
> +        """
> +        self._validate_email_host(email_host)
> +        data = {
> +            'email_host': email_host,
> +            }
> +        r = self._post_request('/3.0/domains', data)
> +        return _Domain(self.host, email_host)

Better to use 'response' rather than 'r'.

> +
> +    def get_domain(self, email_host):
> +        """Return a domain object.
> +        
> +        :param email_host: host domain
> +        :type email_host: string
> +        :rtype object
> +        """
> +        self._validate_email_host(email_host)
> +        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
> +        """
> +        r = self._get_request('/3.0/lists')
> +        if 'entries' not in r:
> +            return []
> +        else:
> +            # Return a dict with entries sorted by fqdn_listname
> +            return sorted(r['entries'],
> +                key=lambda list: list['fqdn_listname'])

lambdas are kind of icky, how about:

# Move this to the imports at the top of the file.
    from operator import itemgetter

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
> +        """
> +        self._validate_email_address(fqdn_listname)
> +        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
> +        """
> +        r = self._get_request('/3.0/members')
> +        if 'entries' not in r:
> +            return []
> +        else:
> +            return sorted(r['entries'],
> +                key=lambda member: member['self_link'])

See above.

> +
> +
> +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
> +        """
> +        MailmanRESTClient.__init__(self, host)

For new-style classes:

super(_Domain, self).__init__(host)

> +        self.domain_info = self._get_request('/3.0/domains/' + email_host)
> +
> +    def get_domaininfo(self):
> +        """Get information about the domain.
> +
> +        :rtype: dict
> +        """
> +        return self.domain_info

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'?

> +
> +    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.domain_info['email_host']
> +        self._validate_email_address(fqdn_listname)
> +        data = {
> +            'fqdn_listname': fqdn_listname
> +            }
> +        r = self._post_request('/3.0/lists', data)
> +        return _List(self.host, fqdn_listname)
> +
> +    def delete_list(self, list_name):
> +        return self._delete_request('/3.0/lists/' + fqdn_listname)
> +
> +
> +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
> +        """
> +        MailmanRESTClient.__init__(self, host)
> +        self.list_info = self._get_request('/3.0/lists/' + fqdn_listname)
> +
> +    def get_listinfo(self):
> +        """Get information about the list.
> +
> +        :rtype: dict
> +        """
> +        return self.list_info
> +
> +    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
> +        """
> +        self._validate_email_address(address)
> +        data = {
> +            'fqdn_listname': self.list_info['fqdn_listname'],
> +            'address': address,
> +            'real_name': real_name
> +            }
> +        return self._post_request('/3.0/members', data)
> +
> +    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
> +        """
> +        self._validate_email_address(address)
> +        return self._delete_request('/3.0/lists/' +
> +                                self.list_info['fqdn_listname'] +
> +                                '/member/' +
> +                                address)
> +
> +    def get_members(self):
> +        """Get a list of all list members.
> +
> +        :return: a list of dicts with all members
> +        :rtype: list
> +        """
> +        r = self._get_request('/3.0/lists/' +
> +                          self.list_info['fqdn_listname'] +
> +                          '/roster/members')
> +        if 'entries' not in r:
> +            return []
> +        else:
> +            return sorted(r['entries'],
> +                key=lambda member: member['self_link'])
> +

review: Needs Fixing

lp:~flo-fuchs/mailman/restclient updated on 2010-07-20

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

Revision history for this message

Florian Fuchs (flo-fuchs) wrote on 2010-07-28:

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.

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

> > +    entry 0:
> > +        ...
> > +        self_link: http://localhost:8001/3.0/lists/test-
> one@example.com/member/jack@example.com
> > +    entry 1:
> > +        ...
> > +        self_link: http://localhost:8001/3.0/lists/test-
> one@example.com/member/meg@example.com
> > +    entry 2:
> > +        ...
> > +        self_link: http://localhost:8001/3.0/lists/test-
> two@example.com/member/jack@example.com
> 
> 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

Revision history for this message

Barry Warsaw (barry) wrote on 2010-12-26:

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to status/vote changes:

Bernard

 === 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'postmaster@example.com',
++     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'test-two@example.com',
++     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/test-two@example.com'}
++
++Later the new list can be instantiated using get_list():
++
++    >>> some_list = client.get_list('test-one@example.com')
++
++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'test-one@example.com',
++      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/test-one@example.com'},
++     {u'fqdn_listname': u'test-two@example.com',
++      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/test-two@example.com'}]
++
++
++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('jack@example.com', 'Jack')
++    201
++    >>> new_list.subscribe('meg@example.com', 'Meg')
++    201
++    >>> another_list.subscribe('jack@example.com', '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/test-one@example.com/member/jack@example.com'},
++     {u'http_etag': u'"cd75b7e93216a022573534d948511edfbfea06cd"',
++      u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com'},
++     {u'http_etag': u'"13399f5ebbab8c474926a7ad0ccfda28d717e398"',
++      u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com/member/jack@example.com'}]
++
++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/test-one@example.com/member/jack@example.com'},
++     {u'http_etag': u'"cd75b7e93216a022573534d948511edfbfea06cd"',
++      u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com'}]
++
++After a while Meg decides to unsubscribe from the mailing list (like
++.subscribe() .unsubscribe() returns an HTTP status code, ideally 200).
++
++    >>> new_list.unsubscribe('meg@example.com')
++    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'))
++

GNU Mailman

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

Commit message

Description of the change

Preview Diff

Subscribers