Fixes of some nullaway warnings#15792
Conversation
Review Summary by QodoResolve NullAway warnings with nullable annotations and Optional refactoring
WalkthroughsDescription• Add @Nullable annotations to resolve NullAway warnings across multiple files • Refactor getFieldValue() to return Optional instead of nullable String • Change getString() and getEntryById() methods to return Optional types • Add @Contract annotations to StringUtil methods for null-safety contracts • Remove constructor from ParsedEntryLink and simplify nullable field handling • Update test assertions to work with Optional return types Diagramflowchart LR
A["NullAway Warnings"] -->|Add @Nullable annotations| B["Exception, Comparator, AuthorList"]
A -->|Refactor to Optional| C["getString, getEntryById, getFieldValue"]
A -->|Add @Contract annotations| D["StringUtil methods"]
A -->|Simplify nullable fields| E["ParsedEntryLink, GroupTreeNode"]
B --> F["Null-safety compliance"]
C --> F
D --> F
E --> F
File Changes1. jablib/src/main/java/org/jabref/logic/JabRefException.java
|
Code Review by Qodo
1. ParsedEntryLink stores null state
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| private final BibDatabase database; | ||
|
|
||
| private String key; | ||
| private Optional<BibEntry> linkedEntry; | ||
| private BibDatabase database; | ||
| @Nullable private BibEntry linkedEntry; | ||
|
|
||
| public ParsedEntryLink(String key, BibDatabase database) { | ||
| this.key = key; | ||
| this.linkedEntry = database.getEntryByCitationKey(this.key); | ||
| this.linkedEntry = database.getEntryByCitationKey(this.key).orElse(null); | ||
| this.database = database; | ||
| } | ||
|
|
||
| public ParsedEntryLink(BibEntry bibEntry) { | ||
| this.key = bibEntry.getCitationKey().orElse(""); | ||
| this.linkedEntry = Optional.of(bibEntry); | ||
| } | ||
|
|
||
| public String getKey() { | ||
| return key; | ||
| } | ||
|
|
||
| public Optional<BibEntry> getLinkedEntry() { | ||
| return linkedEntry; | ||
| return Optional.ofNullable(linkedEntry); | ||
| } | ||
|
|
||
| public void setKey(String newKey) { | ||
| this.key = newKey; | ||
| this.linkedEntry = getDatabase().getEntryByCitationKey(this.key); | ||
| this.linkedEntry = getDatabase().getEntryByCitationKey(this.key).orElse(null); | ||
| } |
There was a problem hiding this comment.
1. parsedentrylink stores null state 📘 Rule violation ☼ Reliability
The change replaces an Optional<BibEntry> field with a @Nullable BibEntry field and assigns it via orElse(null), reintroducing null state where Optional handling was already present. This increases NPE risk and violates the Optional-idiom guidance.
Agent Prompt
## Issue description
`ParsedEntryLink` was changed to store `linkedEntry` as a nullable field and uses `orElse(null)`, which reintroduces nulls instead of using Optional idioms.
## Issue Context
The compliance checklist requires preferring Optional operations over converting Optionals to nullable values.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/model/entry/ParsedEntryLink.java[12-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
No, according to big-heads it’s wrong to store optional as fields, and it should be used as a return type only
| sb.append(pair.getKey()); | ||
|
|
||
| sb.append(" ".repeat(padding)); | ||
| sb.repeat(" ", padding); |
There was a problem hiding this comment.
2. Invalid stringbuilder.repeat call 🐞 Bug ≡ Correctness
StringUtil.alignStringTable now calls sb.repeat(" ", padding) where sb is a java.lang.StringBuilder;
there is no StringBuilder method overload that accepts (String,int), so this change will not
compile. This breaks any codepath that builds the aligned string table (and likely fails the build).
Agent Prompt
### Issue description
`alignStringTable` constructs a `StringBuilder sb = new StringBuilder()` and then calls `sb.repeat(" ", padding)`. `StringBuilder` does not provide a `repeat(String, int)` method, so this is a compile-time error.
### Issue Context
This line replaced a working padding implementation (`sb.append(" ".repeat(padding))`).
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/util/strings/StringUtil.java[719-736]
### Suggested fix
Replace `sb.repeat(" ", padding);` with one of:
- `sb.append(" ".repeat(padding));` (closest to original)
- or a small loop appending spaces if preferred.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
I don't understand why architecture test is failing. I used only jspecify nullable. I can't use jetbrains |
1 participant
Related issues and pull requests
Not related to any issue - a continuous process.
PR Description
Just fixing nullaway warnings. The simplest ones.
Ocasionally some code changes - using modern streams, or better string builder usage.
Steps to test
Ideally - run tests for each of the affected files. In reality, just run JabRef unit tests and see that the check is green.
AI usage
I used AI (Google Gemini, ChatGPT, Claude) to consult and dive more into Nullaway and null issues. None of the AI tools were used to generate or edit code, and all my changes are manual.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)