Merge pull request #1447 from adamcik/fix/1445-unify-stream-uri-handling

Make sure the stream backend playback/library handle URIs the same way

docs/changelog.rst

@@ -110,6 +110,12 @@ M3U backend
   - Improve reliability of playlist updates using the core playlist API by
     applying the write-replace pattern for file updates.
 
+Stream backend
+--------------
+
+- Make sure both lookup and playback correctly handle playlists and our
+  blacklist support. (Fixes: :issue:`1445`, PR: :issue:`1447`)
+
 MPD frontend
 ------------
 

mopidy/stream/actor.py

@@ -25,10 +25,19 @@ class StreamBackend(pykka.ThreadingActor, backend.Backend):
             timeout=config['stream']['timeout'],
             proxy_config=config['proxy'])
 
-        self.library = StreamLibraryProvider(
+        self._session = http.get_requests_session(
-            backend=self, blacklist=config['stream']['metadata_blacklist'])
+            proxy_config=config['proxy'],
-        self.playback = StreamPlaybackProvider(
+            user_agent='%s/%s' % (
-            audio=audio, backend=self, config=config)
+                stream.Extension.dist_name, stream.Extension.version))
+
+        blacklist = config['stream']['metadata_blacklist']
+        self._blacklist_re = re.compile(
+            r'^(%s)$' % '|'.join(fnmatch.translate(u) for u in blacklist))
+
+        self._timeout = config['stream']['timeout']
+
+        self.library = StreamLibraryProvider(backend=self)
+        self.playback = StreamPlaybackProvider(audio=audio, backend=self)
         self.playlists = None
 
         self.uri_schemes = audio_lib.supported_uri_schemes(
@@ -43,27 +52,23 @@ class StreamBackend(pykka.ThreadingActor, backend.Backend):
 
 
 class StreamLibraryProvider(backend.LibraryProvider):
-
-    def __init__(self, backend, blacklist):
-        super(StreamLibraryProvider, self).__init__(backend)
-        self._scanner = backend._scanner
-        self._blacklist_re = re.compile(
-            r'^(%s)$' % '|'.join(fnmatch.translate(u) for u in blacklist))
-
     def lookup(self, uri):
         if urllib.parse.urlsplit(uri).scheme not in self.backend.uri_schemes:
             return []
 
-        if self._blacklist_re.match(uri):
+        if self.backend._blacklist_re.match(uri):
             logger.debug('URI matched metadata lookup blacklist: %s', uri)
             return [Track(uri=uri)]
 
-        try:
+        _, scan_result = _unwrap_stream(
-            result = self._scanner.scan(uri)
+            uri, timeout=self.backend._timeout, scanner=self.backend._scanner,
-            track = tags.convert_tags_to_track(result.tags).replace(
+            requests_session=self.backend._session)
-                uri=uri, length=result.duration)
+
-        except exceptions.ScannerError as e:
+        if scan_result:
-            logger.warning('Problem looking up %s: %s', uri, e)
+            track = tags.convert_tags_to_track(scan_result.tags).replace(
+                uri=uri, length=scan_result.duration)
+        else:
+            logger.warning('Problem looking up %s: %s', uri)
             track = Track(uri=uri)
 
         return [track]
@@ -71,23 +76,21 @@ class StreamLibraryProvider(backend.LibraryProvider):
 
 class StreamPlaybackProvider(backend.PlaybackProvider):
 
-    def __init__(self, audio, backend, config):
-        super(StreamPlaybackProvider, self).__init__(audio, backend)
-        self._config = config
-        self._scanner = backend._scanner
-        self._session = http.get_requests_session(
-            proxy_config=config['proxy'],
-            user_agent='%s/%s' % (
-                stream.Extension.dist_name, stream.Extension.version))
-
     def translate_uri(self, uri):
-        return _unwrap_stream(
+        if urllib.parse.urlsplit(uri).scheme not in self.backend.uri_schemes:
-            uri,
+            return None
-            timeout=self._config['stream']['timeout'],
+
-            scanner=self._scanner,
+        if self.backend._blacklist_re.match(uri):
-            requests_session=self._session)
+            logger.debug('URI matched metadata lookup blacklist: %s', uri)
+            return uri
+
+        unwrapped_uri, _ = _unwrap_stream(
+            uri, timeout=self.backend._timeout, scanner=self.backend._scanner,
+            requests_session=self.backend._session)
+        return unwrapped_uri
 
 
+# TODO: cleanup the return value of this.
 def _unwrap_stream(uri, timeout, scanner, requests_session):
     """
     Get a stream URI from a playlist URI, ``uri``.
@@ -105,7 +108,7 @@ def _unwrap_stream(uri, timeout, scanner, requests_session):
             logger.info(
                 'Unwrapping stream from URI (%s) failed: '
                 'playlist referenced itself', uri)
-            return None
+            return None, None
         else:
             seen_uris.add(uri)
 
@@ -117,7 +120,7 @@ def _unwrap_stream(uri, timeout, scanner, requests_session):
                 logger.info(
                     'Unwrapping stream from URI (%s) failed: '
                     'timed out in %sms', uri, timeout)
-                return None
+                return None, None
             scan_result = scanner.scan(uri, timeout=scan_timeout)
         except exceptions.ScannerError as exc:
             logger.debug('GStreamer failed scanning URI (%s): %s', uri, exc)
@@ -130,14 +133,14 @@ def _unwrap_stream(uri, timeout, scanner, requests_session):
             ):
                 logger.debug(
                     'Unwrapped potential %s stream: %s', scan_result.mime, uri)
-                return uri
+                return uri, scan_result
 
         download_timeout = deadline - time.time()
         if download_timeout < 0:
             logger.info(
                 'Unwrapping stream from URI (%s) failed: timed out in %sms',
                 uri, timeout)
-            return None
+            return None, None
         content = http.download(
             requests_session, uri, timeout=download_timeout)
 
@@ -145,14 +148,14 @@ def _unwrap_stream(uri, timeout, scanner, requests_session):
             logger.info(
                 'Unwrapping stream from URI (%s) failed: '
                 'error downloading URI %s', original_uri, uri)
-            return None
+            return None, None
 
         uris = playlists.parse(content)
         if not uris:
             logger.debug(
                 'Failed parsing URI (%s) as playlist; found potential stream.',
                 uri)
-            return uri
+            return uri, None
 
         # TODO Test streams and return first that seems to be playable
         logger.debug(

tests/stream/test_library.py

@@ -4,7 +4,6 @@ import mock
 
 import pytest
 
-from mopidy.audio import scan
 from mopidy.internal import path
 from mopidy.models import Track
 from mopidy.stream import actor
@@ -13,16 +12,23 @@ from tests import path_to_data_dir
 
 
 @pytest.fixture
-def scanner():
+def config():
-    return scan.Scanner(timeout=100, proxy_config={})
+    return {
+        'proxy': {},
+        'stream': {
+            'timeout': 1000,
+            'metadata_blacklist': [],
+            'protocols': ['file'],
+        },
+        'file': {
+            'enabled': False
+        },
+    }
 
 
 @pytest.fixture
-def backend(scanner):
+def audio():
-    backend = mock.Mock()
+    return mock.Mock()
-    backend.uri_schemes = ['file']
-    backend._scanner = scanner
-    return backend
 
 
 @pytest.fixture
@@ -30,26 +36,28 @@ def track_uri():
     return path.path_to_uri(path_to_data_dir('song1.wav'))
 
 
-def test_lookup_ignores_unknown_scheme(backend):
+def test_lookup_ignores_unknown_scheme(audio, config):
-    library = actor.StreamLibraryProvider(backend, [])
+    backend = actor.StreamBackend(audio=audio, config=config)
-
+    assert backend.library.lookup('http://example.com') == []
-    assert library.lookup('http://example.com') == []
 
 
-def test_lookup_respects_blacklist(backend, track_uri):
+def test_lookup_respects_blacklist(audio, config, track_uri):
-    library = actor.StreamLibraryProvider(backend, [track_uri])
+    config['stream']['metadata_blacklist'].append(track_uri)
+    backend = actor.StreamBackend(audio=audio, config=config)
 
-    assert library.lookup(track_uri) == [Track(uri=track_uri)]
+    assert backend.library.lookup(track_uri) == [Track(uri=track_uri)]
 
 
-def test_lookup_respects_blacklist_globbing(backend, track_uri):
+def test_lookup_respects_blacklist_globbing(audio, config, track_uri):
-    blacklist = [path.path_to_uri(path_to_data_dir('')) + '*']
+    blacklist_glob = path.path_to_uri(path_to_data_dir('')) + '*'
-    library = actor.StreamLibraryProvider(backend, blacklist)
+    config['stream']['metadata_blacklist'].append(blacklist_glob)
+    backend = actor.StreamBackend(audio=audio, config=config)
 
-    assert library.lookup(track_uri) == [Track(uri=track_uri)]
+    assert backend.library.lookup(track_uri) == [Track(uri=track_uri)]
 
 
-def test_lookup_converts_uri_metadata_to_track(backend, track_uri):
+def test_lookup_converts_uri_metadata_to_track(audio, config, track_uri):
-    library = actor.StreamLibraryProvider(backend, [])
+    backend = actor.StreamBackend(audio=audio, config=config)
 
-    assert library.lookup(track_uri) == [Track(length=4406, uri=track_uri)]
+    result = backend.library.lookup(track_uri)
+    assert result == [Track(length=4406, uri=track_uri)]

tests/stream/test_playback.py

@@ -4,6 +4,8 @@ import mock
 
 import pytest
 
+import requests.exceptions
+
 import responses
 
 from mopidy import exceptions
@@ -27,6 +29,11 @@ def config():
         'proxy': {},
         'stream': {
             'timeout': TIMEOUT,
+            'metadata_blacklist': [],
+            'protocols': ['http'],
+        },
+        'file': {
+            'enabled': False
         },
     }
 
@@ -36,24 +43,21 @@ def audio():
     return mock.Mock()
 
 
-@pytest.fixture
+@pytest.yield_fixture
 def scanner():
-    scan_mock = mock.Mock(spec=scan.Scanner)
+    patcher = mock.patch.object(scan, 'Scanner')
-    scan_mock.scan.return_value = None
+    yield patcher.start()()
-    return scan_mock
+    patcher.stop()
 
 
 @pytest.fixture
-def backend(scanner):
+def backend(audio, config, scanner):
-    backend = mock.Mock()
+    return actor.StreamBackend(audio=audio, config=config)
-    backend.uri_schemes = ['file']
-    backend._scanner = scanner
-    return backend
 
 
 @pytest.fixture
-def provider(audio, backend, config):
+def provider(backend):
-    return actor.StreamPlaybackProvider(audio, backend, config)
+    return backend.playback
 
 
 class TestTranslateURI(object):
@@ -184,14 +188,24 @@ class TestTranslateURI(object):
             % STREAM_URI in caplog.text())
         assert result == STREAM_URI
 
-    def test_failed_download_returns_none(self, provider, caplog):
+    @responses.activate
-        with mock.patch.object(actor, 'http') as http_mock:
+    def test_failed_download_returns_none(self, scanner, provider, caplog):
-            http_mock.download.return_value = None
+        scanner.scan.side_effect = [
+            mock.Mock(mime='text/foo', playable=False)
+        ]
 
-            result = provider.translate_uri(PLAYLIST_URI)
+        responses.add(
+            responses.GET, PLAYLIST_URI,
+            body=requests.exceptions.HTTPError('Kaboom'))
+
+        result = provider.translate_uri(PLAYLIST_URI)
 
         assert result is None
 
+        assert (
+            'Unwrapping stream from URI (%s) failed: '
+            'error downloading URI' % PLAYLIST_URI) in caplog.text()
+
     @responses.activate
     def test_playlist_references_itself(self, scanner, provider, caplog):
         scanner.scan.side_effect = [