Add error handler for XML parsing

Will output name of source if there is a warning or error

10 files changed, 119 insertions, 67 deletions

diff --git a/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java b/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
index 0e0f992952a..8985dac91aa 100644
--- a/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
+++ b/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
@@ -4,27 +4,16 @@ package com.yahoo.config.model;
 import com.yahoo.config.application.api.ApplicationFile;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.model.ConfigModelContext.ApplicationType;
-import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.builder.xml.ConfigModelBuilder;
 import com.yahoo.config.model.builder.xml.ConfigModelId;
 import com.yahoo.config.model.builder.xml.XmlHelper;
+import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.graph.ModelGraphBuilder;
 import com.yahoo.config.model.graph.ModelNode;
+import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.config.model.provision.HostsXmlProvisioner;
-
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.TreeMap;
-import java.util.logging.Level;
 import com.yahoo.path.Path;
 import com.yahoo.text.XML;
-import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.builder.VespaModelBuilder;
 import com.yahoo.vespa.model.clients.Clients;
@@ -32,13 +21,22 @@ import com.yahoo.vespa.model.content.Content;
 import com.yahoo.vespa.model.routing.Routing;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
 import java.io.IOException;
 import java.io.Reader;
 import java.io.Serializable;
 import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.TreeMap;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -91,7 +89,7 @@ public class ConfigModelRepo implements ConfigModelRepoAdder, Serializable, Iter
         builder.postProc(deployState.getDeployLogger(), root, this);
     }
 
-    private Element getServicesFromApp(ApplicationPackage applicationPackage) throws IOException, SAXException {
+    private Element getServicesFromApp(ApplicationPackage applicationPackage) throws IOException {
         try (Reader servicesFile = applicationPackage.getServices()) {
             return getServicesFromReader(servicesFile);
         }
@@ -188,8 +186,8 @@ public class ConfigModelRepo implements ConfigModelRepoAdder, Serializable, Iter
         return permanentServices;
     }
 
-    private Element getServicesFromReader(Reader reader) throws IOException, SAXException {
-        Document doc = XmlHelper.getDocumentBuilder().parse(new InputSource(reader));
+    private Element getServicesFromReader(Reader reader) {
+        Document doc = XmlHelper.getDocument(reader);
         return doc.getDocumentElement();
     }
 
@@ -274,10 +272,10 @@ public class ConfigModelRepo implements ConfigModelRepoAdder, Serializable, Iter
     }
 
     // TODO: Doctoring on the XML is the wrong level for this. We should be able to mark a model as default instead   -Jon
-    private static Element getImplicitAdmin(DeployState deployState) throws IOException, SAXException {
+    private static Element getImplicitAdmin(DeployState deployState) {
         String defaultAdminElement = deployState.isHosted() ? getImplicitAdminV4() : getImplicitAdminV2();
         log.log(Level.FINE, () -> "No <admin> defined, using " + defaultAdminElement);
-        return XmlHelper.getDocumentBuilder().parse(new InputSource(new StringReader(defaultAdminElement))).getDocumentElement();
+        return XmlHelper.getDocument(new StringReader(defaultAdminElement)).getDocumentElement();
     }
 
     private static String getImplicitAdminV2() {
diff --git a/config-model/src/main/java/com/yahoo/config/model/builder/xml/XmlHelper.java b/config-model/src/main/java/com/yahoo/config/model/builder/xml/XmlHelper.java
index 5763be301a6..220d6bbb768 100644
--- a/config-model/src/main/java/com/yahoo/config/model/builder/xml/XmlHelper.java
+++ b/config-model/src/main/java/com/yahoo/config/model/builder/xml/XmlHelper.java
@@ -3,12 +3,14 @@ package com.yahoo.config.model.builder.xml;
 
 import com.yahoo.component.ComponentId;
 import com.yahoo.component.ComponentSpecification;
-import java.util.logging.Level;
 import com.yahoo.text.XML;
+import com.yahoo.yolean.Exceptions;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
+import org.xml.sax.ErrorHandler;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
+import org.xml.sax.SAXParseException;
 
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -19,6 +21,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 
@@ -74,9 +77,15 @@ public final class XmlHelper {
     }
 
     public static Document getDocument(Reader reader) {
+        return getDocument(reader, "unknown source");
+    }
+
+    public static Document getDocument(Reader reader, String source) {
         Document doc;
         try {
-            doc = getDocumentBuilder().parse(new InputSource(reader));
+            InputSource inputSource = new InputSource(reader);
+            inputSource.setPublicId(source);
+            doc = getDocumentBuilder().parse(inputSource);
         } catch (SAXException | IOException e) {
             throw new IllegalArgumentException(e);
         }
@@ -121,6 +130,7 @@ public final class XmlHelper {
     public static synchronized DocumentBuilder getDocumentBuilder() {
         try {
             DocumentBuilder docBuilder = factory.newDocumentBuilder();
+            docBuilder.setErrorHandler(new CustomErrorHandler(log));
             log.log(Level.FINE, "XML parser now operational!");
             return docBuilder;
         } catch (ParserConfigurationException e) {
@@ -144,4 +154,34 @@ public final class XmlHelper {
         if (child.getFirstChild() == null) return Optional.empty();
         return Optional.ofNullable(child.getFirstChild().getNodeValue());
     }
+
+    /** Error handler which will output name of source for warnings and errors */
+    private static class CustomErrorHandler implements ErrorHandler {
+
+        private final Logger logger;
+
+        CustomErrorHandler(Logger logger) {
+            super();
+            this.logger = logger;
+        }
+
+        public void warning(SAXParseException e) {
+            logger.log(Level.WARNING, message(e));
+        }
+
+        public void error(SAXParseException e) {
+            throw new IllegalArgumentException(message(e));
+        }
+
+        public void fatalError(SAXParseException e) { throw new IllegalArgumentException(message(e)); }
+
+        private String message(SAXParseException e) {
+            String sourceId = e.getPublicId() == null ? "" : e.getPublicId();
+            return "Invalid XML" + (sourceId.isEmpty() ? " (unknown source)" : " in " + sourceId) +
+                    ": " + Exceptions.toMessageString(e) +
+                    " [" + e.getLineNumber() + ":" + e.getColumnNumber() + "]";
+        }
+
+    }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/config/model/provision/Hosts.java b/config-model/src/main/java/com/yahoo/config/model/provision/Hosts.java
index c9a03dad65e..2ef8cb4a0bf 100644
--- a/config-model/src/main/java/com/yahoo/config/model/provision/Hosts.java
+++ b/config-model/src/main/java/com/yahoo/config/model/provision/Hosts.java
@@ -8,12 +8,13 @@ import com.yahoo.text.XML;
 import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.InputSource;
-import org.xml.sax.SAXException;
 
-import java.io.IOException;
 import java.io.Reader;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import java.util.logging.Logger;
 
 /**
@@ -63,12 +64,7 @@ public class Hosts {
      */
     public static Hosts readFrom(Reader hostsFile) {
         List<Host> hosts = new ArrayList<>();
-        Document doc;
-        try {
-            doc = XmlHelper.getDocumentBuilder().parse(new InputSource(hostsFile));
-        } catch (SAXException | IOException e) {
-            throw new IllegalArgumentException(e);
-        }
+        Document doc = XmlHelper.getDocument(hostsFile);
         for (Element hostE : XML.getChildren(doc.getDocumentElement(), "host")) {
             String name = hostE.getAttribute("name");
             if (name.equals("")) {
diff --git a/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java b/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
index b29ff1ee58b..7e2953e6606 100644
--- a/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
+++ b/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
@@ -19,10 +19,7 @@ import com.yahoo.vespa.model.builder.xml.dom.DomAdminV2Builder;
 import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProducer;
 import com.yahoo.vespa.model.filedistribution.FileReferencesRepository;
 import org.w3c.dom.Document;
-import org.xml.sax.InputSource;
-import org.xml.sax.SAXException;
 
-import java.io.IOException;
 import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -143,13 +140,9 @@ public class MockRoot extends AbstractConfigProducerRoot {
                 "<?xml version='1.0' encoding='utf-8' ?>" +
                 "<services>" + xml + "</services>";
 
-        try {
-            Document doc = XmlHelper.getDocumentBuilder().parse(new InputSource(new StringReader(servicesXml)));
-            setAdmin(new DomAdminV2Builder(ConfigModelContext.ApplicationType.DEFAULT, false, new ArrayList<>()).
-                    build(deployState, this, XML.getChildren(doc.getDocumentElement(), "admin").get(0)));
-        } catch (SAXException | IOException e) {
-            throw new RuntimeException(e);
-        }
+        Document doc = XmlHelper.getDocument(new StringReader(servicesXml));
+        setAdmin(new DomAdminV2Builder(ConfigModelContext.ApplicationType.DEFAULT, false, new ArrayList<>())
+                         .build(deployState, this, XML.getChildren(doc.getDocumentElement(), "admin").get(0)));
     }
 
     public final void setAdmin(Admin admin) {
diff --git a/config-model/src/main/java/com/yahoo/config/model/test/TestUtil.java b/config-model/src/main/java/com/yahoo/config/model/test/TestUtil.java
index df916907472..c05d7bf4942 100644
--- a/config-model/src/main/java/com/yahoo/config/model/test/TestUtil.java
+++ b/config-model/src/main/java/com/yahoo/config/model/test/TestUtil.java
@@ -25,9 +25,7 @@ public class TestUtil {
         lines.addAll(Arrays.asList(xmlLines));
 
         try {
-            return XmlHelper.getDocumentBuilder().parse(
-                    inputSource((CollectionUtil.mkString(lines, "\n").replace("'", "\""))))
-                    .getDocumentElement();
+            return XmlHelper.getDocument(new StringReader(CollectionUtil.mkString(lines, "\n").replace("'", "\""))).getDocumentElement();
         } catch (Exception e) {
             throw new RuntimeException(e);
         }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
index df77d83da83..b619210155f 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
@@ -4,11 +4,10 @@ package com.yahoo.vespa.model.builder.xml.dom;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.ApplicationConfigProducerRoot;
 import com.yahoo.config.model.ConfigModelRepo;
-import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.builder.xml.XmlHelper;
+import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.config.model.producer.UserConfigRepo;
-import java.util.logging.Level;
 import com.yahoo.text.XML;
 import com.yahoo.vespa.model.AbstractService;
 import com.yahoo.vespa.model.Affinity;
@@ -24,7 +23,6 @@ import com.yahoo.vespa.model.content.Content;
 import com.yahoo.vespa.model.generic.builder.DomServiceClusterBuilder;
 import com.yahoo.vespa.model.generic.service.ServiceCluster;
 import com.yahoo.vespa.model.search.SearchCluster;
-
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
@@ -33,6 +31,7 @@ import org.w3c.dom.NodeList;
 import java.util.ArrayList;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -94,7 +93,8 @@ public class VespaDomBuilder extends VespaModelBuilder {
     public ApplicationConfigProducerRoot getRoot(String name, DeployState deployState, AbstractConfigProducer parent) {
         try {
             return new DomRootBuilder(name).
-                    build(deployState, parent, XmlHelper.getDocument(deployState.getApplicationPackage().getServices()).getDocumentElement());
+                    build(deployState, parent, XmlHelper.getDocument(deployState.getApplicationPackage().getServices(), "services.xml")
+                                                        .getDocumentElement());
         } catch (Exception e) {
             throw new IllegalArgumentException(e);
         }
@@ -303,7 +303,7 @@ public class VespaDomBuilder extends VespaModelBuilder {
     @Override
     public List<ServiceCluster> getClusters(DeployState deployState, AbstractConfigProducer parent) {
         List<ServiceCluster> clusters = new ArrayList<>();
-        Document services = XmlHelper.getDocument(deployState.getApplicationPackage().getServices());
+        Document services = XmlHelper.getDocument(deployState.getApplicationPackage().getServices(), "services.xml");
         for (Element clusterSpec : XML.getChildren(services.getDocumentElement(), "cluster")) {
             DomServiceClusterBuilder clusterBuilder = new DomServiceClusterBuilder(clusterSpec.getAttribute("name"));
             clusters.add(clusterBuilder.build(deployState, parent.getRoot(), clusterSpec));
diff --git a/config-model/src/test/java/com/yahoo/config/model/builder/xml/XmlErrorHandlingTest.java b/config-model/src/test/java/com/yahoo/config/model/builder/xml/XmlErrorHandlingTest.java
new file mode 100644
index 00000000000..ee616f59d04
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/config/model/builder/xml/XmlErrorHandlingTest.java
@@ -0,0 +1,37 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.config.model.builder.xml;
+
+import org.junit.Test;
+import org.xml.sax.InputSource;
+import java.io.FileReader;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author hmusum
+ */
+public class XmlErrorHandlingTest {
+
+    @Test
+    public void requireExceptionWithSourceAndFilenameAndLineNumber() {
+        try {
+            XmlHelper.getDocument(new FileReader("src/test/cfg/application/invalid-services-syntax/services.xml"), "services.xml");
+        } catch (Exception e) {
+            assertEquals("Invalid XML in services.xml: The element type \"config\" must be terminated by the matching end-tag \"</config>\". [7:5]",
+                         e.getMessage());
+        }
+    }
+
+
+    @Test
+    public void requireExceptionWithLineNumber() {
+        try {
+            XmlHelper.getDocumentBuilder().parse(
+                    new InputSource(new FileReader("src/test/cfg/application/invalid-services-syntax/services.xml")));
+        } catch (Exception e) {
+            assertEquals("Invalid XML (unknown source): The element type \"config\" must be terminated by the matching end-tag \"</config>\". [7:5]",
+                                             e.getMessage());
+        }
+    }
+
+}
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java
index bb3a0f26ee9..114038b884e 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java
@@ -15,7 +15,6 @@ import com.yahoo.vespa.configdefinition.SpecialtokensConfig;
 import org.junit.Test;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.InputSource;
 
 import java.io.Reader;
 import java.io.StringReader;
@@ -112,7 +111,7 @@ public class UserConfigBuilderTest {
         Reader xmlReader = new StringReader("<model>" + xml + "</model>");
         Document doc;
         try {
-            doc = XmlHelper.getDocumentBuilder().parse(new InputSource(xmlReader));
+            doc = XmlHelper.getDocument(xmlReader);
         } catch (Exception e) {
             throw new RuntimeException();
         }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
index 264d102161a..a20ce425ac0 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
@@ -11,11 +11,9 @@ import com.yahoo.vespa.config.ConfigDefinitionBuilder;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.ConfigPayload;
 import com.yahoo.vespa.config.ConfigPayloadBuilder;
-
 import org.junit.Test;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.InputSource;
 
 import java.io.ByteArrayOutputStream;
 import java.io.FileNotFoundException;
@@ -273,7 +271,7 @@ public class DomConfigPayloadBuilderTest {
     private Element getDocument(Reader xmlReader) {
         Document doc;
         try {
-            doc = XmlHelper.getDocumentBuilder().parse(new InputSource(xmlReader));
+            doc = XmlHelper.getDocument(xmlReader);
         } catch (Exception e) {
             throw new RuntimeException();
         }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/BundleInstantiationSpecificationBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/BundleInstantiationSpecificationBuilderTest.java
index 909ac9edef2..686f7bbd1f1 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/BundleInstantiationSpecificationBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/BundleInstantiationSpecificationBuilderTest.java
@@ -8,12 +8,8 @@ import com.yahoo.search.grouping.GroupingValidator;
 import com.yahoo.vespa.model.container.PlatformBundles;
 import org.junit.Test;
 import org.w3c.dom.Element;
-import org.xml.sax.SAXException;
 
-import java.io.ByteArrayInputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.nio.charset.StandardCharsets;
+import java.io.StringReader;
 
 import static org.junit.Assert.assertEquals;
 
@@ -25,34 +21,31 @@ import static org.junit.Assert.assertEquals;
 public class BundleInstantiationSpecificationBuilderTest {
 
     @Test
-    public void bundle_is_not_replaced_for_user_defined_class() throws IOException, SAXException {
+    public void bundle_is_not_replaced_for_user_defined_class() {
         final String userDefinedClass = "my own class that will also be set as bundle";
         verifyExpectedBundle(userDefinedClass, null, userDefinedClass);
     }
 
     @Test
-    public void bundle_is_replaced_for_internal_class() throws IOException, SAXException {
+    public void bundle_is_replaced_for_internal_class() {
         String internalClass = GroupingValidator.class.getName();
         verifyExpectedBundle(internalClass, null, PlatformBundles.searchAndDocprocBundle);
     }
 
     @Test
-    public void bundle_is_not_replaced_for_internal_class_with_explicitly_set_bundle()
-            throws IOException, SAXException {
+    public void bundle_is_not_replaced_for_internal_class_with_explicitly_set_bundle() {
         String internalClass = GroupingValidator.class.getName();
         String explicitBundle = "my-own-implementation";
         verifyExpectedBundle(internalClass, explicitBundle, explicitBundle);
     }
 
-    private static void verifyExpectedBundle(String className, String explicitBundle, String expectedBundle)
-            throws IOException, SAXException {
+    private static void verifyExpectedBundle(String className, String explicitBundle, String expectedBundle) {
         String xml = "<component id=\"_\" class=\"" + className + "\"";
         if (explicitBundle != null) {
             xml += " bundle=\"" + explicitBundle + "\"";
         }
         xml += " />";
-        InputStream xmlStream = new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8));
-        Element component = XmlHelper.getDocumentBuilder().parse(xmlStream).getDocumentElement();
+        Element component = XmlHelper.getDocument(new StringReader(xml)).getDocumentElement();
 
         BundleInstantiationSpecification spec = BundleInstantiationSpecificationBuilder.build(component);
         assertEquals(ComponentSpecification.fromString(expectedBundle), spec.bundle);