Fix PDF relativization test #12621

InAnYan · 2025-03-04T12:57:11Z

Fixes failing test: PdfMergeMetadataImporterTest#importRelativizesFilePath.

Fixes #12620

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
[/] Manually tested changed features in running JabRef (always required)
[/] Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

koppor · 2025-03-04T13:17:12Z

Blocked by #12619

PR Overview

This PR fixes a failing test for PDF metadata file path relativization. The changes include:

Modifying the test to use a manually instantiated FilePreferences rather than a mocked one.
Adding an overloaded importDatabase method in PdfMergeMetadataImporter that relativizes the file path.
Introducing a corresponding overloaded importDatabase method in PdfImporter to support the new file path handling.

Reviewed Changes

File	Description
src/test/java/org/jabref/logic/importer/fileformat/pdf/PdfMergeMetadataImporterTest.java	Updates test to use manual file preferences instantiation for correct working directory setup
src/main/java/org/jabref/logic/importer/fileformat/PdfMergeMetadataImporter.java	Introduces a new method that relativizes the file path before database import
src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfImporter.java	Adds an overloaded method accepting FilePreferences to resolve the file path correctly

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

koppor · 2025-03-04T13:42:24Z

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfImporter.java

@@ -61,6 +62,18 @@ public ParserResult importDatabase(Path filePath) {
        }
    }

+    public ParserResult importDatabase(Path filePath, FilePreferences filePreferences) {


Not sure why this is necessary. Shouldn't the caller take care about the absolute path? - Since this method is a) a duplicate of public ParserResult importDatabase(Path filePath) and b) used only once, I propose to move Path readPath = filePreferences.getWorkingDirectory().resolve(filePath); to the caller.

Yes... the problem is:

XmpUtilReader can't read relativized path. This was the reason why I (mistakenly) removed relativization.

So in this method I leave relativized path in BibEntry, but when I'm reading I use the full path.

Yeah, something's wrong here

src/test/java/org/jabref/logic/importer/fileformat/pdf/PdfMergeMetadataImporterTest.java

koppor

Ah, let's go this through. Better than nothing. :)

I have a small suggestion, which would make the logic more clear.

Maybe, also the JavaDoc should be copied into PdfImporter.

koppor · 2025-03-17T10:36:23Z

src/main/java/org/jabref/logic/importer/fileformat/PdfMergeMetadataImporter.java

+        List<Path> directories = context.getFileDirectories(filePreferences);
+
+        filePath = FileUtil.relativize(filePath, directories);


This should be moved down to ´importDatabase`, then I understand it more...

It is in importDatabase?

…ix/relativization

InAnYan · 2025-03-17T20:18:27Z

I should think about it more

koppor · 2025-03-17T23:08:32Z

I should think about it more

The quick fix is OK for now, isn't it?

src/main/java/org/jabref/logic/importer/fileformat/PdfMergeMetadataImporter.java

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfContentImporter.java

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfEmbeddedBibFileImporter.java

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfImporter.java

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfXmpImporter.java

trag-bot · 2025-03-20T09:50:29Z

src/main/java/org/jabref/logic/externalfiles/ExternalFilesContentImporter.java

-        } catch (IOException e) {
-           return ParserResult.fromError(e);
-        }
+        return new PdfMergeMetadataImporter(importFormatPreferences).importDatabase(file, context, filePreferences);


The try-catch block removal shifts exception handling responsibility to the caller without documenting this change in method signature. Method should declare throws IOException.

IntelliJ said that it is not thrown at all

src/main/java/org/jabref/logic/importer/fileformat/PdfMergeMetadataImporter.java

trag-bot · 2025-03-20T09:53:46Z

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfGrobidImporter.java

@@ -25,8 +25,8 @@ public PdfGrobidImporter(ImportFormatPreferences importFormatPreferences) {
        this.importFormatPreferences = importFormatPreferences;
    }

-    public List<BibEntry> importDatabase(Path filePath, PDDocument document) throws IOException, ParseException {
-        return grobidService.processPDF(filePath, importFormatPreferences);
+    public List<BibEntry> importDatabase(Path fullPath, PDDocument document) throws IOException, ParseException {


The method accepts PDDocument parameter but doesn't use it in the implementation, violating the Single Responsibility Principle by having unused parameters.

Yes, it just uses grobid.

Is there any way to mark this argument as explicitly unused?

Why can't it be removed - I don't see any @Override annotation.

Oh! It should be overriden

Then you can add JavaDoc and use @param and then say that it is ignored ^^

github-actions · 2025-03-20T09:53:49Z

If you made changes that are visible to the user, please add a brief description along with the issue number to the CHANGELOG.md file. More details can be found in our Developer Documentation about the changelog.

trag-bot · 2025-03-20T09:55:41Z

src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfXmpImporter.java

@@ -22,8 +22,8 @@ public PdfXmpImporter(XmpPreferences xmpPreferences) {
        this.xmpPreferences = xmpPreferences;
    }

-    public List<BibEntry> importDatabase(Path filePath, PDDocument document) throws IOException {
-        return new XmpUtilReader().readXmp(filePath, xmpPreferences);
+    public List<BibEntry> importDatabase(Path fullPath, PDDocument document) throws IOException {


Method accepts PDDocument parameter but doesn't use it in the implementation, violating Single Responsibility Principle by having unused parameters.

Yes, this is expected.

Is there any way to mark this argument as explicitly unused?

InAnYan added 2 commits March 4, 2025 13:45

Add old method

d5d1e6e

Fix test case

56a0c5f

Merge branch 'main' into fix/relativization

21cf853

koppor requested a review from Copilot March 4, 2025 13:42

Copilot AI reviewed Mar 4, 2025

View reviewed changes

koppor requested changes Mar 4, 2025

View reviewed changes

InAnYan and others added 2 commits March 12, 2025 11:36

Fix test case

a8a3817

Merge branch 'main' into fix/relativization

46bc11f

koppor requested changes Mar 17, 2025

View reviewed changes

InAnYan added 2 commits March 17, 2025 17:45

Merge remote-tracking branch 'upstream/main' into fix/relativization

9e68955

Merge branch 'fix/relativization' of github.com:InAnYan/jabref into f…

d8d1654

…ix/relativization

InAnYan marked this pull request as draft March 17, 2025 20:18

Fix the logic

3c5bd14