Skip to content

FIX LOC Test and improve Mods Importer entry type parsing - #16279

Merged
Siedlerchr merged 8 commits into
mainfrom
loc
Jul 19, 2026
Merged

FIX LOC Test and improve Mods Importer entry type parsing#16279
Siedlerchr merged 8 commits into
mainfrom
loc

Conversation

@Siedlerchr

Copy link
Copy Markdown
Member

Related issues and pull requests

Closes #16055

PR Description

Fix LOC test and improve Mods entry type mapping, inspired by xml2bib util
Prevents stupid entry types like text

Add rate limiting at 10 request per minute

Steps to test

Import a mods book xml file

AI usage

GPT 5.4


AI CHECKLIST.md walkthrough

Checklist

  • I own the copyright of the code submitted and I license it under the MIT license
  • If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
  • I manually tested my changes in running JabRef (always required)
  • I added JUnit tests for changes (if applicable)
  • [.] I added screenshots in the PR description (if change is visible to the user)
  • [.] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
  • I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
  • [.] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

* upstream/main:
  Chore(deps): Bump com.autonomousapps:dependency-analysis-gradle-plugin (#16276)
  Chore(deps): Bump com.gradleup.shadow:shadow-gradle-plugin (#16275)
  Chore(deps): Bump com.gradleup.shadow:shadow-gradle-plugin (#16277)
  Chore(deps): Bump com.autonomousapps:dependency-analysis-gradle-plugin (#16274)
  Render AI summaries with MarkdownTextFlow instead of WebView (#16189)
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 17, 2026
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix LOC MODS import tests and harden MODS entry type inference

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Infer standard BibTeX entry types from MODS hints to avoid bogus types like "text".
• Correct host-title mapping (journal vs booktitle) for MODS relatedItem export/import.
• Rate-limit Library of Congress fetches and extend regression coverage with real MODS fixtures.
Diagram

graph TD
  U["User / Import"] --> F["LOC fetcher"] --> RL["Rate limiter"] --> LOC{{"LOC MODS endpoint"}} --> MI["ModsImporter"] --> TI["Entry type inference"] --> BE[("BibEntry")]
  BE --> ME["ModsExporter"] --> MX[("MODS XML")]
  subgraph Legend
    direction LR
    _cmp["Component"] ~~~ _ext{{"External"}} ~~~ _dat[("Data")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Table-driven MODS->EntryType mapping (data/config)
  • ➕ Easier to extend/adjust without touching parsing control flow
  • ➕ Can encode precedence rules explicitly and document them
  • ➕ Simplifies unit testing as pure mapping tests
  • ➖ Introduces another artifact to maintain and version
  • ➖ May be overkill if only a few heuristics are needed
2. Infer type only from explicit MODS genre authority values
  • ➕ More standards-aligned; reduces heuristic guessing
  • ➕ Less risk of misclassifying ambiguous records
  • ➖ Many real-world MODS records provide generic values (e.g., "text")
  • ➖ Would not fix the reported "text" entry type problem broadly

Recommendation: The current approach (collect type hints across main/host sections, then infer only known StandardEntryType values) is a good balance of robustness and practicality for real LOC MODS. If this logic grows further, consider extracting the inference rules into a small table-driven mapping to make precedence/coverage easier to evolve.

Files changed (12) +333 / -23

Enhancement (1) +53 / -0
LibraryOfCongress.javaAdd 10 req/min rate limiting and robust fetch/parse handling +53/-0

Add 10 req/min rate limiting and robust fetch/parse handling

• Implements performSearchById with explicit blank-checking, URL construction, and structured exception handling. Introduces a static Guava RateLimiter (10/min) and trace logging of wait time before issuing requests.

jablib/src/main/java/org/jabref/logic/importer/fetcher/LibraryOfCongress.java

Bug fix (2) +146 / -15
ModsExporter.javaExport host title as journal or booktitle depending on entry type +13/-3

Export host title as journal or booktitle depending on entry type

• Passes the entry type into relatedItem export and selects the correct host title field (BOOKTITLE for inbook/incollection/inproceedings, otherwise JOURNAL). This aligns exported MODS host metadata with the BibTeX type semantics.

jablib/src/main/java/org/jabref/logic/exporter/ModsExporter.java

ModsImporter.javaInfer StandardEntryType from MODS hints; avoid invalid types like "text" +133/-12

Infer StandardEntryType from MODS hints; avoid invalid types like "text"

• Collects type hints (main/host genres, typeOfResource, issuance, host title) during parsing and infers only known StandardEntryType values, including a monographic-text fallback to Book. Also improves format recognition regex and maps host relatedItem titles to JOURNAL vs BOOKTITLE based on inferred entry type.

jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java

Tests (8) +133 / -8
LibraryOfCongressTest.javaFix LOC expectations and add fixture-based parser regression test +30/-1

Fix LOC expectations and add fixture-based parser regression test

• Updates expected entries to reflect new type inference (Book) and removal of the previously asserted issuance field. Adds a new test that parses an attached LOC MODS XML fixture to validate stable field/type mapping.

jablib/src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java

ModsExportFormatTestAllFields.bibRemove issuance field from MODS export golden BibTeX +0/-1

Remove issuance field from MODS export golden BibTeX

• Updates the expected BibTeX fixture by removing the issuance field to match current importer/exporter behavior.

jablib/src/test/resources/org/jabref/logic/exporter/ModsExportFormatTestAllFields.bib

ModsExportFormatTestAllFields.xmlRemove issuance element from MODS export golden XML +0/-1

Remove issuance element from MODS export golden XML

• Updates the expected MODS XML fixture by removing the issuance element so the golden file matches exported output.

jablib/src/test/resources/org/jabref/logic/exporter/ModsExportFormatTestAllFields.xml

ModsExportFormatTestMultipleEntries.bibExpect booktitle (not journal) for InProceedings host title +1/-1

Expect booktitle (not journal) for InProceedings host title

• Adjusts the golden BibTeX fixture so inproceedings entries use booktitle, reflecting corrected host-title mapping logic.

jablib/src/test/resources/org/jabref/logic/exporter/ModsExportFormatTestMultipleEntries.bib

library_of_congress_2010045158_mods.xmlAdd real LOC MODS response fixture for regression testing +57/-0

Add real LOC MODS response fixture for regression testing

• Adds a MODS XML sample from the Library of Congress used to validate parsing and type inference (e.g., genre/typeOfResource = "text", issuance = "monographic").

jablib/src/test/resources/org/jabref/logic/importer/fetcher/library_of_congress_2010045158_mods.xml

MODSImporterTestModsCollection.bibUpdate MODS collection expected entry types and remove issuance +2/-4

Update MODS collection expected entry types and remove issuance

• Adjusts expected types (misc -> article) and removes the issuance field from the expected book entry to align with the new inference-based typing and issuance handling.

jablib/src/test/resources/org/jabref/logic/importer/fileformat/MODSImporterTestModsCollection.bib

MODSImporterTestMonographicBookFallback.bibAdd expected BibTeX output for monographic-text book fallback +10/-0

Add expected BibTeX output for monographic-text book fallback

• Introduces a golden BibTeX fixture asserting that MODS records with typeOfResource=text and issuance=monographic are imported as @book with the expected fields.

jablib/src/test/resources/org/jabref/logic/importer/fileformat/MODSImporterTestMonographicBookFallback.bib

MODSImporterTestMonographicBookFallback.xmlAdd MODS XML fixture exercising monographic-text type inference +33/-0

Add MODS XML fixture exercising monographic-text type inference

• Adds a compact MODS sample containing typeOfResource=text, genre=text, and issuance=monographic to drive the new book fallback inference path in tests.

jablib/src/test/resources/org/jabref/logic/importer/fileformat/MODSImporterTestMonographicBookFallback.xml

Documentation (1) +1 / -0
CHANGELOG.mdDocument improved MODS importer entry type mapping +1/-0

Document improved MODS importer entry type mapping

• Adds a changelog entry noting improved MODS importer entry-type mapping tied to issue #16055.

CHANGELOG.md

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (3) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. assertTrue(inputStream != null) used 📘 Rule violation ⚙ Maintainability
Description
The new test asserts non-null via assertTrue(inputStream != null) instead of using a dedicated
JUnit assertion. This reduces test clarity and violates the project’s test conventions preferring
specific assertions over broad boolean checks.
Code

jablib/src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java[R63-65]

+        try (InputStream inputStream = LibraryOfCongressTest.class.getResourceAsStream("library_of_congress_2010045158_mods.xml")) {
+            assertTrue(inputStream != null);
+
Evidence
PR Compliance ID 18 requires avoiding vague assertTrue/assertFalse when more specific assertions
are appropriate. The test currently uses assertTrue(inputStream != null) to check the resource
stream, where assertNotNull(inputStream) (optionally with a message) is the preferred convention.

AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions: AGENTS.md: JUnit/Test Conventions: Naming, No @DisplayName, Use @TempDir, Prefer Assertions on Contents, and Let JUnit Handle Exceptions
jablib/src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java[63-65]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A test uses `assertTrue(inputStream != null)` for a null check, which is discouraged in JabRef’s JUnit conventions.
## Issue Context
The compliance checklist prefers specific JUnit assertions over broad boolean assertions when asserting object state.
## Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java[63-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. inferEntryType uses isPresent() 📘 Rule violation ⚙ Maintainability
Description
New Optional-handling code uses isPresent() branching instead of idiomatic Optional combinators.
This adds verbosity and goes against the project preference for functional Optional APIs.
Code

jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[R544-557]

+    private Optional<EntryType> inferEntryType(TypeHints typeHints) {
+        Optional<EntryType> explicitMainType = typeHints.mainGenres.stream()
+                                                                   .map(this::mapGenre)
+                                                                   .map(this::parseKnownEntryType)
+                                                                   .flatMap(Optional::stream)
+                                                                   .findFirst();
+        if (explicitMainType.isPresent()) {
+            return explicitMainType;
+        }
+
+        Optional<EntryType> hostType = inferHostEntryType(typeHints);
+        if (hostType.isPresent()) {
+            return hostType;
+        }
Evidence
PR Compliance ID 8 requires using idiomatic Optional APIs such as map, ifPresent, orElseThrow,
etc., rather than manual isPresent() checks. The new implementation explicitly checks
explicitMainType.isPresent() and hostType.isPresent() to decide control flow.

AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage: AGENTS.md: Prefer Optional and Functional Optional APIs Over Null-like Optional Usage
jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[544-557]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`inferEntryType(...)` uses `Optional.isPresent()` + early returns instead of using Optional’s functional/combinator APIs.
## Issue Context
The compliance checklist asks to prefer idiomatic Optional usage (e.g., `or(...)`, `map(...)`, `orElseGet(...)`, `ifPresent(...)`) over null-like or manual presence branching.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[544-557]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. TypeHints encodes absence as null 📘 Rule violation ☼ Reliability
Description
The newly added TypeHints stores optional values as nullable fields (e.g., hostTitle) and relies
on null checks to represent absence. This introduces null-as-absence patterns contrary to the
nullability/Optional policy.
Code

jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[R596-618]

+    private void putRelatedItemTitle(Map<Field, String> fields, TypeHints typeHints, EntryType entryType) {
+        if (typeHints.hostTitle == null) {
+            return;
+        }
+
+        if (StandardEntryType.InBook.equals(entryType)
+                || StandardEntryType.InCollection.equals(entryType)
+                || StandardEntryType.InProceedings.equals(entryType)) {
+            putIfValueNotNull(fields, StandardField.BOOKTITLE, typeHints.hostTitle);
+        } else {
+            putIfValueNotNull(fields, StandardField.JOURNAL, typeHints.hostTitle);
+        }
+    }
+
+    private static final class TypeHints {
+        private final List<String> mainGenres = new ArrayList<>();
+        private final List<String> hostGenres = new ArrayList<>();
+        private @Nullable String mainResource;
+        private @Nullable String hostResource;
+        private @Nullable String mainIssuance;
+        private @Nullable String hostIssuance;
+        private @Nullable String hostTitle;
+    }
Evidence
PR Compliance IDs 9 and 33 require avoiding null-as-absence patterns and preferring explicit absence
representations. The new code checks typeHints.hostTitle == null and defines multiple @Nullable
fields inside TypeHints to represent optional state.

AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked: AGENTS.md: Nullability Policy: Avoid Returning/Passing null; Use JSpecify Annotations and @NullMarked
jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[596-618]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New helper state (`TypeHints`) uses `@Nullable` fields and `null` checks (e.g., `hostTitle == null`) to represent absence.
## Issue Context
The compliance checklist discourages null-as-absence and recommends expressing absence explicitly (e.g., via `Optional`) and using JSpecify consistently.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[596-608]
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[610-618]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. Unused hostResource hint ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
ModsImporter stores ` typeOfResource into TypeHints.hostResource`, but the inference code never
reads it, leaving dead state and making the intent of the new inference logic unclear. This
increases maintenance risk and suggests the host-resource-based inference path is
incomplete/mistakenly wired (current host inference consults mainResource instead).
Code

jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[R566-573]

+    private Optional<EntryType> inferHostEntryType(TypeHints typeHints) {
+        if (typeHints.hostGenres.stream().anyMatch("conference publication"::equalsIgnoreCase)) {
+            return Optional.of(StandardEntryType.InProceedings);
+        }
+
+        if (typeHints.hostGenres.stream().anyMatch("book"::equalsIgnoreCase)
+                || ("text".equalsIgnoreCase(typeHints.mainResource) && "monographic".equalsIgnoreCase(typeHints.hostIssuance))) {
+            return Optional.of(StandardEntryType.InBook);
Evidence
The code explicitly assigns typeHints.hostResource while parsing host typeOfResource, but the
only occurrence outside assignment is the field declaration; inference logic reads mainResource
instead, so hostResource cannot affect behavior.

jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[191-223]
jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[566-585]
jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[610-618]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TypeHints.hostResource` is populated when parsing MODS `relatedItem` (host) but is never used during type inference. This leaves dead state and makes the new type inference logic harder to reason about.
### Issue Context
- `hostResource` is assigned in `parseRelatedItem`.
- `inferHostEntryType` uses `typeHints.mainResource` (not `hostResource`) in the host-issuance fallback.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[191-223]
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[566-585]
- jablib/src/main/java/org/jabref/logic/importer/fileformat/ModsImporter.java[610-618]
### Suggested fix
Choose one:
1) **Wire it in**: derive an effective resource like `effectiveResource = Optional.ofNullable(mainResource).orElse(hostResource)` and use that consistently in inference checks.
2) **Remove it**: delete `hostResource` parsing + field if it’s not needed, to avoid misleading future changes.
Add/adjust a test if you decide to use it (e.g., a MODS fixture where `typeOfResource` is present only under the host relatedItem).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. Duplicated fetcher search logic ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
LibraryOfCongress overrides performSearchById by copy/pasting IdBasedParserFetcher’s default
implementation and inserting rate limiting, which can silently miss future fixes made to the shared
default logic. This increases long-term maintenance cost and regression risk for this fetcher.
Code

jablib/src/main/java/org/jabref/logic/importer/fetcher/LibraryOfCongress.java[R49-85]

+    @Override
+    public Optional<BibEntry> performSearchById(String identifier) throws FetcherException {
+        if (StringUtil.isBlank(identifier)) {
+            return Optional.empty();
+        }
+
+        URL urlForIdentifier;
+        try {
+            urlForIdentifier = getUrlForIdentifier(identifier);
+        } catch (URISyntaxException | MalformedURLException e) {
+            throw new FetcherException("Search URI is malformed", e);
+        }
+
+        double waitingTime = RATE_LIMITER.acquire();
+        LOGGER.trace("Thread {}, searching Library of Congress '{}', waited {} because of API rate limiter",
+                Thread.currentThread().threadId(), urlForIdentifier, waitingTime);
+
+        try (InputStream stream = getUrlDownload(urlForIdentifier).asInputStream()) {
+            List<BibEntry> fetchedEntries = getParser().parseEntries(stream);
+            if (fetchedEntries.isEmpty()) {
+                return Optional.empty();
+            }
+            if (fetchedEntries.size() > 1) {
+                LOGGER.info("Fetcher {} found more than one result for identifier {}. We will use the first entry.", getName(), identifier);
+            }
+            BibEntry entry = fetchedEntries.getFirst();
+            doPostCleanup(entry);
+            return Optional.of(entry);
+        } catch (IOException e) {
+            if (e.getCause() instanceof FetcherException fe) {
+                throw fe;
+            }
+            throw new FetcherException(urlForIdentifier, "A network error occurred", e);
+        } catch (ParseException e) {
+            throw new FetcherException(urlForIdentifier, "An internal parser error occurred", e);
+        }
+    }
Evidence
Both methods implement the same sequence (blank check, URL construction, download, parse, select
first entry, post-cleanup, wrap exceptions), meaning LoC has a forked copy of shared logic.

jablib/src/main/java/org/jabref/logic/importer/fetcher/LibraryOfCongress.java[49-90]
jablib/src/main/java/org/jabref/logic/importer/IdBasedParserFetcher.java[33-64]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LibraryOfCongress#performSearchById` duplicates the default implementation in `IdBasedParserFetcher`, differing primarily by the added rate-limiter call/logging. Duplicated logic risks drifting from future bugfixes or behavioral changes in the default implementation.
### Issue Context
The shared default method already handles blank IDs, URL building, download/parse, multi-result handling, and exception wrapping.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fetcher/LibraryOfCongress.java[49-85]
- jablib/src/main/java/org/jabref/logic/importer/IdBasedParserFetcher.java[34-64]
### Suggested fix
Refactor `LibraryOfCongress#performSearchById` to:
- Keep the blank check.
- Acquire the rate limiter.
- Delegate to the interface default method:
- `return IdBasedParserFetcher.super.performSearchById(identifier);`
If you still want URL-based trace logging, log `identifier` (or build URL just for logging in a small helper), but avoid re-implementing the full fetch/parse workflow.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +63 to +65
try (InputStream inputStream = LibraryOfCongressTest.class.getResourceAsStream("library_of_congress_2010045158_mods.xml")) {
assertTrue(inputStream != null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. asserttrue(inputstream != null) used 📘 Rule violation ⚙ Maintainability

The new test asserts non-null via assertTrue(inputStream != null) instead of using a dedicated
JUnit assertion. This reduces test clarity and violates the project’s test conventions preferring
specific assertions over broad boolean checks.
Agent Prompt
## Issue description
A test uses `assertTrue(inputStream != null)` for a null check, which is discouraged in JabRef’s JUnit conventions.

## Issue Context
The compliance checklist prefers specific JUnit assertions over broad boolean assertions when asserting object state.

## Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java[63-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@Siedlerchr
Siedlerchr enabled auto-merge July 19, 2026 13:21
@Siedlerchr
Siedlerchr disabled auto-merge July 19, 2026 13:21
@Siedlerchr
Siedlerchr merged commit f7ee847 into main Jul 19, 2026
66 checks passed
@Siedlerchr
Siedlerchr deleted the loc branch July 19, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix LibraryOfCongressTest

1 participant