fix(caa dims): separate cache key from full-size URL #518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In most cases, we can use the full-size URL as the cache key. However, for release groups, the full-size URL is the CAA-redirected front image. When the RG cover is changed, this URL stays the same, so the cache isn't invalidated properly. Also, for PDFs, we were using the derived JP2 URL as the cache key instead of the PDF URL itself. This change fixes both by separating the cache key from the image URL we query. For RGs, the cache key will now be the thumbnail URL, which will change if the RG cover is changed. Also reworked the tests to do better tests for these URL transformations and extractions. More work is needed there though, I think some of the earlier tests are doing duplicated work. Ideally, the URL transformation and extraction would be split off from the CAAImage class, but I didn't want to go into that much refactoring today. Unfortunately, the RG image cache won't be shared with the cache of the release images, since RGs use thumbnails as keys while releases use the full-size URL. We could fix this by changing the cache key to something like <release_id>/<image_id> but that would require a DB upgrade to transform existing entries, and would also make the cache specific to CAA/IA URLs. The latter probably isn't too big of a problem though.
Codecov Report
@@ Coverage Diff @@
## main #518 +/- ##
==========================================
+ Coverage 99.68% 99.76% +0.08%
==========================================
Files 56 56
Lines 1265 1273 +8
Branches 198 200 +2
==========================================
+ Hits 1261 1270 +9
Misses 3 3
+ Partials 1 0 -1
Continue to review full report at Codecov.
|
github-actions bot
added a commit
that referenced
this pull request
Jul 27, 2022
fix(caa dims): separate cache key from full-size URL (#518)
github-actions bot
added a commit
that referenced
this pull request
Jul 27, 2022
fix(caa dims): separate cache key from full-size URL (#518)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In most cases, we can use the full-size URL as the cache key. However, for release groups, the full-size URL is the CAA-redirected front image. When the RG cover is changed, this URL stays the same, so the cache isn't invalidated properly. Also, for PDFs, we were using the derived JP2 URL as the cache key instead of the PDF URL itself. This change fixes both by separating the cache key from the image URL we query. For RGs, the cache key will now be the thumbnail URL, which will change if the RG cover is changed.
Also reworked the tests to do better tests for these URL transformations and extractions. More work is needed there though, I think some of the earlier tests are doing duplicated work. Ideally, the URL transformation and extraction would be split off from the CAAImage class, but I didn't want to go into that much refactoring today.
Unfortunately, the RG image cache won't be shared with the cache of the release images, since RGs use thumbnails as keys while releases use the full-size URL. We could fix this by changing the cache key to something like
<release_id>/<image_id>but that would require a DB upgrade to transform existing entries, and would also make the cache specific to CAA/IA URLs. The latter probably isn't too big of a problem though.https://community.metabrainz.org/t/ropdebees-userscripts-support-thread/551947/92?u=ropdebee