From 33967c2873c0f5e68b6b488294fabdac28697c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Cr=C3=A9mieux?= Date: Sat, 28 Sep 2024 21:55:32 +0200 Subject: [PATCH] Refactor Citations Relations Service Layer (#11189) * Move logic from repository to service * Refactor repositories * Update tab configuration --- .../CitationRelationsTab.java | 93 ++++--- .../CitationsRelationsTabViewModel.java | 2 +- .../BibEntryRelationsRepository.java | 24 +- .../repository/LRUBibEntryRelationsCache.java | 39 ++- .../LRUBibEntryRelationsRepository.java | 69 ++--- .../SearchCitationsRelationsService.java | 56 +++-- .../SemanticScholarCitationFetcher.java | 4 +- .../org/jabref/logic/util/BackgroundTask.java | 10 + .../CitationsRelationsTabViewModelTest.java | 3 +- ...ntryRelationsRepositoryHelpersForTest.java | 87 +++++++ .../BibEntryRelationsRepositoryTest.java | 128 ---------- ...ibEntryRelationsRepositoryTestHelpers.java | 39 --- .../LRUBibEntryRelationsRepositoryTest.java | 95 +++++++ .../SearchCitationsRelationsServiceTest.java | 235 ++++++++++++------ .../CitationFetcherHelpersForTest.java | 32 +++ 15 files changed, 546 insertions(+), 370 deletions(-) create mode 100644 src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryHelpersForTest.java delete mode 100644 src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTest.java delete mode 100644 src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTestHelpers.java create mode 100644 src/test/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepositoryTest.java create mode 100644 src/test/java/org/jabref/logic/importer/fetcher/CitationFetcherHelpersForTest.java diff --git a/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java b/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java index 048d058462d1..e0f6c6c49a7d 100644 --- a/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java +++ b/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java @@ -30,15 +30,16 @@ import org.jabref.gui.StateManager; import org.jabref.gui.desktop.os.NativeDesktop; import org.jabref.gui.entryeditor.EntryEditorTab; -import org.jabref.logic.citation.repository.BibEntryRelationsCache; -import org.jabref.logic.citation.repository.BibEntryRelationsRepository; -import org.jabref.logic.importer.fetcher.CitationFetcher; -import org.jabref.logic.importer.fetcher.SemanticScholarCitationFetcher; import org.jabref.gui.icon.IconTheme; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.util.NoSelectionModel; import org.jabref.gui.util.ViewModelListCellFactory; +import org.jabref.logic.citation.repository.LRUBibEntryRelationsCache; +import org.jabref.logic.citation.repository.LRUBibEntryRelationsRepository; +import org.jabref.logic.citation.service.SearchCitationsRelationsService; import org.jabref.logic.database.DuplicateCheck; +import org.jabref.logic.importer.fetcher.CitationFetcher; +import org.jabref.logic.importer.fetcher.SemanticScholarCitationFetcher; import org.jabref.logic.l10n.Localization; import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.TaskExecutor; @@ -73,7 +74,7 @@ public class CitationRelationsTab extends EntryEditorTab { private final GuiPreferences preferences; private final LibraryTab libraryTab; private final TaskExecutor taskExecutor; - private final BibEntryRelationsRepository bibEntryRelationsRepository; + private final SearchCitationsRelationsService searchCitationsRelationsService; private final CitationsRelationsTabViewModel citationsRelationsTabViewModel; private final DuplicateCheck duplicateCheck; @@ -94,11 +95,22 @@ public CitationRelationsTab(DialogService dialogService, setTooltip(new Tooltip(Localization.lang("Show articles related by citation"))); this.duplicateCheck = new DuplicateCheck(new BibEntryTypesManager()); - this.bibEntryRelationsRepository = new BibEntryRelationsRepository( + var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( + new LRUBibEntryRelationsCache() + ); + this.searchCitationsRelationsService = new SearchCitationsRelationsService( new SemanticScholarCitationFetcher(preferences.getImporterPreferences()), - new BibEntryRelationsCache() + bibEntryRelationsRepository + ); + citationsRelationsTabViewModel = new CitationsRelationsTabViewModel( + databaseContext, + preferences, + undoManager, + stateManager, + dialogService, + fileUpdateMonitor, + taskExecutor ); - citationsRelationsTabViewModel = new CitationsRelationsTabViewModel(databaseContext, preferences, undoManager, stateManager, dialogService, fileUpdateMonitor, taskExecutor); } /** @@ -347,41 +359,54 @@ private void searchForRelations(BibEntry entry, CheckListView> task; - - if (searchType == CitationFetcher.SearchType.CITES) { - task = BackgroundTask.wrap(() -> { - if (shouldRefresh) { - bibEntryRelationsRepository.forceRefreshReferences(entry); - } - return bibEntryRelationsRepository.getReferences(entry); - }); - citingTask = task; - } else { - task = BackgroundTask.wrap(() -> { - if (shouldRefresh) { - bibEntryRelationsRepository.forceRefreshCitations(entry); - } - return bibEntryRelationsRepository.getCitations(entry); - }); - citedByTask = task; - } - - task.onRunning(() -> prepareToSearchForRelations(abortButton, refreshButton, importButton, progress, task)) - .onSuccess(fetchedList -> onSearchForRelationsSucceed(entry, listView, abortButton, refreshButton, - searchType, importButton, progress, fetchedList, observableList)) + this.createBackGroundTask(entry, searchType, shouldRefresh) + .consumeOnRunning(task -> prepareToSearchForRelations( + abortButton, refreshButton, importButton, progress, task + )) + .onSuccess(fetchedList -> onSearchForRelationsSucceed( + entry, + listView, + abortButton, + refreshButton, + searchType, + importButton, + progress, + fetchedList, + observableList + )) .onFailure(exception -> { LOGGER.error("Error while fetching citing Articles", exception); hideNodes(abortButton, progress, importButton); - listView.setPlaceholder(new Label(Localization.lang("Error while fetching citing entries: %0", - exception.getMessage()))); - + listView.setPlaceholder( + new Label(Localization.lang( + "Error while fetching citing entries: %0", exception.getMessage()) + ) + ); refreshButton.setVisible(true); dialogService.notify(exception.getMessage()); }) .executeWith(taskExecutor); } + private BackgroundTask> createBackGroundTask( + BibEntry entry, CitationFetcher.SearchType searchType, boolean shouldRefresh + ) { + return switch (searchType) { + case CitationFetcher.SearchType.CITES -> { + citingTask = BackgroundTask.wrap( + () -> this.searchCitationsRelationsService.searchReferences(entry, shouldRefresh) + ); + yield citingTask; + } + case CitationFetcher.SearchType.CITED_BY -> { + citedByTask = BackgroundTask.wrap( + () -> this.searchCitationsRelationsService.searchCitations(entry, shouldRefresh) + ); + yield citedByTask; + } + }; + } + private void onSearchForRelationsSucceed(BibEntry entry, CheckListView listView, Button abortButton, Button refreshButton, CitationFetcher.SearchType searchType, Button importButton, diff --git a/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java b/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java index 0d0d5646ff13..f095e08e45ee 100644 --- a/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java +++ b/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java @@ -8,11 +8,11 @@ import org.jabref.gui.DialogService; import org.jabref.gui.StateManager; -import org.jabref.logic.importer.fetcher.CitationFetcher; import org.jabref.gui.externalfiles.ImportHandler; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.logic.citationkeypattern.CitationKeyGenerator; import org.jabref.logic.citationkeypattern.CitationKeyPatternPreferences; +import org.jabref.logic.importer.fetcher.CitationFetcher; import org.jabref.logic.util.TaskExecutor; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; diff --git a/src/main/java/org/jabref/logic/citation/repository/BibEntryRelationsRepository.java b/src/main/java/org/jabref/logic/citation/repository/BibEntryRelationsRepository.java index 48f38e2a38d0..84b4c73a1d7b 100644 --- a/src/main/java/org/jabref/logic/citation/repository/BibEntryRelationsRepository.java +++ b/src/main/java/org/jabref/logic/citation/repository/BibEntryRelationsRepository.java @@ -1,26 +1,20 @@ package org.jabref.logic.citation.repository; import java.util.List; + import org.jabref.model.entry.BibEntry; public interface BibEntryRelationsRepository { + + void insertCitations(BibEntry entry, List citations); + List readCitations(BibEntry entry); + boolean containsCitations(BibEntry entry); + + void insertReferences(BibEntry entry, List citations); + List readReferences(BibEntry entry); - /** - * Fetch citations for a bib entry and update local database. - * @param entry should not be null - * @deprecated fetching citations should be done by the service layer (calling code) - */ - @Deprecated - void forceRefreshCitations(BibEntry entry); - - /** - * Fetch references made by a bib entry and update local database. - * @param entry should not be null - * @deprecated fetching references should be done by the service layer (calling code) - */ - @Deprecated - void forceRefreshReferences(BibEntry entry); + boolean containsReferences(BibEntry entry); } diff --git a/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java b/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java index 8e6d491ce752..f87455fc0338 100644 --- a/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java +++ b/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java @@ -1,38 +1,57 @@ package org.jabref.logic.citation.repository; -import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import org.eclipse.jgit.util.LRUMap; +import java.util.Set; + import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.identifier.DOI; +import org.eclipse.jgit.util.LRUMap; + public class LRUBibEntryRelationsCache { private static final Integer MAX_CACHED_ENTRIES = 100; - private static final Map> CITATIONS_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); - private static final Map> REFERENCES_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); + private static final Map> CITATIONS_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); + private static final Map> REFERENCES_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); public List getCitations(BibEntry entry) { - return CITATIONS_MAP.getOrDefault(entry.getDOI().map(DOI::getDOI).orElse(""), Collections.emptyList()); + return entry + .getDOI() + .stream() + .flatMap(doi -> CITATIONS_MAP.getOrDefault(doi, Set.of()).stream()) + .toList(); } public List getReferences(BibEntry entry) { - return REFERENCES_MAP.getOrDefault(entry.getDOI().map(DOI::getDOI).orElse(""), Collections.emptyList()); + return entry + .getDOI() + .stream() + .flatMap(doi -> REFERENCES_MAP.getOrDefault(doi, Set.of()).stream()) + .toList(); } public void cacheOrMergeCitations(BibEntry entry, List citations) { - entry.getDOI().ifPresent(doi -> CITATIONS_MAP.put(doi.getDOI(), citations)); + entry.getDOI().ifPresent(doi -> { + var cachedRelations = CITATIONS_MAP.getOrDefault(doi, new LinkedHashSet<>()); + cachedRelations.addAll(citations); + CITATIONS_MAP.put(doi, cachedRelations); + }); } public void cacheOrMergeReferences(BibEntry entry, List references) { - entry.getDOI().ifPresent(doi -> REFERENCES_MAP.putIfAbsent(doi.getDOI(), references)); + entry.getDOI().ifPresent(doi -> { + var cachedRelations = REFERENCES_MAP.getOrDefault(doi, new LinkedHashSet<>()); + cachedRelations.addAll(references); + REFERENCES_MAP.put(doi, cachedRelations); + }); } public boolean citationsCached(BibEntry entry) { - return CITATIONS_MAP.containsKey(entry.getDOI().map(DOI::getDOI).orElse("")); + return entry.getDOI().map(CITATIONS_MAP::containsKey).orElse(false); } public boolean referencesCached(BibEntry entry) { - return REFERENCES_MAP.containsKey(entry.getDOI().map(DOI::getDOI).orElse("")); + return entry.getDOI().map(REFERENCES_MAP::containsKey).orElse(false); } } diff --git a/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java b/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java index 23ab083ee4bb..18c360ec17ee 100644 --- a/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java +++ b/src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java @@ -1,78 +1,49 @@ package org.jabref.logic.citation.repository; import java.util.List; +import java.util.Objects; -import org.jabref.logic.importer.fetcher.CitationFetcher; -import org.jabref.logic.importer.FetcherException; import org.jabref.model.entry.BibEntry; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - public class LRUBibEntryRelationsRepository implements BibEntryRelationsRepository { - private static final Logger LOGGER = LoggerFactory - .getLogger(LRUBibEntryRelationsRepository.class); - private final CitationFetcher fetcher; private final LRUBibEntryRelationsCache cache; - public LRUBibEntryRelationsRepository(CitationFetcher fetcher, LRUBibEntryRelationsCache cache) { - this.fetcher = fetcher; + public LRUBibEntryRelationsRepository(LRUBibEntryRelationsCache cache) { this.cache = cache; } @Override - public List readCitations(BibEntry entry) { - if (needToRefreshCitations(entry)) { - forceRefreshCitations(entry); - } - - return cache.getCitations(entry); + public void insertCitations(BibEntry entry, List citations) { + cache.cacheOrMergeCitations( + entry, Objects.requireNonNullElseGet(citations, List::of) + ); } @Override - public List readReferences(BibEntry entry) { - if (needToRefreshReferences(entry)) { - List references; - try { - references = fetcher.searchCiting(entry); - } catch (FetcherException e) { - LOGGER.error("Error while fetching references", e); - references = List.of(); - } - cache.cacheOrMergeReferences(entry, references); - } - - return cache.getReferences(entry); + public List readCitations(BibEntry entry) { + return cache.getCitations(entry); } @Override - public void forceRefreshCitations(BibEntry entry) { - try { - List citations = fetcher.searchCitedBy(entry); - cache.cacheOrMergeCitations(entry, citations); - } catch (FetcherException e) { - LOGGER.error("Error while fetching citations", e); - } + public boolean containsCitations(BibEntry entry) { + return cache.citationsCached(entry); } - private boolean needToRefreshCitations(BibEntry entry) { - return !cache.citationsCached(entry); + @Override + public void insertReferences(BibEntry entry, List references) { + cache.cacheOrMergeReferences( + entry, Objects.requireNonNullElseGet(references, List::of) + ); } - private boolean needToRefreshReferences(BibEntry entry) { - return !cache.referencesCached(entry); + @Override + public List readReferences(BibEntry entry) { + return cache.getReferences(entry); } @Override - public void forceRefreshReferences(BibEntry entry) { - List references; - try { - references = fetcher.searchCiting(entry); - } catch (FetcherException e) { - LOGGER.error("Error while fetching references", e); - references = List.of(); - } - cache.cacheOrMergeReferences(entry, references); + public boolean containsReferences(BibEntry entry) { + return cache.referencesCached(entry); } } diff --git a/src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java b/src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java index 210821d708d7..8dfc6ac56595 100644 --- a/src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java +++ b/src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java @@ -1,36 +1,60 @@ package org.jabref.logic.citation.service; import java.util.List; + import org.jabref.logic.citation.repository.BibEntryRelationsRepository; +import org.jabref.logic.importer.fetcher.CitationFetcher; import org.jabref.model.entry.BibEntry; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class SearchCitationsRelationsService { - BibEntryRelationsRepository relationsRepository; + private static final Logger LOGGER = LoggerFactory + .getLogger(SearchCitationsRelationsService.class); - public SearchCitationsRelationsService(BibEntryRelationsRepository repository) { - this.relationsRepository = repository; - } + private final CitationFetcher citationFetcher; + private final BibEntryRelationsRepository relationsRepository; - public List searchReferences(BibEntry referencer) { - return this.relationsRepository.readReferences(referencer); + public SearchCitationsRelationsService( + CitationFetcher citationFetcher, BibEntryRelationsRepository repository + ) { + this.citationFetcher = citationFetcher; + this.relationsRepository = repository; } public List searchReferences(BibEntry referencer, boolean forceUpdate) { - if (forceUpdate) { - this.relationsRepository.forceRefreshReferences(referencer); + if (forceUpdate || !this.relationsRepository.containsReferences(referencer)) { + try { + var references = this.citationFetcher.searchCiting(referencer); + if (!references.isEmpty()) { + this.relationsRepository.insertReferences(referencer, references); + } + } catch (Exception e) { + var errMsg = "Error while fetching references for entry %s".formatted( + referencer.getTitle() + ); + LOGGER.error(errMsg); + } } - return this.searchReferences(referencer); - } - - public List searchCitations(BibEntry cited) { - return this.relationsRepository.readCitations(cited); + return this.relationsRepository.readReferences(referencer); } public List searchCitations(BibEntry cited, boolean forceUpdate) { - if (forceUpdate) { - this.relationsRepository.forceRefreshCitations(cited); + if (forceUpdate || !this.relationsRepository.containsCitations(cited)) { + try { + var citations = this.citationFetcher.searchCitedBy(cited); + if (!citations.isEmpty()) { + this.relationsRepository.insertCitations(cited, citations); + } + } catch (Exception e) { + var errMsg = "Error while fetching citations for entry %s".formatted( + cited.getTitle() + ); + LOGGER.error(errMsg); + } } - return this.searchCitations(cited); + return this.relationsRepository.readCitations(cited); } } diff --git a/src/main/java/org/jabref/logic/importer/fetcher/SemanticScholarCitationFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/SemanticScholarCitationFetcher.java index e55b188ff8c6..961b033e4733 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/SemanticScholarCitationFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/SemanticScholarCitationFetcher.java @@ -9,11 +9,11 @@ import org.jabref.logic.importer.ImporterPreferences; import org.jabref.logic.net.URLDownload; import org.jabref.logic.util.BuildInfo; +import org.jabref.model.citation.semanticscholar.CitationsResponse; +import org.jabref.model.citation.semanticscholar.ReferencesResponse; import org.jabref.model.entry.BibEntry; import com.google.gson.Gson; -import org.jabref.model.citation.semanticscholar.CitationsResponse; -import org.jabref.model.citation.semanticscholar.ReferencesResponse; public class SemanticScholarCitationFetcher implements CitationFetcher, CustomizableKeyFetcher { private static final String SEMANTIC_SCHOLAR_API = "https://api.semanticscholar.org/graph/v1/"; diff --git a/src/main/java/org/jabref/logic/util/BackgroundTask.java b/src/main/java/org/jabref/logic/util/BackgroundTask.java index 1a9054329453..11e2b4083e4f 100644 --- a/src/main/java/org/jabref/logic/util/BackgroundTask.java +++ b/src/main/java/org/jabref/logic/util/BackgroundTask.java @@ -172,6 +172,16 @@ public BackgroundTask onRunning(Runnable onRunning) { return this; } + /** + * Curry a consumer to on an on running runnable and invoke it after the task is started. + * + * @param onRunningConsumer should not be null + * @see BackgroundTask#consumeOnRunning(Consumer) + */ + public BackgroundTask consumeOnRunning(Consumer> onRunningConsumer) { + return this.onRunning(() -> onRunningConsumer.accept(this)); + } + /** * Sets the {@link Consumer} that is invoked after the task is successfully finished. * The consumer always runs on the JavaFX thread. diff --git a/src/test/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModelTest.java b/src/test/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModelTest.java index 48e074fd00c5..3d9c1b81129f 100644 --- a/src/test/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModelTest.java +++ b/src/test/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModelTest.java @@ -9,8 +9,6 @@ import org.jabref.gui.DialogService; import org.jabref.gui.StateManager; -import org.jabref.logic.importer.fetcher.CitationFetcher; -import org.jabref.gui.externalfiles.ImportHandler; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.logic.FilePreferences; import org.jabref.logic.bibtex.FieldPreferences; @@ -19,6 +17,7 @@ import org.jabref.logic.database.DuplicateCheck; import org.jabref.logic.importer.ImportFormatPreferences; import org.jabref.logic.importer.ImporterPreferences; +import org.jabref.logic.importer.fetcher.CitationFetcher; import org.jabref.logic.preferences.OwnerPreferences; import org.jabref.logic.preferences.TimestampPreferences; import org.jabref.logic.util.CurrentThreadTaskExecutor; diff --git a/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryHelpersForTest.java b/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryHelpersForTest.java new file mode 100644 index 000000000000..f2305b38b330 --- /dev/null +++ b/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryHelpersForTest.java @@ -0,0 +1,87 @@ +package org.jabref.logic.citation.repository; + +import java.util.List; +import java.util.Map; +import java.util.function.BiConsumer; +import java.util.function.Function; + +import org.jabref.model.entry.BibEntry; + +public class BibEntryRelationsRepositoryHelpersForTest { + public static class Mocks { + public static BibEntryRelationsRepository from( + Function> retrieveCitations, + BiConsumer> insertCitations, + Function> retrieveReferences, + BiConsumer> insertReferences + ) { + return new BibEntryRelationsRepository() { + @Override + public void insertCitations(BibEntry entry, List citations) { + insertCitations.accept(entry, citations); + } + + @Override + public List readCitations(BibEntry entry) { + return retrieveCitations.apply(entry); + } + + @Override + public boolean containsCitations(BibEntry entry) { + return true; + } + + @Override + public void insertReferences(BibEntry entry, List citations) { + insertReferences.accept(entry, citations); + } + + @Override + public List readReferences(BibEntry entry) { + return retrieveReferences.apply(entry); + } + + @Override + public boolean containsReferences(BibEntry entry) { + return true; + } + }; + } + + public static BibEntryRelationsRepository from( + Map> citationsDB, Map> referencesDB + ) { + return new BibEntryRelationsRepository() { + @Override + public void insertCitations(BibEntry entry, List citations) { + citationsDB.put(entry, citations); + } + + @Override + public List readCitations(BibEntry entry) { + return citationsDB.getOrDefault(entry, List.of()); + } + + @Override + public boolean containsCitations(BibEntry entry) { + return citationsDB.containsKey(entry); + } + + @Override + public void insertReferences(BibEntry entry, List citations) { + referencesDB.put(entry, citations); + } + + @Override + public List readReferences(BibEntry entry) { + return referencesDB.getOrDefault(entry, List.of()); + } + + @Override + public boolean containsReferences(BibEntry entry) { + return referencesDB.containsKey(entry); + } + }; + } + } +} diff --git a/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTest.java b/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTest.java deleted file mode 100644 index 0b436ac9187f..000000000000 --- a/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTest.java +++ /dev/null @@ -1,128 +0,0 @@ -package org.jabref.logic.citation.repository; - -import java.util.HashSet; -import java.util.List; - -import java.util.function.Function; -import java.util.stream.IntStream; -import java.util.stream.Stream; -import org.jabref.logic.importer.FetcherException; -import org.jabref.logic.importer.fetcher.CitationFetcher; -import org.jabref.model.entry.BibEntry; -import org.jabref.model.entry.field.StandardField; - -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.DisplayName; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.MethodSource; - -class BibEntryRelationsRepositoryTest { - - private static Stream createBibEntries() { - return IntStream - .range(0, 150) - .mapToObj(BibEntryRelationsRepositoryTest::createBibEntry); - } - - private static List getCitedBy(BibEntry entry) { - return List.of(BibEntryRelationsRepositoryTest.createCitingBibEntry(entry)); - } - - private static BibEntry createBibEntry(int i) { - return new BibEntry() - .withCitationKey("entry" + i) - .withField(StandardField.DOI, "10.1234/5678" + i); - } - - private static BibEntry createCitingBibEntry(Integer i) { - return new BibEntry() - .withCitationKey("citing_entry" + i) - .withField(StandardField.DOI, "10.2345/6789" + i); - } - - private static BibEntry createCitingBibEntry(BibEntry citedEntry) { - return createCitingBibEntry( - Integer.valueOf(citedEntry.getCitationKey().orElseThrow().substring(5)) - ); - } - - /** - * Simple mock to avoid using Mockito (reduce overall complexity) - */ - private record CitationFetcherMock( - Function> searchCiteByDelegate, - Function> searchCitingDelegate, - String name - ) implements CitationFetcher { - - @Override - public List searchCitedBy(BibEntry entry) throws FetcherException { - return this.searchCiteByDelegate.apply(entry); - } - - @Override - public List searchCiting(BibEntry entry) throws FetcherException { - return this.searchCitingDelegate.apply(entry); - } - - @Override - public String getName() { - return this.name; - } - } - - @ParameterizedTest - @MethodSource("createBibEntries") - @DisplayName( - "Given a new bib entry when reading citations for it should call the fetcher" - ) - void givenANewEntryWhenReadingCitationsForItShouldCallTheFetcher(BibEntry bibEntry) { - // GIVEN - var entryCaptor = new HashSet(); - var citationFetcherMock = new CitationFetcherMock( - entry -> { - entryCaptor.add(entry); - return BibEntryRelationsRepositoryTest.getCitedBy(entry); - }, - null, - null - ); - var bibEntryRelationsCache = new LRUBibEntryRelationsCache(); - var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( - citationFetcherMock, bibEntryRelationsCache - ); - - // WHEN - var citations = bibEntryRelationsRepository.readCitations(bibEntry); - - // THEN - Assertions.assertFalse(citations.isEmpty()); - Assertions.assertTrue(entryCaptor.contains(bibEntry)); - } - - @Test - @DisplayName( - "Given an empty cache for a valid entry when reading the citations should populate cache" - ) - void givenAnEmptyCacheAndAValidBibEntryWhenReadingCitationsShouldPopulateTheCache() { - // GIVEN - var citationFetcherMock = new CitationFetcherMock( - BibEntryRelationsRepositoryTest::getCitedBy, null, null - ); - var bibEntryRelationsCache = new LRUBibEntryRelationsCache(); - var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( - citationFetcherMock, bibEntryRelationsCache - ); - var bibEntry = BibEntryRelationsRepositoryTest.createBibEntry(1); - - // WHEN - Assertions.assertEquals(List.of(), bibEntryRelationsCache.getCitations(bibEntry)); - var citations = bibEntryRelationsRepository.readCitations(bibEntry); - var fromCacheCitations = bibEntryRelationsCache.getCitations(bibEntry); - - // THEN - Assertions.assertFalse(fromCacheCitations.isEmpty()); - Assertions.assertEquals(citations, fromCacheCitations); - } -} diff --git a/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTestHelpers.java b/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTestHelpers.java deleted file mode 100644 index 12b0a3929447..000000000000 --- a/src/test/java/org/jabref/logic/citation/repository/BibEntryRelationsRepositoryTestHelpers.java +++ /dev/null @@ -1,39 +0,0 @@ -package org.jabref.logic.citation.repository; - -import java.util.List; -import java.util.function.Consumer; -import java.util.function.Function; -import org.jabref.model.entry.BibEntry; - -public class BibEntryRelationsRepositoryTestHelpers { - public static class CreateRepository { - public static BibEntryRelationsRepository from( - Function> retrieveCitations, - Function> retrieveReferences, - Consumer forceRefreshCitations, - Consumer forceRefreshReferences - ) { - return new BibEntryRelationsRepository() { - @Override - public List readCitations(BibEntry entry) { - return retrieveCitations.apply(entry); - } - - @Override - public List readReferences(BibEntry entry) { - return retrieveReferences.apply(entry); - } - - @Override - public void forceRefreshCitations(BibEntry entry) { - forceRefreshCitations.accept(entry); - } - - @Override - public void forceRefreshReferences(BibEntry entry) { - forceRefreshReferences.accept(entry); - } - }; - } - } -} diff --git a/src/test/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepositoryTest.java b/src/test/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepositoryTest.java new file mode 100644 index 000000000000..5b926f9fdeee --- /dev/null +++ b/src/test/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepositoryTest.java @@ -0,0 +1,95 @@ +package org.jabref.logic.citation.repository; + +import java.util.List; +import java.util.random.RandomGenerator; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.StandardField; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +class LRUBibEntryRelationsRepositoryTest { + + private static Stream createBibEntries() { + return IntStream + .range(0, 150) + .mapToObj(LRUBibEntryRelationsRepositoryTest::createBibEntry); + } + + private static BibEntry createBibEntry(int i) { + return new BibEntry() + .withCitationKey(String.valueOf(i)) + .withField(StandardField.DOI, "10.1234/5678" + i); + } + + private static List createRelations(BibEntry entry) { + return entry + .getCitationKey() + .map(key -> RandomGenerator + .StreamableGenerator.of("L128X256MixRandom").ints(150) + .mapToObj(i -> new BibEntry() + .withCitationKey("%s relation %s".formatted(key, i)) + .withField(StandardField.DOI, "10.2345/6789" + i) + ) + ) + .orElseThrow() + .toList(); + } + + @ParameterizedTest + @MethodSource("createBibEntries") + void repositoryShouldMergeCitationsWhenInserting(BibEntry bibEntry) { + // GIVEN + var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( + new LRUBibEntryRelationsCache() + ); + Assertions.assertFalse(bibEntryRelationsRepository.containsCitations(bibEntry)); + + // WHEN + var firstRelations = createRelations(bibEntry); + var secondRelations = createRelations(bibEntry); + bibEntryRelationsRepository.insertCitations(bibEntry, firstRelations); + bibEntryRelationsRepository.insertCitations(bibEntry, secondRelations); + + // THEN + var uniqueRelations = Stream + .concat(firstRelations.stream(), secondRelations.stream()) + .distinct() + .toList(); + var relationFromCache = bibEntryRelationsRepository.readCitations(bibEntry); + Assertions.assertFalse(uniqueRelations.isEmpty()); + Assertions.assertNotSame(uniqueRelations, relationFromCache); + Assertions.assertEquals(uniqueRelations, relationFromCache); + } + + @ParameterizedTest + @MethodSource("createBibEntries") + void repositoryShouldMergeReferencesWhenInserting(BibEntry bibEntry) { + // GIVEN + var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( + new LRUBibEntryRelationsCache() + ); + Assertions.assertFalse(bibEntryRelationsRepository.containsReferences(bibEntry)); + + // WHEN + var firstRelations = createRelations(bibEntry); + var secondRelations = createRelations(bibEntry); + bibEntryRelationsRepository.insertReferences(bibEntry, firstRelations); + bibEntryRelationsRepository.insertReferences(bibEntry, secondRelations); + + // THEN + var uniqueRelations = Stream + .concat(firstRelations.stream(), secondRelations.stream()) + .distinct() + .collect(Collectors.toList()); + var relationFromCache = bibEntryRelationsRepository.readReferences(bibEntry); + Assertions.assertFalse(uniqueRelations.isEmpty()); + Assertions.assertNotSame(uniqueRelations, relationFromCache); + Assertions.assertEquals(uniqueRelations, relationFromCache); + } +} diff --git a/src/test/java/org/jabref/logic/citation/service/SearchCitationsRelationsServiceTest.java b/src/test/java/org/jabref/logic/citation/service/SearchCitationsRelationsServiceTest.java index 61473e8fd05a..78eb76778f93 100644 --- a/src/test/java/org/jabref/logic/citation/service/SearchCitationsRelationsServiceTest.java +++ b/src/test/java/org/jabref/logic/citation/service/SearchCitationsRelationsServiceTest.java @@ -1,89 +1,176 @@ package org.jabref.logic.citation.service; -import java.util.ArrayList; +import java.util.HashMap; import java.util.List; -import org.jabref.logic.citation.repository.BibEntryRelationsRepositoryTestHelpers; + +import org.jabref.logic.citation.repository.BibEntryRelationsRepositoryHelpersForTest; +import org.jabref.logic.importer.fetcher.CitationFetcherHelpersForTest; import org.jabref.model.entry.BibEntry; + import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; class SearchCitationsRelationsServiceTest { - @Test - void serviceShouldSearchForReferences() { - // GIVEN - var referencesToReturn = List.of(new BibEntry()); - var repository = BibEntryRelationsRepositoryTestHelpers.CreateRepository.from( - List::of, e -> referencesToReturn, e -> {}, e -> {} - ); - var searchCitationsRelationsService = new SearchCitationsRelationsService(repository); - - // WHEN - var referencer = new BibEntry(); - List references = searchCitationsRelationsService.searchReferences(referencer); - - // THEN - Assertions.assertEquals(referencesToReturn, references); - } + @Nested + class CitationsTests { + @Test + void serviceShouldSearchForCitations() { + // GIVEN + var cited = new BibEntry(); + var citationsToReturn = List.of(new BibEntry()); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + e -> citationsToReturn, null, null, null + ); + var searchService = new SearchCitationsRelationsService(null, repository); - @Test - void serviceShouldForceReferencesUpdate() { - // GiVEN - var newReference = new BibEntry(); - var referencesToReturn = List.of(newReference); - var referenceToUpdate = new ArrayList(); - var repository = BibEntryRelationsRepositoryTestHelpers.CreateRepository.from( - List::of, e -> referencesToReturn, e -> {}, e -> referenceToUpdate.add(newReference) - ); - var searchCitationsRelationsService = new SearchCitationsRelationsService(repository); - - // WHEN - var referencer = new BibEntry(); - var references = searchCitationsRelationsService.searchReferences(referencer, true); - - // THEN - Assertions.assertEquals(referencesToReturn, references); - Assertions.assertEquals(1, referenceToUpdate.size()); - Assertions.assertSame(newReference, referenceToUpdate.getFirst()); - Assertions.assertNotSame(referencesToReturn, referenceToUpdate); - } + // WHEN + List citations = searchService.searchCitations(cited, false); + + // THEN + Assertions.assertEquals(citationsToReturn, citations); + } + + @Test + void serviceShouldForceCitationsUpdate() { + // GiVEN + var cited = new BibEntry(); + var newCitations = new BibEntry(); + var citationsToReturn = List.of(newCitations); + var citationsDatabase = new HashMap>(); + var fetcher = CitationFetcherHelpersForTest.Mocks.from( + entry -> { + if (entry == cited) { + return citationsToReturn; + } + return List.of(); + }, + null + ); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + e -> citationsToReturn, + citationsDatabase::put, + List::of, + (e, r) -> { } + ); + var searchService = new SearchCitationsRelationsService(fetcher, repository); + + // WHEN + var citations = searchService.searchCitations(cited, true); + + // THEN + Assertions.assertTrue(citationsDatabase.containsKey(cited)); + Assertions.assertEquals(citationsToReturn, citationsDatabase.get(cited)); + Assertions.assertEquals(citationsToReturn, citations); + } + + @Test + void serviceShouldFetchCitationsIfRepositoryIsEmpty() { + var cited = new BibEntry(); + var newCitations = new BibEntry(); + var citationsToReturn = List.of(newCitations); + var citationsDatabase = new HashMap>(); + var fetcher = CitationFetcherHelpersForTest.Mocks.from( + entry -> { + if (entry == cited) { + return citationsToReturn; + } + return List.of(); + }, + null + ); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + citationsDatabase, null + ); + var searchService = new SearchCitationsRelationsService(fetcher, repository); - @Test - void serviceShouldSearchForCitations() { - // GIVEN - var citationsToReturn = List.of(new BibEntry()); - var repository = BibEntryRelationsRepositoryTestHelpers.CreateRepository.from( - e -> citationsToReturn, List::of, e -> {}, e -> {} - ); - var searchCitationsRelationsService = new SearchCitationsRelationsService(repository); - - // WHEN - var cited = new BibEntry(); - List citations = searchCitationsRelationsService.searchCitations(cited); - - // THEN - Assertions.assertEquals(citationsToReturn, citations); + // WHEN + var citations = searchService.searchCitations(cited, false); + + // THEN + Assertions.assertTrue(citationsDatabase.containsKey(cited)); + Assertions.assertEquals(citationsToReturn, citationsDatabase.get(cited)); + Assertions.assertEquals(citationsToReturn, citations); + } } - @Test - void serviceShouldForceCitationsUpdate() { - // GiVEN - var newCitations = new BibEntry(); - var citationsToReturn = List.of(newCitations); - var citationsToUpdate = new ArrayList(); - var repository = BibEntryRelationsRepositoryTestHelpers.CreateRepository.from( - e -> citationsToReturn, List::of, e -> citationsToUpdate.add(newCitations), e -> {} - ); - var searchCitationsRelationsService = new SearchCitationsRelationsService(repository); - - // WHEN - var cited = new BibEntry(); - var citations = searchCitationsRelationsService.searchCitations(cited, true); - - // THEN - Assertions.assertEquals(citationsToReturn, citations); - Assertions.assertEquals(1, citationsToUpdate.size()); - Assertions.assertSame(newCitations, citationsToUpdate.getFirst()); - Assertions.assertNotSame(citationsToReturn, citationsToUpdate); + @Nested + class ReferencesTests { + @Test + void serviceShouldSearchForReferences() { + // GIVEN + var referencer = new BibEntry(); + var referencesToReturn = List.of(new BibEntry()); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + null, null, e -> referencesToReturn, null + ); + var searchService = new SearchCitationsRelationsService(null, repository); + + // WHEN + List references = searchService.searchReferences(referencer, false); + + // THEN + Assertions.assertEquals(referencesToReturn, references); + } + + @Test + void serviceShouldCallTheFetcherForReferencesIWhenForceUpdateIsTrue() { + // GIVEN + var referencer = new BibEntry(); + var newReference = new BibEntry(); + var referencesToReturn = List.of(newReference); + var referencesDatabase = new HashMap>(); + var fetcher = CitationFetcherHelpersForTest.Mocks.from(null, entry -> { + if (entry == referencer) { + return referencesToReturn; + } + return List.of(); + }); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + List::of, + (e, c) -> { }, + e -> referencesToReturn, + referencesDatabase::put + ); + var searchService = new SearchCitationsRelationsService(fetcher, repository); + + // WHEN + var references = searchService.searchReferences(referencer, true); + + // THEN + Assertions.assertTrue(referencesDatabase.containsKey(referencer)); + Assertions.assertEquals(referencesToReturn, referencesDatabase.get(referencer)); + Assertions.assertEquals(referencesToReturn, references); + } + + @Test + void serviceShouldFetchReferencesIfRepositoryIsEmpty() { + var reference = new BibEntry(); + var newCitations = new BibEntry(); + var referencesToReturn = List.of(newCitations); + var referencesDatabase = new HashMap>(); + var fetcher = CitationFetcherHelpersForTest.Mocks.from( + null, + entry -> { + if (entry == reference) { + return referencesToReturn; + } + return List.of(); + } + ); + var repository = BibEntryRelationsRepositoryHelpersForTest.Mocks.from( + null, referencesDatabase + ); + var searchService = new SearchCitationsRelationsService(fetcher, repository); + + // WHEN + var references = searchService.searchReferences(reference, false); + + // THEN + Assertions.assertTrue(referencesDatabase.containsKey(reference)); + Assertions.assertEquals(referencesToReturn, referencesDatabase.get(reference)); + Assertions.assertEquals(referencesToReturn, references); + } } } diff --git a/src/test/java/org/jabref/logic/importer/fetcher/CitationFetcherHelpersForTest.java b/src/test/java/org/jabref/logic/importer/fetcher/CitationFetcherHelpersForTest.java new file mode 100644 index 000000000000..8a33679359ee --- /dev/null +++ b/src/test/java/org/jabref/logic/importer/fetcher/CitationFetcherHelpersForTest.java @@ -0,0 +1,32 @@ +package org.jabref.logic.importer.fetcher; + +import java.util.List; +import java.util.function.Function; + +import org.jabref.model.entry.BibEntry; + +public class CitationFetcherHelpersForTest { + public static class Mocks { + public static CitationFetcher from( + Function> retrieveCitedBy, + Function> retrieveCiting + ) { + return new CitationFetcher() { + @Override + public List searchCitedBy(BibEntry entry) { + return retrieveCitedBy.apply(entry); + } + + @Override + public List searchCiting(BibEntry entry) { + return retrieveCiting.apply(entry); + } + + @Override + public String getName() { + return "Test citation fetcher"; + } + }; + } + } +}