LibrePhotos / librephotos

A self-hosted open source photo management service. This is the repository of the backend.
MIT License
6.97k stars 307 forks source link

Adding ETag support for /api/albums/date/list/ endpoints #1177

Closed savvasdalkitsis closed 1 month ago

savvasdalkitsis commented 8 months ago

Describe the enhancement you'd like UhuruPhotos currently has a recurring job (by default needing charging) to fetch all the photo summaries of the user's 'feed' using the /api/albums/date/list/ and /api/albums/date/list/{id} endpoints.

The reason for that is that there is no way for the app to know if the user has added media on the server that belong to a much earlier date, therefore a deep sync is needed to make sure the app isn't missing data.

Describe why this will benefit the LibrePhotos

I think adding support for ETags on the above endpoints would allow the clients to perform syncs with minimal overhead

Additional context https://docs.djangoproject.com/en/5.0/topics/conditional-view-processing/

derneuere commented 7 months ago

We use Django Rest Framework, which does not work in the same ways as regular Django views. I researched a bit, but it looks to me like a similar thing is not implemented in the framework itself, but only with third party libraries, which are not maintained.

Your request sounds to me like you want a parameter for last modified for both endpoints.

In order to implement that, we can either add a last_modified_field, which we update on every save of the model, similar to saving EXIF data back, or we can add something like https://github.com/jazzband/django-simple-history to provide a complete history and an option to revert to a previous model state.

Is there a need for having a complete history, or do you think just adding a new parameter to the endpoints and a new field is enough? Do you just need new photos or also updated photos? How do you handle deleted photos?

savvasdalkitsis commented 7 months ago

I would imagine a last updated would work for the main endpoint but not much use in the individual ones since I would need to get them to check the date anyway so at that point I already have the latest version.

The point of the etag is that client libraries automatically support it and the server can skip sending the body (and more importantly skip any dB lookups) if the client already has the latest version. Making the calls much faster

derneuere commented 3 months ago

I still could not find any support for it in Django Rest Framework.

I added a last_modified query paramater for both /api/albums/date/list/ and /api/albums/date/list/{id} instead :)

savvasdalkitsis commented 3 months ago

This will work great it looks like! thanks :)

savvasdalkitsis commented 3 months ago

Actually, @derneuere, I was wondering how to deal with deleted items in the two lists: /api/albums/date/list/ and /api/albums/date/list/{id}

From what i can see, the new parameter simply returns new or modified items in each endpoint since the time specified, but won't return deleted items (deleted media for the second endpoint and completely empty days in the first endpoint)

The way i am currently dealing with deleted items is that i simply wipe all local copies and replace them with the responses since i am getting the full feed

Thoughts?

derneuere commented 3 months ago

If an image is deleted, it's no longer in the database, making it difficult to include in the response. It seems we need a soft delete mechanism for photos.

Currently, we have a "deleted" field on the photos model that moves images to the trashcan before permanently deleting the media files. However, we need an additional field to mark them as soft deleted.

I propose renaming the "deleted" field to "in_trashcan". Then, I will reintroduce the "deleted" field to mark photos as soft deleted. These soft deleted photos will be included in the "last_modified" endpoint.

I also need to ensure that deleted photos do not reappear anywhere else in the system.

What do you think?

derneuere commented 2 months ago

Image Handling: Images marked as "deleted" are now referred to as "in_trashcan." This change affects the PhotoDetails, PhotoEditView, and AlbumList views.

New Field Added: A new field, removed, has been introduced in PhotoDetails. This field represents photos that have been permanently deleted.

Automatic Deletion: Photos marked as removed will be automatically deleted from the database after 30 days. Please keep this in mind for any related processes or data management considerations.

Let me know @savvasdalkitsis if this now works for you :)

savvasdalkitsis commented 2 months ago

Might work in theory.

How does this affect the {{HOST}}/api/photosedit/setdeleted and {{HOST}}/api/photosedit/delete endpoints? do the fields change for them?

Another question is how would days that have no longer got any media in them but only got soft deleted media since the provided date show up in the {{HOST}}/api/albums/date/list?last_modified={{DATE}} endpoint? Will that day show up as modified? I would assume so but just double checking

derneuere commented 2 months ago

{{HOST}}/api/photosedit/delete stayed the same, as you only need to provide the image_hashes at the moment and get image_hashes back.

{{HOST}}/api/photosedit/setdeleted return now updated PhotoDetails with removed and in_trashcan

Yes, if you request {{HOST}}/api/albums/date/list?last_modified={{DATE}} you can get days back, which only have removed photos.

savvasdalkitsis commented 1 month ago

Hey @derneuere so the in_trashcan field works well to get the deleted photos in the feed endpoints when specifying the last modified field.

I am now missing a field in the {{HOST}}/api/albums/date/:id?last_modified=2024-09-15T21:09:57Z response to note which of the returned media items were added(or modified i guess) and which ones where removed:

for example i get this response:

{
    "results": {
        "id": "1036",
        "date": "2018-06-16",
        "location": "Brussels",
        "incomplete": false,
        "numberOfItems": 1,
        "items": [
            {
                "id": "c898a08c6c1cbf4dfe286bcfeb1679831",
                "dominantColor": "#ae926e",
                "url": "c898a08c6c1cbf4dfe286bcfeb1679831",
                "location": "Rue Des Bouchers - Beenhouwersstraat 1000 Brussels Brussels-Capital Belgium",
                "date": "2018-06-16T21:10:31+00:00",
                "birthTime": "2018-06-16T21:10:31Z",
                "aspectRatio": 0.75,
                "type": "image",
                "video_length": "",
                "rating": 0,
                "owner": {
                    "id": 1,
                    "username": "kurosavvas",
                    "first_name": "Savvas",
                    "last_name": "Dalkitsis"
                }
            }
        ]
    }
}

There doesn't seem to be any field indicating that the photo result is for a deleted photo and not for an added one

derneuere commented 1 month ago

@savvasdalkitsis I thought, that you were updating all the photo details with api/photos/ to keep the local database up-to-date. In the photo details, there is a new field called "removed", which indicates that it is deleted completely.

I can also add it to the response too, but I feel like some kind of complete overhaul is necessary, because the response is kind of lacking and confusing in general. If you have any additional request on how this response should change, let me know :)

savvasdalkitsis commented 1 month ago

The details api is not called for every media item before hand cause it would be a LOT of calls. I only do that when you open the lightbox (or if you open the map view, I start calling it for every item cause that's the only thing that contains the latlon)

So the deleted field is kinda needed in the current format on the summary endpoint.

As a bonus, if you can also add the latlon to it that would make things MUCH easier for the map view too :D

savvasdalkitsis commented 1 month ago

Ah also to note, will there also be an in_trashcan field in the /api/albums/date/:id call? i want to be able to remove items that have been trashed or deleted so i'll need to know about both scenarios

derneuere commented 1 month ago

@savvasdalkitsis I added in_trashcan, removed and lat and lon to the summary endpoint :)

savvasdalkitsis commented 1 month ago

This worked great @derneuere. I cut a release of the app using these features but won't release it to the play store yet until the next stable version of librephotos goes out to make sure people use it cause otherwise the app will behave weirdly with older versions of the server

derneuere commented 1 month ago

Awesome!

I'll include a note in the release documentation to update the version, ensuring better support for UhuruPhotos :)