Skip to content

Commit

Permalink
Make Redmine class picklable
Browse files Browse the repository at this point in the history
The `Redmine` class couldn't be pickled because the `gettatr` would attempt to
turn any attribute access into a `Resource`, even `__getstate__`.

The proposed fix treats attributes that begin with an underscore (and
double-underscore) as non-`Resource` attributes and returns their value as-is.

Fixes #64
  • Loading branch information
rconradharris committed Nov 3, 2014
1 parent a827812 commit 925a7bc
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Changelog
a list)
- Fixed: `Issue #59 <https://github.com/maxtepkeev/python-redmine/issues/59>`__ (Raise ForbiddenError
when a 403 is encountered) (thanks to `Rick Harris <https://github.com/rconradharris>`__)
- Fixed: `Issue #64 <https://github.com/maxtepkeev/python-redmine/issues/64>`__ (Make the Redmine class picklable) (thanks to `Rick Harris <https://github.com/rconradharris>`__)

1.0.1 (2014-09-23)
++++++++++++++++++
Expand Down
8 changes: 7 additions & 1 deletion redmine/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ def __init__(self, url, **kwargs):

def __getattr__(self, resource):
"""Returns either ResourceSet or Resource object depending on the method used on the ResourceManager"""
return ResourceManager(self, resource)
if resource.startswith('_'):
try:

This comment has been minimized.

Copy link
@maxtepkeev

maxtepkeev Nov 4, 2014

Owner

Why not just

return super(Redmine, self).__getattr__(resource)

instead of try/catch block

This comment has been minimized.

Copy link
@rconradharris

rconradharris Nov 4, 2014

Author Contributor

So that was the first thing I tried :)

Turns out object doesn't have a __getattr__ or getattr method.

Also tried object.__getattr__(resource) just to be sure it wasn't a new-style class issue.

This comment has been minimized.

Copy link
@rconradharris

rconradharris Nov 4, 2014

Author Contributor

Hmm, I just read up on __getattr__ again. So it's only called if the attribute doesn't already exist (as opposed to __getattribute__ which is always called.

So in this case, if the attribute stats with an underscore, we could just raise an AttributeError.

This comment has been minimized.

Copy link
@maxtepkeev

maxtepkeev Nov 5, 2014

Owner

Yeap. You're absolutely right. Raising AttributeError would be enough. I'll also add the same to _Resource class's __getattr__ method, otherwise it goes into infinite recursion when unpickling.

return self.__dict__[resource]
except KeyError:
raise AttributeError
else:
return ResourceManager(self, resource)

def upload(self, filepath):
"""Uploads file from filepath to Redmine and returns an assigned token"""
Expand Down
10 changes: 10 additions & 0 deletions tests/test_redmine.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,13 @@ def test_auth(self):
self.response.status_code = 200
self.response.json = json_response({'user': {'firstname': 'John', 'lastname': 'Smith', 'id': 1}})
self.assertEqual(self.redmine.auth().firstname, 'John')

def test_getattr_on_under_attribute(self):
"""
Attributes that begin with an underscore should not be treated as a
`Resource`, rather we should return the value as-is. The impetus of
this was to make `Redmine` picklable by preventing the `__getstate__`
access from being treated a `Resource`.
"""
self.redmine._foo = True
self.assertEqual(True, self.redmine._foo)

0 comments on commit 925a7bc

Please sign in to comment.