8000 fix(scanner): Always refresh folder image time when adding first image by kgarner7 · Pull Request #3764 · navidrome/navidrome · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(scanner): Always refresh folder image time when adding first image #3764

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 7, 2025
Merged

fix(scanner): Always refresh folder image time when adding first image #3764

merged 3 commits into from
Mar 7, 2025

Conversation

kgarner7
Copy link
Contributor

Currently, the images_updated_at field is only set to the image modification time. However, in cases where a new image is added and said image is older than the folder mod time, the field is not updated properly.

In this the case where images_updated_at is null (no images were ever added) and a new images is found, use the folder modification time instead of image modification time.

Note, this doesn't handle cases such as replacing a newer image with an older one.

@kgarner7
fix(scanner): Always refresh folder image time when adding first image
ba20399 
Currently, the `images_updated_at` field is only set to the image modification time.
However, in cases where a new image is added _and_ said image is older than the folder mod time, the field is not updated properly.

In this the case where `images_updated_at` is null (no images were ever added) and a new images is found, use the folder modification time instead of image modification time.

**Note**, this doesn't handle cases such as replacing a newer image with an older one.
@github-actions GitHub Actions
Copy link
github-actions bot commented Feb 28, 2025 •
edited
Loading

Download the artifacts for this pull request:

deluan
deluan reviewed Feb 28, 2025
View reviewed changes
Copy link
Member
@deluan deluan left a comment

Choose a reason for hiding this comment

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

I think the proper solution for this woyld be to store in the folder a hash of all image names and times found in the folder, instead of the most recent image, and use the hash (images_hash?) in reader_artist instead of the images_last_updated_at. This would cause any changes in the folder's images to invalidate the cache.

What do you think?

@kgarner7
Copy link
Contributor Author

Conceptually it makes sense, but not sure how you would reconcile the external readers requiring date. Not something I would probably be comfortable doing at least.

@deluan
Copy link
Member
deluan commented Mar 6, 2025

Conceptually it makes sense, but not sure how you would reconcile the external readers requiring date. Not something I would probably be comfortable doing at least.

Yeah, it requires more work. I'll think about this when implementing the new /images endpoint for the new API. For now, let's keep using dates only.

@deluan
Copy link
Member
deluan commented Mar 6, 2025

@kgarner7 do you agree with my commit? Can you try it in your env to be sure it is working as you expect?

@kgarner7
Copy link
Contributor Author
kgarner7 commented Mar 7, 2025
  1. timesNewest feels a bit inefficient (you could make the first time time[0], and iterate through the rest).
  2. If an image is removed (for whatever reason), then the cache wouldn't be updated.

@deluan
Copy link
Member
deluan commented Mar 7, 2025
  1. timesNewest feels a bit inefficient (you could make the first time time[0], and iterate through the rest).

Yeah, I didn't want to check if the times array is empty, IMO makes the change less readable, but I can change that.

  1. If an image is removed (for whatever reason), then the cache wouldn't be updated.

If the image is not the newest, then yes. But if it is the newest, the imagesUpdatedAt will change, invalidating the cache. Or am I missing something?

@kgarner7
Copy link
Contributor Author
kgarner7 commented Mar 7, 2025

If you go from one image to no images, then because of the default time being 0, the item won't be invalidated. Maybe that's not an expected use case?

@deluan
Copy link
Member
deluan commented Mar 7, 2025

If you go from one image to no images, then because of the default time being 0, the item won't be invalidated. Maybe that's not an expected use case?

It will set imagesUpdatedAt to 0, then the cache will be invalidated, as the date used in the cache key will change to the newest image from the album folders:

navidrome/core/artwork/reader_artist.go

Lines 46 to 68 in c795bcf

albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, als...)
if err != nil {
return nil, err
}
artistFolder, artistFolderLastUpdate, err := loadArtistFolder(ctx, artwork.ds, als, albumPaths)
if err != nil {
return nil, err
}
a := &artistReader{
a: artwork,
em: em,
artist: *ar,
artistFolder: artistFolder,
imgFiles: imgFiles,
}
// TODO Find a way to factor in the ExternalUpdateInfoAt in the cache key. Problem is that it can
// change _after_ retrieving from external sources, making the key invalid
//a.cacheKey.lastUpdate = ar.ExternalInfoUpdatedAt
a.cacheKey.lastUpdate = *imagesUpdatedAt
if artistFolderLastUpdate.After(a.cacheKey.lastUpdate) {
a.cacheKey.lastUpdate = artistFolderLastUpdate
}

@kgarner7
Copy link
Contributor Author
kgarner7 commented Mar 7, 2025 •
edited
Loading

Consider the following case in https://github.com/navidrome/navidrome/blob/c795bcfcf7471c244b0735e990fe8ccd0252d0c8/core/artwork/reader_album.go#L47C1-L51C3

  1. The image was first added at the same time as the album. Thus a.cacheKey.lastUpdate is originally al.UpdatedAt
  2. The image is then removed. Because the image was added at the same time as the album, the lastUpdated key doesn't change.

I'm less convinced this is possible for artist, but it seems possible for album.

@deluan deluan merged commit 36ed880 into navidrome:master Mar 7, 2025
31 checks passed
@kgarner7 kgarner7 deleted the update-image-date-on-first-image branch March 7, 2025 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deluan deluan deluan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kgarner7 @deluan
0