diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 921b9b155..f6656b011 100644 --- a/lib/Db/BookmarkMapper.php +++ b/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); } diff --git a/lib/Service/BookmarkService.php b/lib/Service/BookmarkService.php index 6c4e03992..a704f5215 100644 --- a/lib/Service/BookmarkService.php +++ b/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); } diff --git a/tests/BookmarkControllerTest.php b/tests/BookmarkControllerTest.php index dcebd854c..92f9721d5 100644 --- a/tests/BookmarkControllerTest.php +++ b/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 diff --git a/tests/BookmarkMapperTest.php b/tests/BookmarkMapperTest.php index 1861cbf72..db83e265d 100644 --- a/tests/BookmarkMapperTest.php +++ b/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 */