Refactor oEmbed previews #10814

clokep · 2021-09-13T18:55:37Z

This reworks how we do oEmbed quite a bit, with an eye towards being able to support auto-discovery and other features. It attempts to fit the oEmbed previews a bit better into the preview framework by moving it to a higher level. This allows for us re-using more of the framework we use for generating previews.

There's a few commits that refactor things to split out methods / move some code around.
Move oEmbed higher up in the process chain -- use the standard _download_file method and then post-process the result (like we do for HTML).

This also updates the tests to be a bit more reasonable.

Related to #2752.

This makes oEmbed previews more like image / HTML previews and takes beter advantage of the caching layer.

clokep · 2021-09-14T18:34:05Z

I think this is ready for review. There's currently two other PRs built on this that might be useful to look at where this is heading (and why we want to refactor this code).

…ovements

reivilibre

I don't think I know enough to review this properly, but a couple of comments

synapse/rest/media/v1/oembed.py

tests/rest/media/v1/test_url_preview.py

…ovements

richvdh

looks generally great! a few comments.

synapse/rest/media/v1/preview_url_resource.py

tests/rest/media/v1/test_url_preview.py

synapse/rest/media/v1/oembed.py

synapse/rest/media/v1/preview_url_resource.py

richvdh · 2021-09-21T12:59:27Z

synapse/rest/media/v1/preview_url_resource.py

+
+            # Use the cache age from the oEmbed result, instead of the HTTP response.
+            if oembed_response.cache_age is not None:
+                expiration_ts_ms = oembed_response.cache_age + media_info.created_ts_ms


should we cap this, to avoid things being cached for ever?

Yes, we definitely should. Do you have any thoughts for a max-age? 24 hours? A week?

I went with 24 hours, but should be easy enough to change. I also chose to apply this to all previews, not just ones from oEmbed. Although other previews currently always use 1 hour:

synapse/synapse/rest/media/v1/preview_url_resource.py

Lines 418 to 420 in 9ffa787

# FIXME: we should calculate a proper expiration based on the

# Cache-Control and Expire headers. But for now, assume 1 hour.

expires = ONE_HOUR

synapse/rest/media/v1/oembed.py

clokep · 2021-09-21T14:25:37Z

A couple of outstanding questions so I put it back on your review queue @richvdh!

richvdh

lgtm

synapse/rest/media/v1/oembed.py

Co-authored-by: Richard van der Hoff <[email protected]>

clokep mentioned this pull request Sep 13, 2021

Make oEmbed globs configurable and extract more info from the response #10392

Closed

4 tasks

clokep requested review from a team and removed request for a team September 14, 2021 12:10

clokep force-pushed the clokep/oembed-improvements branch from 057d6b4 to b00f68a Compare September 14, 2021 16:31

clokep changed the title ~~Revamp oEmbed previews~~ Refactor oEmbed previews Sep 14, 2021

clokep mentioned this pull request Sep 14, 2021

Improve oEmbed previews #10819

Merged

clokep added 7 commits September 14, 2021 12:40

Factor out calculating a description from an HTML tree.

84c4a53

Factor out calculating the expiration timestamp.

160b2ca

Factor out pre-caching a related image.

611849d

Update test to actually return an image.

05076b7

Process oEmbed by downloading the result to a file for later processing.

0501d08

This makes oEmbed previews more like image / HTML previews and takes beter advantage of the caching layer.

Assume required properties exist.

0c763bf

Newsfragment

3ffaee6

clokep force-pushed the clokep/oembed-improvements branch from b00f68a to 3ffaee6 Compare September 14, 2021 16:53

clokep mentioned this pull request Sep 14, 2021

Autodiscover oEmbed endpoint from returned HTML #10822

Merged

clokep requested a review from a team September 14, 2021 18:34

Merge remote-tracking branch 'origin/develop' into clokep/oembed-impr…

91f5d0c

…ovements

reivilibre reviewed Sep 16, 2021

View reviewed changes

synapse/rest/media/v1/oembed.py Outdated Show resolved Hide resolved

tests/rest/media/v1/test_url_preview.py Outdated Show resolved Hide resolved

reivilibre requested a review from a team September 16, 2021 13:25

clokep added 2 commits September 16, 2021 10:55

Rename some variables.

c98626c

Clarify docs more.

c29713f

clokep removed the request for review from a team September 16, 2021 15:01

clokep mentioned this pull request Sep 16, 2021

Create a constant for a small png image in tests. #10834

Merged

clokep added 2 commits September 16, 2021 12:01

Merge remote-tracking branch 'origin/develop' into clokep/oembed-impr…

ea0870f

…ovements

Use png constant.

e9cf11f

clokep requested a review from a team September 16, 2021 16:06

Clearer property names.

53b733b

richvdh self-assigned this Sep 21, 2021

richvdh reviewed Sep 21, 2021

View reviewed changes

clokep added 5 commits September 21, 2021 09:31

Return early in _precache_image_url.

86b75fb

Fix charset in tests.

93c0c25

Clarify when cache_age is None.

5327053

Cap the amount of time which URL previews will be kept to 24 hours.

a2a40d7

Fix typo.

6643b3f

clokep requested a review from richvdh September 21, 2021 14:25

richvdh mentioned this pull request Sep 21, 2021

Media files in url_cache/ and url_cache_thumbnails/ hang around for too long #10863

Closed

richvdh approved these changes Sep 21, 2021

View reviewed changes

synapse/rest/media/v1/oembed.py Outdated Show resolved Hide resolved

clokep and others added 3 commits September 21, 2021 11:43

Pass raw-bytes to the oEmbed code.

1f975d6

Clarify comment.

c31f23b

Co-authored-by: Richard van der Hoff <[email protected]>

Re-use a constant.

62b36c0

clokep enabled auto-merge (squash) September 21, 2021 15:50

clokep merged commit ba7a91a into develop Sep 21, 2021

clokep deleted the clokep/oembed-improvements branch September 21, 2021 16:09

clokep mentioned this pull request Oct 12, 2021

Be more lenient when parsing the version for oEmbed responses. #11065

Merged

blastrock mentioned this pull request Dec 23, 2021

Imgur "GIFs" don't appear in URL previews #11563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refactor oEmbed previews #10814

Refactor oEmbed previews #10814

Uh oh!

clokep commented Sep 13, 2021 •

edited

Loading

Uh oh!

clokep commented Sep 14, 2021

Uh oh!

reivilibre left a comment

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Sep 21, 2021

Uh oh!

clokep Sep 21, 2021

Uh oh!

clokep Sep 21, 2021

Uh oh!

Uh oh!

clokep commented Sep 21, 2021

Uh oh!

richvdh left a comment

Uh oh!

Uh oh!

Uh oh!

	# FIXME: we should calculate a proper expiration based on the
	# Cache-Control and Expire headers. But for now, assume 1 hour.
	expires = ONE_HOUR

Uh oh!

Refactor oEmbed previews #10814

Refactor oEmbed previews #10814

Uh oh!

Conversation

clokep commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clokep commented Sep 14, 2021

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

clokep Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

clokep Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clokep commented Sep 21, 2021

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clokep commented Sep 13, 2021 •

edited

Loading