From 50cc0b30a950e5b56284ae71a1b039e81639f090 Mon Sep 17 00:00:00 2001 From: korelstar Date: Sat, 23 May 2020 14:12:04 +0200 Subject: [PATCH 1/2] switch from database.xml to migrations --- appinfo/database.xml | 61 ---------------- .../Version3004Date20200522095422.php | 70 +++++++++++++++++++ 2 files changed, 70 insertions(+), 61 deletions(-) delete mode 100644 appinfo/database.xml create mode 100644 lib/Migration/Version3004Date20200522095422.php diff --git a/appinfo/database.xml b/appinfo/database.xml deleted file mode 100644 index 7fdff79a3..000000000 --- a/appinfo/database.xml +++ /dev/null @@ -1,61 +0,0 @@ - - - *dbname* - true - utf8 - - *dbprefix*notes_meta - - - id - integer - true - true - - - file_id - integer - true - - - user_id - text - 64 - true - - - last_update - integer - true - - - etag - text - 32 - true - - - notes_meta_file_id_index - - file_id - - - - notes_meta_user_id_index - - user_id - - - - notes_meta_file_user_index - true - - file_id - - - user_id - - - -
- diff --git a/lib/Migration/Version3004Date20200522095422.php b/lib/Migration/Version3004Date20200522095422.php new file mode 100644 index 000000000..bebee2493 --- /dev/null +++ b/lib/Migration/Version3004Date20200522095422.php @@ -0,0 +1,70 @@ +hasTable('notes_meta')) { + $table = $schema->createTable('notes_meta'); + $table->addColumn('id', 'integer', [ + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->addColumn('file_id', 'integer', [ + 'notnull' => true, + ]); + $table->addColumn('user_id', 'string', [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('last_update', 'integer', [ + 'notnull' => true, + ]); + $table->addColumn('etag', 'string', [ + 'notnull' => true, + 'length' => 32, + ]); + $table->setPrimaryKey(['id']); + $table->addIndex(['file_id'], 'notes_meta_file_id_index'); + $table->addIndex(['user_id'], 'notes_meta_user_id_index'); + $table->addUniqueIndex(['file_id', 'user_id'], 'notes_meta_file_user_index'); + } + return $schema; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { + } +} From 0be9c4aabfa479768f4dc5e7878cc2205409dcb2 Mon Sep 17 00:00:00 2001 From: korelstar Date: Sat, 30 May 2020 14:42:25 +0200 Subject: [PATCH 2/2] speed-up sync (rewrite MetaService, add Hooks, ..) --- appinfo/info.xml | 47 ++++--- lib/Application.php | 9 ++ lib/Controller/Helper.php | 2 +- lib/Controller/NotesApiController.php | 2 +- lib/Db/Meta.php | 29 ++--- lib/Db/MetaMapper.php | 27 +++- lib/Migration/Cleanup.php | 31 +++++ ....php => Version3005Date20200528204430.php} | 36 +---- .../Version3005Date20200528204431.php | 72 ++++++++++ lib/NotesHooks.php | 76 +++++++++++ lib/Service/MetaService.php | 123 ++++++++++++++++-- lib/Service/Note.php | 10 +- lib/Service/NotesService.php | 7 +- 13 files changed, 378 insertions(+), 93 deletions(-) create mode 100644 lib/Migration/Cleanup.php rename lib/Migration/{Version3004Date20200522095422.php => Version3005Date20200528204430.php} (50%) create mode 100644 lib/Migration/Version3005Date20200528204431.php create mode 100644 lib/NotesHooks.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 65bdc8143..04033741a 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -1,27 +1,32 @@ - notes - Notes - Distraction-free notes and writing - notes + Notes + Distraction-free notes and writing + - 3.4.0 - agpl - Kristof Hamann - Bernhard Posselt - Hendrik Leppelsack - Jan-Christoph Borchardt - Notes - office - organization - tools - https://github.com/nextcloud/notes - https://github.com/nextcloud/notes/issues - https://github.com/nextcloud/notes.git - https://raw.githubusercontent.com/nextcloud/screenshots/master/apps/Notes/notes.png - - - + 3.5.0-dev + agpl + Kristof Hamann + Bernhard Posselt + Hendrik Leppelsack + Jan-Christoph Borchardt + Notes + office + organization + tools + https://github.com/nextcloud/notes + https://github.com/nextcloud/notes/issues + https://github.com/nextcloud/notes.git + https://raw.githubusercontent.com/nextcloud/screenshots/master/apps/Notes/notes.png + + + + + + OCA\Notes\Migration\Cleanup + + diff --git a/lib/Application.php b/lib/Application.php index 0de16a2e3..671bb7a7e 100644 --- a/lib/Application.php +++ b/lib/Application.php @@ -13,6 +13,11 @@ public function __construct(array $urlParams = []) { } public function register() : void { + $this->registerNavigation(); + $this->registerHooks(); + } + + private function registerNavigation() : void { $container = $this->getContainer(); $container->registerCapability(Capabilities::class); $server = $container->getServer(); @@ -28,4 +33,8 @@ public function register() : void { ]; }); } + + public function registerHooks() : void { + $this->getContainer()->query('OCA\\Notes\\NotesHooks')->register(); + } } diff --git a/lib/Controller/Helper.php b/lib/Controller/Helper.php index f4664381e..b8b63cb37 100644 --- a/lib/Controller/Helper.php +++ b/lib/Controller/Helper.php @@ -12,7 +12,7 @@ class Helper { - private $logger; + public $logger; private $appName; public function __construct( diff --git a/lib/Controller/NotesApiController.php b/lib/Controller/NotesApiController.php index 4d5d7a89c..f89b15e19 100644 --- a/lib/Controller/NotesApiController.php +++ b/lib/Controller/NotesApiController.php @@ -52,12 +52,12 @@ public function index(?string $category = null, string $exclude = '', int $prune $exclude = explode(',', $exclude); $now = new \DateTime(); // this must be before loading notes if there are concurrent changes possible $notes = $this->service->getAll($this->getUID())['notes']; + $metas = $this->metaService->updateAll($this->getUID(), $notes); if ($category !== null) { $notes = array_values(array_filter($notes, function ($note) use ($category) { return $note->getCategory() === $category; })); } - $metas = $this->metaService->updateAll($this->getUID(), $notes); $notesData = array_map(function ($note) use ($metas, $pruneBefore, $exclude) { $lastUpdate = $metas[$note->getId()]->getLastUpdate(); if ($pruneBefore && $lastUpdate<$pruneBefore) { diff --git a/lib/Db/Meta.php b/lib/Db/Meta.php index 7e8b08214..a29cb1ead 100644 --- a/lib/Db/Meta.php +++ b/lib/Db/Meta.php @@ -2,8 +2,6 @@ namespace OCA\Notes\Db; -use OCA\Notes\Service\Note; - use OCP\AppFramework\Db\Entity; /** @@ -16,25 +14,18 @@ * @method void setLastUpdate(integer $value) * @method string getEtag() * @method void setEtag(string $value) + * @method string getContentEtag() + * @method void setContentEtag(string $value) + * @method string getFileEtag() + * @method void setFileEtag(string $value) * @package OCA\Notes\Db */ 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 { - $meta = new static(); - $meta->setUserId($userId); - $meta->setFileId($note->getId()); - $meta->setLastUpdate(time()); - $meta->setEtag($note->getEtag()); - return $meta; - } + protected $userId; + protected $fileId; + protected $lastUpdate; + protected $etag; + protected $contentEtag; + protected $fileEtag; } diff --git a/lib/Db/MetaMapper.php b/lib/Db/MetaMapper.php index e406063cd..5ccbd5c97 100644 --- a/lib/Db/MetaMapper.php +++ b/lib/Db/MetaMapper.php @@ -15,10 +15,35 @@ public function __construct(IDBConnection $db) { public function getAll($userId) : array { $qb = $this->db->getQueryBuilder(); $qb->select('*') - ->from('*PREFIX*notes_meta') + ->from($this->tableName) ->where( $qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)) ); return $this->findEntities($qb); } + + public function findById(string $userId, int $fileId) : Meta { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->tableName) + ->where( + $qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)), + $qb->expr()->eq('file_id', $qb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT)) + ); + return $this->findEntity($qb); + } + + public function deleteAll() : void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->tableName)->execute(); + } + + public function deleteByNote(int $id) : void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->tableName) + ->where( + $qb->expr()->eq('file_id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)) + ) + ->execute(); + } } diff --git a/lib/Migration/Cleanup.php b/lib/Migration/Cleanup.php new file mode 100644 index 000000000..5c001cece --- /dev/null +++ b/lib/Migration/Cleanup.php @@ -0,0 +1,31 @@ +metaMapper = $metaMapper; + } + + /* + * @inheritdoc + */ + public function getName() { + return 'Clean up meta table'; + } + + /** + * @inheritdoc + */ + public function run(IOutput $output) { + $this->metaMapper->deleteAll(); + } +} diff --git a/lib/Migration/Version3004Date20200522095422.php b/lib/Migration/Version3005Date20200528204430.php similarity index 50% rename from lib/Migration/Version3004Date20200522095422.php rename to lib/Migration/Version3005Date20200528204430.php index bebee2493..27b026e62 100644 --- a/lib/Migration/Version3004Date20200522095422.php +++ b/lib/Migration/Version3005Date20200528204430.php @@ -1,6 +1,4 @@ -hasTable('notes_meta')) { - $table = $schema->createTable('notes_meta'); - $table->addColumn('id', 'integer', [ - 'autoincrement' => true, - 'notnull' => true, - ]); - $table->addColumn('file_id', 'integer', [ - 'notnull' => true, - ]); - $table->addColumn('user_id', 'string', [ - 'notnull' => true, - 'length' => 64, - ]); - $table->addColumn('last_update', 'integer', [ - 'notnull' => true, - ]); - $table->addColumn('etag', 'string', [ - 'notnull' => true, - 'length' => 32, - ]); - $table->setPrimaryKey(['id']); - $table->addIndex(['file_id'], 'notes_meta_file_id_index'); - $table->addIndex(['user_id'], 'notes_meta_user_id_index'); - $table->addUniqueIndex(['file_id', 'user_id'], 'notes_meta_file_user_index'); + if ($schema->hasTable('notes_meta')) { + $schema->dropTable('notes_meta'); } + return $schema; } diff --git a/lib/Migration/Version3005Date20200528204431.php b/lib/Migration/Version3005Date20200528204431.php new file mode 100644 index 000000000..68914a1ac --- /dev/null +++ b/lib/Migration/Version3005Date20200528204431.php @@ -0,0 +1,72 @@ +createTable('notes_meta'); + $table->addColumn('id', 'integer', [ + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->addColumn('file_id', 'integer', [ + 'notnull' => true, + ]); + $table->addColumn('user_id', 'string', [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('last_update', 'integer', [ + 'notnull' => true, + ]); + $table->addColumn('etag', 'string', [ + 'notnull' => true, + 'length' => 32, + ]); + $table->addColumn('content_etag', 'string', [ + 'notnull' => true, + 'length' => 32, + ]); + $table->addColumn('file_etag', 'string', [ + 'notnull' => true, + 'length' => 40, + ]); + $table->setPrimaryKey(['id']); + $table->addIndex(['file_id'], 'notes_meta_file_id_index'); + $table->addIndex(['user_id'], 'notes_meta_user_id_index'); + $table->addUniqueIndex(['file_id', 'user_id'], 'notes_meta_file_user_index'); + + return $schema; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { + } +} diff --git a/lib/NotesHooks.php b/lib/NotesHooks.php new file mode 100644 index 000000000..4c672f51e --- /dev/null +++ b/lib/NotesHooks.php @@ -0,0 +1,76 @@ +logger = $logger; + $this->rootFolder = $rootFolder; + $this->metaService = $metaService; + } + + public function register() : void { + $this->listenTo( + $this->rootFolder, + '\OC\Files', + 'preWrite', + function (Node $node) { + // $this->logger->warning('preWrite: ' . $node->getPath(), ['app'=>'notes']); + $this->onFileModified($node); + } + ); + $this->listenTo( + $this->rootFolder, + '\OC\Files', + 'preTouch', + function (Node $node) { + // $this->logger->warning('preTouch: ' . $node->getPath(), ['app'=>'notes']); + $this->onFileModified($node); + } + ); + $this->listenTo( + $this->rootFolder, + '\OC\Files', + 'preDelete', + function (Node $node) { + // $this->logger->warning('preDelete: ' . $node->getPath(), ['app'=>'notes']); + $this->onFileModified($node); + } + ); + $this->listenTo( + $this->rootFolder, + '\OC\Files', + 'preRename', + function (Node $source, Node $target) { + // $this->logger->warning('preRename: ' . $source->getPath(), ['app'=>'notes']); + $this->onFileModified($source); + } + ); + } + + private function listenTo($service, string $scope, string $method, callable $callback) : void { + /* @phan-suppress-next-line PhanUndeclaredMethod */ + $service->listen($scope, $method, $callback); + } + + private function onFileModified(Node $node) : void { + try { + $this->metaService->deleteByNote($node->getId()); + } catch (\Throwable $e) { + } + } +} diff --git a/lib/Service/MetaService.php b/lib/Service/MetaService.php index e4da1333c..f731f317f 100644 --- a/lib/Service/MetaService.php +++ b/lib/Service/MetaService.php @@ -5,6 +5,47 @@ use OCA\Notes\Db\Meta; use OCA\Notes\Db\MetaMapper; +/** MetaService. + * + * The MetaService maintains information about notes that cannot be gathered + * from Nextcloud middleware. + * + * Background: we want to minimize the transfered data size during + * synchronization with mobile clients. Therefore, the full note is only sent + * to the client if it was updated since last synchronization. For this + * purpose, we need to know at which time a file's content was changed. + * Unfortunately, Nextcloud does not save this information. Important: the + * filemtime is not sufficient for this, since a file's content can be changed + * without changing it's filemtime! + * + * Therefore, the Notes app maintains this information on its own. It is saved + * in the database table `notes_meta`. To be honest, we do not store the exact + * changed time, but a time `t` that is at some point between the real changed + * time and the next synchronization time. However, this is totally sufficient + * for this purpose. + * + * Therefore, on synchronization, the method `MetaService.updateAll` is called. + * It generates an ETag for each note and compares it with the ETag from + * `notes_meta` database table in order to detect changes (or creates an entry + * if not existent). If there are changes, the ETag is updated and `LastUpdate` + * is set to the current time. The ETag is a hash over all note attributes + * (except content, see below). + * + * But in order to further speed up synchronization, the content is not + * compared every time (this would be very expensive!). Instead, a file hook + * (see `OCA\Notes\NotesHook`) deletes the meta entry on every file change. As + * a consequence, a new entry in `note_meta` is created on next + * synchronization. + * + * Hence, instead of using the real content for generating the note's ETag, it + * uses a "content ETag" which is a hash over the content. Additionaly to the + * file hooks, this "content ETag" is updated if Nextcloud's "file ETag" has + * changed (but again, the "file ETag" is just an indicator, since it is not a + * hash over the content). + * + * All in all, this has some complexity, but we can speed up synchronization + * with this approach! :-) + */ class MetaService { private $metaMapper; @@ -13,10 +54,17 @@ public function __construct(MetaMapper $metaMapper) { $this->metaMapper = $metaMapper; } - public function updateAll(string $userId, array $notes) : array { + public function deleteByNote(int $id) : void { + $this->metaMapper->deleteByNote($id); + } + + public function updateAll(string $userId, array $notes, bool $forceUpdate = false) : array { + // load data $metas = $this->metaMapper->getAll($userId); $metas = $this->getIndexedArray($metas, 'fileId'); $notes = $this->getIndexedArray($notes, 'id'); + + // delete obsolete notes foreach ($metas as $id => $meta) { if (!array_key_exists($id, $notes)) { // DELETE obsolete notes @@ -24,19 +72,36 @@ public function updateAll(string $userId, array $notes) : array { unset($metas[$id]); } } + + // insert/update changes 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()) { + $metas[$note->getId()] = $this->createMeta($userId, $note); + } else { // UPDATE changed notes $meta = $metas[$id]; - $this->updateIfNeeded($meta, $note); + if ($this->updateIfNeeded($meta, $note, $forceUpdate)) { + $this->metaMapper->update($meta); + } } } return $metas; } + public function update(string $userId, Note $note) : void { + $meta = null; + try { + $meta = $this->metaMapper->findById($userId, $note->getId()); + } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { + } + if ($meta === null) { + $this->createMeta($userId, $note); + } elseif ($this->updateIfNeeded($meta, $note, true)) { + $this->metaMapper->update($meta); + } + } + private function getIndexedArray(array $data, string $property) : array { $property = ucfirst($property); $getter = 'get'.$property; @@ -47,17 +112,55 @@ private function getIndexedArray(array $data, string $property) : array { return $result; } - private function create(string $userId, Note $note) : Meta { - $meta = Meta::fromNote($note, $userId); + private function createMeta(string $userId, Note $note) : Meta { + $meta = new Meta(); + $meta->setUserId($userId); + $meta->setFileId($note->getId()); + $meta->setLastUpdate(time()); + $this->updateIfNeeded($meta, $note, true); $this->metaMapper->insert($meta); return $meta; } - private function updateIfNeeded(Meta &$meta, Note $note) : void { - if ($note->getEtag()!==$meta->getEtag()) { - $meta->setEtag($note->getEtag()); + private function updateIfNeeded(Meta &$meta, Note $note, bool $forceUpdate) : bool { + $generateContentEtag = $forceUpdate; + $fileEtag = $note->getFileEtag(); + // a changed File-ETag is an indicator for changed content + if ($fileEtag !== $meta->getFileEtag()) { + $meta->setFileEtag($fileEtag); + $generateContentEtag = true; + } + // generate new Content-ETag + if ($generateContentEtag) { + $contentEtag = $this->generateContentEtag($note); // this is expensive + if ($contentEtag !== $meta->getContentEtag()) { + $meta->setContentEtag($contentEtag); + } + } + // always update ETag based on meta data (not content!) + $etag = $this->generateEtag($meta, $note); + if ($etag !== $meta->getEtag()) { + $meta->setEtag($etag); $meta->setLastUpdate(time()); - $this->metaMapper->update($meta); } + return !empty($meta->getUpdatedFields()); + } + + // warning: this is expensive + private function generateContentEtag(Note $note) : string { + return md5($note->getContent()); + } + + // this is not expensive, since we use the content ETag instead of the content itself + private function generateEtag(Meta &$meta, Note $note) : string { + $data = [ + $note->getId(), + $note->getTitle(), + $note->getModified(), + $note->getCategory(), + $note->getFavorite(), + $meta->getContentEtag(), + ]; + return md5(json_encode($data)); } } diff --git a/lib/Service/Note.php b/lib/Service/Note.php index 1fa3192c1..78526469a 100644 --- a/lib/Service/Note.php +++ b/lib/Service/Note.php @@ -83,14 +83,8 @@ public function getData(array $exclude = []) : array { return $data; } - public function getEtag() : string { - $data = $this->getData(); - // collect all relevant attributes - $str = ''; - foreach ($data as $key => $val) { - $str .= $val; - } - return md5($str); + public function getFileEtag() : string { + return $this->file->getEtag(); } diff --git a/lib/Service/NotesService.php b/lib/Service/NotesService.php index 9d5a70e8c..f51f9686d 100644 --- a/lib/Service/NotesService.php +++ b/lib/Service/NotesService.php @@ -10,13 +10,16 @@ class NotesService { + private $metaService; private $settings; private $noteUtil; public function __construct( + MetaService $metaService, SettingsService $settings, NoteUtil $noteUtil ) { + $this->metaService = $metaService; $this->settings = $settings; $this->noteUtil = $noteUtil; } @@ -37,7 +40,9 @@ public function getAll(string $userId) : array { public function get(string $userId, int $id) : Note { $notesFolder = $this->getNotesFolder($userId); - return new Note($this->getFileById($notesFolder, $id), $notesFolder, $this->noteUtil); + $note = new Note($this->getFileById($notesFolder, $id), $notesFolder, $this->noteUtil); + $this->metaService->update($userId, $note); + return $note; }