Fix null-away issues for TexGroup

[InAnYan]

Related issues and pull requests

Closes nothing.

PR Description

Just fixing a null-away issue. Introduced new class, potentially very useful.

Steps to test

JabRef should behave normally.

AI usage

AI helped me in writing the LazyValue class (OpenAI ChatGPT). And we discussed various approaches.

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

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. LazyValue uses // block ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

LazyValue introduces a multi-line explanatory comment using // instead of the required Markdown
Javadoc /// style, reducing consistency with JabRef comment conventions.

Code

jablib/src/main/java/org/jabref/logic/util/LazyValue.java[R8-20]

+    // Implementation details of the class:
+    //
+    // There are 3 ways to make it:
+    // 1. A pair of `T value` and `boolean initialized`.
+    // 2. (Chosen option) use a sentinel value.
+    // 3. Use subclasses like `Empty` and `StoredValue<T>`.
+    //
+    // The problem boils down to null-safety and complexity.
+    // The first option is the simplest and efficient one, but it has 2 types of `null` values: one for uninitialized
+    // state, and the other is the return result of the factory method. However, `@Nullable T value` would mean that the
+    // value could *always* be `null`. Which is false because the supplier may always return a non-null value. As a result,
+    // options 2 and 3 should be used. However, option 3 is more complex, so option 2 was chosen, even if one has to use
+    // an unchecked cast.

Evidence

PR Compliance ID 11 requires new multi-line explanatory comments to use Markdown Javadoc ///. The
added comment block at LazyValue.java[8-20] is a multi-line explanation but uses // comment
style.

AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments: AGENTS.md: Use Markdown Javadoc triple-slash (///) for multi-line comments
jablib/src/main/java/org/jabref/logic/util/LazyValue.java[8-20]

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 new multi-line explanatory comment in `LazyValue` is written using `//` lines instead of the required `///` Markdown Javadoc style.
## Issue Context
The project policy requires `///` for new multi-line explanatory comments to keep Java documentation consistent.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/util/LazyValue.java[8-20]

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

---

2. LazyValue.toString() NPE risk ✓ Resolved 📘 Rule violation ☼ Reliability

Description

LazyValue documents that its computed value may be null, but toString() dereferences value
via value.toString() once initialized, which will throw a NullPointerException if the supplier
returned null, making runtime behavior inconsistent with the documented nullness intent. This can
also surface indirectly in logging/diagnostics (e.g., via TexGroup.toString() delegating into
LazyValue.toString()).

Code

jablib/src/main/java/org/jabref/logic/util/LazyValue.java[R27-56]

+    /// Either:
+    ///
+    /// - [#UNINITIALIZED] (not computed yet)
+    /// - Computed value (might be `null``)
+    private Object value = UNINITIALIZED;
+
+    public LazyValue(Supplier<T> supplier) {
+        this.supplier = supplier;
+    }
+
+    @SuppressWarnings("unchecked")
+    public T get() {
+        if (value == UNINITIALIZED) {
+            value = supplier.get();
+        }
+
+        return (T) value;
+    }
+
+    public void invalidate() {
+        value = UNINITIALIZED;
+    }
+
+    @Override
+    public String toString() {
+        if (value == UNINITIALIZED) {
+            return "null";
+        } else {
+            return value.toString();
+        }

Evidence

PR Compliance ID 31 requires consistency between documented nullability and runtime behavior:
LazyValue’s comment indicates the stored computed value might be null, and the implementation
assigns supplier.get() directly into value, so null is a valid computed state. However,
toString() assumes the initialized value is non-null and unconditionally calls
value.toString(), which will fail when the computed value is null; this failure path can be
reached indirectly because TexGroup.toString() includes keysUsedInAux, which delegates to
LazyValue.toString().

jablib/src/main/java/org/jabref/logic/util/LazyValue.java[27-56]
jablib/src/main/java/org/jabref/logic/util/LazyValue.java[27-44]
jablib/src/main/java/org/jabref/logic/util/LazyValue.java[50-57]
jablib/src/main/java/org/jabref/model/groups/TexGroup.java[109-117]
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
`LazyValue` allows its supplier to compute and store a `null` value, but `toString()` dereferences `value` (`value.toString()`) after initialization, which can throw `NullPointerException` when the supplier returns `null`. Align `toString()`’s runtime behavior with the documented nullness intent and make it safe for both uninitialized and computed-null states.
## Issue Context
PR Compliance ID 31 requires nullability to be consistent between documentation/comments, runtime behavior, and (when applicable) annotations. This issue may surface indirectly in logging/diagnostics because `TexGroup.toString()` includes `keysUsedInAux`, which delegates to `LazyValue.toString()`.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/util/LazyValue.java[27-57]
- jablib/src/main/java/org/jabref/model/groups/TexGroup.java[109-117]

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

---

3. Parses wrong aux path ✓ Resolved 🐞 Bug ≡ Correctness

Description

TexGroup expands and stores the resolved AUX path in this.filePath, but the LazyValue supplier still
parses the original constructor parameter filePath, so it may read a different (often
relative/non-existent) path than the one being monitored, producing incorrect group membership
results.

Code

jablib/src/main/java/org/jabref/model/groups/TexGroup.java[R44-48]

   this.filePath = expandPath(filePath);
   this.auxParser = auxParser;
   this.fileMonitor = fileMonitor;
+
+        this.keysUsedInAux = new LazyValue<>(() -> auxParser.parse(filePath).getUniqueKeys());

Evidence

TexGroup computes a resolved/expanded path into this.filePath but initializes the lazy parse using
the original filePath parameter; Aux parsing reads the passed Path directly from disk, so the
mismatch can lead to reading the wrong file or failing to open it.

jablib/src/main/java/org/jabref/model/groups/TexGroup.java[42-49]
jablib/src/main/java/org/jabref/model/groups/TexGroup.java[51-61]
jablib/src/main/java/org/jabref/model/groups/TexGroup.java[145-148]
jablib/src/main/java/org/jabref/logic/auxparser/DefaultAuxParser.java[45-69]

Agent prompt

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

## Issue description
`TexGroup` expands the input AUX path into `this.filePath`, but `keysUsedInAux`’s `LazyValue` supplier still calls `auxParser.parse(filePath)` using the *constructor parameter*, not the expanded field. This makes parsing inconsistent with the resolved path used elsewhere (including file monitoring), and can cause AUX parsing to silently fail or read the wrong file.
### Issue Context
- `DefaultAuxParser.parse(Path)` reads the `Path` directly with `Files.newBufferedReader(file)`, so passing an unresolved/relative path can fail.
- `TexGroup.create(...)` registers the file update listener for the resolved path returned by `getFilePathResolved()`.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/model/groups/TexGroup.java[42-49]
- jablib/src/main/java/org/jabref/model/groups/TexGroup.java[51-61]
- jablib/src/main/java/org/jabref/model/groups/TexGroup.java[145-148]
### Suggested change
Initialize the lazy supplier using the resolved field, e.g.

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

---

4. Null strings inserted 🐞 Bug ≡ Correctness

Description

AuxParserResult is now @NullMarked, so insertStrings(Collection) implicitly requires non-null
elements, but it is called with BibDatabase.getUsedStrings(...), which is built via Map::get and
can yield null values. If a null slips through, AuxParserResult.insertStrings will throw
NullPointerException when calling auxDatabase.addString(string).

Code

jablib/src/main/java/org/jabref/logic/auxparser/AuxParserResult.java[R12-15]

+import org.jspecify.annotations.NullMarked;
+
+@NullMarked
public class AuxParserResult {

Evidence

AuxParserResult is now null-marked, so its unannotated insertStrings parameter implies non-null
elements, but the upstream producer (BibDatabase.getUsedStrings) maps IDs through Map.get
without filtering, which can yield null. DefaultAuxParser wires these together, and
BibDatabase.addString will NPE if given a null string.

jablib/src/main/java/org/jabref/logic/auxparser/AuxParserResult.java[12-15]
jablib/src/main/java/org/jabref/logic/auxparser/AuxParserResult.java[65-70]
jablib/src/main/java/org/jabref/logic/auxparser/DefaultAuxParser.java[131-135]
jablib/src/main/java/org/jabref/model/database/BibDatabase.java[371-388]
jablib/src/main/java/org/jabref/model/database/BibDatabase.java[292-305]

Agent prompt

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

## Issue description
`AuxParserResult` is annotated with `@NullMarked`, making `insertStrings(Collection<BibtexString>)` treat the collection elements as non-null by default. However, callers pass `BibDatabase.getUsedStrings(...)`, which is computed using `bibtexStrings::get` and can produce nulls. This can crash AUX parsing with an NPE when `auxDatabase.addString(string)` executes.
### Issue Context
- `DefaultAuxParser.resolveTags` passes `masterDatabase.getUsedStrings(...)` into `AuxParserResult.insertStrings(...)`.
- `BibDatabase.getUsedStrings(...)` returns `allUsedIds.stream().map(bibtexStrings::get).toList()`, and `Map.get(...)` may return null.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/auxparser/AuxParserResult.java[65-70]
- jablib/src/main/java/org/jabref/model/database/BibDatabase.java[371-388]
- jablib/src/main/java/org/jabref/logic/auxparser/DefaultAuxParser.java[131-135]
### Suggested fix
Choose one of:
1) Make `BibDatabase.getUsedStrings(...)` guarantee non-null elements (e.g., filter nulls or map via `Objects.requireNonNull(...)` with a clear message), so it matches the `@NullMarked` contract.
2) If null elements are considered acceptable, update `AuxParserResult.insertStrings` to accept nullable elements (`Collection<@Nullable BibtexString>`) and skip (or log) nulls before calling `auxDatabase.addString(...)`.

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

---

5. Unsynchronized lazy invalidation 🐞 Bug ☼ Reliability

Description

LazyValue is not thread-safe (no volatile/synchronization), but TexGroup invalidates it from the
file-watcher background thread, so other threads may observe stale values and miss AUX updates or
recompute unpredictably.

Code

jablib/src/main/java/org/jabref/logic/util/LazyValue.java[R31-48]

+    private Object value = UNINITIALIZED;
+
+    public LazyValue(Supplier<T> supplier) {
+        this.supplier = supplier;
+    }
+
+    @SuppressWarnings("unchecked")
+    public T get() {
+        if (value == UNINITIALIZED) {
+            value = supplier.get();
+        }
+
+        return (T) value;
+    }
+
+    public void invalidate() {
+        value = UNINITIALIZED;
+    }

Evidence

The file update monitor is executed on a background thread pool and calls fileUpdated() directly;
TexGroup then invalidates the LazyValue. Since LazyValue has no synchronization/volatile, there is
no guaranteed happens-before relationship for readers on other threads.

jablib/src/main/java/org/jabref/logic/util/LazyValue.java[22-48]
jablib/src/main/java/org/jabref/model/groups/TexGroup.java[128-133]
jabgui/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java[39-84]
jabgui/src/main/java/org/jabref/gui/JabRefGUI.java[165-172]
jablib/src/main/java/org/jabref/logic/util/HeadlessExecutorService.java[35-47]
jablib/src/main/java/org/jabref/logic/util/HeadlessExecutorService.java[99-101]

Agent prompt

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

## Issue description
`LazyValue` uses a plain `Object value` with no `volatile` and no synchronization in `get()`/`invalidate()`. `TexGroup.fileUpdated()` calls `keysUsedInAux.invalidate()` from the file update monitor thread, which can race with reads/calls to `get()` from other threads, causing stale reads or lost invalidations.
### Issue Context
- `DefaultFileUpdateMonitor` runs on a background executor and calls listeners directly on that thread.
- `TexGroup` mutates `keysUsedInAux` in `fileUpdated()`.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/util/LazyValue.java[22-48]
- jablib/src/main/java/org/jabref/model/groups/TexGroup.java[128-133]
- jabgui/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java[39-84]
- jabgui/src/main/java/org/jabref/gui/JabRefGUI.java[165-172]
- jablib/src/main/java/org/jabref/logic/util/HeadlessExecutorService.java[35-47]
- jablib/src/main/java/org/jabref/logic/util/HeadlessExecutorService.java[99-101]
### Suggested change
Make `LazyValue` provide safe publication and consistent invalidation, e.g.:
- `private volatile Object value = UNINITIALIZED;`
- synchronize around compute/invalidate, or use an `AtomicReference<Object>` and CAS.
One common pattern:

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

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix NullAway nullability in TexGroup and AuxParserResult
🐞 Bug fix 🕐 10-20 Minutes

Description

• Mark affected classes as @NullMarked to enforce non-null defaults for NullAway.
• Annotate TexGroup’s lazy aux-key cache as @Nullable to reflect deferred initialization.
• Tighten method/parameter nullability (e.g., equals(@Nullable Object)) to match contracts.

Diagram

graph TD
  TG["TexGroup"] --> AP["AuxParser"] --> APR["AuxParserResult"]
  TG --> FUM["FileUpdateMonitor"]
  TG --> MD["MetaData"]
  TG --> JS{{"JSpecify annotations"}}
  APR --> JS

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Package-level @NullMarked (package-info.java)

• ➕ Applies a consistent non-null default across the whole package
• ➕ Reduces repetition of @NullMarked on each class
• ➖ Broader blast radius; may surface many new NullAway findings at once
• ➖ Harder to scope/roll back if other classes have inconsistent nullness contracts

2. Keep nullable fields non-null via eager initialization

• ➕ Avoids nullable state and reduces need for @nullable annotations
• ➕ Simplifies reasoning about invariants
• ➖ May introduce unnecessary computation/IO if the value is rarely needed
• ➖ Could change performance characteristics compared to the current lazy behavior

Recommendation: The current targeted, class-level @NullMarked plus a @nullable lazy-cache field is a good scoped fix for NullAway without expanding the annotation surface area. Consider package-level @NullMarked only if the team is ready to address nullness across the entire package in one effort.

Files changed (2) +10 / -8

Bug fix (2) +10 / -8

AuxParserResult.javaMark AuxParserResult as @NullMarked for non-null defaults +3/-0

Mark AuxParserResult as @NullMarked for non-null defaults

• Adds JSpecify @NullMarked to make non-null the default within AuxParserResult, reducing NullAway warnings and clarifying nullness contracts for fields/methods in this class.

jablib/src/main/java/org/jabref/logic/auxparser/AuxParserResult.java

TexGroup.javaAlign TexGroup nullability with lazy aux-key cache semantics +7/-8

Align TexGroup nullability with lazy aux-key cache semantics

• Marks TexGroup as @NullMarked, explicitly annotates the lazily computed keysUsedInAux cache as @Nullable, and updates equals to accept @Nullable Object. Removes prior @NonNull usage on the Path parameter since @NullMarked provides the non-null default.

jablib/src/main/java/org/jabref/model/groups/TexGroup.java

[jabref-machine]

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 - [x] (done), - [ ] (yet to be done) or - [/] (not applicable). Please adhere to our pull request template.

[koppor]

This is too much for us

• Larger would be to find this pattern somewhere in the Java eco system (Guava)
• Just use @Nullable for keysUsedInAux

DevCall opts for B to "reduce" cognitive load in comparison to A.

[InAnYan]

This is not AI. Something wrong with the style.. IDK, solving it

[InAnYan]

Fixed with a simple nullable.

(IMHO, I think that LazyValue has less cognitive load than a nullable. + easier to work with. I also think that I found this pattern several times in JabRef code base. But I don't have any proofs right now)

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit ac844c7