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

use unique name for cache folder #2331

Merged
merged 1 commit into from
Aug 26, 2023
Merged

use unique name for cache folder #2331

merged 1 commit into from
Aug 26, 2023

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Aug 22, 2023

Summary

We had multiple reports now of shared hoster environments where the updating of some feeds fail because they can't write to the temp directory.

One possible issue is that they can't write to /tmp at all (which is the default nextcloud uses).
Or that they can write to /tmp but for some reason the hoster allows all users to share /tmp.

This PR changes that in using the instance ID of Nextcloud a unique identifier that is stable.

Example from my dev instance:

root@31fa29494eed:/var/www/html# ls -l /tmp/news-oc2iqvwq927d/cache/
total 12
drwxr-x--- 2 www-data www-data 4096 Aug 22 16:50 feedFavicon
drwxr-x--- 2 www-data www-data 4096 Aug 22 16:42 feedLogo
drwxr-x--- 4 www-data www-data 4096 Aug 22 16:42 purifier

Checklist

@Grotax
Copy link
Member Author

Grotax commented Aug 22, 2023

depends on #2328 since that required similar changes

Copy link
Contributor

@anoymouserver anoymouserver left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to use the instance ID for the name of the temp folder.

Since you use it in multiple places, have you thought about creating a utility for it so you don't always have to duplicate the code? (get paths, join and create them)

What I don't quite understand ... what is the cache of logos/favicons for, when each one is in the DB with the original URL and is also loaded by the clients by it?

@Grotax
Copy link
Member Author

Grotax commented Aug 22, 2023

Favicon I'm not sure the library uses that cache without asking and will create a directoy on it's own if we don't provide one.

Logo, is because you said we should check the image if it actually is usable for new and if not we should fall back to the favicon. For example if the file is actually an image and if it is square I think.
And the cache is there so that we don't need to repeat that check every time.
We check for the local file, check the timestamp and ask the server if the file has changed since timestamp.
#1164 (comment)

Since you use it in multiple places, have you thought about creating a utility for it so you don't always have to duplicate the code? (get paths, join and create them)

Yea I will check that.

@anoymouserver
Copy link
Contributor

Favicon I'm not sure the library uses that cache without asking and will create a directoy on it's own if we don't provide one.

I've looked through the code and it's caching the urls after followed redirections and, if used, the actual image file.
Your change improves and unifies the cache location, as it's currently cluttering the /tmp/ itself with url<md5> files. 👍
This obsoletes the global configuration here:

$favicon->cache(['dir' => $c->get(ITempManager::class)->getTempBaseDir()]);

Logo, is because you said we should check the image if it actually is usable for new and if not we should fall back to the favicon.

Interesting, I've forgot about this one 😄
As you've said the cache dir is used to allow checking the actual file and it isn't downloaded again if there is no change (as long as the remote server respects If-Modified-Since). The actual check result (size comparison, ..) isn't cached.

@SMillerDev
Copy link
Contributor

I think this should also be suggested upstream as the default because we can't be the only app running into this.

@Grotax
Copy link
Member Author

Grotax commented Aug 23, 2023

Favicon I'm not sure the library uses that cache without asking and will create a directoy on it's own if we don't provide one.

I've looked through the code and it's caching the urls after followed redirections and, if used, the actual image file.
Your change improves and unifies the cache location, as it's currently cluttering the /tmp/ itself with url<md5> files. 👍
This obsoletes the global configuration here:

$favicon->cache(['dir' => $c->get(ITempManager::class)->getTempBaseDir()]);

Logo, is because you said we should check the image if it actually is usable for new and if not we should fall back to the favicon.

Interesting, I've forgot about this one 😄
As you've said the cache dir is used to allow checking the actual file and it isn't downloaded again if there is no change (as long as the remote server respects If-Modified-Since). The actual check result (size comparison, ..) isn't cached.

Ah I didn't see the global config, makes more sense to adjust it there I think.

@Grotax Grotax force-pushed the fix/tmpdir branch 5 times, most recently from e7324af to 935edec Compare August 25, 2023 07:48
Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax merged commit da83f9a into master Aug 26, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/tmpdir branch August 26, 2023 08:53
Grotax added a commit that referenced this pull request Aug 26, 2023
Changed

- Drop support for Nextcloud 25, Supported: 26, 27 (#2316)
- Add a new command for occ `./occ news:updater:job` allows to check and reset the update job (#2166)
- Check for available http(s) compression options and use them (gzip, deflate, brotli) (#2328)
- Change and unify [cache](https://nextcloud.github.io/news/install/#cache) to use the instance ID of Nextcloud (#2331)

Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax mentioned this pull request Aug 26, 2023
Grotax added a commit that referenced this pull request Aug 27, 2023
Changed

- Drop support for Nextcloud 25, Supported: 26, 27 (#2316)
- Add a new command for occ `./occ news:updater:job` allows to check and reset the update job (#2166)
- Check for available http(s) compression options and use them (gzip, deflate, brotli) (#2328)
- Change and unify [cache](https://nextcloud.github.io/news/install/#cache) to use the instance ID of Nextcloud (#2331)

Signed-off-by: Benjamin Brahmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some feeds stopped being updated by cron
3 participants