Skip to content

Commit 1f91370

Browse files
vladakvladak
authored
ensure out.write() is not called with null for path descriptions (#4341)
fixes #4340
1 parent d795a89 commit 1f91370

File tree

2 files changed

+98
-22
lines changed

2 files changed

+98
-22
lines changed

‎opengrok-web/src/main/java/org/opengrok/web/DirectoryListing.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ public void extraListTo(String contextPath, File dir, Writer out,
306306
printDateSize(out, child, entry.getDate(), dateFormatter);
307307
printNumlines(out, entry, isDir);
308308
printLoc(out, entry, isDir);
309-
if (entriesWithPathDescriptionsPresent) {
309+
if (entriesWithPathDescriptionsPresent && entry.getPathDescription() != null) {
310310
out.write("<td>");
311311
out.write(entry.getPathDescription());
312312
out.write("</td>");

‎opengrok-web/src/test/java/org/opengrok/web/DirectoryListingTest.java‎

Lines changed: 97 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,22 @@
3333
import java.util.Arrays;
3434
import java.util.Collections;
3535
import java.util.Date;
36+
import java.util.HashSet;
3637
import java.util.List;
38+
import java.util.Objects;
39+
import java.util.Set;
40+
import java.util.stream.Stream;
3741
import javax.xml.parsers.DocumentBuilder;
3842
import javax.xml.parsers.DocumentBuilderFactory;
3943

44+
import org.jetbrains.annotations.Nullable;
4045
import org.junit.jupiter.api.AfterEach;
4146
import org.junit.jupiter.api.BeforeEach;
4247
import org.junit.jupiter.api.Test;
4348
import org.junit.jupiter.params.ParameterizedTest;
44-
import org.junit.jupiter.params.provider.ValueSource;
49+
import org.junit.jupiter.params.provider.Arguments;
50+
import org.junit.jupiter.params.provider.MethodSource;
51+
import org.mockito.ArgumentMatchers;
4552
import org.opengrok.indexer.configuration.Filter;
4653
import org.opengrok.indexer.configuration.IgnoredNames;
4754
import org.opengrok.indexer.configuration.Project;
@@ -52,7 +59,9 @@
5259
import org.opengrok.indexer.index.Indexer;
5360
import org.opengrok.indexer.search.DirectoryEntry;
5461
import org.opengrok.indexer.util.TestRepository;
62+
import org.opengrok.indexer.web.EftarFile;
5563
import org.opengrok.indexer.web.EftarFileReader;
64+
import org.opengrok.indexer.web.PathDescription;
5665
import org.opengrok.indexer.web.Util;
5766
import org.w3c.dom.Document;
5867
import org.w3c.dom.Element;
@@ -65,6 +74,7 @@
6574
import static org.mockito.ArgumentMatchers.anyString;
6675
import static org.mockito.Mockito.atLeast;
6776
import static org.mockito.Mockito.mock;
77+
import static org.mockito.Mockito.never;
6878
import static org.mockito.Mockito.when;
6979
import static org.mockito.Mockito.spy;
7080
import static org.mockito.Mockito.verify;
@@ -116,17 +126,20 @@ static class FileEntry implements Comparable<FileEntry> {
116126
long size;
117127
String readableSize;
118128
List<FileEntry> subdirs;
129+
String pathDesc;
119130

120131
FileEntry() {
121132
}
122133

123-
private FileEntry(String name, String href, Long lastModified, long size, List<FileEntry> subdirs) {
134+
private FileEntry(String name, String href, Long lastModified, long size, List<FileEntry> subdirs,
135+
String pathDesc) {
124136
this.name = name;
125137
this.href = href;
126138
this.lastModified = lastModified;
127139
this.size = size;
128140
this.readableSize = Util.readableSize(size);
129141
this.subdirs = subdirs;
142+
this.pathDesc = pathDesc;
130143
}
131144

132145
/**
@@ -138,7 +151,7 @@ private FileEntry(String name, String href, Long lastModified, long size, List<F
138151
* @param subdirs list of sub entries (may be empty)
139152
*/
140153
FileEntry(String name, String href, long lastModified, List<FileEntry> subdirs) {
141-
this(name, href, lastModified, DIRECTORY_INTERNAL_SIZE, subdirs);
154+
this(name, href, lastModified, DIRECTORY_INTERNAL_SIZE, subdirs, null);
142155
assertNotNull(subdirs);
143156
}
144157

@@ -151,7 +164,7 @@ private FileEntry(String name, String href, Long lastModified, long size, List<F
151164
* @param size the desired size of the file on the disc
152165
*/
153166
FileEntry(String name, String href, long lastModified, int size) {
154-
this(name, href, lastModified, size, null);
167+
this(name, href, lastModified, size, null, null);
155168
}
156169

157170
@Override
@@ -166,6 +179,10 @@ public int compareTo(FileEntry fe) {
166179
return ret;
167180
}
168181

182+
if (!Objects.equals(pathDesc, fe.pathDesc)) {
183+
return -1;
184+
}
185+
169186
if ((ret = Long.compare(lastModified, fe.lastModified)) != 0) {
170187
return ret;
171188
}
@@ -323,20 +340,34 @@ private String getStringSize(Node item) throws NumberFormatException {
323340
return val.getNodeValue().trim();
324341
}
325342

343+
private @Nullable String getStringOrNull(Node item) throws NumberFormatException {
344+
Node val = item.getFirstChild();
345+
if (val == null) {
346+
return null;
347+
}
348+
assertEquals(Node.TEXT_NODE, val.getNodeType());
349+
return val.getNodeValue().trim();
350+
}
351+
326352
/**
327353
* Validate this file-entry in the table.
328354
*
329355
* @param element The &lt;tr&gt; element
330356
*/
331-
private void validateEntry(Element element) throws Exception {
357+
private void validateEntry(Element element, boolean usePathDescriptions) throws Exception {
332358
FileEntry entry = new FileEntry();
333359
NodeList nl = element.getElementsByTagName("td");
334360
int len = nl.getLength();
335361
// There should be 5 columns or fewer in the table.
336362
if (len < 5) {
337363
return;
338364
}
339-
assertEquals(7, len, "table <td> count");
365+
366+
if (usePathDescriptions) {
367+
assertTrue(len >= 7, "table <td> count");
368+
} else {
369+
assertEquals(7, len, "table <td> count");
370+
}
340371

341372
// item(0) is a decoration placeholder, i.e. no content
342373
entry.name = getFilename(nl.item(1));
@@ -346,6 +377,10 @@ private void validateEntry(Element element) throws Exception {
346377
if (entry.size == INVALID_SIZE) {
347378
entry.readableSize = getStringSize(nl.item(4));
348379
}
380+
// item(5) and item(6) are Lines# and LOC, respectively.
381+
if (len > 7) {
382+
entry.pathDesc = getStringOrNull(nl.item(7));
383+
}
349384

350385
// Try to look it up in the list of files.
351386
for (FileEntry e : entries) {
@@ -358,14 +393,23 @@ private void validateEntry(Element element) throws Exception {
358393
throw new AssertionError("Could not find a match for: " + entry.name);
359394
}
360395

396+
private static Stream<Arguments> provideArgumentsForTestDirectoryListing() {
397+
return Stream.of(
398+
Arguments.of(true, true),
399+
Arguments.of(true, false),
400+
Arguments.of(false, true),
401+
Arguments.of(false, false)
402+
);
403+
}
404+
361405
/**
362406
* Test directory listing.
363407
*
364408
* @throws java.lang.Exception if an error occurs while generating the list.
365409
*/
366410
@ParameterizedTest
367-
@ValueSource(booleans = {true, false})
368-
void testDirectoryListing(boolean useHistoryCache) throws Exception {
411+
@MethodSource("provideArgumentsForTestDirectoryListing")
412+
void testDirectoryListing(boolean useHistoryCache, boolean usePathDescriptions) throws Exception {
369413

370414
env.setUseHistoryCacheForDirectoryListing(useHistoryCache);
371415

@@ -382,10 +426,26 @@ void testDirectoryListing(boolean useHistoryCache) throws Exception {
382426
env, true, true,
383427
null, List.of(env.getPathRelativeToSourceRoot(directory)));
384428

385-
Document document = getDocumentWithDirectoryListing();
386-
387-
// Construct the expected directory entries.
388-
setEntries(useHistoryCache);
429+
Document document;
430+
if (usePathDescriptions) {
431+
File eftarFile = File.createTempFile("paths", ".eftar");
432+
Set<PathDescription> descriptions = new HashSet<>();
433+
descriptions.add(new PathDescription("/" + PROJECT_NAME + "/main.c",
434+
"Description for main.c"));
435+
EftarFile ef = new EftarFile();
436+
ef.create(descriptions, eftarFile.getAbsolutePath());
437+
try (EftarFileReader eftarFileReader = new EftarFileReader(eftarFile)) {
438+
document = getDocumentWithDirectoryListing(eftarFileReader);
439+
// Construct the expected directory entries.
440+
setEntries(useHistoryCache, eftarFileReader);
441+
// Make sure there are some entries with path description.
442+
assertTrue(entries.stream().anyMatch(e -> e.pathDesc != null));
443+
}
444+
} else {
445+
document = getDocumentWithDirectoryListing(null);
446+
// Construct the expected directory entries.
447+
setEntries(useHistoryCache, null);
448+
}
389449

390450
// Verify the values of directory entries.
391451
NodeList nl = document.getElementsByTagName("tr");
@@ -394,11 +454,11 @@ void testDirectoryListing(boolean useHistoryCache) throws Exception {
394454
assertEquals(entries.size() + 2, len);
395455
// Skip the header and parent link.
396456
for (int i = 2; i < len; ++i) {
397-
validateEntry((Element) nl.item(i));
457+
validateEntry((Element) nl.item(i), usePathDescriptions);
398458
}
399459
}
400460

401-
private void setEntries(boolean useHistoryCache) throws Exception {
461+
private void setEntries(boolean useHistoryCache, EftarFileReader eftarFileReader) throws Exception {
402462
File[] files = directory.listFiles();
403463
assertNotNull(files);
404464
entries = new ArrayList<>();
@@ -412,10 +472,18 @@ private void setEntries(boolean useHistoryCache) throws Exception {
412472
// See the comment about always creating history cache in the caller.
413473
assertTrue(historyGuru.hasHistoryCacheForFile(file));
414474

475+
String pathDesc = null;
476+
if (eftarFileReader != null) {
477+
EftarFileReader.FNode parentFNode = eftarFileReader.getNode("/" + PROJECT_NAME);
478+
if (parentFNode != null) {
479+
pathDesc = eftarFileReader.getChildTag(parentFNode, file.getName());
480+
}
481+
}
482+
415483
if (useHistoryCache) {
416484
if (file.isDirectory()) {
417485
entries.add(new FileEntry(file.getName(), file.getName() + "/",
418-
NO_DATE, DIRECTORY_INTERNAL_SIZE, null));
486+
NO_DATE, DIRECTORY_INTERNAL_SIZE, null, pathDesc));
419487
} else {
420488
HistoryEntry historyEntry = historyGuru.getLastHistoryEntry(file, true, false);
421489
assertNotNull(historyEntry);
@@ -424,7 +492,7 @@ private void setEntries(boolean useHistoryCache) throws Exception {
424492
String dateString = dateFormatter.format(historyEntry.getDate());
425493
long lastModTime = dateFormatter.parse(dateString).getTime();
426494
entries.add(new FileEntry(file.getName(), file.getName(),
427-
lastModTime, file.length(), null));
495+
lastModTime, file.length(), null, pathDesc));
428496
}
429497
} else {
430498
// The date string displayed in the UI has simple form so use the following
@@ -440,24 +508,32 @@ private void setEntries(boolean useHistoryCache) throws Exception {
440508

441509
if (file.isDirectory()) {
442510
entries.add(new FileEntry(file.getName(), file.getName() + "/",
443-
lastModTime, DIRECTORY_INTERNAL_SIZE, null));
511+
lastModTime, DIRECTORY_INTERNAL_SIZE, null, pathDesc));
444512
} else {
445513
entries.add(new FileEntry(file.getName(), file.getName(),
446-
lastModTime, file.length(), null));
514+
lastModTime, file.length(), null, pathDesc));
447515
}
448516
}
449517
}
450518
}
451519

452-
private Document getDocumentWithDirectoryListing() throws Exception {
453-
StringWriter out = new StringWriter();
520+
private Document getDocumentWithDirectoryListing(@Nullable EftarFileReader eftarFileReader) throws Exception {
521+
StringWriter outOrig = new StringWriter();
522+
StringWriter out = spy(outOrig);
454523
out.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<start>\n");
455524

456-
DirectoryListing instance = new DirectoryListing();
525+
DirectoryListing instance;
526+
if (eftarFileReader != null) {
527+
instance = new DirectoryListing(eftarFileReader);
528+
} else {
529+
instance = new DirectoryListing();
530+
}
457531
assertNotNull(directory.list());
458532
instance.listTo("ctx", directory, out, directory.getName(),
459533
Arrays.asList(directory.list()));
460534

535+
verify(out, never()).write((String) ArgumentMatchers.isNull());
536+
461537
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
462538
assertNotNull(factory, "DocumentBuilderFactory is null");
463539

0 commit comments

Comments
 (0)