From 83d795b81312d68548a74de5bde965be9bdeb985 Mon Sep 17 00:00:00 2001 From: Abhishek Date: Wed, 5 Aug 2026 19:58:50 +0530 Subject: [PATCH] fix(bookmarks): keep explicit title over scraped title An explicit title on POST /api/bookmark was thrown away whenever the url turned out to be scrapable, so a scrapable bookmark could never be given a custom title. addBookmark now resolves the title in order of precedence: the title the user gave us, then the scraped page title, then the host of the url. Clients default the title field to the url itself, so a title equal to the url is treated as unset and the scraped title wins. The scrape and the title resolution are split into scrape(), resolveTitle() and hasExplicitTitle(). The page is still fetched, indexed in Typesense and screenshotted when the title is explicit, only the title is left alone. This also removes an NPE when a null title met a failed scrape, and a debug log that dereferenced retDoc.connection().response(). Tests cover a scrapable request with no title (null, empty, blank and url), with an explicit title, with a page that has no title of its own, and a failed scrape falling back to the host. The updateBookmark controller test asserted the old behaviour and now expects the posted title to be kept. --- .../core/service/BookmarkService.java | 70 +++++++-- .../controller/BookmarkControllerTest.java | 10 +- .../core/service/BookmarkServiceTest.java | 138 +++++++++++++++++- 3 files changed, 195 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/dev/findfirst/core/service/BookmarkService.java b/server/src/main/java/dev/findfirst/core/service/BookmarkService.java index 4be72ec4..e27e1038 100644 --- a/server/src/main/java/dev/findfirst/core/service/BookmarkService.java +++ b/server/src/main/java/dev/findfirst/core/service/BookmarkService.java @@ -171,7 +171,6 @@ public BookmarkDTO addBookmark(AddBkmkReq reqBkmk) } Document retDoc = null; - String title = reqBkmk.title(); var screenshotUrlOpt = Optional.of(""); boolean shouldScrape = reqBkmk.scrapable(); @@ -182,22 +181,12 @@ public BookmarkDTO addBookmark(AddBkmkReq reqBkmk) if (shouldScrape) { log.debug("Scrapable: true.\tScrapping URL and taking screenshot."); - - try { - retDoc = Jsoup.connect(reqBkmk.url()).get(); - log.debug("Response: {}\tTitle: {}", retDoc.connection().response().statusMessage(), - retDoc.title()); - title = !retDoc.title().isEmpty() ? retDoc.title() : reqBkmk.title(); - } catch (IOException e) { - log.error(e.toString()); - } - - title = !title.isEmpty() ? title : reqBkmk.title(); + retDoc = scrape(reqBkmk.url()); screenshotUrlOpt = sManager.getScreenshot(reqBkmk.url()); - } else if (title == null || title.isEmpty()) { - title = new URI(reqBkmk.url()).getHost(); } + String title = resolveTitle(reqBkmk, retDoc); + var user = userService.getUserById(uContext.getUserId()).orElseThrow(); var savedTags = new HashSet(); @@ -223,6 +212,59 @@ public BookmarkDTO addBookmark(AddBkmkReq reqBkmk) return convertBookmarkJDBCToDTO(List.of(newBkmkJdbc), user.getUserId()).get(0); } + /** + * Fetches the page so its title and text can be indexed. + * + * @param url the url to fetch. + * @return the fetched document, or null when the page could not be reached. + */ + private Document scrape(String url) { + try { + var doc = Jsoup.connect(url).get(); + log.debug("Scraped Title: {}", doc.title()); + return doc; + } catch (IOException e) { + log.error(e.toString()); + return null; + } + } + + /** + * Resolves the title to store for a new bookmark. A title the user explicitly gave us always wins + * over the one found while scraping the page, otherwise there would be no way to give a scrapable + * bookmark a custom title. + * + * @param reqBkmk the add request as it came from the client. + * @param scrapedDoc the scraped page, null when the url was not or could not be scraped. + * @return the explicit title, else the scraped title, else the host of the url. + * @throws URISyntaxException if there is no title to fall back on and the url is malformed. + */ + private String resolveTitle(AddBkmkReq reqBkmk, Document scrapedDoc) throws URISyntaxException { + if (hasExplicitTitle(reqBkmk)) { + return reqBkmk.title(); + } + + if (scrapedDoc != null && !scrapedDoc.title().isBlank()) { + return scrapedDoc.title(); + } + + // Nothing was given and nothing was scraped, so the host is the best we can do. + var title = reqBkmk.title(); + return title == null || title.isBlank() ? new URI(reqBkmk.url()).getHost() : title; + } + + /** + * A title only counts as explicit when the user actually chose it. Clients default the field to + * the url itself, which means unset rather than a user's choice. + * + * @param reqBkmk the add request as it came from the client. + * @return true when the request carries a user chosen title. + */ + private boolean hasExplicitTitle(AddBkmkReq reqBkmk) { + var title = reqBkmk.title(); + return title != null && !title.isBlank() && !title.equals(reqBkmk.url()); + } + public List addBookmarks(List bookmarks) { return bookmarks.stream().map(t -> { try { diff --git a/server/src/test/java/dev/findfirst/core/controller/BookmarkControllerTest.java b/server/src/test/java/dev/findfirst/core/controller/BookmarkControllerTest.java index ba0f0476..12601ab0 100644 --- a/server/src/test/java/dev/findfirst/core/controller/BookmarkControllerTest.java +++ b/server/src/test/java/dev/findfirst/core/controller/BookmarkControllerTest.java @@ -217,7 +217,6 @@ void updateBookmark() { .setRequestFactory(new HttpComponentsClientHttpRequestFactory()); String oldTitle = "Dark mode guide"; - String ifScrapableTitle = "Dark mode in React: An in-depth guide - LogRocket Blog"; String newTitle = "Dark MODE"; String url = "https://blog.logrocket.com/dark-mode-react-in-depth-guide/#what-dark-mode"; @@ -229,14 +228,9 @@ void updateBookmark() { getHttpEntity(restTemplate, new UpdateBookmarkReq(id, null, null, null)), BookmarkDTO.class); var bkmkDTO = noChangeReq.getBody(); - var scrapable = bkmkDTO.scrapable(); - - if (scrapable) { - assertEquals(ifScrapableTitle, bkmkDTO.title()); - } else { - assertEquals(oldTitle, bkmkDTO.title()); - } + // The title was given on the add request, so it is kept even though the url was scraped. + assertEquals(oldTitle, bkmkDTO.title()); assertEquals(url, bkmkDTO.url()); var updateReq = restTemplate.exchange(bookmarkURI, HttpMethod.PATCH, diff --git a/server/src/test/java/dev/findfirst/core/service/BookmarkServiceTest.java b/server/src/test/java/dev/findfirst/core/service/BookmarkServiceTest.java index 1e45d05e..109cea4a 100644 --- a/server/src/test/java/dev/findfirst/core/service/BookmarkServiceTest.java +++ b/server/src/test/java/dev/findfirst/core/service/BookmarkServiceTest.java @@ -1,22 +1,37 @@ package dev.findfirst.core.service; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; +import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Optional; +import dev.findfirst.core.dto.AddBkmkReq; +import dev.findfirst.core.dto.BookmarkDTO; import dev.findfirst.core.model.jdbc.BookmarkJDBC; import dev.findfirst.core.repository.jdbc.BookmarkJDBCRepository; +import dev.findfirst.core.repository.jdbc.BookmarkTagRepository; +import dev.findfirst.security.userauth.context.UserContext; +import dev.findfirst.users.model.user.User; +import dev.findfirst.users.service.UserManagementService; +import org.jsoup.Connection; +import org.jsoup.Jsoup; +import org.jsoup.nodes.Document; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EmptySource; import org.junit.jupiter.params.provider.NullSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -27,12 +42,30 @@ class BookmarkServiceTest { @Mock private BookmarkJDBCRepository bookmarkRepository; @Mock + private BookmarkTagRepository bookmarkTagRepository; + @Mock + private TagService tagService; + @Mock + private WebCheckService webCheckService; + @Mock private ScreenshotManager sManager; + @Mock + private UserContext uContext; + @Mock + private UserManagementService userService; + @Mock + private TypesenseService typesense; + + private static final int USER_ID = 1; + private static final long BOOKMARK_ID = 42L; + private static final String URL = "https://a-website.com/some/page"; + private static final String SCRAPED_TITLE = "A Website - Some Page"; + private static final String SCREENSHOT_URL = "http://example.com/screenshot.png"; /** * Tests that addMissingScreenShotUrlToBookMarks method of BookmarkService class add screenshots * urls to scrapable bookmarks. - * + * * @param scrapable */ @ParameterizedTest @@ -60,4 +93,107 @@ void addMissingScreenShotUrlToBookMarksTests(Boolean scrapable) { verify(bookmarkRepository, times(1)).findBookmarksWithEmptyOrBlankScreenShotUrl(); verify(bookmarkRepository, times(1)).saveAll(list); } + + /** + * A scrapable request without a title of its own falls back to the scraped title. Clients default + * the title to the url, so that counts as no title at all. + * + * @param requestedTitle the title as it arrived on the request. + */ + @ParameterizedTest + @NullSource + @EmptySource + @ValueSource(strings = {" ", URL}) + @DisplayName("addBookmark() -> scraped title is used when no title was provided") + void addBookmarkUsesScrapedTitleWhenNoneProvided(String requestedTitle) throws Exception { + var bkmk = addScrapableBookmark(requestedTitle, pageTitled(SCRAPED_TITLE), null); + + Assertions.assertEquals(SCRAPED_TITLE, bkmk.title()); + } + + /** + * An explicitly given title is the user's choice and must survive the scrape, otherwise a + * scrapable bookmark could never be given a custom title. + */ + @Test + @DisplayName("addBookmark() -> explicit title wins over the scraped title") + void addBookmarkKeepsExplicitTitleOverScrapedTitle() throws Exception { + String explicitTitle = "My Own Title"; + var scrapedPage = pageTitled(SCRAPED_TITLE); + + var bkmk = addScrapableBookmark(explicitTitle, scrapedPage, null); + + Assertions.assertEquals(explicitTitle, bkmk.title()); + // The page is still scraped and indexed, only the title is left alone. + verify(typesense).addText(any(BookmarkJDBC.class), eq(scrapedPage)); + verify(sManager).getScreenshot(URL); + } + + /** + * The page has no title of its own, so there is nothing to override the requested title with. + */ + @Test + @DisplayName("addBookmark() -> requested title is kept when the page has no title") + void addBookmarkKeepsRequestedTitleWhenPageHasNoTitle() throws Exception { + String explicitTitle = "My Own Title"; + var pageWithoutTitle = Jsoup.parse("no title here"); + + var bkmk = addScrapableBookmark(explicitTitle, pageWithoutTitle, null); + + Assertions.assertEquals(explicitTitle, bkmk.title()); + } + + /** + * Nothing was given and nothing could be scraped, so the host is used as the title. + */ + @Test + @DisplayName("addBookmark() -> host is used when there is no title to be found") + void addBookmarkFallsBackToHostWhenScrapeFails() throws Exception { + var bkmk = addScrapableBookmark(null, null, new IOException("could not reach the page")); + + Assertions.assertEquals("a-website.com", bkmk.title()); + } + + /** + * @param title the title the page carries. + * @return a document as it would come back from a scrape. + */ + private static Document pageTitled(String title) { + return Jsoup.parse("" + title + ""); + } + + /** + * Adds a bookmark for {@link #URL} as a scrapable request. + * + * @param requestedTitle the title to send along with the request. + * @param scraped the document the scrape should return. + * @param scrapeFailure thrown by the scrape instead of returning a document, may be null. + * @return the created bookmark. + */ + private BookmarkDTO addScrapableBookmark(String requestedTitle, Document scraped, + IOException scrapeFailure) throws Exception { + when(uContext.getUserId()).thenReturn(USER_ID); + when(bookmarkRepository.findByUrl(URL, USER_ID)).thenReturn(Optional.empty()); + when(webCheckService.isScrapable(URL)).thenReturn(true); + when(sManager.getScreenshot(URL)).thenReturn(Optional.of(SCREENSHOT_URL)); + when(userService.getUserById(USER_ID)).thenReturn(Optional.of(new User(USER_ID, "jsmith", ""))); + when(bookmarkRepository.save(any(BookmarkJDBC.class))).thenAnswer(invocation -> { + BookmarkJDBC toSave = invocation.getArgument(0); + toSave.setId(BOOKMARK_ID); + return toSave; + }); + + var connection = mock(Connection.class); + // Jsoup.connect is static, so the whole class is stubbed for the duration of the call. Any + // document has to be parsed before that happens. + try (MockedStatic jsoup = mockStatic(Jsoup.class)) { + jsoup.when(() -> Jsoup.connect(URL)).thenReturn(connection); + if (scrapeFailure != null) { + when(connection.get()).thenThrow(scrapeFailure); + } else { + when(connection.get()).thenReturn(scraped); + } + return bookmarkService.addBookmark(new AddBkmkReq(requestedTitle, URL, null, true)); + } + } }