Support comment and per-directive newline removal

10 files changed, 177 insertions, 50 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Template.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Template.java
index 4fc49afea02..69fd84fd008 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Template.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Template.java
@@ -16,14 +16,17 @@ import java.util.function.Consumer;
  *
  * <pre>
  *     template: section*
- *     section: literal | variable | list
- *     literal: # plain text not containing %{
+ *     section: literal | variable | list | line-comment
+ *     literal: plain text not containing %{
  *     variable: %{=identifier}
  *     list: %{list identifier}template%{end}
- *     identifier: # a valid Java identifier
+ *     line-comment: %{#}
+ *     identifier: a valid Java identifier
  * </pre>
  *
- * <p>Any newline (\n) following a non-variable directive is removed.</p>
+ * <p>If the directive's end delimiter (}) is preceded by a "|" char, then any newline (\n)
+ * following the end delimiter is removed.  Or in the case of line-comment: the newline terminating
+ * the line comment is removed.</p>
  *
  * <p>To use the template, <b>Instantiate</b> it to get a form ({@link #instantiate()}), fill it (e.g.
  * {@link Form#set(String, String) Form.set()}), and render the String ({@link Form#render()}).</p>
@@ -46,6 +49,12 @@ public class Template {
     /** The value contains the location of the name of a sample variable section (with that name). */
     private final HashMap<String, Cursor> names = new HashMap<>();
 
+    public static Template from(String text) { return from(text, new TemplateDescriptor()); }
+
+    public static Template from(String text, TemplateDescriptor descriptor) {
+        return TemplateParser.parse(text, descriptor).template();
+    }
+
     Template(Cursor start) {
         this.start = new Cursor(start);
         this.end = new Cursor(start);
@@ -73,6 +82,10 @@ public class Template {
         sections.add(formBuilder -> formBuilder.addListSection(range, name, body));
     }
 
+    void appendCommentSection(Cursor end) {
+        verifyAndUpdateEnd(end);
+    }
+
     private CursorRange range() { return new CursorRange(start, end); }
 
     private CursorRange verifyAndUpdateEnd(Cursor newEnd) {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateDescriptor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateDescriptor.java
index 40e2c051f6a..568ed20aafb 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateDescriptor.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateDescriptor.java
@@ -7,16 +7,18 @@ package com.yahoo.vespa.hosted.node.admin.task.util.template;
  * @author hakonhall
  */
 public class TemplateDescriptor {
+    private static final char VARIABLE_DIRECTIVE_CHAR = '=';
+    private static final char REMOVE_NEWLINE_CHAR = '|';
+    private static final char COMMENT_CHAR = '#';
+
     private String startDelimiter = "%{";
     private String endDelimiter = "}";
-    private boolean removeNewlineAfterSection = true;
 
     public TemplateDescriptor() {}
 
     public TemplateDescriptor(TemplateDescriptor that) {
         this.startDelimiter = that.startDelimiter;
         this.endDelimiter = that.endDelimiter;
-        this.removeNewlineAfterSection = that.removeNewlineAfterSection;
     }
 
     /** Use these delimiters instead of the standard "%{" and "}" to start and end a template directive. */
@@ -26,13 +28,10 @@ public class TemplateDescriptor {
         return this;
     }
 
-    /** Whether to remove a newline following each (non-variable) section, by default true. */
-    public TemplateDescriptor setRemoveNewlineAfterSection(boolean removeNewlineAfterSection) {
-        this.removeNewlineAfterSection = removeNewlineAfterSection;
-        return this;
-    }
-
     public String startDelimiter() { return startDelimiter; }
     public String endDelimiter() { return endDelimiter; }
-    public boolean removeNewlineAfterSection() { return removeNewlineAfterSection; }
+
+    char variableDirectiveChar() { return VARIABLE_DIRECTIVE_CHAR; }
+    char removeNewlineChar() { return REMOVE_NEWLINE_CHAR; }
+    char commentChar() { return COMMENT_CHAR; }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFile.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFile.java
index 402d7f8c5e5..0c1a26f4f65 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFile.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFile.java
@@ -15,6 +15,6 @@ public class TemplateFile {
 
     public static Template read(Path path, TemplateDescriptor descriptor) {
         String content = new UnixPath(path).readUtf8File();
-        return TemplateParser.parse(descriptor, content);
+        return Template.from(content, descriptor);
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateParser.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateParser.java
index efd63e1ab37..4c74e8553b9 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateParser.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateParser.java
@@ -11,15 +11,15 @@ import java.util.Optional;
  *
  * @author hakonhall
  */
-public class TemplateParser {
+class TemplateParser {
     private final TemplateDescriptor descriptor;
     private final Cursor start;
     private final Cursor current;
     private final Template template;
     private final FormEndsIn formEndsIn;
 
-    public static Template parse(TemplateDescriptor descriptor, String text) {
-        return parse(new TemplateDescriptor(descriptor), new Cursor(text), FormEndsIn.EOT).template;
+    static TemplateParser parse(String text, TemplateDescriptor descriptor) {
+        return parse(new TemplateDescriptor(descriptor), new Cursor(text), FormEndsIn.EOT);
     }
 
     private static TemplateParser parse(TemplateDescriptor descriptor, Cursor start, FormEndsIn formEndsIn) {
@@ -28,7 +28,7 @@ public class TemplateParser {
         return parser;
     }
 
-    enum FormEndsIn { EOT, END }
+    private enum FormEndsIn { EOT, END }
 
     TemplateParser(TemplateDescriptor descriptor, Cursor start, FormEndsIn formEndsIn) {
         this.descriptor = descriptor;
@@ -48,7 +48,14 @@ public class TemplateParser {
                 template.appendLiteralSection(current);
             }
 
-            if (current.eot()) return;
+            if (current.eot()) {
+                if (formEndsIn == FormEndsIn.END) {
+                    throw new BadTemplateException(current,
+                                                   "Missing end directive for section started at " +
+                                                   start.calculateLocation().lineAndColumnText());
+                }
+                return;
+            }
 
             if (!parseSection()) return;
         } while (true);
@@ -56,10 +63,13 @@ public class TemplateParser {
 
     /** Returns true if end was reached (according to formEndsIn). */
     private boolean parseSection() {
+        var startOfDirective = new Cursor(current);
         current.skip(descriptor.startDelimiter());
 
-        if (current.skip('=')) {
+        if (current.skip(descriptor.variableDirectiveChar())) {
             parseVariableSection();
+        } else if (current.skip(descriptor.commentChar())) {
+            parseCommentSection(startOfDirective);
         } else {
             var startOfType = new Cursor(current);
             String type = skipId().orElseThrow(() -> new BadTemplateException(current, "Missing section name"));
@@ -84,16 +94,26 @@ public class TemplateParser {
     private void parseVariableSection() {
         var nameStart = new Cursor(current);
         String name = parseId();
-        parseEndDelimiter(false);
+        parseEndDelimiter(true);
         template.appendVariableSection(name, nameStart, current);
     }
 
+    private void parseCommentSection(Cursor startOfDirective) {
+        if (parseEndDelimiter(false)) {
+            current.advancePast('\n');
+        } else {
+            current.advanceTo('\n');
+        }
+
+        template.appendCommentSection(current);
+    }
+
     private void parseEndDirective() {
         parseEndDelimiter(true);
     }
 
     private void parseListSection() {
-        skipWhitespace();
+        skipRequiredWhitespaces();
         var startOfName = new Cursor(current);
         String name = parseId();
         parseEndDelimiter(true);
@@ -104,8 +124,8 @@ public class TemplateParser {
         template.appendListSection(name, startOfName, current, bodyParser.template());
     }
 
-    private void skipWhitespace() {
-        if (!current.skipWhitespace()) {
+    private void skipRequiredWhitespaces() {
+        if (!current.skipWhitespaces()) {
             throw new BadTemplateException(current, "Expected whitespace");
         }
     }
@@ -116,11 +136,14 @@ public class TemplateParser {
 
     private Optional<String> skipId() { return Token.skipId(current); }
 
-    private void parseEndDelimiter(boolean newlineMayBeRemoved) {
+    private boolean parseEndDelimiter(boolean skipNewline) {
+        boolean removeNewline = current.skip(descriptor.removeNewlineChar());
         if (!current.skip(descriptor.endDelimiter()))
             throw new BadTemplateException(current, "Expected section end (" + descriptor.endDelimiter() + ")");
 
-        if (descriptor.removeNewlineAfterSection() && newlineMayBeRemoved)
+        if (skipNewline && removeNewline)
             current.skip('\n');
+
+        return removeNewline;
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Token.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Token.java
index 4dd40091e78..2fdb4fa9fbb 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Token.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Token.java
@@ -14,10 +14,10 @@ class Token {
         if (cursor.eot() || !isIdStart(cursor.getChar())) return Optional.empty();
 
         Cursor start = new Cursor(cursor);
-        cursor.advance(1);
+        cursor.increment();
 
         while (!cursor.eot() && isIdPart(cursor.getChar()))
-            cursor.advance(1);
+            cursor.increment();
 
         return Optional.of(new CursorRange(start, cursor).string());
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/Cursor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/Cursor.java
index bc0fa642339..14abebfa51c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/Cursor.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/Cursor.java
@@ -30,6 +30,7 @@ public class Cursor {
     public String toString() { return text.substring(offset); }
 
     public int offset() { return offset; }
+    public boolean bot() { return offset == 0; }
     public boolean eot() { return offset == text.length(); }
     public boolean startsWith(char c) { return offset < text.length() && text.charAt(offset) == c; }
     public boolean startsWith(String prefix) { return text.startsWith(prefix, offset); }
@@ -90,30 +91,68 @@ public class Cursor {
         }
     }
 
-    /** Returns true if at least one whitespace was skipped. */
+    public boolean skipBackwards(String substring) {
+        int newOffset = offset - substring.length();
+        if (newOffset < 0) return false;
+        if (text.startsWith(substring, newOffset)) {
+            offset = newOffset;
+            return true;
+        } else {
+            return false;
+        }
+    }
+
+    /** If the current char is a whitespace, skip it and return true. */
     public boolean skipWhitespace() {
         if (!eot() && Character.isWhitespace(getChar())) {
-            do {
-                ++offset;
-            } while (!eot() && Character.isWhitespace(getChar()));
-
+            ++offset;
             return true;
+        } else {
+            return false;
         }
+    }
 
-        return false;
+    /** Returns true if at least one whitespace was skipped. */
+    public boolean skipWhitespaces() {
+        if (skipWhitespace()) {
+            while (skipWhitespace())
+                ++offset;
+            return true;
+        } else {
+            return false;
+        }
     }
 
-    /** Advance to the next char (if any) and return eot(). */
+    /** Return false if eot(), otherwise advance to the next char and return true. */
     public boolean increment() {
-        if (eot()) return true;
+        if (eot()) return false;
         ++offset;
-        return eot();
+        return true;
+    }
+
+    /** Return false if bot(), otherwise retreat to the previous char and return true. */
+    public boolean decrement() {
+        if (bot()) return false;
+        --offset;
+        return true;
     }
 
-    /** Advance {@code distance} chars and return {@link #eot()}. */
+    /**
+     * Advance {@code distance} chars until bot() or eot() is reached (distance may be negative),
+     * and return true if this cursor moved the full distance.
+     */
     public boolean advance(int distance) {
-        this.offset = Math.max(0, Math.min(this.offset + distance, text.length()));
-        return eot();
+        int newOffset = offset + distance;
+        if (newOffset < 0) {
+            this.offset = 0;
+            return false;
+        } else if (newOffset > text.length()) {
+            this.offset = text.length();
+            return false;
+        } else {
+            this.offset = newOffset;
+            return true;
+        }
     }
 
     /** Advance pointer until start of needle is found (and return true), or EOT is reached (and return false). */
@@ -128,6 +167,18 @@ public class Cursor {
         }
     }
 
+    /** Advance pointer until start of needle is found (and return true), or EOT is reached (and return false). */
+    public boolean advanceTo(char needle) {
+        int index = text.indexOf(needle, offset);
+        if (index == -1) {
+            offset = text.length();
+            return false; // and eot() is true
+        } else {
+            offset = index;
+            return true; // and eot() is false
+        }
+    }
+
     /** Advance pointer past needle (and return true), or to EOT (and return false). */
     public boolean advancePast(String needle) {
         if (advanceTo(needle)) {
@@ -138,6 +189,16 @@ public class Cursor {
         }
     }
 
+    /** Advance pointer past needle (and return true), or to EOT (and return false). */
+    public boolean advancePast(char needle) {
+        if (advanceTo(needle)) {
+            ++offset;
+            return true; // and eot() may or may not be true
+        } else {
+            return false; // and eot() is true
+        }
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFileTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFileTest.java
index dd1a1abb6bc..be73caf9d1d 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFileTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/template/TemplateFileTest.java
@@ -67,7 +67,38 @@ class TemplateFileTest {
                      form.render());
     }
 
+    @Test
+    void verifyNewlineRemoval() {
+        Form form = makeForm("a%{list a}\n" +
+                             "b%{end}\n" +
+                             "c%{list c|}\n" +
+                             "d%{end|}\n" +
+                             "e\n");
+        form.add("a");
+        form.add("c");
+
+        assertEquals("a\n" +
+                     "b\n" +
+                     "cde\n",
+                     form.render());
+    }
+
+    @Test
+    void verifyComment() {
+        assertEquals("first\n" +
+                     "second\n" +
+                     "third and still third\n",
+                     makeForm("first\n" +
+                              "second%{#} rest of line is ignored\n" +
+                              "third%{#|} this line continues on the next\n" +
+                              " and still third\n").render());
+    }
+
     private Form getForm(String filename) {
         return TemplateFile.read(Path.of("src/test/resources/" + filename)).instantiate();
     }
+
+    private Form makeForm(String templateText) {
+        return Template.from(templateText).instantiate();
+    }
 }
\ No newline at end of file
diff --git a/node-admin/src/test/resources/template1.tmp b/node-admin/src/test/resources/template1.tmp
index a020dbb0739..62b629520bf 100644
--- a/node-admin/src/test/resources/template1.tmp
+++ b/node-admin/src/test/resources/template1.tmp
@@ -1,10 +1,10 @@
 variable section '%{=varname}'
-%{list listname}
+%{list listname|}
 same variable section '%{=varname}'
 different variable section '%{=varname2}'
-%{list innerlistname}
+%{list innerlistname|}
 inner list text
-%{end}
+%{end|}
 between ends
-%{end}
+%{end|}
 end of text
diff --git a/node-admin/src/test/resources/template2.tmp b/node-admin/src/test/resources/template2.tmp
index d36cb4a4a48..4a6be865d18 100644
--- a/node-admin/src/test/resources/template2.tmp
+++ b/node-admin/src/test/resources/template2.tmp
@@ -1,4 +1,4 @@
-%{list listA}body A
-%{list listB}body B
-%{end}
-%{end}
+%{list listA|}body A
+%{list listB|}body B
+%{end|}
+%{end|}
diff --git a/node-admin/src/test/resources/template3.tmp b/node-admin/src/test/resources/template3.tmp
index 27566e72a9d..372dfa62193 100644
--- a/node-admin/src/test/resources/template3.tmp
+++ b/node-admin/src/test/resources/template3.tmp
@@ -1,6 +1,6 @@
 %{=varname}
 %{=varname}
-%{list l}
+%{list l|}
 inner %{=varname}
 %{=innerVarSetAtTop}
-%{end}
+%{end|}