Add unused xml detection (#1221)

Signed-off-by: Pablo Herrera <[email protected]>

core/src/main/java/tc/oc/pgm/PGMConfig.java

@@ -63,6 +63,7 @@ public final class PGMConfig implements Config {
   private final List<MapSourceFactory> mapSourceFactories;
   private final Path mapPoolFile;
   private final Path includesDirectory;
+  private final boolean showUnusedXml;
 
   // countdown.*
   private final Duration startTime;
@@ -161,6 +162,7 @@ public final class PGMConfig implements Config {
 
     this.mapPoolFile = getPath(dataFolder.toPath(), config.getString("map.pools"));
     this.includesDirectory = getPath(dataFolder.toPath(), config.getString("map.includes"));
+    this.showUnusedXml = parseBoolean(config.getString("map.show-unused-xml", "true"));
 
     this.startTime = parseDuration(config.getString("countdown.start", "30s"));
     this.huddleTime = parseDuration(config.getString("countdown.huddle", "0s"));
@@ -475,6 +477,11 @@ public Path getMapPoolFile() {
     return includesDirectory;
   }
 
+  @Override
+  public boolean showUnusedXml() {
+    return showUnusedXml;
+  }
+
   @Override
   public Duration getStartTime() {
     return startTime;

core/src/main/java/tc/oc/pgm/api/Config.java

@@ -69,6 +69,9 @@ public interface Config {
   @Nullable
   Path getIncludesDirectory();
 
+  /** @return If unused XML tags should be reported or ignored */
+  boolean showUnusedXml();
+
   /**
    * Gets a duration to wait before starting a match.
    *

core/src/main/java/tc/oc/pgm/map/MapFactoryImpl.java

@@ -6,11 +6,13 @@
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.jdom2.Document;
 import org.jdom2.Element;
 import org.jdom2.input.JDOMParseException;
 import tc.oc.pgm.api.Modules;
+import tc.oc.pgm.api.PGM;
 import tc.oc.pgm.api.map.MapContext;
 import tc.oc.pgm.api.map.MapModule;
 import tc.oc.pgm.api.map.MapProtos;
@@ -33,7 +35,9 @@
 import tc.oc.pgm.regions.RegionParser;
 import tc.oc.pgm.util.ClassLogger;
 import tc.oc.pgm.util.Version;
+import tc.oc.pgm.util.xml.DocumentWrapper;
 import tc.oc.pgm.util.xml.InvalidXMLException;
+import tc.oc.pgm.util.xml.Node;
 import tc.oc.pgm.util.xml.XMLUtils;
 
 public class MapFactoryImpl extends ModuleGraph<MapModule<?>, MapModuleFactory<?>>
@@ -106,6 +110,15 @@ private void postLoad() throws InvalidXMLException {
     for (MapModule<?> module : getModules()) {
       module.postParse(this, logger, document);
     }
+
+    if (PGM.get().getConfiguration().showUnusedXml()) {
+      ((DocumentWrapper) document).checkUnvisited(this::printUnvisitedNode);
+    }
+  }
+
+  private void printUnvisitedNode(Node node) {
+    InvalidXMLException ex = new InvalidXMLException("Unused node, maybe a typo?", node);
+    logger.log(Level.WARNING, ex.getMessage(), ex);
   }
 
   @Override

core/src/main/java/tc/oc/pgm/map/MapFilePreprocessor.java

@@ -21,6 +21,7 @@
 import tc.oc.pgm.api.map.exception.MapMissingException;
 import tc.oc.pgm.api.map.includes.MapInclude;
 import tc.oc.pgm.api.map.includes.MapIncludeProcessor;
+import tc.oc.pgm.util.xml.DocumentWrapper;
 import tc.oc.pgm.util.xml.InvalidXMLException;
 import tc.oc.pgm.util.xml.Node;
 import tc.oc.pgm.util.xml.SAXHandler;
@@ -60,29 +61,32 @@ private MapFilePreprocessor(MapSource source, MapIncludeProcessor includeProcess
 
   public Document getDocument()
       throws MapMissingException, IOException, JDOMException, InvalidXMLException {
-    Document document;
+    DocumentWrapper document;
     try (final InputStream stream = source.getDocument()) {
-      document = DOCUMENT_FACTORY.get().build(stream);
-      document.setBaseURI(source.getId());
+      document = (DocumentWrapper) DOCUMENT_FACTORY.get().build(stream);
+      document.setBaseURI(source.getId() + ("default".equals(variant) ? "" : "[" + variant + "]"));
     }
 
-    MapInclude global = includeProcessor.getGlobalInclude();
-    if (global != null) {
-      document.getRootElement().addContent(0, global.getContent());
-      includes.add(global);
-    }
+    document.runWithoutVisitation(
+        () -> {
+          MapInclude global = includeProcessor.getGlobalInclude();
+          if (global != null) {
+            document.getRootElement().addContent(0, global.getContent());
+            includes.add(global);
+          }
 
-    preprocessChildren(document.getRootElement());
-    source.setIncludes(includes);
+          preprocessChildren(document.getRootElement());
+          source.setIncludes(includes);
+        });
 
     for (Element constant :
         XMLUtils.flattenElements(document.getRootElement(), "constants", "constant", 0)) {
       constants.put(XMLUtils.parseRequiredId(constant), constant.getText());
     }
 
     // If no constants are set, assume we can skip the step
-    if (constants.size() > 0) {
-      postprocessChildren(document.getRootElement());
+    if (!constants.isEmpty()) {
+      document.runWithoutVisitation(() -> postprocessChildren(document.getRootElement()));
     }
 
     return document;

core/src/main/java/tc/oc/pgm/map/includes/MapIncludeImpl.java

@@ -14,14 +14,18 @@
 import tc.oc.pgm.api.PGM;
 import tc.oc.pgm.api.map.exception.MapMissingException;
 import tc.oc.pgm.api.map.includes.MapInclude;
+import tc.oc.pgm.util.xml.DocumentWrapper;
 
 public class MapIncludeImpl implements MapInclude {
 
+  private final String id;
   private final File file;
   private final AtomicLong lastRead;
   private Document source;
 
-  public MapIncludeImpl(File file) throws MapMissingException, JDOMException, IOException {
+  public MapIncludeImpl(String id, File file)
+      throws MapMissingException, JDOMException, IOException {
+    this.id = id;
     this.file = file;
     this.lastRead = new AtomicLong(-1);
     reload();
@@ -30,6 +34,9 @@ public MapIncludeImpl(File file) throws MapMissingException, JDOMException, IOEx
   private void reload() throws MapMissingException, JDOMException, IOException {
     try (InputStream is = new FileInputStream(file)) {
       this.source = MapIncludeProcessorImpl.DOCUMENT_FACTORY.get().build(is);
+      // Includes should never visit, let the map XML visit instead
+      ((DocumentWrapper) this.source).setVisitingAllowed(false);
+      this.source.setBaseURI("#" + id);
     } catch (FileNotFoundException e) {
       throw new MapMissingException(file.getPath(), "Unable to read map include document", e);
     } finally {

core/src/main/java/tc/oc/pgm/map/includes/MapIncludeProcessorImpl.java

@@ -92,10 +92,9 @@ public void loadNewIncludes() {
       if (deletedIncludes.remove(id)) continue;
 
       try {
-        this.includes.put(id, new MapIncludeImpl(file));
+        this.includes.put(id, new MapIncludeImpl(id, file));
       } catch (MapMissingException | JDOMException | IOException error) {
         logger.log(Level.WARNING, "Failed to load " + filename + " include document", error);
-        error.printStackTrace();
       }
     }
 

core/src/main/resources/config.yml

@@ -35,6 +35,10 @@ map:
   # A path to the includes folder, or empty to disable map includes.
   includes: "includes"
 
+  # Show unused XML nodes as map errors. Helps with finding typos in xml.
+  show-unused-xml: true
+
+
 # Sets the duration of various countdowns.
 #
 #  "30s"  = 30 seconds

util/src/main/java/tc/oc/pgm/util/bukkit/Platform.java

@@ -3,6 +3,7 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import org.bukkit.Bukkit;
+import org.bukkit.Server;
 import tc.oc.pgm.util.ClassLogger;
 import tc.oc.pgm.util.nms.NMSHacksNoOp;
 import tc.oc.pgm.util.nms.NMSHacksPlatform;
@@ -32,7 +33,8 @@ public enum Platform {
   public static Platform SERVER_PLATFORM = computeServerPlatform();
 
   private static Platform computeServerPlatform() {
-    String versionString = Bukkit.getServer().getVersion();
+    Server sv = Bukkit.getServer();
+    String versionString = sv == null ? "" : sv.getVersion();
     for (Platform platform : Platform.values()) {
       if (versionString.contains(platform.variant)
           && versionString.contains("MC: " + platform.majorVersion)) {

util/src/main/java/tc/oc/pgm/util/xml/DocumentWrapper.java

@@ -0,0 +1,75 @@
+package tc.oc.pgm.util.xml;
+
+import com.google.common.collect.Sets;
+import java.util.Set;
+import java.util.function.Consumer;
+import org.jdom2.Attribute;
+import org.jdom2.Content;
+import org.jdom2.DocType;
+import org.jdom2.Document;
+import org.jdom2.Element;
+import org.jdom2.Namespace;
+
+public class DocumentWrapper extends Document {
+
+  private static final Set<String> IGNORED = Sets.newHashSet("variant", "tutorial");
+
+  private boolean visitingAllowed = true;
+
+  public DocumentWrapper() {
+    super();
+  }
+
+  public DocumentWrapper(Element rootElement, DocType docType, String baseURI) {
+    super(rootElement, docType, baseURI);
+  }
+
+  public DocumentWrapper(Element rootElement, DocType docType) {
+    super(rootElement, docType);
+  }
+
+  public DocumentWrapper(Element rootElement) {
+    super(rootElement);
+  }
+
+  public void setVisitingAllowed(boolean visitingAllowed) {
+    this.visitingAllowed = visitingAllowed;
+  }
+
+  public void runWithoutVisitation(DocumentWorker worker) throws InvalidXMLException {
+    this.visitingAllowed = false;
+    worker.run();
+    this.visitingAllowed = true;
+  }
+
+  public boolean isVisitingAllowed() {
+    return visitingAllowed;
+  }
+
+  public void checkUnvisited(Consumer<Node> unvisited) {
+    checkVisited(getRootElement(), unvisited);
+  }
+
+  private void checkVisited(Element el, Consumer<Node> unvisited) {
+    for (Attribute attribute : el.getAttributes()) {
+      if (attribute.getNamespace() == Namespace.NO_NAMESPACE
+          && !((VisitableAttribute) attribute).wasVisited())
+        unvisited.accept(Node.fromNullable(attribute));
+    }
+
+    for (int i = 0; i < el.getContentSize(); i++) {
+      Content c = el.getContent(i);
+      if (!(c instanceof InheritingElement)) continue;
+      InheritingElement child = (InheritingElement) c;
+
+      if (child.getNamespace() == Namespace.NO_NAMESPACE && !IGNORED.contains(child.getName())) {
+        if (!child.wasVisited()) unvisited.accept(Node.fromNullable(child));
+        else checkVisited(child, unvisited);
+      }
+    }
+  }
+
+  public interface DocumentWorker {
+    void run() throws InvalidXMLException;
+  }
+}

util/src/main/java/tc/oc/pgm/util/xml/InheritingElement.java

@@ -1,5 +1,9 @@
 package tc.oc.pgm.util.xml;
 
+import com.google.common.collect.Iterables;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.jdom2.Attribute;
 import org.jdom2.Element;
 import org.jdom2.Namespace;
@@ -16,6 +20,8 @@
  */
 public class InheritingElement extends LocatedElement {
 
+  private AtomicBoolean visited = new AtomicBoolean();
+  private String originalUri;
   private int startLine, endLine;
   private int indexInParent = Integer.MIN_VALUE;
 
@@ -29,6 +35,8 @@ public InheritingElement(Element el) {
     setStartLine(bounded.getStartLine());
     setEndLine(bounded.getEndLine());
     this.indexInParent = bounded.indexInParent();
+    this.originalUri = bounded.originalUri;
+    this.visited = bounded.visited;
 
     setContent(el.cloneContent());
 
@@ -86,13 +94,65 @@ protected int indexInParent() {
     return indexInParent;
   }
 
+  public boolean wasVisited() {
+    return visited.get();
+  }
+
+  private void setVisited() {
+    visited.set(true);
+  }
+
+  @Override
+  public List<Element> getChildren() {
+    List<Element> children = super.getChildren();
+    if (visitingAllowed()) children.forEach(child -> ((InheritingElement) child).setVisited());
+    return children;
+  }
+
+  public Iterable<Element> getChildren(Set<String> names) {
+    boolean visitingAllowed = visitingAllowed();
+    return Iterables.filter(
+        super.getChildren(),
+        el -> {
+          if (names.contains(el.getName())) {
+            if (visitingAllowed) ((InheritingElement) el).setVisited();
+            return true;
+          }
+          return false;
+        });
+  }
+
+  @Override
+  public List<Element> getChildren(String cname, Namespace ns) {
+    List<Element> children = super.getChildren(cname, ns);
+    if (visitingAllowed()) children.forEach(child -> ((InheritingElement) child).setVisited());
+    return children;
+  }
+
+  @Override
+  public Element getChild(String cname, Namespace ns) {
+    Element child = super.getChild(cname, ns);
+    if (visitingAllowed() && child != null) ((InheritingElement) child).setVisited();
+    return child;
+  }
+
+  private boolean visitingAllowed() {
+    return ((DocumentWrapper) getDocument()).isVisitingAllowed();
+  }
+
+  public String getOriginalUri() {
+    return originalUri;
+  }
+
   @Override
   public InheritingElement clone() {
     final InheritingElement that = (InheritingElement) super.clone();
     that.setLine(getLine());
     that.setColumn(getColumn());
     that.setStartLine(getStartLine());
     that.setEndLine(getEndLine());
+    that.visited = this.visited;
+    that.originalUri = getDocument().getBaseURI();
     return that;
   }
 }