diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 0c2d7c71c..a48cfaac3 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -43,7 +43,7 @@ jobs: strategy: matrix: - php-versions: ['8.3'] + php-versions: ['8.5'] name: cs php${{ matrix.php-versions }} steps: @@ -59,7 +59,7 @@ jobs: coverage: none - name: Install dependencies - run: composer i + run: composer i --ignore-platform-req=php - name: Run coding standards check run: composer run cs:check || ( echo 'Please run `composer run cs:fix` to format your code' && exit 1 ) diff --git a/lib/Controller/BookmarkController.php b/lib/Controller/BookmarkController.php index 3998a5705..7c20fa865 100644 --- a/lib/Controller/BookmarkController.php +++ b/lib/Controller/BookmarkController.php @@ -89,7 +89,7 @@ private function _returnBookmarkAsArray(Bookmark $bookmark, bool $deleted = fals $array['folders'] = array_map(function (Folder $folder) { return $this->toExternalFolderId($folder->getId()); }, $this->treeMapper->findParentsOf(TreeMapper::TYPE_BOOKMARK, $bookmark->getId())); - }else { + } else { $array['folders'] = array_map(function (Folder $folder) { return $this->toExternalFolderId($folder->getId()); }, $this->treeMapper->findParentsOfDeletedBookmarks($bookmark->getId())); diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 392854407..c928ea521 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -913,23 +913,29 @@ public function insert(Entity $entity): Bookmark { $entityToInsert->setLastPreview(0); $entityToInsert->setClickcount(0); + // Wrap the insert in a (possibly nested) transaction so that a failed + // statement – e.g. a unique constraint violation when the URL already + // exists – can be rolled back cleanly. When this runs inside a larger + // transaction (e.g. the HTML importer) Nextcloud uses a SAVEPOINT, so + // rolling back discards only the failed INSERT instead of poisoning or + // committing the outer transaction. This keeps the connection usable for + // the insertOrUpdate() fallback below on SQLite, MySQL and Postgres alike. + $this->db->beginTransaction(); try { parent::insert($entityToInsert); - if (isset($this->userBookmarkCount[$entityToInsert->getUserId()])) { - $this->userBookmarkCount[$entityToInsert->getUserId()] += 1; - } - $this->eventDispatcher->dispatchTyped(new InsertEvent('bookmark', $entityToInsert->getId())); - return $entityToInsert; + $this->db->commit(); } catch (Exception $e) { + $this->db->rollBack(); if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - if ($this->db->inTransaction()) { - $this->db->commit(); - $this->db->beginTransaction(); - } throw new AlreadyExistsError('A bookmark with this URL already exists'); } throw $e; } + if (isset($this->userBookmarkCount[$entityToInsert->getUserId()])) { + $this->userBookmarkCount[$entityToInsert->getUserId()] += 1; + } + $this->eventDispatcher->dispatchTyped(new InsertEvent('bookmark', $entityToInsert->getId())); + return $entityToInsert; } /** diff --git a/lib/Service/HtmlImporter.php b/lib/Service/HtmlImporter.php index 82e00dde4..dacfad862 100644 --- a/lib/Service/HtmlImporter.php +++ b/lib/Service/HtmlImporter.php @@ -125,8 +125,16 @@ public function import(string $userId, string $content, ?int $rootFolderId = nul } $imported[] = ['type' => 'bookmark', 'id' => $bm->getId(), 'title' => $bookmark['title'], 'url' => $bookmark['href']]; } - } finally { $this->connection->commit(); + } catch (\Throwable $e) { + // Roll back instead of committing a transaction that may contain a + // failed statement – committing such a transaction throws and masks + // the original error. + if ($this->connection->inTransaction()) { + $this->connection->rollBack(); + } + $this->hashManager->setInvalidationEnabled(true); + throw $e; } $this->hashManager->setInvalidationEnabled(true); diff --git a/src/components/LoadingModal.vue b/src/components/LoadingModal.vue index de8a1c8c0..61d501492 100644 --- a/src/components/LoadingModal.vue +++ b/src/components/LoadingModal.vue @@ -24,6 +24,7 @@ export default { copySelection: this.t('bookmkarks', 'Adding selection to folders'), emptyTrashbin: this.t('bookmkarks', 'Emptying trashbin'), deleted_folders: this.t('bookmkarks', 'Loading trashbin'), + undeleteFolder: this.t('bookmarks', 'Restoring folder'), }, showNcModal: false, showTimeout: null, diff --git a/src/store/actions.js b/src/store/actions.js index c2478093e..74bbc298a 100644 --- a/src/store/actions.js +++ b/src/store/actions.js @@ -461,7 +461,7 @@ export default { if (oldFolder) { const response2 = await axios.delete( url(state, `/folder/${oldFolder}/bookmarks/${bookmark}?hardDelete=true`), - ); + ) if (response2.data.status !== 'success') { throw new Error(response2.data) } @@ -943,6 +943,7 @@ export default { try { const parentFolderId = this.getters.getFolder(id)[0].parent_folder const parentFolderItem = this.getters.getFolder(parentFolderId)[0] + commit(mutations.FETCH_START, { type: 'undeleteFolder' }) const response = await axios.post(url(state, `/folder/${id}/undelete`)) const { data: { status }, @@ -958,10 +959,12 @@ export default { actions.LOAD_FOLDER_CHILDREN_ORDER, parentFolderItem ? parentFolderId : '-1', ) - await dispatch(actions.LOAD_FOLDERS) + await dispatch(actions.LOAD_FOLDERS, /* force: */ true) await dispatch(actions.LOAD_DELETED_FOLDERS) } + commit(mutations.FETCH_END, 'undeleteFolder') } catch (err) { + commit(mutations.FETCH_END, 'undeleteFolder') console.error(err) commit( mutations.SET_ERROR, diff --git a/tests/HtmlImportExportTest.php b/tests/HtmlImportExportTest.php index e07713157..7fc714d72 100644 --- a/tests/HtmlImportExportTest.php +++ b/tests/HtmlImportExportTest.php @@ -112,6 +112,33 @@ public function testImportFile(string $file): void { $this->assertEquals(1231231234, $firstBookmark->getAdded()); } + /** + * Re-importing the same file must not fail: every bookmark already exists, + * so the importer takes the unique-constraint -> update path for each one. + * This used to break the import transaction and raise a 500 + * ("cannot commit transaction - SQL statements in progress"). + * + * @dataProvider importProvider + * @param string $file + * @throws DoesNotExistException + * @throws MultipleObjectsReturnedException + * @throws UnauthorizedAccessError + * @throws AlreadyExistsError + * @throws UserLimitExceededError + * @throws HtmlParseError + */ + public function testReimportFile(string $file): void { + $this->htmlImporter->importFile($this->userId, $file); + // Second import hits the duplicate-URL update path for every bookmark + $result = $this->htmlImporter->importFile($this->userId, $file); + + $this->assertEmpty($result['errors']); + + $firstBookmark = $this->bookmarkMapper->find($result['imported'][0]['children'][0]['id']); + $this->assertSame('Title 0', $firstBookmark->getTitle()); + $this->assertSame('http://url0.net/', $firstBookmark->getUrl()); + } + /** * @dataProvider exportProvider * @param array $bookmarks