2. No tests for index update 📘 Rule violation ☼ Reliability

Core model behavior was changed by updating citationIndex during FieldChangedEvent handling, but
this PR does not include corresponding test additions/updates to prevent regressions. This violates
the requirement to adapt/update tests when modifying org.jabref.model behavior.

## Issue description
A behavior change in `BibDatabase` (updating `citationIndex` on `FieldChangedEvent`) is not covered by tests in this PR.

## Issue Context
This code lives under `org.jabref.model` and affects crossref/link behavior; a regression here can reintroduce stale/orphan link indexing.

## Fix Focus Areas
- jablib/src/main/java/org/jabref/model/database/BibDatabase.java[548-586]

There is no "before" or "after" in a codebase. A person reading will only know the current state.

This is a telltale sign of AI usage. Seems like you did not read our contributing guidelines at all.

here for after i meant after the checks i made.

There is also "just like before" at the end of the sentence.

this part is written by me as i had to restyle to match the styling convention of jabref and then again write tests for the Junit to get accepted in the qodo ai review

This discussion is in context of the highlighted comment above, not code style conventions or unit tests.

Your thought process was - it would be unclear to reviewers that the functionality of an untouched part of code is still the same unless you add a comment stating that the functionality of an untouched part of code is unchanged?

You've talked about JabRef style/conventions above. If you had even read this class once yourself, or at least the surrounding code, you'd have realized that nobody writes obvious comments like "delete from old key set" or "add to new key set" or refer to history of code. That, however takes time.
We are really not new to this experience, we've dealt with an uncountable number of pull requests now (and the senior maintainers here even more), and some patterns which you're unaware of, are persistent every time someone uses AI. They may not be obvious to you and it might feel really easy to claim they're intentionally made thinking we won't understand, but that's not really the case. We can see through them.

I've made mistakes like you (which are not even considered mistakes for newcomers), and also use AI during my day to day. Only thing that matters is the honesty and consistency of the human being involved.

I do not wish to continue this discussion further, so I leave it on other maintainers

Since you at least tested the changes - we will not reject your contribution and give you another chance. Please be careful henceforth.

I am being honest @subhramit . I am really serious about this contribution. I went through the other parts of the code in the same class as well. I went through a considerable part of the codebase during the debugging process. I have said in my previous comment that i have taken help of ai assistant to guide me. I mentioned that in my previous gitter chats as well. I have no reason to lie as it would harm my own efforts. I am being honest, that I wrote that part of code myself including that comment line. I used AI to assist me. But the entire code is written by me. I have not copy pasted a single line in that method. I can share my logic report as well as to how i formulated the logic. I humbly appeal to you, please check my code again, I will be happy to explain my thought process for any logic part. I apologise if i overstepped at any point. I hope you will consider my request. Thank you for guiding me, I have learnt a lot from this first contribution and wish to improve further.

It's fine. You deserve a benefit of doubt, and you shall be given that.
I have reviewed your code in the next comments, considering your contribution honest.

Please remove this comment from the code.

Thank you @subhramit for your consideration. I will try to explain your questions in comments with as much clarity as i can.
Thank you for your guidance. I will remove that comment from the code and commit the changes. If i need to make any other changes, please mention that, i would try to fix them as well.

How can oldvalue ever be null?

When a child (ex - InProceedings) is added as entry for first time, there is no crossref field attached. This field is not there for the child. So this will be null. When the user types the crossref and saves it, then it is saved with some value. As the field was empty before, the old value can be null. Now since the citationIndex in BibDatabase.java is initialised as a final ConcurrentHashMap and ConcurrentHashMap does not support null values, there can be a null pointer exception. Hence I added this check. if there is no crossref for a new entry, then it will be seen as empty string to prevent any null pointer exception error.

I understand that you are talking about getOldValue(). I inquired about oldValue.
You define oldValue as

String oldValue = event.getOldValue() == null ? "" : event.getOldValue();

above.
Hence, it can be "" but never null.
Hence your logic regarding ConcurrentHashMap doesn't follow, nor this check if (oldValue != null) because it will always be non-null.

Yes the if statement is redundant part. When i first wrote the code i did the check with

if(oldVaue!=null&&!oldValue.isEmpty())
{
}

But then while checking i realised that i need the empty strings for checking too otherwise the new entry will not be linked. So i removed the isEmpty part and added a ternary operator above. I forgot to remove the redundant if parts for this and the newvalue. I will rectify them the same time when i remove the comment you suggested.

How can newvalue ever be null?

The new value can be null if suddenly a user deletes a crossref field for the child. If there is only 1 crossref entry for a child, and user deletes it, then that field will be removed from the BibEntry object of this entry. Then the new value becomes null. Now as i mentioned in my previous old value comment, the citationIndex in Bibdatabase.java is initialised with a final ConcurrentHashMap. and ConcurrentHashMap does not support null keys. So trying to put a null key will cause null pointer exception. Hence i made the check to keep an empty string for the null value to avoid that error.

Similar response as above - what does this check signify?
Did you read your code before submitting?

I mentioned my response in the above comment about my initial code. This is a screenshot of my earlier code. I have kept the entire process in a word document. i can share it if you want

See @programmer1128 - it doesn't matter how many preliminary steps you went through before arriving at a final state and submitting a PR as ready for review. The bottom line is that you did not check your final code before submitting - hence we are spending time on this.
We understand - you're a human and this can happen, but as far as the PR review is concerned, the maintainers really don't have time for these endless excuses or knowing the entire history of your process. Please just say "oh I missed this from a previous draft" and just fix it.

I apologise for my mistake @subhramit . I will make all the necessary changes that you have suggested, recheck my code and then commit it. Thank you for your thorough review and time to check my code and guide me.

No issues, Aritra, as blunt my tone may sound, I am keeping in mind that this is your first time - and trying to make you pick up as many things as possible.

I really appreciate it @subhramit . I have learnt a lot from this and I am still learning and will continue to do so. It is a really good experience for me. Thank you for taking the time to mentor me.

Why a concurrent hashmap?

I have used a concurrent hashmap here firstly because the citationIndex field itself is initialised as a final ConcurrentHashMap in the BibDatabase.java file.

To ensure that the key insertion, update and deletion is thread safe, I followed the same ConcurrentHashMap as done in the class previously.

Also, as I understood, jabref uses javafx for ui. So there is JavaFx ui thread plus many other task threads. Now the change i made is inside a method that is relayEntryChangeEvent(FieldChangedEvent event). This method is called, whenever an event is posted to the eventbus and these events can come from many threads. Now if we use a normal HashMap a thread that posted event to change citationIndex can interfere with the JavaFx thread reading it to show in interface which can cause a ConcurrentModificationException error in java.
So if we use concurrent hashmap then the operations can happen safely with all the threads without the ui thread being interfered.

You are correct about javafx and background threads posting to the eventbus, and relayEntryChangeEvent can run off the fx thread while the UI reads the index, so thread-safety does matter here.

However, ConcurrentHashMap doesn't make compound operations atomic. Consider your L562 onwards:

Set<BibEntry> referenceEntries = citationIndex.get(oldValue);
if (referenceEntries != null) {
    referenceEntries.remove(entry);
    if (referenceEntries.isEmpty()) {
        citationIndex.remove(oldValue);
    }
}

Between isEmpty() and remove(oldValue), another thread can computeIfAbsent(oldValue...) and repopulate the set, the remove would then delete a non-empty mapping. However as far as I see, a similar pattern already exists in the code which you probably faithfully followed (in removeEntryFromIndex), so for the scope of this PR you can ignore this - and the situation is unlikely to occur during JabRef's practical use.
Just thought of putting this here as the topic of concurrency was raised, so that you can also take this opportunity learn about this more deeply.

Thank you @subhramit for your guidance. I think this is similar to time of check to time of use condition. I may be wrong. I will keep your advice in mind . Thank you for pointing it out and reviewing my PR thouroughly.

MULTIPLE_ENTRY_LINK fields can hold multiple keys in one value, so citationIndex.get(oldValue) misses when oldValue is something like "Key1, Key2".

BibDatabase.java already has forEachCitationKey(), which parses link fields with entry.getEntryLinkList(field, this) and is reused by indexEntry and removeEntryFromIndex, consider using it here too so all link parsing stays in one place

Hi @LoayTarek5, Thank you for reviewing my code. i though initially that crossref multiple entry fields were stored individually by default in separate strings all containing same child. Thank you for pointing out my mistake. I will try to implemet the logic you suggested using the forEachCitationKey() method and correct my logic and test it with a testcase similar to "key1,key2.." for multiple entry fields.

The existing crossrefChanged test in KeyChangeListenerTest.java sets the crossref before insertEntry, so it never triggers the stale index, the real bug when crossref is set after insertion

Please add a test that:
inserts parent and child separately, then sets child.crossref after insertion.
renames the parent key --> verifies the child's crossref follows
renames the parent key a second time --> verifies it still follows

Thank you @LoayTarek5 for this new test case. I will test it with my new correct logic with a new test that will implement the 2 time rename of parent. I will try to record the results and post it here.

Use StringUtil isBlank()

Use isBlank

isBlank

isBlank

When a change is suggested, please don't just blindly replace as per your gut feeling. Go into the function isBlank, see what it does. If it does the same as isEmpty(), a critical thinker would think - then why the need for replacement?
If you go into the function body, you'll find that the null check is already being done there - so the review comments were intended to reduce extra code.

Also, you did not use StringUtil as mentioned in #15582 (comment)

I apologise @subhramit , i misunderstood your suggested change. I thought you meant the built in isBlank() method for string class in java and since the built in method does not check the null, i kept the null check.
For the isEmpty() part I thought you have suggested the isBlank() because .trim() creates a new String object in java and to avoid that.
I apologise for my mistakes. I will fix them immediately and commit the corrected code.

Same comments on the other related parts.

Thank you @subhramit for providing the code segment. It is a great help. I will fix the code immediately to use this method and then commit the corrected code.
If there is any more change that i need to make, please mention them, i will fix them and commit the proper code.

No, that is all. Thanks.

Trim is also no longer needed. See what isBlank does by Ctrl+click.

Hi @subhramit , I saw what the method does. By using !StringUtil.isBlank(rawKey) i can avoid the new string creation that trim does even for empty invalid strings.
I just want to clarify, that when the string is not empty and contains characters with leading spaces or spaces at end, then i have to do trim so that the citationIndex can properly match the keys. For that reason, i think i should move the trim part to inside the if statement that trims the rawKey only when it is not empty and checks in citationIndex. Can you please clarify my approach.

You can keep the trim where you are using the trimmed version beyond the checks

Thanks a lot @subhramit for clarifying my approach. I will make the fixes immediately and commit the proper code with the changes you suggested.
Thank you for your guidance.