Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Refactor MediaRepository to separate out storage #2767

Merged
merged 17 commits into from
Jan 16, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 115 additions & 28 deletions synapse/rest/media/v1/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,38 +70,11 @@ def respond_with_file(request, media_type, file_path,
logger.debug("Responding with %r", file_path)

if os.path.isfile(file_path):
request.setHeader(b"Content-Type", media_type.encode("UTF-8"))
if upload_name:
if is_ascii(upload_name):
request.setHeader(
b"Content-Disposition",
b"inline; filename=%s" % (
urllib.quote(upload_name.encode("utf-8")),
),
)
else:
request.setHeader(
b"Content-Disposition",
b"inline; filename*=utf-8''%s" % (
urllib.quote(upload_name.encode("utf-8")),
),
)

# cache for at least a day.
# XXX: we might want to turn this off for data we don't want to
# recommend caching as it's sensitive or private - or at least
# select private. don't bother setting Expires as all our
# clients are smart enough to be happy with Cache-Control
request.setHeader(
b"Cache-Control", b"public,max-age=86400,s-maxage=86400"
)
if file_size is None:
stat = os.stat(file_path)
file_size = stat.st_size

request.setHeader(
b"Content-Length", b"%d" % (file_size,)
)
add_file_headers(request, media_type, file_size, upload_name)

with open(file_path, "rb") as f:
yield logcontext.make_deferred_yieldable(
Expand All @@ -111,3 +84,117 @@ def respond_with_file(request, media_type, file_path,
finish_request(request)
else:
respond_404(request)


def add_file_headers(request, media_type, file_size, upload_name):
"""Adds the correct response headers in preparation for responding with the
media.

Args:
request (twisted.web.http.Request)
media_type (str): The media/content type.
file_size (int): Size in bytes of the media, if known.
upload_name (str): The name of the requested file, if any.
"""
request.setHeader(b"Content-Type", media_type.encode("UTF-8"))
if upload_name:
if is_ascii(upload_name):
request.setHeader(
b"Content-Disposition",
b"inline; filename=%s" % (
urllib.quote(upload_name.encode("utf-8")),
),
)
else:
request.setHeader(
b"Content-Disposition",
b"inline; filename*=utf-8''%s" % (
urllib.quote(upload_name.encode("utf-8")),
),
)

# cache for at least a day.
# XXX: we might want to turn this off for data we don't want to
# recommend caching as it's sensitive or private - or at least
# select private. don't bother setting Expires as all our
# clients are smart enough to be happy with Cache-Control
request.setHeader(
b"Cache-Control", b"public,max-age=86400,s-maxage=86400"
)

request.setHeader(
b"Content-Length", b"%d" % (file_size,)
)


@defer.inlineCallbacks
def respond_with_responder(request, responder, media_type, file_size, upload_name=None):
"""Responds to the request with given responder. If responder is None then
returns 404.

Args:
request (twisted.web.http.Request)
responder (Responder)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responder|None, please

media_type (str): The media/content type.
file_size (int): Size in bytes of the media, if known.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not known, it should be None?

upload_name (str): The name of the requested file, if any.
"""
if not responder:
respond_404(request)
return

add_file_headers(request, media_type, file_size, upload_name)
yield responder.write_to_consumer(request)
finish_request(request)


class Responder(object):
"""Represents a response that can be streamed to the requester.

Either `write_to_consumer` or `cancel` must be called to clean up any open
resources.
"""
def write_to_consumer(self, consumer):
"""Stream response into consumer

Args:
consumer (IConsumer)

Returns:
Deferred: Resolves once the response has finished being written
"""
pass

def cancel(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to make Responder implement an __exit__ method, so that you can use with

"""Called when the responder is not going to be used after all.
"""
pass


class FileInfo(object):
"""Details about a requested/uploaded file.

Attributes:
server_name (str): The server name where the media originated from,
or None if local.
file_id (str): The local ID of the file. For local files this is the
same as the media_id
media_type (str): Type of the file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem to exist

url_cache (bool): If the file is for the url preview cache
thumbnail (bool): Whether the file is a thumbnail or not.
thumbnail_width (int)
thumbnail_height (int)
thumbnail_method (int)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str, surely? (some info on what the values might be would be helpful)

thumbnail_type (str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this jpg/gif/etc? or?

"""
def __init__(self, server_name, file_id, url_cache=False,
thumbnail=False, thumbnail_width=None, thumbnail_height=None,
thumbnail_method=None, thumbnail_type=None):
self.server_name = server_name
self.file_id = file_id
self.url_cache = url_cache
self.thumbnail = thumbnail
self.thumbnail_width = thumbnail_width
self.thumbnail_height = thumbnail_height
self.thumbnail_method = thumbnail_method
self.thumbnail_type = thumbnail_type
24 changes: 1 addition & 23 deletions synapse/rest/media/v1/download_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,34 +57,12 @@ def _async_render_GET(self, request):
)
server_name, media_id, name = parse_media_id(request)
if server_name == self.server_name:
yield self._respond_local_file(request, media_id, name)
yield self.media_repo.get_local_media(request, media_id, name)
else:
yield self._respond_remote_file(
request, server_name, media_id, name
)

@defer.inlineCallbacks
def _respond_local_file(self, request, media_id, name):
media_info = yield self.store.get_local_media(media_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.store is now redundant in the constructor (as are self.clock and self.version_string while you are there)

if not media_info or media_info["quarantined_by"]:
respond_404(request)
return

media_type = media_info["media_type"]
media_length = media_info["media_length"]
upload_name = name if name else media_info["upload_name"]
if media_info["url_cache"]:
# TODO: Check the file still exists, if it doesn't we can redownload
# it from the url `media_info["url_cache"]`
file_path = self.filepaths.url_cache_filepath(media_id)
else:
file_path = self.filepaths.local_media_filepath(media_id)

yield respond_with_file(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would have been helpful if you could have put the switch from respond_with_file to respond_with_responder in a separate commit to the switch from _respond_local_file to get_local_media, ftr.

request, media_type, file_path, media_length,
upload_name=upload_name,
)

@defer.inlineCallbacks
def _respond_remote_file(self, request, server_name, media_id, name):
# don't forward requests for remote media if allow_remote is false
Expand Down
Loading