Fix thumbnailing remote files #2789
Conversation
|
are the sytest failures a source of concern? |
Yup
The commit ones passed :/ Will have a look. |
erikjohnston
force-pushed
the
erikj/fix_thumbnails
branch
from
23602ed to
a4c5e4a
Compare
| @@ -285,6 +285,7 @@ def _get_remote_media_impl(self, server_name, media_id): | |||
| # If we have an entry in the DB, try and look for it | |||
| if media_info: | |||
| if media_info["quarantined_by"]: | |||
| logger.info("Media is quarentined") | |||
| @@ -85,6 +85,7 @@ def _respond_local_thumbnail(self, request, media_id, width, height, | |||
| media_info = yield self.store.get_local_media(media_id) | |||
|
|
|||
| if not media_info or media_info["quarantined_by"]: | |||
| logger.info("Media is quarantined") | |||
There was a problem hiding this comment.
we don't know for sure it is quarantined here
| @@ -120,6 +122,7 @@ def _select_or_generate_local_thumbnail(self, request, media_id, desired_width, | |||
| media_info = yield self.store.get_local_media(media_id) | |||
|
|
|||
| if not media_info or media_info["quarantined_by"]: | |||
| logger.info("Media is quarantined") | |||
| @@ -197,7 +201,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id, | |||
| yield respond_with_responder(request, responder, t_type, t_length) | |||
| return | |||
|
|
|||
| logger.debug("We don't have a local thumbnail of that size. Generating") | |||
| logger.debug("We don't have a remote thumbnail of that size. Generating") | |||
There was a problem hiding this comment.
hrm. we're looking for local thumbnails of remote media, no?
| @@ -208,6 +212,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id, | |||
| if file_path: | |||
| yield respond_with_file(request, desired_type, file_path) | |||
| else: | |||
| logger.warn("Failed to generate remote thumbnail") | |||
| if server_name: | ||
| file_id = media_info["filesystem_id"] | ||
| else: | ||
| file_id = media_id |
There was a problem hiding this comment.
the fact that media_info["filesystem_id"] and media_id give different values feels like a red flag to me. why doesn't media_info["filesystem_id"] work for the local case?
There was a problem hiding this comment.
Mainly because we handle local and remote media differently in the DB, and so it returns different things. I think what has happened is that the local/remote code paths were almost entirely separate and have slowly converged over time, making this a bit strange.
There was a problem hiding this comment.
Ok. I've now had a bit of a look at how this works. Taking a dict which can take one of two completely unspecified shapes is a horrible API. media_info is only used to pass the media_type and the file_id. Why don't we make both of those explicit parameters?
There was a problem hiding this comment.
[Note to self: this is effectively reinstating the condition at https://github.com//pull/2767/files#diff-043e4890d840f3862222c34adf3a9459L476, which constructed the local filename from either file_id or media_id. We now always use file_id so need to get it right]
| @@ -85,7 +85,9 @@ def _respond_local_thumbnail(self, request, media_id, width, height, | |||
| media_info = yield self.store.get_local_media(media_id) | |||
|
|
|||
| if not media_info or media_info["quarantined_by"]: | |||
| logger.info("Media is quarantined") | |||
| if media_info: | |||
There was a problem hiding this comment.
fwiw I'd have gone with:
if not media_info:
respond_404(request)
return
if media_info["quarantined_by"]:
logger.info("Media is quarantined")
respond_404(request)
return
but fine
There was a problem hiding this comment.
This is as well inconsistent to lines 127-128. Intended?
There was a problem hiding this comment.
I swear to god I just copied and pasted that. Today is not going well.
| @@ -151,7 +151,9 @@ def create_content(self, media_type, upload_name, content, content_length, | |||
| "media_length": content_length, | |||
| } | |||
|
|
|||
| yield self._generate_thumbnails(None, media_id, media_id, media_info) | |||
| yield self._generate_thumbnails( | |||
| None, media_id, media_id, media_info["media_type"], | |||
There was a problem hiding this comment.
s/media_info["media_type"]/media_type/
media_info now appears to be redundant here
| @@ -534,14 +536,13 @@ def _generate_thumbnails(self, server_name, media_id, file_id, media_info, | |||
| media_id (str): The media ID of the content. (This is the same as | |||
| the file_id for local content) | |||
| file_id (str): Local file ID | |||
| media_info (dict) | |||
| media_type (str) | |||
| @@ -413,7 +415,7 @@ def _download_remote_file(self, server_name, media_id, file_id): | |||
| } | |||
|
|
|||
| yield self._generate_thumbnails( | |||
| server_name, media_id, file_id, media_info, | |||
| server_name, media_id, file_id, media_info["media_type"], | |||
| @@ -187,7 +187,8 @@ def _do_preview(self, url, user, ts): | |||
| if _is_media(media_info['media_type']): | |||
| file_id = media_info['filesystem_id'] | |||
| dims = yield self.media_repo._generate_thumbnails( | |||
| None, file_id, file_id, media_info, url_cache=True, | |||
| None, file_id, file_id, media_info["media_type"], | |||
There was a problem hiding this comment.
would be nice to factor out a local media_type here too, given the number of times it's referenced, but shrug
|
possibly modulo the doc on media_type, but whatevs |
No description provided.