Skip to content

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

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

Merged
Henni merged 1 commit into from
Jun 27, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions appinfo/database.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?xml version="1.0" encoding="UTF-8" ?>
<database>
<name>*dbname*</name>
<create>true</create>
<charset>utf8</charset>
<table>
<name>*dbprefix*notes_meta</name>
<declaration>
<field>
<name>id</name>
<type>integer</type>
<notnull>true</notnull>
<autoincrement>true</autoincrement>
</field>
<field>
<name>file_id</name>
<type>integer</type>
<notnull>true</notnull>
</field>
<field>
<name>user_id</name>
<type>text</type>
<notnull>true</notnull>
<length>64</length>
</field>
<field>
<name>last_update</name>
<type>integer</type>
<notnull>true</notnull>
</field>
<field>
<name>etag</name>
<type>text</type>
<notnull>true</notnull>
<length>32</length>
</field>
<index>
<name>notes_meta_file_id_index</name>
<field>
<name>file_id</name>
</field>
</index>
<index>
<name>notes_meta_user_id_index</name>
<field>
<name>user_id</name>
</field>
</index>
<index>
<name>notes_meta_file_id_user_id_index</name>
<unique>true</unique>
<field>
<name>file_id</name>
<name>user_id</name>
</field>
</index>
</declaration>
</table>
</database>
31 changes: 25 additions & 6 deletions controller/notesapicontroller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
namespace OCA\Notes\Controller;

use OCP\AppFramework\ApiController;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\IRequest;

use OCA\Notes\Service\NotesService;
use OCA\Notes\Service\MetaService;
use OCA\Notes\Db\Note;

/**
Expand All @@ -29,6 +31,8 @@ class NotesApiController extends ApiController {

/** @var NotesService */
private $service;
/** @var MetaService */
private $metaService;
/** @var string */
private $userId;

Expand All @@ -38,10 +42,10 @@ class NotesApiController extends ApiController {
* @param NotesService $service
* @param string $UserId
*/
public function __construct($AppName, IRequest $request,
NotesService $service, $UserId){
public function __construct($AppName, IRequest $request, NotesService $service, MetaService $metaService, $UserId) {
parent::__construct($AppName, $request);
$this->service = $service;
$this->metaService = $metaService;
$this->userId = $UserId;
}

Expand All @@ -52,7 +56,7 @@ public function __construct($AppName, IRequest $request,
* notes
* @return Note
*/
private function excludeFields(Note $note, array $exclude) {
private function excludeFields(Note &$note, array $exclude) {
if(count($exclude) > 0) {
foreach ($exclude as $field) {
if(property_exists($note, $field)) {
Expand All @@ -72,13 +76,28 @@ private function excludeFields(Note $note, array $exclude) {
* @param string $exclude
* @return DataResponse
*/
public function index($exclude='') {
public function index($exclude='', $pruneBefore=0) {
$exclude = explode(',', $exclude);
$now = new \DateTime(); // this must be before loading notes if there are concurrent changes possible
$notes = $this->service->getAll($this->userId);
$metas = $this->metaService->updateAll($this->userId, $notes);
foreach ($notes as $note) {
$note = $this->excludeFields($note, $exclude);
$lastUpdate = $metas[$note->getId()]->getLastUpdate();
if($pruneBefore && $lastUpdate<$pruneBefore) {
$vars = get_object_vars($note);
unset($vars['id']);
$this->excludeFields($note, array_keys($vars));
} else {
$this->excludeFields($note, $exclude);
}
}
$etag = md5(json_encode($notes));
if ($this->request->getHeader('If-None-Match') === '"'.$etag.'"') {
return new DataResponse([], Http::STATUS_NOT_MODIFIED);
}
return new DataResponse($notes);
return (new DataResponse($notes))
->setLastModified($now)
->setETag($etag);
}


Expand Down
32 changes: 32 additions & 0 deletions db/meta.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php
/**
* Nextcloud - Notes
*
* This file is licensed under the Affero General Public License version 3 or
* later. See the COPYING file.
*/

namespace OCA\Notes\Db;

use OCP\AppFramework\Db\Entity;

class Meta extends Entity {

public $userId;
public $fileId;
public $lastUpdate;
public $etag;

/**
* @param Note $note
* @return static
*/
public static function fromNote(Note $note, $userId) {
$meta = new static();
$meta->setUserId($userId);
$meta->setFileId($note->getId());
$meta->setLastUpdate(time());
$meta->setEtag($note->getEtag());
return $meta;
}
}
29 changes: 29 additions & 0 deletions db/metamapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php
/**
* Nextcloud - Notes
*
* This file is licensed under the Affero General Public License version 3 or
* later. See the COPYING file.
*/

namespace OCA\Notes\Db;

use OCP\IDBConnection;
use OCP\AppFramework\Db\Mapper;

class MetaMapper extends Mapper {

public function __construct(IDBConnection $db) {
parent::__construct($db, 'notes_meta');
}

public function getAll($userId) {
$sql = 'SELECT * FROM `*PREFIX*notes_meta` WHERE user_id=?';
return $this->findEntities($sql, [$userId]);
}

public function get($userId, $fileId) {
$sql = 'SELECT * FROM `*PREFIX*notes_meta` WHERE user_id=? AND file_id=?';
return $this->findEntity($sql, [$userId, $fileId]);
}

@Henni Henni Jun 4, 2017

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.

@korelstar korelstar Jun 4, 2017

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.

@Henni Henni Jun 4, 2017

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?

@korelstar korelstar Jun 4, 2017

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.

@Henni Henni Jun 4, 2017

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?

@korelstar korelstar Jun 5, 2017

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).

@korelstar korelstar Jun 27, 2017

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?

@Henni Henni Jun 27, 2017

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.

}
16 changes: 16 additions & 0 deletions db/note.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* Class Note
* @method integer getId()
* @method void setId(integer $value)
* @method string getEtag()
* @method void setEtag(string $value)
* @method integer getModified()
* @method void setModified(integer $value)
* @method string getTitle()
Expand All @@ -33,6 +35,7 @@
*/
class Note extends Entity {

public $etag;
public $modified;
public $title;
public $category;
Expand Down Expand Up @@ -60,6 +63,7 @@ public static function fromFile(File $file, Folder $notesFolder, $tags=[]){
$note->setFavorite(true);
//unset($tags[array_search(\OC\Tags::TAG_FAVORITE, $tags)]);
}
$note->updateETag();
$note->resetUpdatedFields();
return $note;
}
Expand All @@ -70,4 +74,16 @@ private static function convertEncoding($str) {
}
return $str;
}

private function updateETag() {
// collect all relevant attributes
$data = '';
foreach(get_object_vars($this) as $key => $val) {
if($key!=='etag') {
$data .= $val;
}
}
$etag = md5($data);
$this->setEtag($etag);
}
}
69 changes: 69 additions & 0 deletions service/metaservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php
/**
* Nextcloud - Notes
*
* This file is licensed under the Affero General Public License version 3 or
* later. See the COPYING file.
*/

namespace OCA\Notes\Service;

use OCA\Notes\Db\Note;
use OCA\Notes\Db\Meta;
use OCA\Notes\Db\MetaMapper;

/**
* Class MetaService
*
* @package OCA\Notes\Service
*/
class MetaService {

private $metaMapper;

public function __construct(MetaMapper $metaMapper) {
$this->metaMapper = $metaMapper;
}

public function updateAll($userId, Array $notes) {
$metas = $this->metaMapper->getAll($userId);
$metas = $this->getIndexedArray($metas, 'fileId');
$notes = $this->getIndexedArray($notes, 'id');
foreach($metas as $id=>$meta) {
if(!array_key_exists($id, $notes)) {
// DELETE obsolete notes
$this->metaMapper->delete($meta);
unset($metas[$id]);
}
}
foreach($notes as $id=>$note) {
if(!array_key_exists($id, $metas)) {
// INSERT new notes
$metas[$note->getId()] = $this->create($userId, $note);
} elseif($note->getEtag()!==$metas[$id]->getEtag()) {
// UPDATE changed notes
$meta = $metas[$id];
$this->updateIfNeeded($meta, $note);
}
}
return $metas;
}

private function getIndexedArray(array $data, $property) {
$property = ucfirst($property);
$getter = 'get'.$property;
$result = array();
foreach($data as $entity) {
$result[$entity->$getter()] = $entity;
}
return $result;
}

private function updateIfNeeded(&$meta, $note) {
if($note->getEtag()!==$meta->getEtag()) {
$meta->setEtag($note->getEtag());
$meta->setLastUpdate(time());
$this->metaMapper->update($meta);
}
}
}
3 changes: 3 additions & 0 deletions tests/unit/controller/NotesApiControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,14 @@ public function testGetAllHide(){

$this->assertEquals(json_encode([
[
'etag' => null,
'modified' => 123,
'category' => null,
'favorite' => false,
'id' => 3,
],
[
'etag' => null,
'modified' => 111,
'category' => null,
'favorite' => false,
Expand Down Expand Up @@ -139,6 +141,7 @@ public function testGetHide(){
$response = $this->controller->get(3, 'title,content');

$this->assertEquals(json_encode([
'etag' => null,
'modified' => 123,
'category' => null,
'favorite' => false,
Expand Down