Merge pull request #2460 from nextcloud/fixes

Fixes

Author: marcelklehr

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

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()));

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;
 	}
 
 	/**

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

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,

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,

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