Fix: Duplicate creation handling of title feld

lib/Db/BookmarkMapper.php

@@ -928,18 +928,27 @@ public function insert(Entity $entity): Bookmark {
 	/**
 	 * @psalm-param Bookmark $entity
 	 * @param Entity $entity
+	 * @param string[] $preserveFields Fields of an already existing bookmark that
+	 *                                 must not be overwritten with the values of
+	 *                                 $entity, e.g. because the caller derived
+	 *                                 those from scraping rather than from user
+	 *                                 input. Only takes effect when the insert
+	 *                                 turns out to be an update.
 	 * @return Bookmark
 	 * @throws AlreadyExistsError
 	 * @throws UrlParseError
 	 * @throws UserLimitExceededError|MultipleObjectsReturnedException
 	 */
-	public function insertOrUpdate(Entity $entity): Bookmark {
+	public function insertOrUpdate(Entity $entity, array $preserveFields = []): Bookmark {
 		try {
 			$newEntity = $this->insert($entity);
 		} catch (AlreadyExistsError) {
 			try {
 				$bookmark = $this->findByUrl($entity->getUserId(), $entity->getUrl());
 				foreach ($entity->getUpdatedFields() as $field => $_value) {
+					if (in_array($field, $preserveFields, true)) {
+						continue;
+					}
 					$value = $entity->{'get' . ucfirst($field)}();
 					$bookmark->{'set' . ucfirst($field)}($value);
 				}

lib/Service/BookmarkService.php

@@ -115,22 +115,31 @@ public function create(string $userId, string $url = '', ?string $title = null,
 	private function _addBookmark($userId, $url, ?string $title = null, ?string $description = null, ?array $tags = null, array $folders = []): Bookmark {
 		$isInsert = true;
 		$bookmark = null;
+		$derivedFields = [];
 
 		try {
 			$bookmark = $this->bookmarkMapper->findByUrl($userId, $url);
 			$isInsert = false;
 		} catch (DoesNotExistException) {
-			if (!preg_match(self::PROTOCOLS_REGEX, $url)) {
-				// if no allowed protocol is given, evaluate https and https
-				foreach (['https://', 'http://'] as $protocol) {
-					try {
-						$testUrl = $this->urlNormalizer->normalize($protocol . $url);
-						$bookmark = $this->bookmarkMapper->findByUrl($userId, $testUrl);
-						$isInsert = false;
-						break;
-					} catch (UrlParseError|DoesNotExistException) {
-						continue;
-					}
+			// The mapper hashes the URL verbatim, but insert() normalizes web links
+			// before hashing them, so the stored hash is the one of the normalized
+			// URL. Retry with the normalized form before treating this as a new
+			// bookmark - otherwise we'd scrape a title below and insertOrUpdate()
+			// would write it over the title the user gave the existing bookmark.
+			$candidates = preg_match(self::PROTOCOLS_REGEX, $url)
+				// if no allowed protocol is given, evaluate https and http
+				? [$url]
+				: ['https://' . $url, 'http://' . $url];
+			foreach ($candidates as $candidate) {
+				try {
+					$testUrl = preg_match('/^https?:\/\//i', $candidate)
+						? $this->urlNormalizer->normalize($candidate)
+						: $candidate;
+					$bookmark = $this->bookmarkMapper->findByUrl($userId, $testUrl);
+					$isInsert = false;
+					break;
+				} catch (UrlParseError|DoesNotExistException) {
+					continue;
 				}
 			}
 		}
@@ -163,6 +172,14 @@ private function _addBookmark($userId, $url, ?string $title = null, ?string $des
 			}
 			$url = $this->urlNormalizer->normalize($url);
 
+			// Remember which values we made up ourselves instead of getting them from
+			// the caller, so that the fallback update in insertOrUpdate() can't write
+			// scraped page data over what an existing bookmark already says.
+			$derivedFields = array_merge(
+				isset($title) ? [] : ['title'],
+				isset($description) ? [] : ['description'],
+			);
+
 			$title = $title ?? trim($data['basic']['title']) ?? trim($url);
 			$description = $description ?? $data['basic']['description'] ?? '';
 
@@ -178,7 +195,7 @@ private function _addBookmark($userId, $url, ?string $title = null, ?string $des
 		}
 		$bookmark->setUserId($userId);
 		if ($isInsert) {
-			$bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark);
+			$bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark, $derivedFields);
 		} else {
 			$bookmark = $this->bookmarkMapper->update($bookmark);
 		}

tests/BookmarkControllerTest.php

@@ -393,6 +393,44 @@ public function testCreate(): void {
 		$this->assertCount(0, $data['data']);
 	}
 
+	/**
+	 * Creating a bookmark for a URL the user already has must not touch the title
+	 * and description they gave it. The URL is normalized before it is stored
+	 * ('https://www.heise.de' becomes 'https://www.heise.de/'), so the duplicate
+	 * check has to normalize too - otherwise the create is taken for an insert,
+	 * the page gets scraped and the existing bookmark ends up with the page title.
+	 *
+	 * @throws AlreadyExistsError
+	 * @throws DoesNotExistException
+	 * @throws MultipleObjectsReturnedException
+	 * @throws UrlParseError
+	 * @throws UserLimitExceededError
+	 */
+	public function testCreateExistingUnnormalizedUrlKeepsTitle(): void {
+		$this->cleanUp();
+		$this->setupBookmarks();
+		$this->authorizer->setUserId($this->userId);
+
+		$res = $this->controller->newBookmark('https://www.heise.de', 'My own title', 'My own description');
+		$this->assertEquals('success', $res->getData()['status'], var_export($res->getData(), true));
+		$id = $res->getData()['item']['id'];
+		$this->assertEquals('https://www.heise.de/', $this->bookmarkMapper->find($id)->getUrl());
+
+		// Add the same URL again in the shape the user pasted it, without a title -
+		// the shape a browser extension or the bookmarklet sends.
+		$res = $this->controller->newBookmark('https://www.heise.de');
+		$this->assertEquals('success', $res->getData()['status'], var_export($res->getData(), true));
+		$this->assertEquals($id, $res->getData()['item']['id']);
+
+		$bookmark = $this->bookmarkMapper->find($id);
+		$this->assertEquals('My own title', $bookmark->getTitle());
+		$this->assertEquals('My own description', $bookmark->getDescription());
+
+		// No duplicate row was created either.
+		$params = new QueryParameters();
+		$this->assertCount(1, $this->bookmarkMapper->findAll($this->userId, $params->setUrl('https://www.heise.de/')));
+	}
+
 	/**
 	 * @throws AlreadyExistsError
 	 * @throws DoesNotExistException

tests/BookmarkMapperTest.php

@@ -387,6 +387,54 @@ public function testCountAllClicksInSubfolderOfSharedFolderCountsEachBookmarkOnc
 		$this->assertSame(7, $this->bookmarkMapper->countAllClicks($recipientId));
 	}
 
+	/**
+	 * insertOrUpdate() falls back to updating an existing bookmark when the insert
+	 * hits the unique index. Fields the caller marks as preserved must survive that
+	 * fallback, so derived data (e.g. a scraped page title) can never overwrite what
+	 * the user put on the existing bookmark.
+	 *
+	 * @throws AlreadyExistsError
+	 * @throws UserLimitExceededError
+	 * @throws UrlParseError
+	 * @throws MultipleObjectsReturnedException
+	 * @throws DoesNotExistException
+	 */
+	public function testInsertOrUpdatePreservesFields() {
+		$existing = $this->bookmarkMapper->insertOrUpdate(Db\Bookmark::fromArray([
+			'userId' => $this->userId,
+			'url' => 'https://example.org/preserve-fields',
+			'title' => 'The title the user chose',
+			'description' => 'The description the user wrote',
+		]));
+
+		$incoming = Db\Bookmark::fromArray([
+			'userId' => $this->userId,
+			'url' => 'https://example.org/preserve-fields',
+			'title' => 'Scraped page title',
+			'description' => 'Scraped page description',
+		]);
+		$updated = $this->bookmarkMapper->insertOrUpdate($incoming, ['title', 'description']);
+
+		$this->assertSame($existing->getId(), $updated->getId());
+		$this->assertSame('The title the user chose', $updated->getTitle());
+		$this->assertSame('The description the user wrote', $updated->getDescription());
+
+		// And it is still the same, single row.
+		$stored = $this->bookmarkMapper->find($existing->getId());
+		$this->assertSame('The title the user chose', $stored->getTitle());
+		$this->assertSame('The description the user wrote', $stored->getDescription());
+
+		// Without preserveFields the fallback keeps overwriting, as before.
+		$overwritten = $this->bookmarkMapper->insertOrUpdate(Db\Bookmark::fromArray([
+			'userId' => $this->userId,
+			'url' => 'https://example.org/preserve-fields',
+			'title' => 'Scraped page title',
+			'description' => 'Scraped page description',
+		]));
+		$this->assertSame($existing->getId(), $overwritten->getId());
+		$this->assertSame('Scraped page title', $overwritten->getTitle());
+	}
+
 	/**
 	 * @return array
 	 */