From 674d60be263ea647450f4464913e512b18d19fc8 Mon Sep 17 00:00:00 2001 From: korelstar Date: Sun, 7 Mar 2021 21:42:35 +0100 Subject: [PATCH 1/4] API: expose the note's ETag --- docs/api/v1.md | 19 ++++++----- lib/Controller/Helper.php | 49 +++++++++++++++++++++++++++ lib/Controller/NotesApiController.php | 24 +++---------- lib/Controller/NotesController.php | 36 +++++--------------- lib/Service/MetaService.php | 5 +-- tests/api/AbstractAPITest.php | 1 + tests/api/CommonAPITest.php | 12 ++++--- 7 files changed, 85 insertions(+), 61 deletions(-) diff --git a/docs/api/v1.md b/docs/api/v1.md index f53f99478..bcd1aa84a 100644 --- a/docs/api/v1.md +++ b/docs/api/v1.md @@ -19,12 +19,13 @@ The app and the API is mainly about notes. So, let's have a look about the attri | Attribute | Type | Description | since API version | |:----------|:-----|:-------------------------|:-------------------| -| `id` | integer | Every note has a unique identifier which is created by the server. It can be used to query and update a specific note. | 1.0 | -| `content` | string | Notes can contain arbitrary text. Formatting should be done using Markdown, but not every markup can be supported by every client. Therefore, markup should be used with care. | 1.0 | -| `title` | string | The note's title is also used as filename for the note's file. Therefore, some special characters are automatically removed and a sequential number is added if a note with the same title in the same category exists. When saving a title, the sanitized value is returned and should be adopted by your client. | 1.0 | -| `category` | string | Every note is assigned to a category. By default, the category is an empty string (not null), which means the note is uncategorized. Categories are mapped to folders in the file backend. Illegal characters are automatically removed and the respective folder is automatically created. Sub-categories (mapped to sub-folders) can be created by using `/` as delimiter. | 1.0 | -| `favorite` | boolean | If a note is marked as favorite, it is displayed at the top of the notes' list. Default is `false`. | 1.0 | -| `modified` | integer | Unix timestamp for the last modified date/time of the note. If not provided on note creation or content update, the current time is used. | 1.0 | +| `id` | integer (read‑only) | Every note has a unique identifier which is created by the server. It can be used to query and update a specific note. | 1.0 | +| `etag` | string (read‑only) | The note's entity tag (ETag) indicates if a note's attribute has changed. I.e., if the note changes, the ETag changes, too. Clients can use the ETag for detecting if the local note has to be updated from server and for [optimistic concurrency control](https://en.wikipedia.org/wiki/Optimistic_concurrency_control). | 1.2 | +| `content` | string (read/write) | Notes can contain arbitrary text. Formatting should be done using Markdown, but not every markup can be supported by every client. Therefore, markup should be used with care. | 1.0 | +| `title` | string (read/write) | The note's title is also used as filename for the note's file. Therefore, some special characters are automatically removed and a sequential number is added if a note with the same title in the same category exists. When saving a title, the sanitized value is returned and should be adopted by your client. | 1.0 | +| `category` | string (read/write) | Every note is assigned to a category. By default, the category is an empty string (not null), which means the note is uncategorized. Categories are mapped to folders in the file backend. Illegal characters are automatically removed and the respective folder is automatically created. Sub-categories (mapped to sub-folders) can be created by using `/` as delimiter. | 1.0 | +| `favorite` | boolean (read/write) | If a note is marked as favorite, it is displayed at the top of the notes' list. Default is `false`. | 1.0 | +| `modified` | integer (read/write) | Unix timestamp for the last modified date/time of the note. If not provided on note creation or content update, the current time is used. | 1.0 | ## Settings @@ -68,6 +69,7 @@ All defined routes in the specification are appended to this url. To access all [ { "id": 76, + "etag": "be284e00488c61c101ee28309d235e0b", "modified": 1376753464, "title": "New note", "category": "sub-directory", @@ -96,6 +98,7 @@ No valid authentication credentials supplied. ```js { "id": 76, + "etag": "be284e00488c61c101ee28309d235e0b", "modified": 1376753464, "title": "New note", "category": "sub-directory", @@ -118,7 +121,7 @@ Note not found.
Details #### Request parameters -- **Body**: See section [Note attributes](#note-attributes) (except for `id`). All attributes are optional. Example: +- **Body**: some or all "read/write" attributes (see section [Note attributes](#note-attributes)), example: ```js { "title": "New note", @@ -149,7 +152,7 @@ Not enough free storage for saving the note's content. | Parameter | Type | Description | |:------|:-----|:-----| | `id` | integer, required (path) | ID of the note to update. | -- **Body**: See section [Note attributes](#note-attributes) (except for `id`). All attributes are optional. Example see section [Create note](#create-note-post-notes). +- **Body**: some or all "read/write" attributes (see section [Note attributes](#note-attributes)), example see section [Create note](#create-note-post-notes). #### Response ##### 200 OK diff --git a/lib/Controller/Helper.php b/lib/Controller/Helper.php index 69936731f..aabe09bfd 100644 --- a/lib/Controller/Helper.php +++ b/lib/Controller/Helper.php @@ -5,6 +5,10 @@ namespace OCA\Notes\Controller; use OCA\Notes\AppInfo\Application; +use OCA\Notes\Db\Meta; +use OCA\Notes\Service\Note; +use OCA\Notes\Service\NotesService; +use OCA\Notes\Service\MetaService; use OCA\Notes\Service\Util; use OCP\AppFramework\Http; @@ -15,15 +19,23 @@ class Helper { + /** @var NotesService */ + private $notesService; + /** @var MetaService */ + private $metaService; /** @var LoggerInterface */ public $logger; /** @var IUserSession */ private $userSession; public function __construct( + NotesService $notesService, + MetaService $metaService, IUserSession $userSession, LoggerInterface $logger ) { + $this->notesService = $notesService; + $this->metaService = $metaService; $this->userSession = $userSession; $this->logger = $logger; } @@ -32,6 +44,43 @@ public function getUID() : string { return $this->userSession->getUser()->getUID(); } + public function getNoteData(Note $note, array $exclude = [], Meta $meta = null) : array { + if ($meta === null) { + $meta = $this->metaService->update($this->getUID(), $note); + } + $data = $note->getData($exclude); + $data['etag'] = $meta->getEtag(); + return $data; + } + + public function getNotesAndCategories( + int $pruneBefore, + array $exclude, + string $category = null + ) : array { + $userId = $this->getUID(); + $data = $this->notesService->getAll($userId); + $notes = $data['notes']; + $metas = $this->metaService->updateAll($userId, $notes); + if ($category !== null) { + $notes = array_values(array_filter($notes, function ($note) use ($category) { + return $note->getCategory() === $category; + })); + } + $notesData = array_map(function ($note) use ($metas, $pruneBefore, $exclude) { + $meta = $metas[$note->getId()]; + if ($pruneBefore && $meta->getLastUpdate() < $pruneBefore) { + return [ 'id' => $note->getId() ]; + } else { + return $this->getNoteData($note, $exclude, $meta); + } + }, $notes); + return [ + 'notes' => $notesData, + 'categories' => $data['categories'], + ]; + } + public function logException(\Throwable $e) : void { $this->logger->error('Controller failed with '.get_class($e), [ 'exception' => $e ]); } diff --git a/lib/Controller/NotesApiController.php b/lib/Controller/NotesApiController.php index 2de4d44b5..bb7a9f1d4 100644 --- a/lib/Controller/NotesApiController.php +++ b/lib/Controller/NotesApiController.php @@ -49,21 +49,8 @@ public function index(?string $category = null, string $exclude = '', int $prune return $this->helper->handleErrorResponse(function () use ($category, $exclude, $pruneBefore) { $exclude = explode(',', $exclude); $now = new \DateTime(); // this must be before loading notes if there are concurrent changes possible - $notes = $this->service->getAll($this->helper->getUID())['notes']; - $metas = $this->metaService->updateAll($this->helper->getUID(), $notes); - if ($category !== null) { - $notes = array_values(array_filter($notes, function ($note) use ($category) { - return $note->getCategory() === $category; - })); - } - $notesData = array_map(function ($note) use ($metas, $pruneBefore, $exclude) { - $lastUpdate = $metas[$note->getId()]->getLastUpdate(); - if ($pruneBefore && $lastUpdate < $pruneBefore) { - return [ 'id' => $note->getId() ]; - } else { - return $note->getData($exclude); - } - }, $notes); + $data = $this->helper->getNotesAndCategories($pruneBefore, $exclude, $category); + $notesData = $data['notes']; $etag = md5(json_encode($notesData)); return (new JSONResponse($notesData)) ->setLastModified($now) @@ -81,7 +68,7 @@ public function get(int $id, string $exclude = '') : JSONResponse { return $this->helper->handleErrorResponse(function () use ($id, $exclude) { $exclude = explode(',', $exclude); $note = $this->service->get($this->helper->getUID(), $id); - return $note->getData($exclude); + return $this->helper->getNoteData($note, $exclude); }); } @@ -113,7 +100,7 @@ public function create( $this->service->delete($this->helper->getUID(), $note->getId()); throw $e; } - return $note->getData(); + return $this->helper->getNoteData($note); }); } @@ -171,8 +158,7 @@ public function update( if ($favorite !== null) { $note->setFavorite($favorite); } - $this->metaService->update($this->helper->getUID(), $note); - return $note->getData(); + return $this->helper->getNoteData($note); }); } diff --git a/lib/Controller/NotesController.php b/lib/Controller/NotesController.php index 5bffbfa01..ae3abea9b 100644 --- a/lib/Controller/NotesController.php +++ b/lib/Controller/NotesController.php @@ -49,24 +49,6 @@ public function __construct( $this->l10n = $l10n; } - private function getNotesAndCategories(string $userId, int $pruneBefore) : array { - $data = $this->notesService->getAll($userId); - $metas = $this->metaService->updateAll($userId, $data['notes']); - $notes = array_map(function ($note) use ($metas, $pruneBefore) { - $lastUpdate = $metas[$note->getId()]->getLastUpdate(); - if ($pruneBefore && $lastUpdate < $pruneBefore) { - return [ 'id' => $note->getId() ]; - } else { - return $note->getData([ 'content' ]); - } - }, $data['notes']); - return [ - 'notes' => $notes, - 'categories' => $data['categories'], - ]; - } - - /** * @NoAdminRequired */ @@ -86,7 +68,7 @@ public function index(int $pruneBefore = 0) : JSONResponse { $categories = null; try { - $nac = $this->getNotesAndCategories($userId, $pruneBefore); + $nac = $this->helper->getNotesAndCategories($pruneBefore, [ 'etag', 'content' ]); [ 'notes' => $notes, 'categories' => $categories ] = $nac; } catch (\Throwable $e) { $this->helper->logException($e); @@ -161,10 +143,9 @@ public function get(int $id) : JSONResponse { strval($id) ); - $result = $note->getData(); - $etag = md5(json_encode($result)); - return (new JSONResponse($result)) - ->setETag($etag) + $noteData = $this->helper->getNoteData($note); + return (new JSONResponse($noteData)) + ->setETag($noteData['etag']) ; }); } @@ -176,7 +157,7 @@ public function get(int $id) : JSONResponse { public function create(string $category) : JSONResponse { return $this->helper->handleErrorResponse(function () use ($category) { $note = $this->notesService->create($this->helper->getUID(), '', $category); - return $note->getData(); + return $this->helper->getNoteData($note); }); } @@ -203,7 +184,7 @@ public function undo( try { // check if note still exists $note = $this->notesService->get($this->helper->getUID(), $id); - $noteData = $note->getData(); + $noteData = $this->helper->getNoteData($note); if ($noteData['error']) { throw new \Exception(); } @@ -214,7 +195,7 @@ public function undo( $note->setContent($content); $note->setModified($modified); $note->setFavorite($favorite); - return $note->getData(); + return $this->helper->getNoteData($note); } }); } @@ -243,8 +224,7 @@ public function update(int $id, string $content) : JSONResponse { return $this->helper->handleErrorResponse(function () use ($id, $content) { $note = $this->notesService->get($this->helper->getUID(), $id); $note->setContent($content); - $this->metaService->update($this->helper->getUID(), $note); - return $note->getData(); + return $this->helper->getNoteData($note); }); } diff --git a/lib/Service/MetaService.php b/lib/Service/MetaService.php index d4b9b5f34..5a04cf3f2 100644 --- a/lib/Service/MetaService.php +++ b/lib/Service/MetaService.php @@ -92,17 +92,18 @@ public function updateAll(string $userId, array $notes, bool $forceUpdate = fals return $metas; } - public function update(string $userId, Note $note) : void { + public function update(string $userId, Note $note) : Meta { $meta = null; try { $meta = $this->metaMapper->findById($userId, $note->getId()); } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { } if ($meta === null) { - $this->createMeta($userId, $note); + $meta = $this->createMeta($userId, $note); } elseif ($this->updateIfNeeded($meta, $note, true)) { $this->metaMapper->update($meta); } + return $meta; } private function getIndexedArray(array $data, string $property) : array { diff --git a/tests/api/AbstractAPITest.php b/tests/api/AbstractAPITest.php index e7a4eee23..e706e04ce 100644 --- a/tests/api/AbstractAPITest.php +++ b/tests/api/AbstractAPITest.php @@ -199,6 +199,7 @@ protected function updateNote(\stdClass &$note, \stdClass $request, \stdClass $e $note->$key = $val; } $this->checkReferenceNote($note, $responseNote, 'Updated note'); + return $responseNote; } protected function checkObject( diff --git a/tests/api/CommonAPITest.php b/tests/api/CommonAPITest.php index e1ee384da..27f1456b0 100644 --- a/tests/api/CommonAPITest.php +++ b/tests/api/CommonAPITest.php @@ -12,6 +12,7 @@ abstract class CommonAPITest extends AbstractAPITest { 'category' => 'string', 'modified' => 'integer', 'favorite' => 'boolean', + 'etag' => 'string', ]; private $requiredSettings = [ @@ -184,7 +185,7 @@ public function testUpdateNotes(array $refNotes, array $testNotes) : array { $this->checkGetReferenceNotes(array_merge($refNotes, $testNotes), 'Pre-condition'); $note = $testNotes[0]; // test update note with all attributes - $this->updateNote($note, (object)[ + $rn1 = $this->updateNote($note, (object)[ 'title' => 'First *manual* edited title', 'content' => '# *First* edited/ note'.PHP_EOL.'This is some body content with some data.', 'favorite' => false, @@ -194,19 +195,22 @@ public function testUpdateNotes(array $refNotes, array $testNotes) : array { 'title' => $this->autotitle ? 'First edited note' : 'First manual edited title', ]); // test update note with single attributes - $this->updateNote($note, (object)[ + $rn2 = $this->updateNote($note, (object)[ 'category' => 'Test/Third Category', ], (object)[]); // TODO test update category with read-only folder (target category) - $this->updateNote($note, (object)[ + $rn3 = $this->updateNote($note, (object)[ 'favorite' => true, ], (object)[]); - $this->updateNote($note, (object)[ + $rn4 = $this->updateNote($note, (object)[ 'content' => '# First multi edited note'.PHP_EOL.'This is some body content with some data.', ], (object)[ 'title' => $this->autotitle ? 'First multi edited note' : 'First manual edited title', 'modified' => time(), ]); + $this->assertNotEquals($rn1->etag, $rn2->etag, 'ETag changed on update (1)'); + $this->assertNotEquals($rn2->etag, $rn3->etag, 'ETag changed on update (2)'); + $this->assertNotEquals($rn3->etag, $rn4->etag, 'ETag changed on update (3)'); $this->checkGetReferenceNotes(array_merge($refNotes, $testNotes), 'After updating notes'); return $testNotes; } From b7440ca484bc4b3a32f780280a4fbbf3ec131e8e Mon Sep 17 00:00:00 2001 From: korelstar Date: Mon, 8 Mar 2021 09:35:51 +0100 Subject: [PATCH 2/4] API: check note's ETag (HTTP If-Match / 412) --- docs/api/v1.md | 31 +++++++++++++++++++- lib/Controller/ETagDoesNotMatchException.php | 17 +++++++++++ lib/Controller/Helper.php | 17 +++++++++++ lib/Controller/NotesApiController.php | 2 +- lib/Controller/NotesController.php | 2 +- tests/api/AbstractAPITest.php | 16 ++++++++-- tests/api/CommonAPITest.php | 8 ++++- 7 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 lib/Controller/ETagDoesNotMatchException.php diff --git a/docs/api/v1.md b/docs/api/v1.md index bcd1aa84a..27977fcac 100644 --- a/docs/api/v1.md +++ b/docs/api/v1.md @@ -20,7 +20,7 @@ The app and the API is mainly about notes. So, let's have a look about the attri | Attribute | Type | Description | since API version | |:----------|:-----|:-------------------------|:-------------------| | `id` | integer (read‑only) | Every note has a unique identifier which is created by the server. It can be used to query and update a specific note. | 1.0 | -| `etag` | string (read‑only) | The note's entity tag (ETag) indicates if a note's attribute has changed. I.e., if the note changes, the ETag changes, too. Clients can use the ETag for detecting if the local note has to be updated from server and for [optimistic concurrency control](https://en.wikipedia.org/wiki/Optimistic_concurrency_control). | 1.2 | +| `etag` | string (read‑only) | The note's entity tag (ETag) indicates if a note's attribute has changed. I.e., if the note changes, the ETag changes, too. Clients can use the ETag for detecting if the local note has to be updated from server and for optimistic concurrency control (see section [Preventing lost updates and conflict solution](#preventing-lost-updates-and-conflict-solution)). | 1.2 | | `content` | string (read/write) | Notes can contain arbitrary text. Formatting should be done using Markdown, but not every markup can be supported by every client. Therefore, markup should be used with care. | 1.0 | | `title` | string (read/write) | The note's title is also used as filename for the note's file. Therefore, some special characters are automatically removed and a sequential number is added if a note with the same title in the same category exists. When saving a title, the sanitized value is returned and should be adopted by your client. | 1.0 | | `category` | string (read/write) | Every note is assigned to a category. By default, the category is an empty string (not null), which means the note is uncategorized. Categories are mapped to folders in the file backend. Illegal characters are automatically removed and the respective folder is automatically created. Sub-categories (mapped to sub-folders) can be created by using `/` as delimiter. | 1.0 | @@ -152,6 +152,7 @@ Not enough free storage for saving the note's content. | Parameter | Type | Description | |:------|:-----|:-----| | `id` | integer, required (path) | ID of the note to update. | +| `If-Match` | HTTP header, optional | Use this for optimistic concurrency control (optional, but strongly recommended in order to prevent lost updates). As value of this HTTP header, the client has to use the last known note's etag (see section [Note attributes](#note-attributes)). If the note has changed in the meanwhile (concurrent change), the update request is blocked with HTTP status 412 (see below). Otherwise, the request will be processed normally. | 1.2 | - **Body**: some or all "read/write" attributes (see section [Note attributes](#note-attributes)), example see section [Create note](#create-note-post-notes). #### Response @@ -167,6 +168,11 @@ No valid authentication credentials supplied. ##### 404 Not Found Note not found. +##### 412 Precondition Failed +*(since API v1.2)* +Update cannot be performed since the note has been changed on the server in the meanwhile (concurrent change). The body contains the current note's state from server (see section [Note attributes](#note-attributes)), example see section [Get single note](#get-single-note-get-notesid). The client should use this response data in order to perform a conflict solution (see section [Preventing lost updates and conflict solution](#preventing-lost-updates-and-conflict-solution)). + + ##### 507 Insufficient Storage Not enough free storage for saving the note's content.
@@ -249,3 +255,26 @@ Endpoint not supported by installed notes app version (requires API version 1.2) ##### 401 Unauthorized No valid authentication credentials supplied. + + + +## Preventing lost updates and conflict solution + +While changing a note using a Notes client, the same note may be changed by another client. +In order to prevent lost updates of those concurrent changes, the notes API uses a well established mechanism called [optimistic concurrency control](https://en.wikipedia.org/wiki/Optimistic_concurrency_control). +For this purpose, notes have the attribute `etag` which is an identifier that changes if (and only if) the note changes on the server. +Clients have to store the `etag` for every note and send its value with every update request (HTTP header `If-Match`, see section [Update note](#update-note-put-notesid)). +If there was no parallel change on the server (i.e., the `etag` on server is the same as the one send from the client), the update request is performed as usual. +But if there was a parallel change, the `etag` on the server has changed and the server will refuse the update request. + +In this case, the client has to perform a conflict resolution, i.e. the local changes have to be merged with the remote changes. +In order to compare local changes with remote changes, it is useful that the client stores the full note's state as reference state before performing any local updates. +If an update conflict occurs, the client can use this reference state in order to merge all changes attribute-wise: +- Attributes, that have changed only locally or remotely, can be merged by picking the (local resp. remote) change. +- Attributes, that have changed both localy and remotely, have to be merged (see below). + +There are several options on how to merge an attribute: +- a) *Let the user decide*: ask the user whether i) overwrite local changes, ii) overwrite remote changes, or iii) save local (or remote) changes as new note. +- b) *Let the user merge*: provide an interface which allows for merging the files (you know it from your version control). +- c) *Try to merge automatically*: merge all changes automatically, e.g. for the `content` attribute using the [google-diff-match-patch](https://code.google.com/p/google-diff-match-patch/) ([Demo](https://neil.fraser.name/software/diff_match_patch/svn/trunk/demos/demo_patch.html), [Code](https://github.com/bystep15/google-diff-match-patch)) library. + diff --git a/lib/Controller/ETagDoesNotMatchException.php b/lib/Controller/ETagDoesNotMatchException.php new file mode 100644 index 000000000..4da9390dc --- /dev/null +++ b/lib/Controller/ETagDoesNotMatchException.php @@ -0,0 +1,17 @@ +note = $note; + } +} diff --git a/lib/Controller/Helper.php b/lib/Controller/Helper.php index aabe09bfd..f234fa242 100644 --- a/lib/Controller/Helper.php +++ b/lib/Controller/Helper.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; +use OCP\IRequest; use OCP\IUserSession; use Psr\Log\LoggerInterface; @@ -44,6 +45,20 @@ public function getUID() : string { return $this->userSession->getUser()->getUID(); } + public function getNoteWithETagCheck(int $id, IRequest $request) : Note { + $userId = $this->getUID(); + $note = $this->notesService->get($userId, $id); + $ifMatch = $request->getHeader('If-Match'); + if ($ifMatch) { + $matchEtags = preg_split('/,\s*/', $ifMatch); + $meta = $this->metaService->update($userId, $note); + if (!in_array('"'.$meta->getEtag().'"', $matchEtags)) { + throw new ETagDoesNotMatchException($note); + } + } + return $note; + } + public function getNoteData(Note $note, array $exclude = [], Meta $meta = null) : array { if ($meta === null) { $meta = $this->metaService->update($this->getUID(), $note); @@ -96,6 +111,8 @@ public function handleErrorResponse(callable $respond) : JSONResponse { try { $data = Util::retryIfLocked($respond); $response = $data instanceof JSONResponse ? $data : new JSONResponse($data); + } catch (\OCA\Notes\Controller\ETagDoesNotMatchException $e) { + $response = new JSONResponse($this->getNoteData($e->note), Http::STATUS_PRECONDITION_FAILED); } catch (\OCA\Notes\Service\NoteDoesNotExistException $e) { $this->logException($e); $response = $this->createErrorResponse($e, Http::STATUS_NOT_FOUND); diff --git a/lib/Controller/NotesApiController.php b/lib/Controller/NotesApiController.php index bb7a9f1d4..30928eaee 100644 --- a/lib/Controller/NotesApiController.php +++ b/lib/Controller/NotesApiController.php @@ -143,7 +143,7 @@ public function update( $category, $favorite ) { - $note = $this->service->get($this->helper->getUID(), $id); + $note = $this->helper->getNoteWithETagCheck($id, $this->request); if ($content !== null) { $note->setContent($content); } diff --git a/lib/Controller/NotesController.php b/lib/Controller/NotesController.php index ae3abea9b..22af341e9 100644 --- a/lib/Controller/NotesController.php +++ b/lib/Controller/NotesController.php @@ -222,7 +222,7 @@ public function autotitle(int $id) : JSONResponse { */ public function update(int $id, string $content) : JSONResponse { return $this->helper->handleErrorResponse(function () use ($id, $content) { - $note = $this->notesService->get($this->helper->getUID(), $id); + $note = $this->helper->getNoteWithETagCheck($id, $this->request); $note->setContent($content); return $this->helper->getNoteData($note); }); diff --git a/tests/api/AbstractAPITest.php b/tests/api/AbstractAPITest.php index e706e04ce..c95e47e88 100644 --- a/tests/api/AbstractAPITest.php +++ b/tests/api/AbstractAPITest.php @@ -188,9 +188,19 @@ protected function createNote(\stdClass $note, \stdClass $expected) : \stdClass return $note; } - protected function updateNote(\stdClass &$note, \stdClass $request, \stdClass $expected) { - $response = $this->http->request('PUT', 'notes/'.$note->id, [ 'json' => $request ]); - $this->checkResponse($response, 'Update note '.$note->title, 200); + protected function updateNote( + \stdClass &$note, + \stdClass $request, + \stdClass $expected, + string $etag = null, + int $statusExp = 200 + ) { + $requestOptions = [ 'json' => $request ]; + if ($etag !== null) { + $requestOptions['headers'] = [ 'If-Match' => '"'.$etag.'"' ]; + } + $response = $this->http->request('PUT', 'notes/'.$note->id, $requestOptions); + $this->checkResponse($response, 'Update note '.$note->title, $statusExp); $responseNote = json_decode($response->getBody()->getContents()); foreach (get_object_vars($request) as $key => $val) { $note->$key = $val; diff --git a/tests/api/CommonAPITest.php b/tests/api/CommonAPITest.php index 27f1456b0..f4106d5a6 100644 --- a/tests/api/CommonAPITest.php +++ b/tests/api/CommonAPITest.php @@ -197,7 +197,13 @@ public function testUpdateNotes(array $refNotes, array $testNotes) : array { // test update note with single attributes $rn2 = $this->updateNote($note, (object)[ 'category' => 'Test/Third Category', - ], (object)[]); + ], (object)[], $rn1->etag); + // test update note with wrong ETag + $this->updateNote($note, (object)[ + 'category' => 'New Fail', + ], (object)[ + 'category' => 'Test/Third Category', + ], 'wrongetag', 412); // TODO test update category with read-only folder (target category) $rn3 = $this->updateNote($note, (object)[ 'favorite' => true, From a76a167f9e6c79f2de0074c864b2882be6011cbd Mon Sep 17 00:00:00 2001 From: korelstar Date: Thu, 11 Mar 2021 10:17:40 +0100 Subject: [PATCH 3/4] check for update conflicts and provide solutions --- package-lock.json | 44 +++++++------ package.json | 2 + src/NotesService.js | 96 ++++++++++++++++++++++++----- src/Util.js | 21 +++++++ src/components/ConflictSolution.vue | 94 ++++++++++++++++++++++++++++ src/components/Note.vue | 92 ++++++++++++++++++++++++--- src/store/notes.js | 9 ++- 7 files changed, 310 insertions(+), 48 deletions(-) create mode 100644 src/Util.js create mode 100644 src/components/ConflictSolution.vue diff --git a/package-lock.json b/package-lock.json index 09b149a71..a8c1a85dd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,11 +14,13 @@ "@nextcloud/vue": "^3.8.0", "@nextcloud/vue-dashboard": "^2.0.0", "@nextcloud/webpack-vue-config": "^4.0.1", + "diff": "^5.0.0", "easymde": "^2.14.0", "markdown-it": "^12.0.4", "stylelint-webpack-plugin": "^2.1.1", "vue": "^2.6.12", "vue-fragment": "1.5.1", + "vue-material-design-icons": "^4.11.0", "vue-observe-visibility": "^1.0.0", "vue-router": "^3.5.1", "vuex": "^3.6.2" @@ -3492,6 +3494,14 @@ "minimalistic-assert": "^1.0.0" } }, + "node_modules/diff": { + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-5.0.0.tgz", + "integrity": "sha512-/VTCrvm5Z0JGty/BWHljh+BAiw3IK+2j87NGMu8Nwc/f48WoDAC395uomO9ZD117ZOBaHmkX1oyLvkVM/aIT3w==", + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/diffie-hellman": { "version": "5.0.3", "resolved": "https://registry.npmjs.org/diffie-hellman/-/diffie-hellman-5.0.3.tgz", @@ -5019,19 +5029,6 @@ "resolved": "https://registry.npmjs.org/fs.realpath/-/fs.realpath-1.0.0.tgz", "integrity": "sha1-FQStJSMVjKpA20onh8sBQRmU6k8=" }, - "node_modules/fsevents": { - "version": "2.3.2", - "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", - "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", - "hasInstallScript": true, - "optional": true, - "os": [ - "darwin" - ], - "engines": { - "node": "^8.16.0 || ^10.6.0 || >=11.0.0" - } - }, "node_modules/function-bind": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.1.tgz", @@ -10511,6 +10508,11 @@ } } }, + "node_modules/vue-material-design-icons": { + "version": "4.11.0", + "resolved": "https://registry.npmjs.org/vue-material-design-icons/-/vue-material-design-icons-4.11.0.tgz", + "integrity": "sha512-3Tyeqi9jtONQ/x8WkJqiBs4t5Bd5O1t7RdM/GIPKVYoVdaRy0oy3nbRjnMGyONBlqC/NpPjzhWeoZWUMEI04nA==" + }, "node_modules/vue-multiselect": { "version": "2.1.6", "resolved": "https://registry.npmjs.org/vue-multiselect/-/vue-multiselect-2.1.6.tgz", @@ -13668,6 +13670,11 @@ "minimalistic-assert": "^1.0.0" } }, + "diff": { + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-5.0.0.tgz", + "integrity": "sha512-/VTCrvm5Z0JGty/BWHljh+BAiw3IK+2j87NGMu8Nwc/f48WoDAC395uomO9ZD117ZOBaHmkX1oyLvkVM/aIT3w==" + }, "diffie-hellman": { "version": "5.0.3", "resolved": "https://registry.npmjs.org/diffie-hellman/-/diffie-hellman-5.0.3.tgz", @@ -14809,12 +14816,6 @@ "resolved": "https://registry.npmjs.org/fs.realpath/-/fs.realpath-1.0.0.tgz", "integrity": "sha1-FQStJSMVjKpA20onh8sBQRmU6k8=" }, - "fsevents": { - "version": "2.3.2", - "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", - "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", - "optional": true - }, "function-bind": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.1.tgz", @@ -18904,6 +18905,11 @@ "vue-style-loader": "^4.1.0" } }, + "vue-material-design-icons": { + "version": "4.11.0", + "resolved": "https://registry.npmjs.org/vue-material-design-icons/-/vue-material-design-icons-4.11.0.tgz", + "integrity": "sha512-3Tyeqi9jtONQ/x8WkJqiBs4t5Bd5O1t7RdM/GIPKVYoVdaRy0oy3nbRjnMGyONBlqC/NpPjzhWeoZWUMEI04nA==" + }, "vue-multiselect": { "version": "2.1.6", "resolved": "https://registry.npmjs.org/vue-multiselect/-/vue-multiselect-2.1.6.tgz", diff --git a/package.json b/package.json index aff085422..294c33632 100644 --- a/package.json +++ b/package.json @@ -21,11 +21,13 @@ "@nextcloud/vue": "^3.8.0", "@nextcloud/vue-dashboard": "^2.0.0", "@nextcloud/webpack-vue-config": "^4.0.1", + "diff": "^5.0.0", "easymde": "^2.14.0", "markdown-it": "^12.0.4", "stylelint-webpack-plugin": "^2.1.1", "vue": "^2.6.12", "vue-fragment": "1.5.1", + "vue-material-design-icons": "^4.11.0", "vue-observe-visibility": "^1.0.0", "vue-router": "^3.5.1", "vuex": "^3.6.2" diff --git a/src/NotesService.js b/src/NotesService.js index f6868e673..0273c1d48 100644 --- a/src/NotesService.js +++ b/src/NotesService.js @@ -3,6 +3,7 @@ import { generateUrl } from '@nextcloud/router' import { showError } from '@nextcloud/dialogs' import store from './store' +import { copyNote } from './Util' function url(url) { url = `apps/notes${url}` @@ -103,7 +104,7 @@ export const fetchNote = noteId => { const localNote = store.getters.getNote(parseInt(noteId)) // only overwrite if there are no unsaved changes if (!localNote || !localNote.unsaved) { - store.commit('updateNote', response.data) + _updateLocalNote(response.data) } return response.data }) @@ -125,17 +126,22 @@ export const refreshNote = (noteId, lastETag) => { if (lastETag) { headers['If-None-Match'] = lastETag } - const oldContent = store.getters.getNote(noteId).content + const note = store.getters.getNote(noteId) + const oldContent = note.content return axios .get( url('/notes/' + noteId), { headers } ) .then(response => { + if (note.conflict) { + store.commit('setNoteAttribute', { noteId, attribute: 'conflict', value: response.data }) + return response.headers.etag + } const currentContent = store.getters.getNote(noteId).content // only update if local content has not changed if (oldContent === currentContent) { - store.commit('updateNote', response.data) + _updateLocalNote(response.data) return response.headers.etag } return null @@ -166,7 +172,7 @@ export const createNote = category => { return axios .post(url('/notes'), { category }) .then(response => { - store.commit('updateNote', response.data) + _updateLocalNote(response.data) return response.data }) .catch(err => { @@ -176,27 +182,87 @@ export const createNote = category => { }) } +function _updateLocalNote(note, reference) { + if (reference === undefined) { + reference = copyNote(note, {}) + } + store.commit('updateNote', note) + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'reference', value: reference }) +} + function _updateNote(note) { + const requestOptions = { headers: { 'If-Match': '"' + note.etag + '"' } } return axios - .put(url('/notes/' + note.id), { content: note.content }) + .put(url('/notes/' + note.id), { content: note.content }, requestOptions) .then(response => { - const updated = response.data note.saveError = false - note.title = updated.title - note.modified = updated.modified + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'conflict', value: undefined }) + const updated = response.data if (updated.content === note.content) { - note.unsaved = false + // everything is fine + // => update note with remote data + _updateLocalNote( + { ...updated, unsaved: false } + ) + } else { + // content has changed locally in the meanwhile + // => merge note, but exclude content + _updateLocalNote( + copyNote(updated, note, ['content']), + copyNote(updated, {}) + ) } - store.commit('updateNote', note) - return note }) .catch(err => { - store.commit('setNoteAttribute', { noteId: note.id, attribute: 'saveError', value: true }) - console.error(err) - handleSyncError(t('notes', 'Saving note {id} has failed.', { id: note.id }), err) + if (err.response && err.response.status === 412) { + // ETag does not match, try to merge changes + note.saveError = false + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'conflict', value: undefined }) + const reference = note.reference + const remote = err.response.data + if (remote.content === note.content) { + // content is already up-to-date + // => update note with remote data + _updateLocalNote( + { ...remote, unsaved: false } + ) + } else if (remote.content === reference.content) { + // remote content has not changed + // => use all other attributes and sync again + _updateLocalNote( + copyNote(remote, note, ['content']), + copyNote(remote, {}) + ) + saveNote(note.id) + } else { + console.info('Note update conflict. Manual resolution required.') + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'conflict', value: remote }) + } + } else { + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'saveError', value: true }) + console.error(err) + handleSyncError(t('notes', 'Saving note {id} has failed.', { id: note.id }), err) + } }) } +export const conflictSolutionLocal = note => { + note.etag = note.conflict.etag + _updateLocalNote( + copyNote(note.conflict, note, ['content']), + copyNote(note.conflict, {}) + ) + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'conflict', value: undefined }) + saveNote(note.id) +} + +export const conflictSolutionRemote = note => { + _updateLocalNote( + { ...note.conflict, unsaved: false } + ) + store.commit('setNoteAttribute', { noteId: note.id, attribute: 'conflict', value: undefined }) +} + export const autotitleNote = noteId => { return axios .put(url('/notes/' + noteId + '/autotitle')) @@ -213,7 +279,7 @@ export const undoDeleteNote = (note) => { return axios .post(url('/notes/undo'), note) .then(response => { - store.commit('updateNote', response.data) + _updateLocalNote(response.data) return response.data }) .catch(err => { diff --git a/src/Util.js b/src/Util.js new file mode 100644 index 000000000..dd4aa5f21 --- /dev/null +++ b/src/Util.js @@ -0,0 +1,21 @@ +export const noteAttributes = [ + 'id', + 'etag', + 'title', + 'content', + 'modified', + 'favorite', + 'category', +] + +export const copyNote = (from, to, exclude) => { + if (exclude === undefined) { + exclude = [] + } + noteAttributes.forEach(attr => { + if (!exclude.includes(attr)) { + to[attr] = from[attr] + } + }) + return to +} diff --git a/src/components/ConflictSolution.vue b/src/components/ConflictSolution.vue new file mode 100644 index 000000000..535570e77 --- /dev/null +++ b/src/components/ConflictSolution.vue @@ -0,0 +1,94 @@ + + + diff --git a/src/components/Note.vue b/src/components/Note.vue index 979fd12f5..2b4054487 100644 --- a/src/components/Note.vue +++ b/src/components/Note.vue @@ -5,6 +5,27 @@ class="note-container" :class="{ fullscreen: fullscreen }" > + +
+
+ {{ t('notes', 'The note has been changed in another session. Please choose which content should be saved.') }} +
+
+ + +
+
+
{{ preview ? t('notes', 'Empty note') : t('notes', 'Write …') }} @@ -35,11 +56,18 @@ {{ fullscreen ? t('notes', 'Exit full screen') : t('notes', 'Full screen') }} -
@@ -50,16 +78,20 @@ import { Actions, ActionButton, AppContent, + Modal, Tooltip, isMobile, } from '@nextcloud/vue' import { showError } from '@nextcloud/dialogs' import { emit } from '@nextcloud/event-bus' +import SyncAlertIcon from 'vue-material-design-icons/SyncAlert' + import { config } from '../config' -import { fetchNote, refreshNote, saveNote, saveNoteManually, autotitleNote, routeIsNewNote } from '../NotesService' +import { fetchNote, refreshNote, saveNote, saveNoteManually, autotitleNote, routeIsNewNote, conflictSolutionLocal, conflictSolutionRemote } from '../NotesService' import TheEditor from './EditorEasyMDE' import ThePreview from './EditorMarkdownIt' +import ConflictSolution from './ConflictSolution' import store from '../store' export default { @@ -69,6 +101,9 @@ export default { Actions, ActionButton, AppContent, + ConflictSolution, + Modal, + SyncAlertIcon, TheEditor, ThePreview, }, @@ -96,6 +131,7 @@ export default { autotitleTimer: null, refreshTimer: null, etag: null, + showConflict: false, } }, @@ -124,6 +160,11 @@ export default { } }, title: 'onUpdateTitle', + 'note.conflict'(newConflict, oldConflict) { + if (newConflict) { + this.showConflict = true + } + }, }, created() { @@ -242,7 +283,7 @@ export default { }, refreshNote() { - if (this.note.unsaved) { + if (this.note.unsaved && !this.note.conflict) { this.startRefreshTimer() return } @@ -317,6 +358,16 @@ export default { store.commit('updateNote', note) saveNoteManually(this.note.id) }, + + onUseLocalVersion() { + conflictSolutionLocal(this.note) + this.showConflict = false + }, + + onUseRemoteVersion() { + conflictSolutionRemote(this.note) + this.showConflict = false + }, }, } @@ -379,8 +430,8 @@ export default { } .action-buttons .action-error { - width: 44px; - height: 44px; + background-color: var(--color-error); + margin-top: 1ex; } .note-container.fullscreen .action-buttons { @@ -390,4 +441,27 @@ export default { .action-buttons button { padding: 15px; } + +/* Conflict Modal */ +.conflict-modal { + width: 70vw; +} + +.conflict-header { + padding: 1ex 1em; +} + +.conflict-solutions { + display: flex; + flex-direction: row-reverse; + max-height: 75vh; + overflow-y: auto; +} + +@media (max-width: 60em) { + .conflict-solutions { + flex-direction: column; + } +} + diff --git a/src/store/notes.js b/src/store/notes.js index 4474ad8a7..a1a9f4521 100644 --- a/src/store/notes.js +++ b/src/store/notes.js @@ -1,4 +1,5 @@ import Vue from 'vue' +import { copyNote } from '../Util' const state = { categories: [], @@ -76,13 +77,11 @@ const mutations = { updateNote(state, updated) { const note = state.notesIds[updated.id] if (note) { - note.title = updated.title - note.modified = updated.modified - note.favorite = updated.favorite - note.category = updated.category + copyNote(updated, note, ['id', 'etag', 'content']) // don't update meta-data over full data - if (updated.content !== undefined || note.content === undefined) { + if (updated.content !== undefined && updated.etag !== undefined) { note.content = updated.content + note.etag = updated.etag Vue.set(note, 'unsaved', updated.unsaved) Vue.set(note, 'error', updated.error) Vue.set(note, 'errorMessage', updated.errorMessage) From 7819ad85bc1b03f0adf063a55291bbbe912e435f Mon Sep 17 00:00:00 2001 From: korelstar Date: Sat, 13 Mar 2021 09:44:33 +0100 Subject: [PATCH 4/4] API: ETag/If-None-Match for Get single note --- docs/api/v1.md | 2 ++ lib/Controller/NotesApiController.php | 5 ++++- package-lock.json | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/api/v1.md b/docs/api/v1.md index 27977fcac..f59befa27 100644 --- a/docs/api/v1.md +++ b/docs/api/v1.md @@ -91,9 +91,11 @@ No valid authentication credentials supplied. | Parameter | Type | Description | |:------|:-----|:-----| | `id` | integer, required (path) | ID of the note to query. | +| `If-None-Match` | HTTP header, optional | Use this in order to reduce transferred data size (see [HTTP ETag](https://en.wikipedia.org/wiki/HTTP_ETag)). You should use the value from the note's attribute `etag` or from the last request's HTTP response header `ETag`. | 1.2 | #### Response ##### 200 OK +- **HTTP Header**: `ETag` (see [HTTP ETag](https://en.wikipedia.org/wiki/HTTP_ETag)). The value is identical to the note's attribute `etag` (see section [Note attributes](#note-attributes)). - **Body**: note (see section [Note attributes](#note-attributes)), example: ```js { diff --git a/lib/Controller/NotesApiController.php b/lib/Controller/NotesApiController.php index 30928eaee..9c27ad112 100644 --- a/lib/Controller/NotesApiController.php +++ b/lib/Controller/NotesApiController.php @@ -68,7 +68,10 @@ public function get(int $id, string $exclude = '') : JSONResponse { return $this->helper->handleErrorResponse(function () use ($id, $exclude) { $exclude = explode(',', $exclude); $note = $this->service->get($this->helper->getUID(), $id); - return $this->helper->getNoteData($note, $exclude); + $noteData = $this->helper->getNoteData($note, $exclude); + return (new JSONResponse($noteData)) + ->setETag($noteData['etag']) + ; }); } diff --git a/package-lock.json b/package-lock.json index a8c1a85dd..3c0c57bd9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5029,6 +5029,19 @@ "resolved": "https://registry.npmjs.org/fs.realpath/-/fs.realpath-1.0.0.tgz", "integrity": "sha1-FQStJSMVjKpA20onh8sBQRmU6k8=" }, + "node_modules/fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "hasInstallScript": true, + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": "^8.16.0 || ^10.6.0 || >=11.0.0" + } + }, "node_modules/function-bind": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.1.tgz", @@ -14816,6 +14829,12 @@ "resolved": "https://registry.npmjs.org/fs.realpath/-/fs.realpath-1.0.0.tgz", "integrity": "sha1-FQStJSMVjKpA20onh8sBQRmU6k8=" }, + "fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "optional": true + }, "function-bind": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.1.tgz",