From 622b98502c12b7eff4a3980f04c18cecb9d3176d Mon Sep 17 00:00:00 2001 From: Christian Johansen Date: Fri, 23 Nov 2012 00:38:36 +0100 Subject: [PATCH 01/13] Add installation instructions for Fedora --- docs/installation/index.rst | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/installation/index.rst b/docs/installation/index.rst index 4ae04c40..89eca193 100644 --- a/docs/installation/index.rst +++ b/docs/installation/index.rst @@ -167,11 +167,18 @@ can install Mopidy from PyPI using Pip. sudo pacman -S base-devel python2-pip + + And on Fedora Linux from the official repositories:: + + sudo yum install -y gcc python-devel python-pip + #. Then you'll need to install all of Mopidy's hard dependencies: - Pykka >= 1.0:: sudo pip install -U pykka + # On Fedora the binary is called pip-python: + # sudo pip-python install -U pykka - GStreamer 0.10.x, with Python bindings. GStreamer is packaged for most popular Linux distributions. Search for GStreamer in your package manager, @@ -189,6 +196,12 @@ can install Mopidy from PyPI using Pip. sudo pacman -S gstreamer0.10-python gstreamer0.10-good-plugins \ gstreamer0.10-ugly-plugins + + If you use Fedora you can install GStreamer like this:: + + sudo yum install -y python-gst0.10 gstreamer0.10-plugins-good \ + gstreamer0.10-plugins-ugly gstreamer0.10-tools + #. Optional: If you want Spotify support in Mopidy, you'll need to install libspotify and the Python bindings, pyspotify. @@ -212,14 +225,24 @@ can install Mopidy from PyPI using Pip. Remember to adjust the above example for the latest libspotify version supported by pyspotify, your OS, and your CPU architecture. + #. If you're on Fedora, you must add a configuration file so libspotify.so + can be found: + + su -c 'echo "/usr/local/lib" > /etc/ld.so.conf.d/libspotify.conf' + sudo ldconfig + #. Then get, build, and install the latest release of pyspotify using Pip:: sudo pip install -U pyspotify + # Fedora: + # sudo python-pip install -U pyspotify #. Optional: If you want to scrobble your played tracks to Last.fm, you need pylast:: sudo pip install -U pylast + # Fedora: + # sudo python-pip install -U pylast #. Optional: To use MPRIS, e.g. for controlling Mopidy from the Ubuntu Sound Menu or from an UPnP client via Rygel, you need some additional @@ -233,6 +256,8 @@ can install Mopidy from PyPI using Pip. #. Then, to install the latest release of Mopidy:: sudo pip install -U mopidy + # Fedora: + # sudo python-pip install -U mopidy To upgrade Mopidy to future releases, just rerun this command. From 18b5496affde3c12dd16ce46839ccc3735fc8cb4 Mon Sep 17 00:00:00 2001 From: Christian Johansen Date: Fri, 23 Nov 2012 00:46:05 +0100 Subject: [PATCH 02/13] Typo! python-pip is actually pip-python Mass confusion: The package is called python-pip, the binary is pip-python --- docs/installation/index.rst | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/installation/index.rst b/docs/installation/index.rst index 4ae04c40..4d6c6238 100644 --- a/docs/installation/index.rst +++ b/docs/installation/index.rst @@ -161,17 +161,23 @@ can install Mopidy from PyPI using Pip. This is how you install it on Debian/Ubuntu:: - sudo apt-get install build-essential python-dev python-pip + sudo apt-get install build-essential python-dev pip-python And on Arch Linux from the official repository:: sudo pacman -S base-devel python2-pip + And on Fedora Linux from the official repositories:: + + sudo yum install -y gcc python-devel python-pip + #. Then you'll need to install all of Mopidy's hard dependencies: - Pykka >= 1.0:: sudo pip install -U pykka + # On Fedora the binary is called pip-python: + # sudo pip-python install -U pykka - GStreamer 0.10.x, with Python bindings. GStreamer is packaged for most popular Linux distributions. Search for GStreamer in your package manager, @@ -189,6 +195,12 @@ can install Mopidy from PyPI using Pip. sudo pacman -S gstreamer0.10-python gstreamer0.10-good-plugins \ gstreamer0.10-ugly-plugins + + If you use Fedora you can install GStreamer like this:: + + sudo yum install -y python-gst0.10 gstreamer0.10-plugins-good \ + gstreamer0.10-plugins-ugly gstreamer0.10-tools + #. Optional: If you want Spotify support in Mopidy, you'll need to install libspotify and the Python bindings, pyspotify. @@ -212,14 +224,24 @@ can install Mopidy from PyPI using Pip. Remember to adjust the above example for the latest libspotify version supported by pyspotify, your OS, and your CPU architecture. + #. If you're on Fedora, you must add a configuration file so libspotify.so + can be found: + + su -c 'echo "/usr/local/lib" > /etc/ld.so.conf.d/libspotify.conf' + sudo ldconfig + #. Then get, build, and install the latest release of pyspotify using Pip:: sudo pip install -U pyspotify + # Fedora: + # sudo pip-python install -U pyspotify #. Optional: If you want to scrobble your played tracks to Last.fm, you need pylast:: sudo pip install -U pylast + # Fedora: + # sudo pip-python install -U pylast #. Optional: To use MPRIS, e.g. for controlling Mopidy from the Ubuntu Sound Menu or from an UPnP client via Rygel, you need some additional @@ -233,6 +255,8 @@ can install Mopidy from PyPI using Pip. #. Then, to install the latest release of Mopidy:: sudo pip install -U mopidy + # Fedora: + # sudo pip-python install -U mopidy To upgrade Mopidy to future releases, just rerun this command. From c53f6838a6a09abbb0351946785bead6148d5ae8 Mon Sep 17 00:00:00 2001 From: Christian Johansen Date: Fri, 23 Nov 2012 00:54:38 +0100 Subject: [PATCH 03/13] Un-"fix" Ubuntu typo --- docs/installation/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/installation/index.rst b/docs/installation/index.rst index 4d6c6238..1ba214db 100644 --- a/docs/installation/index.rst +++ b/docs/installation/index.rst @@ -161,7 +161,7 @@ can install Mopidy from PyPI using Pip. This is how you install it on Debian/Ubuntu:: - sudo apt-get install build-essential python-dev pip-python + sudo apt-get install build-essential python-dev python-pip And on Arch Linux from the official repository:: From ec639d17ae63556a414adbb52decfa6509521977 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 23 Nov 2012 12:47:57 +0100 Subject: [PATCH 04/13] docs: Unbreak build --- mopidy/utils/path.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 27be1864..73063183 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -25,9 +25,9 @@ XDG_DIRS = { 'XDG_DATA_DIR': XDG_DATA_DIR, 'XDG_MUSIC_DIR': XDG_MUSIC_DIR, } -DATA_PATH = os.path.join(XDG_DATA_DIR, 'mopidy') -SETTINGS_PATH = os.path.join(XDG_CONFIG_DIR, 'mopidy') -SETTINGS_FILE = os.path.join(SETTINGS_PATH, 'settings.py') +DATA_PATH = os.path.join(unicode(XDG_DATA_DIR), 'mopidy') +SETTINGS_PATH = os.path.join(unicode(XDG_CONFIG_DIR), 'mopidy') +SETTINGS_FILE = os.path.join(unicode(SETTINGS_PATH), 'settings.py') def get_or_create_folder(folder): From 6403e3e3d10df28b521cea955a891c6c7de36944 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 23 Nov 2012 12:48:41 +0100 Subject: [PATCH 05/13] spotify: Reuse artist, album, and track models This adds a module-level cache of artist, album, and track models, whose content is almost entirely static. This cache is used when we convert from pyspotify to Mopidy objects. Previously, an album with 15 tracks would create 15 track objects, 15 artists objects on the tracks, 15 album objects on the tracks, and 15 artist objects on the 15 album objects, in total 60 objects. With this change, we only create 15 track objects, 1 album object, and 1 artist object, in total 17 objects. Measurements with 90 playlists containing about 6500 tracks in total shows that this reduces the number of Artist objects from 13600 to 3800, and the number of Album objects from 6500 to 4500 objects. An unscientific measurement of memory usage using ps(1) indicated a reduction in RSS from 71MB to 65MB, measured right after the Spotify playlists was loaded the first time. --- mopidy/backends/spotify/translator.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/mopidy/backends/spotify/translator.py b/mopidy/backends/spotify/translator.py index 92b4514e..4d2f36a0 100644 --- a/mopidy/backends/spotify/translator.py +++ b/mopidy/backends/spotify/translator.py @@ -6,32 +6,45 @@ from mopidy import settings from mopidy.models import Artist, Album, Track, Playlist +artist_cache = {} +album_cache = {} +track_cache = {} + + def to_mopidy_artist(spotify_artist): if spotify_artist is None: return uri = str(Link.from_artist(spotify_artist)) + if uri in artist_cache: + return artist_cache[uri] if not spotify_artist.is_loaded(): return Artist(uri=uri, name='[loading...]') - return Artist(uri=uri, name=spotify_artist.name()) + artist_cache[uri] = Artist(uri=uri, name=spotify_artist.name()) + return artist_cache[uri] def to_mopidy_album(spotify_album): if spotify_album is None: return uri = str(Link.from_album(spotify_album)) + if uri in album_cache: + return album_cache[uri] if not spotify_album.is_loaded(): return Album(uri=uri, name='[loading...]') - return Album( + album_cache[uri] = Album( uri=uri, name=spotify_album.name(), artists=[to_mopidy_artist(spotify_album.artist())], date=spotify_album.year()) + return album_cache[uri] def to_mopidy_track(spotify_track): if spotify_track is None: return uri = str(Link.from_track(spotify_track, 0)) + if uri in track_cache: + return track_cache[uri] if not spotify_track.is_loaded(): return Track(uri=uri, name='[loading...]') spotify_album = spotify_track.album() @@ -39,7 +52,7 @@ def to_mopidy_track(spotify_track): date = spotify_album.year() else: date = None - return Track( + track_cache[uri] = Track( uri=uri, name=spotify_track.name(), artists=[to_mopidy_artist(a) for a in spotify_track.artists()], @@ -48,6 +61,7 @@ def to_mopidy_track(spotify_track): date=date, length=spotify_track.duration(), bitrate=settings.SPOTIFY_BITRATE) + return track_cache[uri] def to_mopidy_playlist(spotify_playlist): From 7f987cb1e25827a6cb78f38d53de201450e6e085 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 20:36:04 +0100 Subject: [PATCH 06/13] jsonrpc: Lookup methods using the objects map directly --- mopidy/utils/jsonrpc.py | 61 ++++++++++++------------------------- tests/utils/jsonrpc_test.py | 2 ++ 2 files changed, 21 insertions(+), 42 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index 23f50580..c280fc27 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -37,10 +37,7 @@ class JsonRpcWrapper(object): hello -> lambda abc.def -> abc.def() - Only the public methods of the objects will be exposed. Attributes are not - exposed by themself, but public methods on public attributes are exposed, - using dotted paths from the exposed object to the method at the end of the - path. + Only the public methods of the mounted objects will be exposed. If a method returns a :class:`pykka.Future`, the future will be completed and its value unwrapped before the JSON-RPC wrapper returns the response. @@ -59,32 +56,10 @@ class JsonRpcWrapper(object): """ def __init__(self, objects, decoders=None, encoders=None): - self.obj = self._build_exported_object(objects) + self.objects = objects self.decoder = get_combined_json_decoder(decoders or []) self.encoder = get_combined_json_encoder(encoders or []) - def _build_exported_object(self, objects): - class EmptyObject(object): - pass - - if '' in objects: - exported_object = objects[''] - else: - exported_object = EmptyObject() - - mounts = sorted(objects.keys(), key=lambda x: len(x)) - for mount in mounts: - parent = exported_object - path = mount.split('.') - for part in path[:-1]: - if not hasattr(parent, part): - setattr(parent, part, EmptyObject()) - parent = getattr(parent, part) - if path[-1]: - setattr(parent, path[-1], objects[mount]) - - return exported_object - def handle_json(self, request): """ Handles an incoming request encoded as a JSON string. @@ -205,15 +180,21 @@ class JsonRpcWrapper(object): raise JsonRpcInvalidRequestError( data='"params", if given, must be an array or an object') - def _get_method(self, name): + def _get_method(self, method_path): + if inspect.isroutine(self.objects.get(method_path, None)): + # The mounted object is the callable + return self.objects[method_path] + + # The mounted object contains the callable + if '.' in method_path: + mount, method_name = method_path.rsplit('.', 1) + else: + mount, method_name = '', method_path try: - path = name.split('.') - this = self.obj - for part in path: - if part.startswith('_'): - raise AttributeError - this = getattr(this, part) - return this + if method_name.startswith('_'): + raise AttributeError + obj = self.objects[mount] + return getattr(obj, method_name) except (AttributeError, KeyError): raise JsonRpcMethodNotFoundError() @@ -309,13 +290,9 @@ class JsonRpcInspector(object): 'abc': Abc, }) - Since this inspector is based on inspecting classes and not instances, it - will not give you a complete picture of what is actually exported by - :class:`JsonRpcWrapper`. In particular: - - - it will not include methods added dynamically, and - - it will not include public methods on attributes on the instances that - are to be exposed. + Since the inspector is based on inspecting classes and not instances, it + will not include methods added dynamically. The wrapper works with + instances, and it will thus export dynamically added methods as well. :param objects: mapping between mounts and exposed functions or classes :type objects: dict diff --git a/tests/utils/jsonrpc_test.py b/tests/utils/jsonrpc_test.py index 39ad8a81..ea99a8f3 100644 --- a/tests/utils/jsonrpc_test.py +++ b/tests/utils/jsonrpc_test.py @@ -45,6 +45,8 @@ class JsonRpcTestBase(unittest.TestCase): objects={ 'hello': lambda: 'Hello, world!', 'core': self.core, + 'core.playback': self.core.playback, + 'core.tracklist': self.core.tracklist, '': Calculator(), }, encoders=[models.ModelJSONEncoder], From 869fcd2d8e43253e1e40432d0e55219daee1b58b Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 20:42:48 +0100 Subject: [PATCH 07/13] jsonrpc: Move future handling code to its own method --- mopidy/utils/jsonrpc.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index c280fc27..4287a94d 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -126,8 +126,7 @@ class JsonRpcWrapper(object): if self._is_notification(request): return None - if self._is_future(result): - result = result.get() + result = self._unwrap_result(result) return { 'jsonrpc': '2.0', @@ -201,8 +200,10 @@ class JsonRpcWrapper(object): def _is_notification(self, request): return 'id' not in request - def _is_future(self, result): - return isinstance(result, pykka.Future) + def _unwrap_result(self, result): + if isinstance(result, pykka.Future): + result = result.get() + return result class JsonRpcError(Exception): From 253695222be212919e3621d157b97d173ece40f0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 20:58:34 +0100 Subject: [PATCH 08/13] jsonrpc: Clearify usage docs --- mopidy/utils/jsonrpc.py | 51 +++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index 4287a94d..aa6e42ce 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -15,29 +15,46 @@ class JsonRpcWrapper(object): processing of JSON-RPC 2.0 messages. The transport of the messages over HTTP, WebSocket, TCP, or whatever is of no concern to this class. - To expose a single object, add it to the objects mapping using the empty - string as the key:: + The wrapper supports exporting the methods of multiple objects. If so, they + must be exported with different prefixes, called "mounts". - jrw = JsonRpcWrapper(objects={'': my_object}) + - To expose a single object, add it to the objects mapping using the empty + string as the mount:: - To expose multiple objects, add them all to the objects mapping. The key in - the mapping is used as the object's mounting point in the exposed API:: + jrw = JsonRpcWrapper(objects={'': my_object}) - jrw = JsonRpcWrapper(objects={ - '': foo, - 'hello': lambda: 'Hello, world!', - 'abc': abc, - }) + If ``my_object`` got a method named ``my_method()`` will be exported as + the JSON-RPC 2.0 method name ``my_method``. - This will create the following mapping between JSON-RPC 2.0 method names - and Python callables:: + - To expose multiple objects, add them all to the objects mapping. The key + in the mapping is used as the object's mounting point in the exposed + API:: - bar -> foo.bar() - baz -> foo.baz() - hello -> lambda - abc.def -> abc.def() + jrw = JsonRpcWrapper(objects={ + '': foo, + 'hello': lambda: 'Hello, world!', + 'abc': abc, + }) - Only the public methods of the mounted objects will be exposed. + This will export the Python callables on the left as the JSON-RPC 2.0 + method names on the right:: + + foo.bar() -> bar + foo.baz() -> baz + lambda -> hello + abc.def() -> abc.def + + If the ``foo`` object mounted at the root also got a method named + ``hello``, there will be a name collision between ``foo.hello()`` and the + lambda function mounted at ``hello``. In that case, the JSON-RPC 2.0 + method name ``hello`` will refer to the lambda function, because it was + mounted explicitly as ``hello``. + + It is recommended to avoid name collisions entirely by using non-empty + mounts for all objects. + + Only the public methods of the mounted objects, or functions/methods + included directly in the mapping, will be exposed. If a method returns a :class:`pykka.Future`, the future will be completed and its value unwrapped before the JSON-RPC wrapper returns the response. From 609fdc46ca649f4dec5fa1ce49f4093a1d9f43c1 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 21:13:55 +0100 Subject: [PATCH 09/13] jsonrpc: Explain why call to private method failed --- mopidy/utils/jsonrpc.py | 5 +++-- tests/utils/jsonrpc_test.py | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index aa6e42ce..4bcf3a1c 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -206,9 +206,10 @@ class JsonRpcWrapper(object): mount, method_name = method_path.rsplit('.', 1) else: mount, method_name = '', method_path + if method_name.startswith('_'): + raise JsonRpcMethodNotFoundError( + data='Private methods are not exported') try: - if method_name.startswith('_'): - raise AttributeError obj = self.objects[mount] return getattr(obj, method_name) except (AttributeError, KeyError): diff --git a/tests/utils/jsonrpc_test.py b/tests/utils/jsonrpc_test.py index ea99a8f3..fa9d2b4c 100644 --- a/tests/utils/jsonrpc_test.py +++ b/tests/utils/jsonrpc_test.py @@ -405,6 +405,7 @@ class JsonRpcSingleCommandErrorTest(JsonRpcTestBase): error = response['error'] self.assertEqual(error['code'], -32601) self.assertEqual(error['message'], 'Method not found') + self.assertEqual(error['data'], 'Private methods are not exported') def test_invalid_params_causes_invalid_params_error(self): request = { From 50814d3929b15c7077ea423e34ee7b0c4c31f8fc Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 21:23:55 +0100 Subject: [PATCH 10/13] jsonrpc: Explain why a method wasn't found --- mopidy/utils/jsonrpc.py | 14 ++++++++++++-- tests/utils/jsonrpc_test.py | 20 +++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index 4bcf3a1c..9aed38cd 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -202,18 +202,28 @@ class JsonRpcWrapper(object): return self.objects[method_path] # The mounted object contains the callable + if '.' in method_path: mount, method_name = method_path.rsplit('.', 1) else: mount, method_name = '', method_path + if method_name.startswith('_'): raise JsonRpcMethodNotFoundError( data='Private methods are not exported') + try: obj = self.objects[mount] + except KeyError: + raise JsonRpcMethodNotFoundError( + data='No object found at "%s"' % mount) + + try: return getattr(obj, method_name) - except (AttributeError, KeyError): - raise JsonRpcMethodNotFoundError() + except AttributeError: + raise JsonRpcMethodNotFoundError( + data='Object mounted at "%s" has no member "%s"' % ( + mount, method_name)) def _is_notification(self, request): return 'id' not in request diff --git a/tests/utils/jsonrpc_test.py b/tests/utils/jsonrpc_test.py index fa9d2b4c..8c487e87 100644 --- a/tests/utils/jsonrpc_test.py +++ b/tests/utils/jsonrpc_test.py @@ -381,11 +381,10 @@ class JsonRpcSingleCommandErrorTest(JsonRpcTestBase): self.assertEqual( error['data'], '"params", if given, must be an array or an object') - def test_unknown_method_causes_unknown_method_error(self): + def test_method_on_unknown_object_causes_unknown_method_error(self): request = { 'jsonrpc': '2.0', - 'method': 'bogus', - 'params': ['bogus'], + 'method': 'bogus.bogus', 'id': 1, } response = self.jrw.handle_data(request) @@ -393,6 +392,21 @@ class JsonRpcSingleCommandErrorTest(JsonRpcTestBase): error = response['error'] self.assertEqual(error['code'], -32601) self.assertEqual(error['message'], 'Method not found') + self.assertEqual(error['data'], 'No object found at "bogus"') + + def test_unknown_method_on_known_object_causes_unknown_method_error(self): + request = { + 'jsonrpc': '2.0', + 'method': 'core.bogus', + 'id': 1, + } + response = self.jrw.handle_data(request) + + error = response['error'] + self.assertEqual(error['code'], -32601) + self.assertEqual(error['message'], 'Method not found') + self.assertEqual( + error['data'], 'Object mounted at "core" has no member "bogus"') def test_private_method_causes_unknown_method_error(self): request = { From b33df8200a8e2dc932655c505728ca16c1b5bb40 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 21:25:00 +0100 Subject: [PATCH 11/13] jsonrpc: Grammar --- mopidy/utils/jsonrpc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index 9aed38cd..d31cccbd 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -23,7 +23,7 @@ class JsonRpcWrapper(object): jrw = JsonRpcWrapper(objects={'': my_object}) - If ``my_object`` got a method named ``my_method()`` will be exported as + If ``my_object`` has a method named ``my_method()`` will be exported as the JSON-RPC 2.0 method name ``my_method``. - To expose multiple objects, add them all to the objects mapping. The key From 8f604204dad138fb3dd3078fe566e174aebc56ec Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 21:40:12 +0100 Subject: [PATCH 12/13] jsonrpc: Don't allow objects at the root --- mopidy/utils/jsonrpc.py | 66 ++++++++++++++----------------------- tests/utils/jsonrpc_test.py | 46 +++++++++++++------------- 2 files changed, 47 insertions(+), 65 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index d31cccbd..47b660d3 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -15,43 +15,24 @@ class JsonRpcWrapper(object): processing of JSON-RPC 2.0 messages. The transport of the messages over HTTP, WebSocket, TCP, or whatever is of no concern to this class. - The wrapper supports exporting the methods of multiple objects. If so, they - must be exported with different prefixes, called "mounts". + The wrapper supports exporting the methods of one or more objects. Either + way, the objects must be exported with method name prefixes, called + "mounts". - - To expose a single object, add it to the objects mapping using the empty - string as the mount:: + To expose objects, add them all to the objects mapping. The key in the + mapping is used as the object's mounting point in the exposed API:: - jrw = JsonRpcWrapper(objects={'': my_object}) + jrw = JsonRpcWrapper(objects={ + 'foo': foo, + 'hello': lambda: 'Hello, world!', + }) - If ``my_object`` has a method named ``my_method()`` will be exported as - the JSON-RPC 2.0 method name ``my_method``. + This will export the Python callables on the left as the JSON-RPC 2.0 + method names on the right:: - - To expose multiple objects, add them all to the objects mapping. The key - in the mapping is used as the object's mounting point in the exposed - API:: - - jrw = JsonRpcWrapper(objects={ - '': foo, - 'hello': lambda: 'Hello, world!', - 'abc': abc, - }) - - This will export the Python callables on the left as the JSON-RPC 2.0 - method names on the right:: - - foo.bar() -> bar - foo.baz() -> baz - lambda -> hello - abc.def() -> abc.def - - If the ``foo`` object mounted at the root also got a method named - ``hello``, there will be a name collision between ``foo.hello()`` and the - lambda function mounted at ``hello``. In that case, the JSON-RPC 2.0 - method name ``hello`` will refer to the lambda function, because it was - mounted explicitly as ``hello``. - - It is recommended to avoid name collisions entirely by using non-empty - mounts for all objects. + foo.bar() -> foo.bar + foo.baz() -> foo.baz + lambda -> hello Only the public methods of the mounted objects, or functions/methods included directly in the mapping, will be exposed. @@ -73,6 +54,9 @@ class JsonRpcWrapper(object): """ def __init__(self, objects, decoders=None, encoders=None): + if '' in objects.keys(): + raise AttributeError( + 'The empty string is not allowed as an object mount') self.objects = objects self.decoder = get_combined_json_decoder(decoders or []) self.encoder = get_combined_json_encoder(encoders or []) @@ -305,18 +289,13 @@ class JsonRpcInspector(object): Inspects a group of classes and functions to create a description of what methods they can expose over JSON-RPC 2.0. - To inspect a single class, add it to the objects mapping using the empty - string as the key:: - - jri = JsonRpcInspector(objects={'': MyClass}) - - To inspect multiple classes, add them all to the objects mapping. The key - in the mapping is used as the classes' mounting point in the exposed API:: + To inspect one or more classes, add them all to the objects mapping. The + key in the mapping is used as the classes' mounting point in the exposed + API:: jri = JsonRpcInspector(objects={ - '': Foo, + 'foo': Foo, 'hello': lambda: 'Hello, world!', - 'abc': Abc, }) Since the inspector is based on inspecting classes and not instances, it @@ -328,6 +307,9 @@ class JsonRpcInspector(object): """ def __init__(self, objects): + if '' in objects.keys(): + raise AttributeError( + 'The empty string is not allowed as an object mount') self.objects = objects def describe(self): diff --git a/tests/utils/jsonrpc_test.py b/tests/utils/jsonrpc_test.py index 8c487e87..d4730be2 100644 --- a/tests/utils/jsonrpc_test.py +++ b/tests/utils/jsonrpc_test.py @@ -44,10 +44,10 @@ class JsonRpcTestBase(unittest.TestCase): self.jrw = jsonrpc.JsonRpcWrapper( objects={ 'hello': lambda: 'Hello, world!', + 'calc': Calculator(), 'core': self.core, 'core.playback': self.core.playback, 'core.tracklist': self.core.tracklist, - '': Calculator(), }, encoders=[models.ModelJSONEncoder], decoders=[models.model_json_decoder]) @@ -56,6 +56,12 @@ class JsonRpcTestBase(unittest.TestCase): pykka.ActorRegistry.stop_all() +class JsonRpcSetupTest(JsonRpcTestBase): + def test_empty_object_mounts_is_not_allowed(self): + test = lambda: jsonrpc.JsonRpcWrapper(objects={'': Calculator()}) + self.assertRaises(AttributeError, test) + + class JsonRpcSerializationTest(JsonRpcTestBase): def test_handle_json_converts_from_and_to_json(self): self.jrw.handle_data = mock.Mock() @@ -132,10 +138,10 @@ class JsonRpcSingleCommandTest(JsonRpcTestBase): self.assertNotIn('error', response) self.assertEqual(response['result'], 'Hello, world!') - def test_call_method_on_plain_object_as_root(self): + def test_call_method_on_plain_object(self): request = { 'jsonrpc': '2.0', - 'method': 'model', + 'method': 'calc.model', 'id': 1, } response = self.jrw.handle_data(request) @@ -148,7 +154,7 @@ class JsonRpcSingleCommandTest(JsonRpcTestBase): def test_call_method_which_returns_dict_from_plain_object(self): request = { 'jsonrpc': '2.0', - 'method': 'describe', + 'method': 'calc.describe', 'id': 1, } response = self.jrw.handle_data(request) @@ -510,6 +516,10 @@ class JsonRpcBatchErrorTest(JsonRpcTestBase): class JsonRpcInspectorTest(JsonRpcTestBase): + def test_empty_object_mounts_is_not_allowed(self): + test = lambda: jsonrpc.JsonRpcInspector(objects={'': Calculator}) + self.assertRaises(AttributeError, test) + def test_can_describe_method_on_root(self): inspector = jsonrpc.JsonRpcInspector({ 'hello': lambda: 'Hello, world!', @@ -520,24 +530,24 @@ class JsonRpcInspectorTest(JsonRpcTestBase): self.assertIn('hello', methods) self.assertEqual(len(methods['hello']['params']), 0) - def test_inspector_can_describe_methods_on_the_root_class(self): + def test_inspector_can_describe_an_object_with_methods(self): inspector = jsonrpc.JsonRpcInspector({ - '': Calculator, + 'calc': Calculator, }) methods = inspector.describe() - self.assertIn('add', methods) + self.assertIn('calc.add', methods) self.assertEqual( - methods['add']['description'], + methods['calc.add']['description'], 'Returns the sum of the given numbers') - self.assertIn('sub', methods) - self.assertIn('take_it_all', methods) - self.assertNotIn('_secret', methods) - self.assertNotIn('__init__', methods) + self.assertIn('calc.sub', methods) + self.assertIn('calc.take_it_all', methods) + self.assertNotIn('calc._secret', methods) + self.assertNotIn('calc.__init__', methods) - method = methods['take_it_all'] + method = methods['calc.take_it_all'] self.assertIn('params', method) params = method['params'] @@ -559,16 +569,6 @@ class JsonRpcInspectorTest(JsonRpcTestBase): self.assertNotIn('default', params[4]) self.assertEqual(params[4]['kwargs'], True) - def test_inspector_can_describe_methods_not_on_the_root(self): - inspector = jsonrpc.JsonRpcInspector({ - 'calc': Calculator, - }) - - methods = inspector.describe() - - self.assertIn('calc.add', methods) - self.assertIn('calc.sub', methods) - def test_inspector_can_describe_a_bunch_of_large_classes(self): inspector = jsonrpc.JsonRpcInspector({ 'core.library': core.LibraryController, From 160626b3641efcc5708e50c6168a73fba3ccb806 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 25 Nov 2012 21:51:59 +0100 Subject: [PATCH 13/13] jsonrpc: Give explicit error if calling method without object path --- mopidy/utils/jsonrpc.py | 10 ++++++---- tests/utils/jsonrpc_test.py | 17 ++++++++++++++++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/mopidy/utils/jsonrpc.py b/mopidy/utils/jsonrpc.py index 47b660d3..8230aada 100644 --- a/mopidy/utils/jsonrpc.py +++ b/mopidy/utils/jsonrpc.py @@ -187,10 +187,12 @@ class JsonRpcWrapper(object): # The mounted object contains the callable - if '.' in method_path: - mount, method_name = method_path.rsplit('.', 1) - else: - mount, method_name = '', method_path + if '.' not in method_path: + raise JsonRpcMethodNotFoundError( + data='Could not find object mount in method name "%s"' % ( + method_path)) + + mount, method_name = method_path.rsplit('.', 1) if method_name.startswith('_'): raise JsonRpcMethodNotFoundError( diff --git a/tests/utils/jsonrpc_test.py b/tests/utils/jsonrpc_test.py index d4730be2..64b5e628 100644 --- a/tests/utils/jsonrpc_test.py +++ b/tests/utils/jsonrpc_test.py @@ -387,6 +387,21 @@ class JsonRpcSingleCommandErrorTest(JsonRpcTestBase): self.assertEqual( error['data'], '"params", if given, must be an array or an object') + def test_method_on_without_object_causes_unknown_method_error(self): + request = { + 'jsonrpc': '2.0', + 'method': 'bogus', + 'id': 1, + } + response = self.jrw.handle_data(request) + + error = response['error'] + self.assertEqual(error['code'], -32601) + self.assertEqual(error['message'], 'Method not found') + self.assertEqual( + error['data'], + 'Could not find object mount in method name "bogus"') + def test_method_on_unknown_object_causes_unknown_method_error(self): request = { 'jsonrpc': '2.0', @@ -417,7 +432,7 @@ class JsonRpcSingleCommandErrorTest(JsonRpcTestBase): def test_private_method_causes_unknown_method_error(self): request = { 'jsonrpc': '2.0', - 'method': '_secret', + 'method': 'core._secret', 'id': 1, } response = self.jrw.handle_data(request)