Skip to content

Commit

Permalink
prevent null date in Subversion history entry (#4564)
Browse files Browse the repository at this point in the history 
- propagate stream processing error as -1 exit code
- fix various style issues

fixes #3111
  • Loading branch information
@vladak
vladak committed Mar 23, 2024
1 parent ad96584 commit 261ac2c
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 78 deletions.
41 changes: 24 additions & 17 deletions opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionHistoryParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

/*
* Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2006, 2024, Oracle and/or its affiliates. All rights reserved.
* Portions Copyright (c) 2017, 2020, Chris Fraire <cfraire@me.com>.
* Portions Copyright (c) 2020, 2023, Ric Harris <harrisric@users.noreply.github.com>.
*/
Expand All @@ -37,6 +37,8 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -45,6 +47,7 @@
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;

import org.jetbrains.annotations.VisibleForTesting;
import org.opengrok.indexer.configuration.CommandTimeoutType;
import org.opengrok.indexer.configuration.RuntimeEnvironment;
import org.opengrok.indexer.logger.LoggerFactory;
Expand Down Expand Up @@ -111,12 +114,12 @@ public void startElement(String uri, String localName, String qname, Attributes
entry.setActive(true);
entry.setRevision(attr.getValue("revision"));
} else if ("path".equals(qname) && attr.getIndex(COPYFROM_PATH) != -1) {
LOGGER.log(Level.FINER, "rename for {0}", attr.getValue(COPYFROM_PATH));
if ("dir".equals(attr.getValue("kind"))) {
isRenamedDir = true;
} else {
isRenamedFile = true;
}
LOGGER.log(Level.FINER, "rename for {0}", attr.getValue(COPYFROM_PATH));
if ("dir".equals(attr.getValue("kind"))) {
isRenamedDir = true;
} else {
isRenamedFile = true;
}
}
sb.setLength(0);
}
Expand All @@ -131,8 +134,8 @@ public void endElement(String uri, String localName, String qname) throws SAXExc
// need to strip microseconds off - assume final character is Z otherwise invalid anyway.
String dateString = s;
if (s.length() > SVN_MILLIS_DATE_LENGTH) {
dateString = dateString.substring(0, SVN_MILLIS_DATE_LENGTH - 1) +
dateString.charAt(dateString.length() - 1);
dateString = dateString.substring(0, SVN_MILLIS_DATE_LENGTH - 1) +
dateString.charAt(dateString.length() - 1);
}
entry.setDate(repository.parse(dateString));
} catch (ParseException ex) {
Expand All @@ -153,7 +156,7 @@ public void endElement(String uri, String localName, String qname) throws SAXExc
renamedFiles.add(path.intern());
}
if (isRenamedDir) {
renamedToDirectoryRevisions.put(path.intern(), entry.getRevision());
renamedToDirectoryRevisions.put(path.intern(), entry.getRevision());
}
} else {
LOGGER.log(Level.FINER, "Skipping file ''{0}'' outside repository ''{1}''",
Expand All @@ -163,6 +166,11 @@ public void endElement(String uri, String localName, String qname) throws SAXExc
entry.setMessage(s);
}
if ("logentry".equals(qname)) {
// Avoid adding incomplete history entries.
if (Objects.isNull(entry.getDate())) {
throw new SAXException(String.format("date is null in history entry for revision %s",
Optional.ofNullable(entry.getRevision()).orElse("<unknown>")));
}
entries.add(entry);
}
sb.setLength(0);
Expand Down Expand Up @@ -209,17 +217,15 @@ History parse(File file, SubversionRepository repos, String sinceRevision,

Executor executor;
try {
executor = repos.getHistoryLogExecutor(file, sinceRevision,
numEntries, cmdType);
executor = repos.getHistoryLogExecutor(file, sinceRevision, numEntries, cmdType);
} catch (IOException e) {
throw new HistoryException("Failed to get history for: \"" +
file.getAbsolutePath() + "\"", e);
throw new HistoryException(String.format("Failed to get history for '%s'", file.getAbsolutePath()), e);
}

int status = executor.exec(true, this);
if (status != 0) {
throw new HistoryException("Failed to get history for: \"" +
file.getAbsolutePath() + "\" Exit code: " + status);
throw new HistoryException(String.format("Failed to get history for '%s': Exit code: %d",
file.getAbsolutePath(), status));
}

List<HistoryEntry> entries = handler.entries;
Expand Down Expand Up @@ -266,12 +272,13 @@ public void processStream(InputStream input) throws IOException {
}

/**
* Parse the given string.
* Parse the given string. Only used in tests.
*
* @param buffer The string to be parsed
* @return The parsed history
* @throws IOException if we fail to parse the buffer
*/
@VisibleForTesting
History parse(String buffer) throws IOException {
handler = new Handler("/", "", 0, new SubversionRepository());
processStream(new ByteArrayInputStream(buffer.getBytes(StandardCharsets.UTF_8)));
Expand Down
78 changes: 33 additions & 45 deletions opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;
import java.util.logging.Level;
Expand All @@ -44,6 +45,7 @@
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;

import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;
import org.opengrok.indexer.configuration.CommandTimeoutType;
import org.opengrok.indexer.configuration.RuntimeEnvironment;
Expand All @@ -57,10 +59,10 @@

/**
* Access to a Subversion repository.
*
* <p>
* <b>TODO</b> The current implementation does <b>not</b> support nested
* repositories as described in http://svnbook.red-bean.com/en/1.0/ch07s03.html
*
* </p>
* @author Trond Norbye
*/
public class SubversionRepository extends Repository {
Expand Down Expand Up @@ -120,11 +122,10 @@ private String getValue(Node node) {
}

/**
* Get {@code Document} corresponding to the parsed XML output from
* {@code svn info} command.
* @return document with data from {@code info} or null if the {@code svn}
* command failed
* Get {@code Document} corresponding to the parsed XML output from {@code svn info} command.
* @return document with data from {@code info} or null if the {@code svn} command failed
*/
@Nullable
private Document getInfoDocument() {
Document document = null;
List<String> cmd = new ArrayList<>();
Expand All @@ -146,26 +147,21 @@ private Document getInfoDocument() {
DocumentBuilder builder = factory.newDocumentBuilder();
document = builder.parse(executor.getOutputStream());
} catch (SAXException saxe) {
LOGGER.log(Level.WARNING,
"Parser error parsing svn output", saxe);
LOGGER.log(Level.WARNING, "Parser error parsing svn output", saxe);
} catch (ParserConfigurationException pce) {
LOGGER.log(Level.WARNING,
"Parser configuration error parsing svn output", pce);
LOGGER.log(Level.WARNING, "Parser configuration error parsing svn output", pce);
} catch (IOException ioe) {
LOGGER.log(Level.WARNING,
"IOException reading from svn process", ioe);
LOGGER.log(Level.WARNING, "IOException reading from svn process", ioe);
}
} else {
LOGGER.log(Level.WARNING,
"Failed to execute svn info for [{0}]. Repository disabled.",
getDirectoryName());
LOGGER.log(Level.WARNING, "Failed to execute svn info for ''{0}''", getDirectoryName());
}

return document;
}

/**
* Get value of given tag in 'svn info' document.
* Get value of given tag in @{code svn info} document.
* @param document document object containing {@code info} contents
* @param tagName name of the tag to return value for
* @return value string
Expand All @@ -187,7 +183,7 @@ public void setDirectoryName(File directory) {
String url = getInfoPart(document, URL_ATTR);
if (url == null) {
LOGGER.log(Level.WARNING,
"svn info did not contain an URL for [{0}]. Assuming remote repository.",
"svn info did not contain an URL for ''{0}''. Assuming remote repository.",
getDirectoryName());
setRemote(true);
} else {
Expand All @@ -196,8 +192,7 @@ public void setDirectoryName(File directory) {
}
}

String root
= getValue(document.getElementsByTagName("root").item(0));
String root = getValue(document.getElementsByTagName("root").item(0));
if (url != null && root != null) {
reposPath = url.substring(root.length());
rootFound = Boolean.TRUE;
Expand Down Expand Up @@ -243,7 +238,7 @@ Executor getHistoryLogExecutor(final File file, String sinceRevision,
// fetch the unneeded revision and remove it later.
cmd.add("BASE:" + sinceRevision);
}
if (filename.length() > 0) {
if (!filename.isEmpty()) {
cmd.add(escapeFileName(filename));
}

Expand All @@ -260,8 +255,7 @@ boolean getHistoryGet(OutputStream out, String parent, String basename, String r
try {
filepath = new File(parent, basename).getCanonicalPath();
} catch (IOException exp) {
LOGGER.log(Level.SEVERE,
"Failed to get canonical path: {0}", exp.getClass());
LOGGER.log(Level.SEVERE, "Failed to get canonical path: {0}", exp.getClass());
return false;
}
String filename = filepath.substring(getDirectoryName().length() + 1);
Expand All @@ -280,11 +274,13 @@ boolean getHistoryGet(OutputStream out, String parent, String basename, String r
RuntimeEnvironment.getInstance().getInteractiveCommandTimeout());
if (executor.exec() == 0) {
try {
copyBytes(out::write, executor.getOutputStream());
InputStream stream = executor.getOutputStream();
if (!Objects.isNull(stream)) {
copyBytes(out::write, stream);
}
return true;
} catch (IOException e) {
LOGGER.log(Level.SEVERE, "Failed to get content for {0}",
basename);
LOGGER.log(Level.SEVERE, "Failed to get content for ''{0}''", basename);
}
}

Expand Down Expand Up @@ -318,35 +314,28 @@ private Executor getDirectoryListExecutor(final String dir, String atRevision, C
* Provides a list of files that were in a directory at a given revision.
* This is useful for finding files that will need special renamed file history
* handling because they were in a directory when it was renamed.
*
* <p>
* Note that this doesn't throw an exception even if the command was not completed
* because we will still be able to get the file history up to this point.
*
* </p>
* @param directory the directory to check
* @param revision the revision to check at
* @param cmdType the timeout setting.
* @return the files that were in the directory at that revision
*/
Set<String> getFilesInDirectoryAtRevision(String directory, String revision,
CommandTimeoutType cmdType) {
Set<String> getFilesInDirectoryAtRevision(String directory, String revision, CommandTimeoutType cmdType) {

Executor executor = getDirectoryListExecutor(
RuntimeEnvironment.getInstance().getSourceRootPath() + File.separator + directory,
revision, cmdType);

Set<String> files = new HashSet<>();

StreamHandler directoryLogStreamHandler = new StreamHandler() {

@Override
public void processStream(InputStream in) throws IOException {
new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))
.lines()
.filter(s -> !s.isBlank())
.map(s -> directory + File.separator + s)
.forEach(files::add);
}
};
StreamHandler directoryLogStreamHandler = in -> new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))
.lines()
.filter(s -> !s.isBlank())
.map(s -> directory + File.separator + s)
.forEach(files::add);

int status = executor.exec(true, directoryLogStreamHandler);
if (status != 0) {
Expand All @@ -373,15 +362,14 @@ History getHistory(File file, String sinceRevision) throws HistoryException {
return getHistory(file, sinceRevision, 0, CommandTimeoutType.INDEXER);
}

private History getHistory(File file, String sinceRevision, int numEntries,
CommandTimeoutType cmdType)
private History getHistory(File file, String sinceRevision, int numEntries, CommandTimeoutType cmdType)
throws HistoryException {
return new SubversionHistoryParser().parse(file, this, sinceRevision,
numEntries, cmdType);
}

private String escapeFileName(String name) {
if (name.length() == 0) {
if (name.isEmpty()) {
return name;
}
return name + "@";
Expand Down Expand Up @@ -423,7 +411,7 @@ public boolean fileHasAnnotation(File file) {

@Override
public boolean fileHasHistory(File file) {
// @TODO: Research how to cheaply test if a file in a given
// TODO: Research how to cheaply test if a file in a given
// SVN repo has history. If there is a cheap test, then this
// code can be refined, boosting performance.
return true;
Expand Down Expand Up @@ -517,7 +505,7 @@ public String determineCurrentVersion(CommandTimeoutType cmdType) throws IOExcep
}
}
} catch (HistoryException ex) {
LOGGER.log(Level.WARNING, "cannot get current version info for {0}",
LOGGER.log(Level.WARNING, "cannot get current version info for ''{0}''",
getDirectoryName());
}

Expand Down
Loading

0 comments on commit 261ac2c

Please sign in to comment.