Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix preview and comment work with proxified images #1656

Merged
merged 1 commit into from
Jul 23, 2023

Conversation

paskal
Copy link
Sponsor Collaborator

@paskal paskal commented Jul 17, 2023

Previously, image proxying through API was not tested. The test is added here.

Previously, proxied and local images were checked for presence in the storage before previewing or posting the comment. That logic resulted in an inability to post with an image when a proxy for images is enabled, as proxied images are not downloaded to disk before the first time someone loads them, which could only happen after the user either previews or posts the message.

After this change, preview and post only checks the local images' presence and ignore the proxied ones.

Resolves #1631.

@paskal paskal added the backend label Jul 17, 2023
@paskal paskal changed the title Clarify image storage signatures, add test for #1631 Fix preview and comment work with proxified images Jul 17, 2023
@paskal paskal marked this pull request as ready for review July 20, 2023 17:19
@paskal paskal requested a review from umputun as a code owner July 20, 2023 17:19
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

A minor suggestion to make it more readable

// Proxied images are returned only if the flag is set: this is used in image check on post preview and load,
// as proxied images have lazy loading and wouldn't be present on disk but still valid as they will be loaded
// the first time someone requests them.
func (s *Service) ExtractPictures(commentHTML string, returnProxied bool) (ids []string) {
Copy link
Owner

Choose a reason for hiding this comment

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

this new bool is used in multiple places and each place needs a comment about what this true/false means. How about making a small "enum" type with "image.DIRECT" and "image.PROXIED" values (consts)? Should make the api more readable

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

@umputun please take a look at the current version.

Previously, proxied and local images were checked for presence in the
storage before previewing or posting the comment. That logic resulted in
 an inability to post with an image when a proxy for images is enabled,
 as proxied images are not downloaded to disk before the first time
 someone loads them, which could only happen after the user either
 previews or posts the message.

After this change, preview and post only checks the local images'
presence and ignore the proxied ones.
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM, thx

@umputun umputun merged commit c72f30e into master Jul 23, 2023
3 checks passed
@umputun umputun deleted the paskal/fix_image_proxy branch July 23, 2023 17:10
@paskal paskal added this to the v1.12.0 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with image caching (IMAGE_PROXY_CACHE_EXTERNAL)
2 participants