Skip to content

Commit f02e284

Browse files
marevolclaude
marevol
and
claude
authored
Improve dictionary processing: fix critical bugs and enhance null-safety (#2958)
* Improve dictionary processing code quality and fix critical bugs This commit addresses multiple code quality issues and critical bugs in the dictionary processing implementation: **Critical Bug Fixes (P0):** - Fix PagingList.removeAll() logic error: method was calling retainAll() instead of removeAll() - Fix resource leaks in all 6 Updater constructors: FileOutputStream not properly closed on error - Fix CharMappingItem.equals(): remove state mutation (calling sort()) and add null safety **High Priority Improvements (P1):** - Add null-safe equals/hashCode in StopwordsItem and ProtwordsItem using Objects.equals() - Add defensive copying for exposed mutable arrays in CharMappingItem and SynonymItem getters **Files Modified:** - DictionaryFile.java: Fixed removeAll() bug - CharMappingFile.java, KuromojiFile.java, ProtwordsFile.java, StemmerOverrideFile.java, StopwordsFile.java, SynonymFile.java: Fixed resource leaks - CharMappingItem.java: Fixed equals(), added defensive copying - StopwordsItem.java, ProtwordsItem.java: Null-safe equals/hashCode - SynonymItem.java: Added defensive copying These improvements enhance robustness, prevent potential NPEs and resource leaks, and follow security best practices. * Fix and enhance dictionary Item tests for null-safe implementations Updated tests to align with null-safe equals/hashCode implementations: **Test Fixes:** - ProtwordsItemTest: Fixed test_hashCode_withNullInput and test_equals_withNullInput to expect null-safe behavior instead of NullPointerException - StopwordsItemTest: Added test_hashCode_withNullInput and test_equals_withNullInput for comprehensive null handling coverage - CharMappingItemTest: Added test_hashCode_withNullOutput and test_equals_withNullOutput for null output scenarios **Test Enhancements:** - SynonymItemTest: Added 5 defensive copy tests to verify immutability of returned arrays - CharMappingItemTest: Added 3 defensive copy tests for inputs and newInputs arrays **Files Modified:** - ProtwordsItemTest.java: Updated 2 null-related tests - StopwordsItemTest.java: Added 2 new null-safe tests - CharMappingItemTest.java: Added 5 new tests (2 null tests + 3 defensive copy tests) - SynonymItemTest.java: Added 5 defensive copy tests These tests ensure the null-safe implementations work correctly and verify that defensive copying prevents external modification of internal state. * Fix CharMappingItemTest to use assertArrayEquals for defensive copy Changed test_constructor_withIdZero to use assertArrayEquals instead of assertSame since getNewInputs() now returns a defensive copy (clone) of the array rather than the original array reference. This is the correct behavior for encapsulation and security, and the test should verify content equality, not reference equality. * Add missing assertArrayEquals import to CharMappingItemTest Added static import for org.junit.Assert.assertArrayEquals to fix compilation error when using assertArrayEquals method in test_constructor_withIdZero. --------- Co-authored-by: Claude <[email protected]>
1 parent d8622d7 commit f02e284

File tree

15 files changed

+322
-55
lines changed

15 files changed

+322
-55
lines changed

‎src/main/java/org/codelibs/fess/dict/DictionaryFile.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ public boolean addAll(final int index, final Collection<? extends E> c) {
256256

257257
@Override
258258
public boolean removeAll(final Collection<?> c) {
259-
return parent.retainAll(c);
259+
return parent.removeAll(c);
260260
}
261261

262262
@Override

‎src/main/java/org/codelibs/fess/dict/kuromoji/KuromojiFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,20 @@ protected class KuromojiUpdater implements Closeable {
252252
* @param newItem The new item to be added or updated.
253253
*/
254254
protected KuromojiUpdater(final KuromojiItem newItem) {
255+
FileOutputStream fos = null;
255256
try {
256257
newFile = ComponentUtil.getSystemHelper().createTempFile(KUROMOJI, ".txt");
257-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
258+
fos = new FileOutputStream(newFile);
259+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
260+
fos = null; // Successfully wrapped, no need to close explicitly
258261
} catch (final Exception e) {
262+
if (fos != null) {
263+
try {
264+
fos.close();
265+
} catch (final IOException ioe) {
266+
// Ignore close exception
267+
}
268+
}
259269
if (newFile != null) {
260270
newFile.delete();
261271
}

‎src/main/java/org/codelibs/fess/dict/mapping/CharMappingFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,10 +321,20 @@ protected class MappingUpdater implements Closeable {
321321
* @param newItem the character mapping item to update, or null for read-only operations
322322
*/
323323
protected MappingUpdater(final CharMappingItem newItem) {
324+
FileOutputStream fos = null;
324325
try {
325326
newFile = ComponentUtil.getSystemHelper().createTempFile(MAPPING, ".txt");
326-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
327+
fos = new FileOutputStream(newFile);
328+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
329+
fos = null; // Successfully wrapped, no need to close explicitly
327330
} catch (final Exception e) {
331+
if (fos != null) {
332+
try {
333+
fos.close();
334+
} catch (final IOException ioe) {
335+
// Ignore close exception
336+
}
337+
}
328338
if (newFile != null) {
329339
newFile.delete();
330340
}

‎src/main/java/org/codelibs/fess/dict/mapping/CharMappingItem.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,12 @@ public CharMappingItem(final long id, final String[] inputs, final String output
7777

7878
/**
7979
* Returns the array of new input character sequences for update operations.
80+
* Returns a defensive copy to prevent external modification.
8081
*
81-
* @return array of new input sequences, or null if no updates are pending
82+
* @return array of new input sequences (defensive copy), or null if no updates are pending
8283
*/
8384
public String[] getNewInputs() {
84-
return newInputs;
85+
return newInputs == null ? null : newInputs.clone();
8586
}
8687

8788
/**
@@ -114,11 +115,12 @@ public void setNewOutput(final String newOutput) {
114115

115116
/**
116117
* Returns the array of input character sequences that are mapped to the output.
118+
* Returns a defensive copy to prevent external modification.
117119
*
118-
* @return array of input sequences
120+
* @return array of input sequences (defensive copy)
119121
*/
120122
public String[] getInputs() {
121-
return inputs;
123+
return inputs == null ? null : inputs.clone();
122124
}
123125

124126
/**
@@ -188,6 +190,7 @@ public int hashCode() {
188190
/**
189191
* Compares this CharMappingItem with another object for equality.
190192
* Two CharMappingItem objects are equal if they have the same inputs and output.
193+
* Note: inputs arrays are sorted in the constructor, so no sorting is needed here.
191194
*
192195
* @param obj the object to compare with
193196
* @return true if the objects are equal, false otherwise
@@ -201,12 +204,10 @@ public boolean equals(final Object obj) {
201204
return false;
202205
}
203206
final CharMappingItem other = (CharMappingItem) obj;
204-
sort();
205-
other.sort();
206-
if (!Arrays.equals(inputs, other.inputs) || !output.equals(other.getOutput())) {
207+
if (!Arrays.equals(inputs, other.inputs)) {
207208
return false;
208209
}
209-
return true;
210+
return Objects.equals(output, other.output);
210211
}
211212

212213
/**

‎src/main/java/org/codelibs/fess/dict/protwords/ProtwordsFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,10 +247,20 @@ protected class ProtwordsUpdater implements Closeable {
247247
* @param newItem the item to be updated
248248
*/
249249
protected ProtwordsUpdater(final ProtwordsItem newItem) {
250+
FileOutputStream fos = null;
250251
try {
251252
newFile = ComponentUtil.getSystemHelper().createTempFile(PROTWORDS, ".txt");
252-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
253+
fos = new FileOutputStream(newFile);
254+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
255+
fos = null; // Successfully wrapped, no need to close explicitly
253256
} catch (final Exception e) {
257+
if (fos != null) {
258+
try {
259+
fos.close();
260+
} catch (final IOException ioe) {
261+
// Ignore close exception
262+
}
263+
}
254264
if (newFile != null) {
255265
newFile.delete();
256266
}

‎src/main/java/org/codelibs/fess/dict/protwords/ProtwordsItem.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,7 @@ public boolean isDeleted() {
9696

9797
@Override
9898
public int hashCode() {
99-
final int prime = 31;
100-
final int result = 1;
101-
return prime * result + input.hashCode();
99+
return java.util.Objects.hashCode(input);
102100
}
103101

104102
@Override
@@ -110,10 +108,7 @@ public boolean equals(final Object obj) {
110108
return false;
111109
}
112110
final ProtwordsItem other = (ProtwordsItem) obj;
113-
if (!input.equals(other.input)) {
114-
return false;
115-
}
116-
return true;
111+
return java.util.Objects.equals(input, other.input);
117112
}
118113

119114
@Override

‎src/main/java/org/codelibs/fess/dict/stemmeroverride/StemmerOverrideFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,10 +276,20 @@ protected class StemmerOverrideUpdater implements Closeable {
276276
* @throws DictionaryException if the temporary file cannot be created.
277277
*/
278278
protected StemmerOverrideUpdater(final StemmerOverrideItem newItem) {
279+
FileOutputStream fos = null;
279280
try {
280281
newFile = ComponentUtil.getSystemHelper().createTempFile(STEMMER_OVERRIDE, ".txt");
281-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
282+
fos = new FileOutputStream(newFile);
283+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
284+
fos = null; // Successfully wrapped, no need to close explicitly
282285
} catch (final Exception e) {
286+
if (fos != null) {
287+
try {
288+
fos.close();
289+
} catch (final IOException ioe) {
290+
// Ignore close exception
291+
}
292+
}
283293
if (newFile != null) {
284294
newFile.delete();
285295
}

‎src/main/java/org/codelibs/fess/dict/stopwords/StopwordsFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,10 +263,20 @@ protected class StopwordsUpdater implements Closeable {
263263
* @throws DictionaryException if the temporary file cannot be created.
264264
*/
265265
protected StopwordsUpdater(final StopwordsItem newItem) {
266+
FileOutputStream fos = null;
266267
try {
267268
newFile = ComponentUtil.getSystemHelper().createTempFile(STOPWORDS, ".txt");
268-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
269+
fos = new FileOutputStream(newFile);
270+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
271+
fos = null; // Successfully wrapped, no need to close explicitly
269272
} catch (final Exception e) {
273+
if (fos != null) {
274+
try {
275+
fos.close();
276+
} catch (final IOException ioe) {
277+
// Ignore close exception
278+
}
279+
}
270280
if (newFile != null) {
271281
newFile.delete();
272282
}

‎src/main/java/org/codelibs/fess/dict/stopwords/StopwordsItem.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,7 @@ public boolean isDeleted() {
105105

106106
@Override
107107
public int hashCode() {
108-
final int prime = 31;
109-
final int result = 1;
110-
return prime * result + input.hashCode();
108+
return java.util.Objects.hashCode(input);
111109
}
112110

113111
@Override
@@ -119,10 +117,7 @@ public boolean equals(final Object obj) {
119117
return false;
120118
}
121119
final StopwordsItem other = (StopwordsItem) obj;
122-
if (!input.equals(other.input)) {
123-
return false;
124-
}
125-
return true;
120+
return java.util.Objects.equals(input, other.input);
126121
}
127122

128123
@Override

‎src/main/java/org/codelibs/fess/dict/synonym/SynonymFile.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,10 +340,20 @@ protected class SynonymUpdater implements Closeable {
340340
* @throws DictionaryException if the temporary file cannot be created.
341341
*/
342342
protected SynonymUpdater(final SynonymItem newItem) {
343+
FileOutputStream fos = null;
343344
try {
344345
newFile = ComponentUtil.getSystemHelper().createTempFile(SYNONYM, ".txt");
345-
writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(newFile), Constants.UTF_8));
346+
fos = new FileOutputStream(newFile);
347+
writer = new BufferedWriter(new OutputStreamWriter(fos, Constants.UTF_8));
348+
fos = null; // Successfully wrapped, no need to close explicitly
346349
} catch (final Exception e) {
350+
if (fos != null) {
351+
try {
352+
fos.close();
353+
} catch (final IOException ioe) {
354+
// Ignore close exception
355+
}
356+
}
347357
if (newFile != null) {
348358
newFile.delete();
349359
}

0 commit comments

Comments
 (0)