Do not cache images, web browsers do it well

9 files changed, 161 insertions, 183 deletions

diff --git a/TODO b/TODO
index 6d8a74f..e11ffd7 100644
--- a/TODO
+++ b/TODO
@@ -20,6 +20,7 @@
   - zerosugar/pipe.py uses several mountpoints
   - what mountpoints for deps
 - if feed contains regular implementations and packages, solve() will reuse implementations and there is no way to use packages
+- get rid of get_blob, cache only Implementation.data
 
 1.0
 ===
diff --git a/sugar_network/local/mountset.py b/sugar_network/local/mountset.py
index 111e83c..04c5723 100644
--- a/sugar_network/local/mountset.py
+++ b/sugar_network/local/mountset.py
@@ -172,41 +172,6 @@ class Mountset(dict, ad.CommandsProcessor, Commands, SyncCommands):
         if self.volume is not None:
             self.volume.close()
 
-    @ad.directory_command_pre(method='GET', arguments={'reply': ad.to_list})
-    def _Mountset_find_pre(self, request):
-        self._exclude_blobs(request)
-
-    @ad.directory_command_post(method='GET')
-    def _Mountset_find_post(self, request, response, result):
-        self._include_blobs(request, result['result'])
-        return result
-
-    @ad.document_command_pre(method='GET', arguments={'reply': ad.to_list})
-    def _Mountset_get_pre(self, request):
-        self._exclude_blobs(request)
-
-    @ad.document_command_post(method='GET')
-    def _Mountset_get_post(self, request, response, result):
-        self._include_blobs(request, [result])
-        return result
-
-    def _exclude_blobs(self, request):
-        reply = request.get('reply')
-        if reply:
-            reply_set = set(reply)
-            request.blobs = reply_set & self.get_blobs(request['document'])
-            if request.blobs:
-                reply[:] = list(reply_set - request.blobs)
-
-    def _include_blobs(self, request, result):
-        if not request.blobs:
-            return
-        for props in result:
-            guid = props.get('guid') or request['guid']
-            for name in request.blobs:
-                props[name] = 'http://localhost:%s/%s/%s/%s' % \
-                        (local.ipc_port.value, request['document'], guid, name)
-
     def _discover_masters(self):
         for host in zeroconf.browse_workstations():
             url = 'http://%s:%s' % (host, node.port.default)
diff --git a/sugar_network/node/commands.py b/sugar_network/node/commands.py
index 83a0520..56a3b93 100644
--- a/sugar_network/node/commands.py
+++ b/sugar_network/node/commands.py
@@ -135,31 +135,14 @@ class NodeCommands(ad.VolumeCommands, Commands):
             layer.remove('deleted')
         request['layer'] = layer
 
-    @ad.directory_command_post(method='GET')
-    def _NodeCommands_find_post(self, request, response, result):
-        self._mixin_blobs(request, result['result'])
-        return result
-
     @ad.document_command_post(method='GET')
     def _NodeCommands_get_post(self, request, response, result):
         directory = self.volume[request['document']]
         doc = directory.get(request['guid'])
         enforce('deleted' not in doc['layer'], ad.NotFound,
                 'Document deleted')
-        self._mixin_blobs(request, [result])
         return result
 
-    def _mixin_blobs(self, request, result):
-        document = request['document']
-        for props in result:
-            guid = props.get('guid') or request['guid']
-            for name, value in props.items():
-                if not isinstance(value, ad.PropertyMeta):
-                    continue
-                props[name] = value.url(
-                        default='/'.join(['', document, guid, name]),
-                        prefix='http://' + request.environ['HTTP_HOST'])
-
     def _set_author(self, props):
         users = self.volume['user']
         authors = []
diff --git a/sugar_network/resources/context.py b/sugar_network/resources/context.py
index 4a02e90..1745a82 100644
--- a/sugar_network/resources/context.py
+++ b/sugar_network/resources/context.py
@@ -63,7 +63,7 @@ class Context(Resource):
 
     @ad.active_property(ad.BlobProperty, mime_type='image/png')
     def icon(self, value):
-        if value is None:
+        if not value:
             if 'package' in self['type']:
                 return ad.PropertyMeta(
                         url='/static/images/package.png',
@@ -79,7 +79,7 @@ class Context(Resource):
 
     @ad.active_property(ad.BlobProperty, mime_type='image/svg+xml')
     def artifact_icon(self, value):
-        if value is None:
+        if not value:
             return ad.PropertyMeta(
                     url='/static/images/missing.svg',
                     path=join(static.PATH, 'images', 'missing.svg'),
diff --git a/sugar_network/resources/volume.py b/sugar_network/resources/volume.py
index c949bb1..20dc90e 100644
--- a/sugar_network/resources/volume.py
+++ b/sugar_network/resources/volume.py
@@ -19,9 +19,10 @@ from os.path import join
 
 import active_document as ad
 from active_document import directory as ad_directory
-from active_toolkit import coroutine
+from sugar_network import local
 from sugar_network.toolkit.sneakernet import DiskFull
 from sugar_network.toolkit.collection import Sequence
+from active_toolkit import coroutine
 
 
 ad_directory._LAYOUT_VERSION = 2
@@ -32,7 +33,6 @@ _logger = logging.getLogger('resources.volume')
 
 class Request(ad.Request):
 
-    blobs = None
     mountpoint = None
 
 
@@ -148,7 +148,6 @@ class Commands(object):
 
     def __init__(self):
         self._notifier = coroutine.AsyncResult()
-        self._blobs = {}
         self.connect(lambda event: self._notify(event))
 
     def connect(self, callback, condition=None, **kwargs):
@@ -167,14 +166,35 @@ class Commands(object):
         response['Cache-Control'] = 'no-cache'
         return self._pull_events(only_commits)
 
-    def get_blobs(self, document):
-        blobs = self._blobs.get(document)
-        if blobs is None:
-            blobs = self._blobs[document] = set()
-            for prop in self.volume[document].metadata.values():
-                if isinstance(prop, ad.BlobProperty):
-                    blobs.add(prop.name)
-        return blobs
+    @ad.directory_command_post(method='GET')
+    def _Commands_find_post(self, request, response, result):
+        self._mixin_blobs(request, result['result'])
+        return result
+
+    @ad.document_command_post(method='GET')
+    def _Commands_get_post(self, request, response, result):
+        self._mixin_blobs(request, [result])
+        return result
+
+    def _mixin_blobs(self, request, result):
+        if hasattr(request, 'environ'):
+            prefix = 'http://' + request.environ['HTTP_HOST']
+        else:
+            prefix = 'http://localhost:%s' % local.ipc_port.value
+        if request.mountpoint in (None, '/'):
+            postfix = ''
+        else:
+            postfix = '?mountpoint=' + request.mountpoint
+        document = request['document']
+
+        for props in result:
+            guid = props.get('guid') or request['guid']
+            for name, value in props.items():
+                if not isinstance(value, ad.PropertyMeta):
+                    continue
+                props[name] = value.url(
+                        default='/'.join(['', document, guid, name]) + postfix,
+                        prefix=prefix)
 
     def _pull_events(self, only_commits):
         # Otherwise, gevent's WSGI server doesn't sent HTTP status
diff --git a/tests/units/home_mount.py b/tests/units/home_mount.py
index a632390..48acd21 100755
--- a/tests/units/home_mount.py
+++ b/tests/units/home_mount.py
@@ -4,12 +4,14 @@
 import os
 import json
 import socket
+import urllib2
 from os.path import exists, abspath
 
 from __init__ import tests
 
 from active_toolkit import sockets, coroutine
 from sugar_network.resources.report import Report
+from sugar_network import local
 from sugar_network.local import activities
 from sugar_network import IPCClient
 
@@ -103,41 +105,60 @@ class HomeMountTest(tests.Test):
 
         self.touch(('file', 'blob'))
         local.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = local.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob', file(blob['path']).read())
+        self.assertEqual('blob', local.get(['context', guid, 'preview']).content)
 
         self.touch(('file2', 'blob2'))
         local.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file2'), pass_ownership=True)
-        blob = local.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob2', file(blob['path']).read())
+        self.assertEqual('blob2', local.get(['context', guid, 'preview']).content)
         assert not exists('file2')
 
-    def test_GetAbsetnBLOB(self):
-        self.start_server([Report])
-        local = IPCClient(mountpoint='~')
+    def test_GetBLOBs(self):
+        self.start_server()
+        client = IPCClient(mountpoint='~')
 
-        guid = local.post(['report'], {
-            'context': 'context',
-            'implementation': 'implementation',
+        guid = client.post(['context'], {
+            'type': 'activity',
+            'title': 'title',
+            'summary': 'summary',
             'description': 'description',
             })
 
-        self.assertEqual(None, local.get(['report', guid, 'data'], cmd='get_blob'))
+        self.touch(('file', 'icon-blob'))
+        client.put(['context', guid, 'icon'], cmd='upload_blob', path=abspath('file'))
 
-    def test_GetDefaultBLOB(self):
-        self.start_server()
-        local = IPCClient(mountpoint='~')
+        self.assertEqual(
+                'icon-blob',
+                client.get(['context', guid, 'icon']).content)
+        blob_url = 'http://localhost:%s/context/%s/icon?mountpoint=~' % (local.ipc_port.value, guid)
+        self.assertEqual(
+                [{'guid': guid, 'icon': blob_url}],
+                client.get(['context'], reply=['icon'])['result'])
+        self.assertEqual(
+                {'icon': blob_url},
+                client.get(['context', guid], reply=['icon']))
+        self.assertEqual(
+                'icon-blob',
+                urllib2.urlopen(blob_url).read())
 
-        guid = local.post(['context'], {
-            'type': 'activity',
-            'title': 'title',
-            'summary': 'summary',
+    def test_GetAbsentBLOBs(self):
+        self.start_server([Report])
+        client = IPCClient(mountpoint='~')
+
+        guid = client.post(['report'], {
+            'context': 'context',
+            'implementation': 'implementation',
             'description': 'description',
             })
 
-        blob = local.get(['context', guid, 'icon'], cmd='get_blob')
-        assert blob['path'].endswith('missing.png')
-        assert exists(blob['path'])
+        self.assertRaises(RuntimeError, client.get, ['report', guid, 'data'])
+        blob_url = 'http://localhost:%s/report/%s/data?mountpoint=~' % (local.ipc_port.value, guid)
+        self.assertEqual(
+                [{'guid': guid, 'data': blob_url}],
+                client.get(['report'], reply=['data'])['result'])
+        self.assertEqual(
+                {'data': blob_url},
+                client.get(['report', guid], reply=['data']))
+        self.assertRaises(urllib2.HTTPError, urllib2.urlopen, blob_url)
 
     def test_Subscription(self):
         self.start_server()
diff --git a/tests/units/node.py b/tests/units/node.py
index 52f4b30..54f632f 100755
--- a/tests/units/node.py
+++ b/tests/units/node.py
@@ -307,7 +307,7 @@ class NodeTest(tests.Test):
                 {'guid': guid3, 'icon': 'http://localhost/foo/bar', 'layer': ['public']},
                 call(cp, method='GET', document='context', guid=guid3, reply=['guid', 'icon', 'layer']))
         self.assertEqual(
-                {'guid': guid4, 'data': None, 'layer': ['public']},
+                {'guid': guid4, 'data': 'http://localhost/report/%s/data' % guid4, 'layer': ['public']},
                 call(cp, method='GET', document='report', guid=guid4, reply=['guid', 'data', 'layer']))
 
         self.assertEqual(
@@ -320,7 +320,7 @@ class NodeTest(tests.Test):
                 sorted(call(cp, method='GET', document='context', reply=['guid', 'icon', 'layer'])['result']))
 
         self.assertEqual([
-            {'guid': guid4, 'data': None, 'layer': ['public']},
+            {'guid': guid4, 'data': 'http://localhost/report/%s/data' % guid4, 'layer': ['public']},
             ],
             call(cp, method='GET', document='report', reply=['guid', 'data', 'layer'])['result'])
 
diff --git a/tests/units/node_mount.py b/tests/units/node_mount.py
index 3f0f5e2..c0f04b0 100755
--- a/tests/units/node_mount.py
+++ b/tests/units/node_mount.py
@@ -4,6 +4,7 @@
 import os
 import json
 import socket
+import urllib2
 import zipfile
 from os.path import exists, abspath, join
 
@@ -277,9 +278,9 @@ class NodeMountTest(tests.Test):
     def test_upload_blob(self):
         mounts = self.start_server()
         mounts[tests.tmpdir + '/mnt'].mounted.wait()
-        remote = IPCClient(mountpoint=tests.tmpdir + '/mnt')
+        client = IPCClient(mountpoint=tests.tmpdir + '/mnt')
 
-        guid = remote.post(['context'], {
+        guid = client.post(['context'], {
             'type': 'activity',
             'title': 'title',
             'summary': 'summary',
@@ -287,44 +288,63 @@ class NodeMountTest(tests.Test):
             })
 
         self.touch(('file', 'blob'))
-        remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob', file(blob['path']).read())
+        client.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
+        self.assertEqual('blob', client.get(['context', guid, 'preview']).content)
 
         self.touch(('file2', 'blob2'))
-        remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file2'), pass_ownership=True)
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob2', file(blob['path']).read())
+        client.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file2'), pass_ownership=True)
+        self.assertEqual('blob2', client.get(['context', guid, 'preview']).content)
         assert not exists('file2')
 
-    def test_GetAbsentBLOB(self):
+    def test_GetBLOBs(self):
         mounts = self.start_server()
         mounts[tests.tmpdir + '/mnt'].mounted.wait()
-        remote = IPCClient(mountpoint=tests.tmpdir + '/mnt')
+        client = IPCClient(mountpoint=tests.tmpdir + '/mnt')
 
-        guid = remote.post(['report'], {
-            'context': 'context',
-            'implementation': 'implementation',
+        guid = client.post(['context'], {
+            'type': 'activity',
+            'title': 'title',
+            'summary': 'summary',
             'description': 'description',
             })
 
-        self.assertEqual(None, remote.get(['report', guid, 'data'], cmd='get_blob'))
+        self.touch(('file', 'icon-blob'))
+        client.put(['context', guid, 'icon'], cmd='upload_blob', path=abspath('file'))
+
+        self.assertEqual(
+                'icon-blob',
+                client.get(['context', guid, 'icon']).content)
+        blob_url = 'http://localhost:%s/context/%s/icon?mountpoint=%s' % (local.ipc_port.value, guid, tests.tmpdir + '/mnt')
+        self.assertEqual(
+                [{'guid': guid, 'icon': blob_url}],
+                client.get(['context'], reply=['icon'])['result'])
+        self.assertEqual(
+                {'icon': blob_url},
+                client.get(['context', guid], reply=['icon']))
+        self.assertEqual(
+                'icon-blob',
+                urllib2.urlopen(blob_url).read())
 
-    def test_GetDefaultBLOB(self):
+    def test_GetAbsentBLOBs(self):
         mounts = self.start_server()
         mounts[tests.tmpdir + '/mnt'].mounted.wait()
-        remote = IPCClient(mountpoint=tests.tmpdir + '/mnt')
+        client = IPCClient(mountpoint=tests.tmpdir + '/mnt')
 
-        guid = remote.post(['context'], {
-            'type': 'activity',
-            'title': 'title',
-            'summary': 'summary',
+        guid = client.post(['report'], {
+            'context': 'context',
+            'implementation': 'implementation',
             'description': 'description',
             })
 
-        blob = remote.get(['context', guid, 'icon'], cmd='get_blob')
-        assert blob['path'].endswith('missing.png')
-        assert exists(blob['path'])
+        self.assertRaises(RuntimeError, client.get, ['report', guid, 'data'])
+        blob_url = 'http://localhost:%s/report/%s/data?mountpoint=%s' % (local.ipc_port.value, guid, tests.tmpdir + '/mnt')
+        self.assertEqual(
+                [{'guid': guid, 'data': blob_url}],
+                client.get(['report'], reply=['data'])['result'])
+        self.assertEqual(
+                {'data': blob_url},
+                client.get(['report', guid], reply=['data']))
+        self.assertRaises(urllib2.HTTPError, urllib2.urlopen, blob_url)
 
     def test_get_blob_ExtractImplementations(self):
         Volume.RESOURCES = [
diff --git a/tests/units/remote_mount.py b/tests/units/remote_mount.py
index 49c08e1..986267e 100755
--- a/tests/units/remote_mount.py
+++ b/tests/units/remote_mount.py
@@ -4,6 +4,7 @@
 import os
 import json
 import socket
+import urllib2
 from cStringIO import StringIO
 from os.path import exists, abspath
 
@@ -159,7 +160,7 @@ class RemoteMountTest(tests.Test):
             ],
             events)
 
-    def ___test_Subscription_NotifyOnline(self):
+    def test_Subscription_NotifyOnline(self):
         self.start_ipc_and_restful_server()
         remote = IPCClient(mountpoint='/')
         local = IPCClient(mountpoint='~')
@@ -170,6 +171,7 @@ class RemoteMountTest(tests.Test):
             'title': 'title',
             'summary': 'summary',
             'description': 'description',
+            'keep': True,
             })
 
         def read_events():
@@ -178,13 +180,14 @@ class RemoteMountTest(tests.Test):
                     event.pop('props')
                 events.append(event)
 
+        coroutine.sleep(1)
         job = coroutine.spawn(read_events)
         local.put(['context', guid], {'keep': False})
-        coroutine.sleep(.5)
+        coroutine.sleep(1)
         job.kill()
 
         self.assertEqual([
-            {'document': 'context', 'event': 'update', 'guid': guid, 'seqno': 1},
+            {'document': 'context', 'event': 'update', 'guid': guid, 'seqno': 4},
             ],
             events)
 
@@ -255,44 +258,14 @@ class RemoteMountTest(tests.Test):
 
         self.touch(('file', 'blob'))
         remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob', file(blob['path']).read())
+        self.assertEqual('blob', remote.get(['context', guid, 'preview']).content)
 
         self.touch(('file2', 'blob2'))
         remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file2'), pass_ownership=True)
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob2', file(blob['path']).read())
+        self.assertEqual('blob2', remote.get(['context', guid, 'preview']).content)
         assert not exists('file2')
 
-    def test_GetAbsentBLOB(self):
-        self.start_ipc_and_restful_server([User, Report])
-        remote = IPCClient(mountpoint='/')
-
-        guid = remote.post(['report'], {
-            'context': 'context',
-            'implementation': 'implementation',
-            'description': 'description',
-            })
-
-        self.assertEqual(None, remote.get(['report', guid, 'data'], cmd='get_blob'))
-
-    def test_GetDefaultBLOB(self):
-        self.start_ipc_and_restful_server()
-        remote = IPCClient(mountpoint='/')
-
-        guid = remote.post(['context'], {
-            'type': 'activity',
-            'title': 'title',
-            'summary': 'summary',
-            'description': 'description',
-            })
-
-        blob = remote.get(['context', guid, 'icon'], cmd='get_blob')
-        assert not blob['path'].endswith('missing.png')
-        assert exists(blob['path'])
-        assert file(blob['path'], 'rb').read() == file('../../../sugar_network/static/images/missing.png', 'rb').read()
-
-    def test_StaleBLOBs(self):
+    def test_GetBLOBs(self):
         self.start_ipc_and_restful_server()
         remote = IPCClient(mountpoint='/')
 
@@ -303,50 +276,45 @@ class RemoteMountTest(tests.Test):
             'description': 'description',
             })
 
-        self.touch(('file', 'blob-1'))
+        self.touch(('file', 'icon-blob'))
+        remote.put(['context', guid, 'icon'], cmd='upload_blob', path=abspath('file'))
+        self.touch(('file', 'preview-blob'))
         remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob-1', file(blob['path']).read())
-
-        cache_path = 'cache/context/%s/%s/preview' % (guid[:2], guid)
-        self.touch((cache_path, 'blob-2'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob-2', file(blob['path']).read())
-        seqno = json.load(file(cache_path + '.meta'))['seqno']
 
-        self.touch(('file', 'blob-3'))
-        remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob-3', file(blob['path']).read())
-        assert seqno < json.load(file(cache_path + '.meta'))['seqno']
+        self.assertEqual(
+                'preview-blob',
+                remote.get(['context', guid, 'preview']).content)
+        assert local.ipc_port.value != 8800
+        url_prefix = 'http://localhost:8800/context/' + guid
+        self.assertEqual(
+                [{'guid': guid, 'icon': url_prefix + '/icon', 'preview': url_prefix + '/preview'}],
+                remote.get(['context'], reply=['icon', 'preview'])['result'])
+        self.assertEqual(
+                {'icon': url_prefix + '/icon', 'preview': url_prefix + '/preview'},
+                remote.get(['context', guid], reply=['icon', 'preview']))
+        self.assertEqual(
+                'icon-blob',
+                urllib2.urlopen(url_prefix + '/icon').read())
 
-    def test_DoNotStaleBLOBs(self):
-        self.start_ipc_and_restful_server()
+    def test_GetAbsentBLOBs(self):
+        self.start_ipc_and_restful_server([User, Report])
         remote = IPCClient(mountpoint='/')
 
-        guid = remote.post(['context'], {
-            'type': 'activity',
-            'title': 'title',
-            'summary': 'summary',
+        guid = remote.post(['report'], {
+            'context': 'context',
+            'implementation': 'implementation',
             'description': 'description',
             })
 
-        self.touch(('file', 'blob-1'))
-        remote.put(['context', guid, 'preview'], cmd='upload_blob', path=abspath('file'))
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob-1', file(blob['path']).read())
-
-        cache_path = 'cache/context/%s/%s/preview' % (guid[:2], guid)
-        self.touch((cache_path, 'blob-2'))
-        seqno = json.load(file(cache_path + '.meta'))['seqno']
-
-        # Shift seqno
-        remote.put(['context', guid], {'title': 'title-2'})
-        coroutine.sleep(1)
-
-        blob = remote.get(['context', guid, 'preview'], cmd='get_blob')
-        self.assertEqual('blob-2', file(blob['path']).read())
-        assert seqno < json.load(file(cache_path + '.meta'))['seqno']
+        self.assertRaises(RuntimeError, remote.get, ['report', guid, 'data'])
+        blob_url = 'http://localhost:8800/report/%s/data' % guid
+        self.assertEqual(
+                [{'guid': guid, 'data': blob_url}],
+                remote.get(['report'], reply=['data'])['result'])
+        self.assertEqual(
+                {'data': blob_url},
+                remote.get(['report', guid], reply=['data']))
+        self.assertRaises(urllib2.HTTPError, urllib2.urlopen, blob_url)
 
 
 if __name__ == '__main__':