Skip to content

API speed-up (ETag, Last-Modified)#95

Merged
Henni merged 1 commit into
masterfrom
api-speed-up
Jun 27, 2017
Merged

API speed-up (ETag, Last-Modified)#95
Henni merged 1 commit into
masterfrom
api-speed-up

Conversation

@korelstar

@korelstar korelstar commented May 26, 2017

Copy link
Copy Markdown
Member

This is a rework of #51. It's based on the comments made for the old PR.

The goal of this PR is to enhance the REST-API for 3rd-party (mobile) apps in order to reduce the data that has to be transferred during synchronization. Therefore, only changed notes should be transferred (see also #49 for the full motivation).

API-Changes

1. Don't transfer data if nothing has changed (ETag/If-None-Match)

The server now sends the HTTP header field ETag, which is a hash value of the server response. After processing the server response, clients should save this value locally and send the HTTP header If-None-Match using the saved value in their next request to the server. If the server wants to response with the same data, it will send 304 Not Modified without any data instead. This behavior is conform to the HTTP standard and fully backward compatible.

2. Only transfer changed data, if something has changed (pruneBefore)

If only a single note is changed (edited or deleted), we nevertheless don't want to transfer all notes again. Therefore, the server response includes the HTTP header Last-Modified. After processing the server response, clients should save this value locally and use the parameter pruneBefore in their next request to the server. Then, the server prunes all notes which where changed before this date (i.e. only their id is transferred). With this approach, clients are able to update changed notes and also remove deleted notes while reducing the transferred data size significantly. This behavior is fully backward compatible.

Implementation background

The API changes are quite simple but very effective. However, the implementation is much more complex, since it's not that easy to get the real last modified date from a note (see #51 (comment)). Therefore, I introduced a database table (notes_meta with respective Mapper and Service), which tracks this information (see also #51 (comment) for the rationale). It is only updated, when there is a request using the API. This means, that the detected change-date is not the real change-date, but it is the first time when the changed value is used -- therefore it is sufficient for the goal of this PR.

Review

Please give feedback, @BernhardPosselt, @Henni, @nextcloud/notes! For the Android client, I've made a client-side implementation, which uses this enhanced API: nextcloud/notes-android#220

@korelstar korelstar added feature: API Related to the API for 3rd-party apps, i.e. Android or iOS enhancement New feature or request labels May 26, 2017
Comment thread db/metamapper.php
public function get($userId, $fileId) {
$sql = 'SELECT * FROM `*PREFIX*notes_meta` WHERE user_id=? AND file_id=?';
return $this->findEntity($sql, [$userId, $fileId]);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to store this data in the database? It seems to me like this data is directly bound to the files and it should be possible to get this directly from the file objects.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It would be nice, if there would be a way to do this. But I didn't find one (see #51 (comment) for some use cases which have to be considered). If you have any idea on how to improve this, please give feedback. But also after the discussion with @BernhardPosselt, I don't see another way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So last-modified inside the response header and mtime of the actual note (file) are independent values, right? The header value will be stored in the database and the mtime is directly set on the note.

But would it hurt to generate the ETag for every response instead of caching it in the database?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So last-modified inside the response header and mtime of the actual
note (file) are independent values, right? The header value will be
stored in the database and the mtime is directly set on the note.

Yes, mtime of the file is set by the client ("application-level" change time) and last-modified by the server (more technical level)

But would it hurt to generate the ETag for every response instead of
caching it in the database?

This is already the case. The ETag is used to detect if the file has been changed. But the goal is to update "last-modified" of that file if it has changed. Only for this, we need the database.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But what is the ETag field in the database used for then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is used to detect whether a note was changed (newly calculated ETag of a note differs from the ETag in the database) or not (newly calculated ETag is equal to the one in the database). In the first case, last-modified is updated to now. Based on this, the note will be fully transferred to all clients on their next request, even if they use pruneBefore to reduce the size of the response.

Please note, that this ETag is not equal to the file based ETag of nextcloud/server since it includes all attributes of a note, e.g. favorite tag (and the ETag in the response header of a list response is a completely different one).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Any news on this? Can it be merged, now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with this.

@Henni Henni merged commit 6df9129 into master Jun 27, 2017
@Henni Henni deleted the api-speed-up branch June 27, 2017 13:53
@Henni

Henni commented Jun 27, 2017

Copy link
Copy Markdown
Member

@korelstar awesome job as always!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request feature: API Related to the API for 3rd-party apps, i.e. Android or iOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants