Fix merge dialog overwriting user's citation key choice during duplicate import#15406
Closed
0xRozier wants to merge 27 commits intoJabRef:mainfrom
Closed
Fix merge dialog overwriting user's citation key choice during duplicate import#154060xRozier wants to merge 27 commits intoJabRef:mainfrom
0xRozier wants to merge 27 commits intoJabRef:mainfrom
Conversation
Updated duplicate handling to include generated citation key when managing duplicates during import.
Refactor importCleanedEntries method to conditionally generate citation keys based on the skipKeyGeneration parameter.
Add method to set merged field value in DuplicateResolverDialog.
Add test to ensure merged citation key is preserved during import with duplicate check.
Contributor
|
Hey @0xRozier! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
Contributor
Review Summary by QodoFix merge dialog overwriting user's citation key during import
WalkthroughsDescription• Preserve user's citation key choice in merge dialog during duplicate import • Generate citation key before merge dialog for "From import" column display • Apply generated key only to "Merged entry" column as suggestion • Skip redundant key generation after merge resolution Diagramflowchart LR
A["Import Entry"] --> B["Generate Key String"]
B --> C["Find Duplicate"]
C --> D{Duplicate Found?}
D -->|Yes| E["Show Merge Dialog"]
E --> F["User Edits Key"]
F --> G["Apply User Choice"]
D -->|No| H["Apply Generated Key"]
G --> I["Import with skipKeyGeneration=true"]
H --> I
I --> J["Entry in Database"]
File Changes1. jabgui/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java
|
Contributor
Code Review by Qodo
1.
|
github-actions
Bot
added
good second issue
0xRozier
changed the title
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+388
to
+391
| public DuplicateDecisionResult getDuplicateDecision(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision, Optional<String> generatedKey) { | ||
| DuplicateResolverDialog dialog = new DuplicateResolverDialog(duplicateEntry, originalEntry, DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, stateManager, dialogService, preferences); | ||
| generatedKey.ifPresent(key -> dialog.setMergedFieldValue(InternalField.KEY_FIELD, key)); | ||
| if (decision == BREAK) { |
Contributor
There was a problem hiding this comment.
2. Merged key not displayed 🐞 Bug ✓ Correctness
The generated citation key is injected into the merge dialog via an internal field row that is filtered out of the three-way merge view, so the “Merged entry” column will not show the suggested key and the user cannot edit it there.
Agent Prompt
### Issue description
The merge dialog key suggestion is injected via `InternalField.KEY_FIELD`, but `ThreeWayMergeViewModel` removes internal fields from `visibleFields`, so the citation key row is never rendered. As a result, `setMergedFieldValue(InternalField.KEY_FIELD, key)` does nothing and the suggested/generated key is not shown in the “Merged entry” column.
### Issue Context
- `ImportHandler.getDuplicateDecision(..., generatedKey)` calls `dialog.setMergedFieldValue(InternalField.KEY_FIELD, key)`.
- `ThreeWayMergeViewModel.setVisibleFields(...)` removes internal fields (`FieldFactory::isInternalField`).
- Citation key is stored as `InternalField.KEY_FIELD` inside `BibEntry`.
### Fix Focus Areas
- Ensure the citation key field is visible in the three-way merge dialog for IMPORT_CHECK.
- Options:
- Special-case `InternalField.KEY_FIELD` to NOT be removed from visible fields in `ThreeWayMergeViewModel`.
- Or explicitly add `InternalField.KEY_FIELD` to `visibleFields` when constructing the model/view for merge dialogs.
- Or implement a dedicated UI row for citation key outside the internal field filtering.
- Add/adjust tests to cover the case where the merge dialog is populated with a generated key and that the merged entry returns the edited key.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[388-397]
- jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/ThreeWayMergeViewModel.java[102-112]
- jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/ThreeWayMergeView.java[207-214]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
github-actions
Bot
added
the
status: changes-required
This comment has been minimized.
This comment has been minimized.
Open
2 tasks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Removed duplicate entry for merge dialog citation key fix.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…nt formatter alignment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
Review Summary by QodoFix merge dialog overwriting user's citation key during duplicate import
WalkthroughsDescription• Preserve user's citation key choice in merge dialog during duplicate import • Generate citation key before merge dialog so user can edit it • Pass generated key to merge dialog for "Merged entry" column display • Skip automatic key generation after merge to preserve user's selection Diagramflowchart LR
A["Import Entry"] --> B["Generate Key String"]
B --> C["Find Duplicate"]
C --> D{"Duplicate Found?"}
D -->|Yes| E["Show Merge Dialog<br/>with Generated Key"]
D -->|No| F["Apply Generated Key"]
E --> G["User Edits Key"]
G --> H["Handle Duplicates<br/>with User's Key"]
H --> I["Import Entry<br/>Skip Key Generation"]
F --> I
File Changes1. jabgui/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java
|
Contributor
Code Review by QodoLooking for bugs?Check back in a few minutes. An AI review agent is analyzing this pull request.
|
Contributor
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
This comment has been minimized.
This comment has been minimized.
subhramit
reviewed
Mar 27, 2026
| new CurrentThreadTaskExecutor())); | ||
| // Mock the behavior of getDuplicateDecision to return KEEP_RIGHT | ||
| Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK); | ||
| Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(any(), any(), any(), any()); |
Author
There was a problem hiding this comment.
The 3-param getDuplicateDecision now delegates to the 4-param overload, so the mock needs to target the 4-param version to match the actual call chain. The test still calls handleDuplicates(3 params) which internally chains through to getDuplicateDecision(4 params).
Member
There was a problem hiding this comment.
You do understand that the question was not about number of parameters changing, but the existing parameters changing to any()?
Author
There was a problem hiding this comment.
Oh yes my bad, I switched to any() to avoid adding an eq() import
I reworked the three tests "handleDuplicates" to match their original form, and add the fourth parameter
This comment has been minimized.
This comment has been minimized.
Add a case : if imported key has already the same key as the existing key, the generated key is selected to avoid collision
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: e0b521e Learn more about TestLens at testlens.app. |
Contributor
Review Summary by QodoFix merge dialog overwriting user's citation key choice during duplicate import
WalkthroughsDescription• Preserve user's citation key choice in merge dialog during duplicate import • Generate citation key before merge dialog so user can edit it • Apply generated key only when no duplicate exists or user confirms merge • Add methods to set merged field values in dialog and merge view Diagramflowchart LR
A["Import Entry"] --> B["Generate Key String"]
B --> C["Find Duplicate"]
C --> D{Duplicate Found?}
D -->|No| E["Apply Generated Key"]
D -->|Yes| F["Show Merge Dialog"]
F --> G["User Edits Key"]
G --> H{Resolution Choice}
H -->|Keep Merge| I["Use User's Key"]
H -->|Keep Both| J["Apply Generated Key if Colliding"]
H -->|Keep Import| K["Use Original Key"]
E --> L["Import Entry"]
I --> L
J --> L
K --> L
File Changes1. jabgui/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java
|
Contributor
Code Review by Qodo
1. DuplicateDecisionResult constructed with null
|
Collaborator
|
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
Author
|
Hi @subhramit I reworked some points :
Hope it'll be better, I'm waiting for your feedback |
Comment on lines
+244
to
+245
| DuplicateDecisionResult decisionResult = new DuplicateDecisionResult( | ||
| DuplicateResolverDialog.DuplicateResolverResult.KEEP_BOTH, null); |
Contributor
There was a problem hiding this comment.
1. duplicatedecisionresult constructed with null 📘 Rule violation ≡ Correctness
The new test passes null for DuplicateDecisionResult's mergedEntry, which violates the no-null-passing rule and can lead to NPEs if the value is accessed. Use a real BibEntry instance (or refactor the type to model absence explicitly) instead of passing null.
Agent Prompt
## Issue description
A new test constructs `DuplicateDecisionResult` with a `null` `mergedEntry`, violating the project rule to not pass null unless explicitly intended and risking NPEs.
## Issue Context
`DuplicateDecisionResult` is a `record` with a `BibEntry mergedEntry` component (no `@Nullable`), so callers should not pass `null`.
## Fix Focus Areas
- jabgui/src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java[244-245]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+295
to
314
| // Generate citation key string WITHOUT modifying the entry, | ||
| // so the "From import" column shows the original key | ||
| Optional<String> generatedKey = generateKeyString(entryToInsert); | ||
|
|
||
| BibEntry finalEntry = entryToInsert; | ||
| if (existingDuplicateInLibrary.isPresent()) { | ||
| Optional<BibEntry> duplicateHandledEntry = handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision); | ||
| Optional<BibEntry> duplicateHandledEntry = handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision, generatedKey); | ||
| if (duplicateHandledEntry.isEmpty()) { | ||
| tracker.markSkipped(); | ||
| return; | ||
| } | ||
| finalEntry = duplicateHandledEntry.get(); | ||
| } else { | ||
| // No duplicate: apply the generated key directly | ||
| generatedKey.ifPresent(key -> entryToInsert.setCitationKey(key)); | ||
| } | ||
|
|
||
| importCleanedEntries(transferInformation, List.of(finalEntry)); | ||
| // Skip key generation since it was already handled | ||
| importCleanedEntries(transferInformation, List.of(finalEntry), true); | ||
|
|
Contributor
There was a problem hiding this comment.
2. Duplicate key from pregen 🐞 Bug ≡ Correctness
ImportHandler generates the citation key before inserting the entry and then skips generateKeys(), but CitationKeyGenerator.generateKey() can suppress suffixing when the entry already has the same key as the generated key. This can insert entries with citation keys that already exist in the database (no suffix added), creating duplicate citation keys after import.
Agent Prompt
### Issue description
`generateKeyString(entry)` calls `CitationKeyGenerator.generateKey(entry)` on an entry that may already have a citation key. `CitationKeyGenerator` uses that existing key (`oldKey`) to suppress suffixing, which is correct for *updating* existing entries but incorrect for *pre-insert* generation; combined with skipping `generateKeys(...)`, it can leave duplicate keys in the DB.
### Issue Context
Key generation is now done before insertion (to avoid overwriting the "From import" column) and post-insert key generation is skipped. That makes the pre-insert generated key authoritative and it must be collision-safe.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[284-316]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[432-446]
### Suggested fix
In `generateKeyString`, generate based on a clone with the citation key cleared so `oldKey` is `null`:
- `BibEntry tmp = new BibEntry(entry); tmp.clearCiteKey(); return Optional.of(keyGenerator.generateKey(tmp));`
Optionally, add a safety net: only pass `skipKeyGeneration=true` if `finalEntry` already has a citation key (or the user explicitly chose one), otherwise allow the normal `generateKeys(...)` path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues and pull requests
Closes #8644
PR Description
This PR fixes the merge dialog during duplicate import so it no longer overwrites the user's citation key choice. The generated key is now shown only in the "Merged entry" column, while the "From import" column keeps the original imported key.
Steps to test
.bibfile containing a duplicate of that entry with a different citation keyExpected behavior per resolution choice
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)repository
I used AI to help resolve code review feedback and to troubleshoot formatting.