prevent null date in Subversion history entry (#4564)

- propagate stream processing error as -1 exit code
- fix various style issues

fixes #3111

Author: vladak

opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionHistoryParser.java

@@ -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>.
  */
@@ -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;
@@ -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;
@@ -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);
         }
@@ -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) {
@@ -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}''",
@@ -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);
@@ -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;
@@ -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)));

opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionRepository.java

@@ -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;
@@ -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;
@@ -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 {
@@ -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<>();
@@ -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
@@ -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 {
@@ -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;
@@ -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));
         }
 
@@ -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);
@@ -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);
             }
         }
 
@@ -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) {
@@ -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 + "@";
@@ -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;
@@ -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());
         }