fix(pipelinetemplate): Bubbling up root cause errors (#1271)

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/PipelineTemplatePipelinePreprocessor.java

@@ -70,19 +70,22 @@ public PipelineTemplatePipelinePreprocessor(ObjectMapper pipelineTemplateObjectM
 
   @Override
   public Map<String, Object> process(Map<String, Object> pipeline) {
+    Errors errors;
     try {
       return processInternal(pipeline);
     } catch (TemplateLoaderException e) {
-      return new Errors().addError(new Error().withMessage("failed loading template").withCause(e.getMessage())).toResponse();
+      errors = new Errors().add(new Error().withMessage("failed loading template").withCause(e.getMessage()));
     } catch (TemplateRenderException e) {
-      return new Errors().addError(
-        e.getError() != null ? e.getError() : new Error().withMessage("failed rendering handlebars template").withCause(e.getMessage())
-      ).toResponse();
+      errors = e.getErrors();
+      if (!errors.hasErrors(true)) {
+        errors.add(new Error().withMessage("failed rendering template expression").withCause(e.getMessage()));
+      }
     } catch (IllegalTemplateConfigurationException e) {
-      return new Errors().addError(
+      errors = new Errors().add(
         e.getError() != null ? e.getError() : new Error().withMessage("malformed template configuration").withCause(e.getMessage())
-      ).toResponse();
+      );
     }
+    return errors.toResponse();
   }
 
   private Map<String, Object> processInternal(Map<String, Object> pipeline) {

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/exceptions/IllegalTemplateConfigurationException.java

@@ -26,6 +26,7 @@ public IllegalTemplateConfigurationException(String message) {
   }
 
   public IllegalTemplateConfigurationException(Error error) {
+    this(error.getMessage());
     this.error = error;
   }
 

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/exceptions/InvalidPipelineTemplateException.java

@@ -22,10 +22,6 @@ public class InvalidPipelineTemplateException extends RuntimeException {
 
   private List<Map<String, Object>> errors;
 
-  public InvalidPipelineTemplateException(List<Map<String, Object>> errors) {
-    this.errors = errors;
-  }
-
   public InvalidPipelineTemplateException(String message, List<Map<String, Object>> errors) {
     super(message);
     this.errors = errors;

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/exceptions/TemplateRenderException.java

@@ -15,14 +15,42 @@
  */
 package com.netflix.spinnaker.orca.pipelinetemplate.exceptions;
 
+import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 
 public class TemplateRenderException extends RuntimeException {
 
-  private Error error;
+  public static TemplateRenderException fromError(Error error) {
+    return new TemplateRenderException(error.getMessage(), null, error);
+  }
+
+  public static TemplateRenderException fromError(Error error, Throwable cause) {
+    TemplateRenderException e = new TemplateRenderException(error.getMessage(), cause, error);
+
+    if (cause instanceof TemplateRenderException) {
+      error.withNested(((TemplateRenderException) cause).getErrors());
+    } else if (error.getCause() == null) {
+      error.withCause(cause.getMessage());
+    }
+
+    return e;
+  }
+
+  private Errors errors = new Errors();
+
+  public TemplateRenderException(String message, Throwable cause, Errors errors) {
+    this(message, cause);
+    this.errors = errors;
+  }
+
+  private TemplateRenderException(String message, Throwable cause, Error error) {
+    this(message, cause);
+    this.errors.add(error);
+  }
 
   public TemplateRenderException(Error error) {
-    this.error = error;
+    this(error.getMessage());
+    this.errors.add(error);
   }
 
   public TemplateRenderException(String message) {
@@ -33,7 +61,7 @@ public TemplateRenderException(String message, Throwable cause) {
     super(message, cause);
   }
 
-  public Error getError() {
-    return error;
+  public Errors getErrors() {
+    return errors;
   }
 }

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/RenderTransform.java

@@ -29,6 +29,7 @@
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render.RenderUtil;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render.Renderer;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
+import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 
 import java.util.List;
 import java.util.Map;
@@ -83,22 +84,25 @@ private void renderStages(List<StageDefinition> stages, RenderContext context, S
     }
 
     for (StageDefinition stage : stages) {
+      context.setLocation(String.format("%s:stages.%s", locationNamespace, stage.getId()));
+
       Object rendered;
       try {
         rendered = RenderUtil.deepRender(renderer, stage.getConfig(), context);
       } catch (TemplateRenderException e) {
-        throw new TemplateRenderException(new Errors.Error()
-          .withMessage("Failed rendering stage")
-          .withCause(e.getMessage())
-          .withLocation(String.format("%s:stages.%s", locationNamespace, stage.getId()))
+        throw TemplateRenderException.fromError(
+          new Error()
+            .withMessage("Failed rendering stage")
+            .withLocation(context.getLocation()),
+          e
         );
       }
 
       if (!(rendered instanceof Map)) {
         throw new IllegalTemplateConfigurationException(new Errors.Error()
           .withMessage("A stage's rendered config must be a map")
           .withCause("Received type " + rendered.getClass().toString())
-          .withLocation(String.format("%s:stages.%s", locationNamespace, stage.getId()))
+          .withLocation(context.getLocation())
         );
       }
       stage.setConfig((Map<String, Object>) rendered);
@@ -110,13 +114,13 @@ private void renderStages(List<StageDefinition> stages, RenderContext context, S
 
   private String renderStageProperty(String input, RenderContext context, String location) {
     try {
-      String rendered = (String) RenderUtil.deepRender(renderer, input, context);
-      return rendered;
+      return (String) RenderUtil.deepRender(renderer, input, context);
     } catch (TemplateRenderException e) {
-      throw new TemplateRenderException(new Errors.Error()
-        .withMessage("Failed rendering stage property")
-        .withCause(e.getMessage())
-        .withLocation(location)
+      throw TemplateRenderException.fromError(
+        new Error()
+          .withMessage("Failed rendering stage property")
+          .withLocation(location),
+        e
       );
     }
   }

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/StageInheritanceControlTransform.java

@@ -17,7 +17,7 @@
 
 import com.jayway.jsonpath.DocumentContext;
 import com.jayway.jsonpath.JsonPath;
-import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.TemplateRenderException;
+import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.IllegalTemplateConfigurationException;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.MapMerge;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.PipelineTemplateVisitor;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate;
@@ -46,27 +46,27 @@ private static void merge(DocumentContext dc, Rule rule) {
     Object oldValue = dc.read(rule.getPath());
     if (oldValue instanceof Collection) {
       if (!(rule.getValue() instanceof Collection)) {
-        throw new TemplateRenderException("cannot merge non-collection value into collection");
+        throw new IllegalTemplateConfigurationException("cannot merge non-collection value into collection");
       }
       ((Collection) oldValue).addAll((Collection) rule.getValue());
       dc.set(rule.getPath(), oldValue);
     } else if (oldValue instanceof Map) {
       if (!(rule.getValue() instanceof Map)) {
-        throw new TemplateRenderException("cannot merge non-map value into map");
+        throw new IllegalTemplateConfigurationException("cannot merge non-map value into map");
       }
       Map<String, Object> merged = MapMerge.merge((Map<String, Object>) oldValue, (Map<String, Object>) rule.getValue());
       dc.set(rule.getPath(), merged);
     } else {
-      throw new TemplateRenderException("merge inheritance control must be given a list or map value: " + rule.getPath());
+      throw new IllegalTemplateConfigurationException("merge inheritance control must be given a list or map value: " + rule.getPath());
     }
   }
 
   private static void replace(DocumentContext dc, Rule rule) {
     Object oldValue = dc.read(rule.getPath());
     if (oldValue instanceof Collection && !(rule.getValue() instanceof Collection)) {
-      throw new TemplateRenderException("cannot replace collection value with non-collection value");
+      throw new IllegalTemplateConfigurationException("cannot replace collection value with non-collection value");
     } else if (oldValue instanceof Map && !(rule.getValue() instanceof Map)) {
-      throw new TemplateRenderException("cannot replace map value with non-map value");
+      throw new IllegalTemplateConfigurationException("cannot replace map value with non-map value");
     }
     dc.set(rule.getPath(), rule.getValue());
   }

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/JinjaRenderer.java

@@ -18,15 +18,24 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.hubspot.jinjava.Jinjava;
 import com.hubspot.jinjava.JinjavaConfig;
+import com.hubspot.jinjava.interpret.FatalTemplateErrorsException;
 import com.hubspot.jinjava.interpret.InterpretException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
+import com.hubspot.jinjava.interpret.TemplateError;
+import com.hubspot.jinjava.interpret.TemplateError.ErrorItem;
+import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
+import com.hubspot.jinjava.interpret.TemplateError.ErrorType;
+import com.hubspot.jinjava.interpret.errorcategory.BasicTemplateErrorCategory;
 import com.hubspot.jinjava.loader.ResourceLocator;
 import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.TemplateRenderException;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render.filters.FriggaFilter;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render.tags.ModuleTag;
+import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
+import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Severity;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.yaml.snakeyaml.parser.ParserException;
 
 import java.io.IOException;
 import java.nio.charset.Charset;
@@ -60,12 +69,18 @@ public String render(String template, RenderContext context) {
     String rendered;
     try {
       rendered = jinja.render(template, context.getVariables());
+    } catch (FatalTemplateErrorsException fte) {
+      log.error("Failed rendering jinja template", fte);
+      throw new TemplateRenderException("failed rendering jinja template", fte, unwrapJinjaTemplateErrorException(fte, context.getLocation()));
     } catch (InterpretException e) {
+      log.warn("Caught supertype InterpretException instead of " + e.getClass().getSimpleName());
       log.error("Failed rendering jinja template", e);
-      throw new TemplateRenderException(new Error()
-        .withMessage("failed rendering jinja template")
-        .withCause(e.getMessage())
-        .withLocation(context.getLocation())
+
+      throw TemplateRenderException.fromError(
+        new Error()
+          .withMessage("failed rendering jinja template")
+          .withLocation(context.getLocation()),
+        e
       );
     }
 
@@ -80,11 +95,19 @@ public String render(String template, RenderContext context) {
 
   @Override
   public Object renderGraph(String template, RenderContext context) {
-    // TODO rz - Need to catch exceptions out of this value converter & give more detailed information if it's a nested
-    // value. For example, if iterating over module output, the line number reported by yamlsnake will be for the final
-    // template, not the input, so it's difficult to correlate what actually is broken.
     String renderedValue = render(template, context);
-    return renderedValueConverter.convertRenderedValue(renderedValue);
+    try {
+      return renderedValueConverter.convertRenderedValue(renderedValue);
+    } catch (ParserException e) {
+      throw TemplateRenderException.fromError(
+        new Error()
+          .withMessage("Failed converting rendered value to YAML")
+          .withLocation(context.getLocation())
+          .withDetail("source", template)
+          .withCause(e.getMessage()),
+        e
+      );
+    }
   }
 
   private static class NoopResourceLocator implements ResourceLocator {
@@ -93,4 +116,46 @@ public String getString(String fullName, Charset encoding, JinjavaInterpreter in
       return null;
     }
   }
+
+  private static Errors unwrapJinjaTemplateErrorException(FatalTemplateErrorsException e, String location) {
+    Errors errors = new Errors();
+    for (TemplateError templateError : e.getErrors()) {
+      if (templateError.getException() instanceof TemplateRenderException) {
+        TemplateRenderException tre = (TemplateRenderException) templateError.getException();
+        errors.addAll(tre.getErrors());
+        continue;
+      }
+
+      Error error = new Error()
+        .withMessage(templateError.getMessage())
+        .withSeverity(templateError.getSeverity().equals(ErrorType.FATAL) ? Severity.FATAL : Severity.WARN)
+        .withLocation(location);
+      errors.add(error);
+
+      if (templateError.getReason() != ErrorReason.OTHER) {
+        error.withDetail("reason", templateError.getReason().name());
+      }
+      if (templateError.getItem() != ErrorItem.OTHER) {
+        error.withDetail("item", templateError.getItem().name());
+      }
+      if (templateError.getFieldName() != null) {
+        error.withDetail("fieldName", templateError.getFieldName());
+      }
+      if (templateError.getLineno() > 0) {
+        error.withDetail("lineno", Integer.toString(templateError.getLineno()));
+      }
+      if (templateError.getCategory() != BasicTemplateErrorCategory.UNKNOWN) {
+        error.withDetail("category", templateError.getCategory().toString());
+        templateError.getCategoryErrors().forEach((s, s2) -> error.withDetail("categoryError:" + s, s2));
+      }
+
+      // This gross little bit is necessary for bubbling up any errors that
+      // might've occurred while rendering a nested module.
+      if (templateError.getException() != null && templateError.getException().getCause() instanceof TemplateRenderException) {
+        error.withNested(((TemplateRenderException) templateError.getException().getCause()).getErrors());
+      }
+    }
+    return errors;
+  }
+
 }

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/RenderUtil.java

@@ -16,7 +16,7 @@
 package com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render;
 
 import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.TemplateRenderException;
-import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
+import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -36,10 +36,11 @@ public static Object deepRender(Renderer renderer, Object obj, RenderContext con
   @SuppressWarnings("unchecked")
   private static Object deepRender(Renderer renderer, Object obj, RenderContext context, int depth) {
     if (depth >= MAX_RENDER_DEPTH) {
-      throw new TemplateRenderException(new Errors.Error()
-        .withMessage(String.format("Cannot exceed %d levels of depth in handlebars rendering", MAX_RENDER_DEPTH))
-        .withSuggestion("Try breaking up your templates into smaller, more reusable chunks via modules")
-        .withLocation(context.getLocation())
+      throw TemplateRenderException.fromError(
+        new Error()
+          .withMessage(String.format("Cannot exceed %d levels of depth in handlebars rendering", MAX_RENDER_DEPTH))
+          .withSuggestion("Try breaking up your templates into smaller, more reusable chunks via modules")
+          .withLocation(context.getLocation())
       );
     }
 
@@ -73,11 +74,12 @@ private static Object deepRender(Renderer renderer, Object obj, RenderContext co
       return objMap;
     }
 
-    throw new TemplateRenderException(new Errors.Error()
-      .withMessage("Unknown rendered type, cannot continue render of template")
-      .withCause("Unhandled type: " + obj.getClass().getSimpleName())
-      .withSuggestion("Expected types: primitives, collections and maps")
-      .withLocation(context.getLocation())
+    throw TemplateRenderException.fromError(
+      new Error()
+        .withMessage("Unknown rendered type, cannot continue")
+        .withCause("Unhandled type: " + obj.getClass().getSimpleName())
+        .withSuggestion("Expected types: primitives, collections and maps")
+        .withLocation(context.getLocation())
     );
   }