Fixed multiple variable sections

12 files changed, 138 insertions, 52 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Form.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Form.java
index 2ac0e70ff7f..1bbb9ff3639 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Form.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Form.java
@@ -5,6 +5,7 @@ import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
 
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 /**
  * A form is an instance of a template to be filled, e.g. values set for variable sections, etc.
@@ -13,25 +14,24 @@ import java.util.Map;
  * @author hakonhall
  */
 public class Form extends Section {
+    private final Form parent;
     private final List<Section> sections;
 
-    private final Map<String, VariableSection> variables;
+    private final Map<String, String> variables;
     private final Map<String, ListSection> lists;
 
-    Form(CursorRange range, List<Section> sections, Map<String, VariableSection> variables, Map<String, ListSection> lists) {
+    Form(Form parent, CursorRange range, List<Section> sections, Map<String, String> variables,
+         Map<String, ListSection> lists) {
         super(range);
+        this.parent = parent;
         this.sections = List.copyOf(sections);
-        this.variables = Map.copyOf(variables);
+        this.variables = variables; // Mutable and referenced by the variable sections
         this.lists = Map.copyOf(lists);
     }
 
     /** Set the value of a variable expression, e.g. %{=color}. */
     public Form set(String name, String value) {
-        var section = variables.get(name);
-        if (section == null) {
-            throw new NoSuchNameTemplateException(this, name);
-        }
-        section.set(value);
+        variables.put(name, value);
         return this;
     }
 
@@ -79,4 +79,13 @@ public class Form extends Section {
     public void appendTo(StringBuilder buffer) {
         sections.forEach(section -> section.appendTo(buffer));
     }
+
+    Optional<String> getVariableValue(String name) {
+        String value = variables.get(name);
+        if (value != null) return Optional.of(value);
+        if (parent != null) {
+            return parent.getVariableValue(name);
+        }
+        return Optional.empty();
+    }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/FormBuilder.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/FormBuilder.java
index 965d0b6496a..179af5028b4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/FormBuilder.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/FormBuilder.java
@@ -15,17 +15,19 @@ import java.util.function.Consumer;
  */
 class FormBuilder {
     private final List<Section> sections = new ArrayList<>();
-    private final Map<String, VariableSection> variables = new HashMap<>();
+    private final Map<String, String> variables = new HashMap<>();
     private final Map<String, ListSection> lists = new HashMap<>();
+    private final Form parent;
     private final CursorRange range;
 
-    static Form build(CursorRange range, List<Consumer<FormBuilder>> sections) {
-        var builder = new FormBuilder(range);
+    static Form build(Form parent, CursorRange range, List<Consumer<FormBuilder>> sections) {
+        var builder = new FormBuilder(parent, range);
         sections.forEach(section -> section.accept(builder));
         return builder.build();
     }
 
-    private FormBuilder(CursorRange range) {
+    private FormBuilder(Form parent, CursorRange range) {
+        this.parent = parent;
         this.range = new CursorRange(range);
     }
 
@@ -35,10 +37,8 @@ class FormBuilder {
     }
 
     FormBuilder addVariableSection(CursorRange range, String name, Cursor nameOffset) {
-        checkNameIsAvailable(name, range);
         var section = new VariableSection(range, name, nameOffset);
         sections.add(section);
-        variables.put(section.name(), section);
         return this;
     }
 
@@ -51,12 +51,18 @@ class FormBuilder {
     }
 
     private Form build() {
-        return new Form(range, sections, variables, lists);
+        var form = new Form(parent, range, sections, variables, lists);
+        sections.forEach(section -> section.setForm(form));
+        return form;
     }
 
     private void checkNameIsAvailable(String name, CursorRange range) {
-        if (variables.containsKey(name) || lists.containsKey(name)) {
+        if (nameIsDefined(name)) {
             throw new NameAlreadyExistsTemplateException(name, range);
         }
     }
+
+    private boolean nameIsDefined(String name) {
+        return variables.containsKey(name) || lists.containsKey(name);
+    }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/ListSection.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/ListSection.java
index 4914bdd7de8..7971bb002b6 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/ListSection.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/ListSection.java
@@ -26,7 +26,7 @@ class ListSection extends Section {
     String name() { return name; }
 
     Form add() {
-        var form = body.instantiate();
+        var form = body.instantiate(form());
         elements.add(form);
         return form;
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/NameAlreadyExistsTemplateException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/NameAlreadyExistsTemplateException.java
index cea53f39f25..dd92af14609 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/NameAlreadyExistsTemplateException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/NameAlreadyExistsTemplateException.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin.task.util.template;
 
+import com.yahoo.vespa.hosted.node.admin.task.util.text.Cursor;
 import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
 
 /**
@@ -10,4 +11,12 @@ public class NameAlreadyExistsTemplateException extends TemplateException {
     public NameAlreadyExistsTemplateException(String name, CursorRange range) {
         super("Name '" + name + "' already exists in the " + describeSection(range));
     }
+
+    public NameAlreadyExistsTemplateException(String name, Cursor firstNameLocation,
+                                              Cursor secondNameLocation) {
+        super("Section named '" + name + "' at " +
+              firstNameLocation.calculateLocation().lineAndColumnText() +
+              " conflicts with earlier section with the same name at " +
+              secondNameLocation.calculateLocation().lineAndColumnText());
+    }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Section.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Section.java
index c0d9e651484..f12dfb8d8b7 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Section.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Section.java
@@ -11,11 +11,17 @@ import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
  */
 abstract class Section {
     private final CursorRange range;
+    private Form form;
 
     protected Section(CursorRange range) {
         this.range = range;
     }
 
+    void setForm(Form form) { this.form = form; }
+
+    /** Guaranteed to return non-null after FormBuilder::build() returns. */
+    protected Form form() { return form; }
+
     protected CursorRange range() { return range; }
 
     abstract void appendTo(StringBuilder buffer);
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 3931202c24f..4fc49afea02 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
@@ -5,7 +5,7 @@ import com.yahoo.vespa.hosted.node.admin.task.util.text.Cursor;
 import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
 
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.List;
 import java.util.function.Consumer;
 
@@ -25,8 +25,13 @@ import java.util.function.Consumer;
  *
  * <p>Any newline (\n) following a non-variable directive is removed.</p>
  *
- * <p><b>Instantiate</b> the template 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>
+ * <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>
+ *
+ * <p>A form (like a template) has direct sections, and indirect sections in the body of direct list
+ * sections (recursively). The variables that can be set for a form, are the variables defined in
+ * either direct or indirect variable sections.  Forms can only be added to direct list section in
+ * a form ({@link Form#add(String)}).</p>
  *
  * @see Form
  * @see TemplateParser
@@ -38,15 +43,18 @@ public class Template {
     private final Cursor end;
 
     private final List<Consumer<FormBuilder>> sections = new ArrayList<>();
-    private final HashSet<String> names = new HashSet<>();
+    /** The value contains the location of the name of a sample variable section (with that name). */
+    private final HashMap<String, Cursor> names = new HashMap<>();
 
     Template(Cursor start) {
         this.start = new Cursor(start);
         this.end = new Cursor(start);
     }
 
-    public Form instantiate() {
-        return FormBuilder.build(range(), sections);
+    public Form instantiate() { return instantiate(null); }
+
+    Form instantiate(Form parent) {
+        return FormBuilder.build(parent, range(), sections);
     }
 
     void appendLiteralSection(Cursor end) {
@@ -56,13 +64,12 @@ public class Template {
 
     void appendVariableSection(String name, Cursor nameOffset, Cursor end) {
         CursorRange range = verifyAndUpdateEnd(end);
-        verifyAndAddNewName(name, nameOffset);
         sections.add(formBuilder -> formBuilder.addVariableSection(range, name, nameOffset));
     }
 
     void appendListSection(String name, Cursor nameCursor, Cursor end, Template body) {
         CursorRange range = verifyAndUpdateEnd(end);
-        verifyAndAddNewName(name, nameCursor);
+        verifyNewName(name, nameCursor);
         sections.add(formBuilder -> formBuilder.addListSection(range, name, body));
     }
 
@@ -74,12 +81,10 @@ public class Template {
         return range;
     }
 
-    private String verifyAndAddNewName(String name, Cursor nameOffset) {
-        if (!names.add(name)) {
-            throw new IllegalArgumentException("'" + name + "' at " +
-                                               nameOffset.calculateLocation().lineAndColumnText() +
-                                               " has already been defined");
+    private void verifyNewName(String name, Cursor cursor) {
+        Cursor alreadyDefinedNameCursor = names.put(name, cursor);
+        if (alreadyDefinedNameCursor != null) {
+            throw new NameAlreadyExistsTemplateException(name, alreadyDefinedNameCursor, cursor);
         }
-        return name;
     }
 }
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 afdc3efc553..40e2c051f6a 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
@@ -9,9 +9,16 @@ package com.yahoo.vespa.hosted.node.admin.task.util.template;
 public class TemplateDescriptor {
     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. */
     public TemplateDescriptor setDelimiters(String startDelimiter, String endDelimiter) {
         this.startDelimiter = Token.verifyDelimiter(startDelimiter);
@@ -19,6 +26,13 @@ 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; }
 }
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 c1dfc4a775b..efd63e1ab37 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
@@ -19,7 +19,7 @@ public class TemplateParser {
     private final FormEndsIn formEndsIn;
 
     public static Template parse(TemplateDescriptor descriptor, String text) {
-        return parse(descriptor, new Cursor(text), FormEndsIn.EOT).template();
+        return parse(new TemplateDescriptor(descriptor), new Cursor(text), FormEndsIn.EOT).template;
     }
 
     private static TemplateParser parse(TemplateDescriptor descriptor, Cursor start, FormEndsIn formEndsIn) {
@@ -59,7 +59,7 @@ public class TemplateParser {
         current.skip(descriptor.startDelimiter());
 
         if (current.skip('=')) {
-            parseExpression();
+            parseVariableSection();
         } else {
             var startOfType = new Cursor(current);
             String type = skipId().orElseThrow(() -> new BadTemplateException(current, "Missing section name"));
@@ -68,7 +68,7 @@ public class TemplateParser {
                 case "end":
                     if (formEndsIn == FormEndsIn.EOT)
                         throw new BadTemplateException(startOfType, "Extraneous 'end'");
-                    parseEndAttribute();
+                    parseEndDirective();
                     return false;
                 case "list":
                     parseListSection();
@@ -81,14 +81,14 @@ public class TemplateParser {
         return !current.eot();
     }
 
-    private void parseExpression() {
+    private void parseVariableSection() {
         var nameStart = new Cursor(current);
         String name = parseId();
         parseEndDelimiter(false);
         template.appendVariableSection(name, nameStart, current);
     }
 
-    private void parseEndAttribute() {
+    private void parseEndDirective() {
         parseEndDelimiter(true);
     }
 
@@ -116,11 +116,11 @@ public class TemplateParser {
 
     private Optional<String> skipId() { return Token.skipId(current); }
 
-    private void parseEndDelimiter(boolean removeFollowingNewline) {
-        if (!current.skip(descriptor.endDelimiter())) {
+    private void parseEndDelimiter(boolean newlineMayBeRemoved) {
+        if (!current.skip(descriptor.endDelimiter()))
             throw new BadTemplateException(current, "Expected section end (" + descriptor.endDelimiter() + ")");
-        }
 
-        if (removeFollowingNewline) current.skip('\n');
+        if (descriptor.removeNewlineAfterSection() && newlineMayBeRemoved)
+            current.skip('\n');
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Variable.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Variable.java
new file mode 100644
index 00000000000..6b1ed3d7c35
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/Variable.java
@@ -0,0 +1,24 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.node.admin.task.util.template;
+
+import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
+
+import java.util.Optional;
+
+/**
+ * TODO: remove
+ * @author hakonhall
+ */
+class Variable {
+    private final String name;
+    private final CursorRange firstVariableReferenceRange;
+    private Optional<String> value = Optional.empty();
+
+    Variable(String name, CursorRange firstVariableReferenceRange) {
+        this.name = name;
+        this.firstVariableReferenceRange = firstVariableReferenceRange;
+    }
+
+    void set(String value) { this.value = Optional.of(value); }
+    Optional<String> get() { return value; }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/VariableSection.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/VariableSection.java
index e5745483b31..034ac632a50 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/VariableSection.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/template/VariableSection.java
@@ -4,8 +4,6 @@ package com.yahoo.vespa.hosted.node.admin.task.util.template;
 import com.yahoo.vespa.hosted.node.admin.task.util.text.Cursor;
 import com.yahoo.vespa.hosted.node.admin.task.util.text.CursorRange;
 
-import java.util.Objects;
-
 /**
  * Represents a template variable section
  *
@@ -16,8 +14,6 @@ class VariableSection extends Section {
     private final String name;
     private final Cursor nameOffset;
 
-    private String value = null;
-
     VariableSection(CursorRange range, String name, Cursor nameOffset) {
         super(range);
         this.name = name;
@@ -26,16 +22,10 @@ class VariableSection extends Section {
 
     String name() { return name; }
 
-    void set(String value) {
-        this.value = Objects.requireNonNull(value);
-    }
-
     @Override
     void appendTo(StringBuilder buffer) {
-        if (value == null) {
-            throw new TemplateNameNotSetException(this, name, nameOffset);
-        }
-
+        String value = form().getVariableValue(name)
+                             .orElseThrow(() -> new TemplateNameNotSetException(this, name, nameOffset));
         buffer.append(value);
     }
 }
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 60330ecfb39..dd1a1abb6bc 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
@@ -50,6 +50,23 @@ class TemplateFileTest {
                      form.render());
     }
 
+    @Test
+    void verifyVariableReferences() {
+        Form form = getForm("template3.tmp");
+        form.set("varname", "varvalue")
+            .set("innerVarSetAtTop", "val2");
+        form.add("l");
+        form.add("l")
+            .set("varname", "varvalue2");
+        assertEquals("varvalue\n" +
+                     "varvalue\n" +
+                     "inner varvalue\n" +
+                     "val2\n" +
+                     "inner varvalue2\n" +
+                     "val2\n",
+                     form.render());
+    }
+
     private Form getForm(String filename) {
         return TemplateFile.read(Path.of("src/test/resources/" + filename)).instantiate();
     }
diff --git a/node-admin/src/test/resources/template3.tmp b/node-admin/src/test/resources/template3.tmp
new file mode 100644
index 00000000000..27566e72a9d
--- /dev/null
+++ b/node-admin/src/test/resources/template3.tmp
@@ -0,0 +1,6 @@
+%{=varname}
+%{=varname}
+%{list l}
+inner %{=varname}
+%{=innerVarSetAtTop}
+%{end}