Moved condition errors to their result

Until now if a condition failed to execute (for whatever reason), an exception would be thrown and the watch would just abort. The problem with that approach is while the error message would have been logged in the watch record message, the result of the condition would have been lost.

This commit moves the condition execution error to the condition result (just like we have it today with actions and inputs).

- A `status` field was added to the condition result (can either be `success` or `failure`)
- A `reason` field is added to the condition result in case its status is `failure`
- If the condition result status is `failure`, the watch execution is aborted

Updated the rest APIs to verify the status & type of both the `input` and `condition` results on execution.

Original commit: elastic/x-pack-elasticsearch@dddca03ff5

rest-api-spec/test/execute_watch/10_basic.yaml

@@ -77,8 +77,12 @@
   - match: { "watch_record.trigger_event.type": "manual" }
   - match: { "watch_record.trigger_event.triggered_time": "2015-05-05T20:58:02.443Z" }
   - match: { "watch_record.trigger_event.manual.schedule.scheduled_time": "2015-05-05T20:58:02.443Z" }
-  - match: { "watch_record.result.condition.met": true }
+  - match: { "watch_record.result.input.type": "simple" }
+  - match: { "watch_record.result.input.status": "success" }
   - match: { "watch_record.result.input.payload.foo": "bar" }
+  - match: { "watch_record.result.condition.type": "always" }
+  - match: { "watch_record.result.condition.status": "success" }
+  - match: { "watch_record.result.condition.met": true }
   - match: { "watch_record.result.actions.0.id" : "email_admin" }
   - match: { "watch_record.result.actions.0.status" : "simulated" }
   - match: { "watch_record.result.actions.0.type" : "email" }

rest-api-spec/test/execute_watch/20_minimal_body.yaml

@@ -36,6 +36,11 @@
 
   - match: { "watch_record.watch_id": "my_logging_watch" }
   - match: { "watch_record.state": "executed" }
+  - match: { "watch_record.result.input.type": "simple" }
+  - match: { "watch_record.result.input.status": "success" }
+  - match: { "watch_record.result.input.payload.count": 1 }
+  - match: { "watch_record.result.condition.type": "script" }
+  - match: { "watch_record.result.condition.status": "success" }
   - match: { "watch_record.result.condition.met": true }
   - match: { "watch_record.result.actions.0.id" : "logging" }
   - match: { "watch_record.result.actions.0.type" : "logging" }

src/main/java/org/elasticsearch/watcher/condition/Condition.java

@@ -5,11 +5,13 @@
  */
 package org.elasticsearch.watcher.condition;
 
+import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Locale;
 
 /**
  *
@@ -20,25 +22,64 @@ public interface Condition extends ToXContent {
 
     abstract class Result implements ToXContent {
 
+        public enum Status {
+            SUCCESS, FAILURE
+        }
+
         protected final String type;
+        protected final Status status;
+        private final String reason;
         protected final boolean met;
 
         public Result(String type, boolean met) {
+            this.status = Status.SUCCESS;
             this.type = type;
             this.met = met;
+            this.reason = null;
+        }
+
+        protected Result(String type, Exception e) {
+            this.status = Status.FAILURE;
+            this.type = type;
+            this.met = false;
+            this.reason = ExceptionsHelper.detailedMessage(e);
         }
 
         public String type() {
             return type;
         }
 
-        public boolean met() { return met; }
+        public Status status() {
+            return status;
+        }
+
+        public boolean met() {
+            assert status == Status.SUCCESS;
+            return met;
+        }
+
+        public String reason() {
+            assert status == Status.FAILURE;
+            return reason;
+        }
 
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             builder.startObject();
             builder.field(Field.TYPE.getPreferredName(), type);
-            builder.field(Field.MET.getPreferredName(), met);
+            builder.field(Field.STATUS.getPreferredName(), status.name().toLowerCase(Locale.ROOT));
+            switch (status) {
+                case SUCCESS:
+                    assert reason == null;
+                    builder.field(Field.MET.getPreferredName(), met);
+                    break;
+                case FAILURE:
+                    assert reason != null && !met;
+                    builder.field(Field.REASON.getPreferredName(), reason);
+                    break;
+                default:
+                    assert false;
+            }
             typeXContent(builder, params);
             return builder.endObject();
         }
@@ -53,6 +94,8 @@ public interface Condition extends ToXContent {
 
     interface Field {
         ParseField TYPE = new ParseField("type");
+        ParseField STATUS = new ParseField("status");
         ParseField MET = new ParseField("met");
+        ParseField REASON = new ParseField("reason");
     }
 }

src/main/java/org/elasticsearch/watcher/condition/ExecutableCondition.java

@@ -39,7 +39,7 @@ public abstract class ExecutableCondition<C extends Condition, R extends Conditi
     /**
      * Executes this condition
      */
-    public abstract R execute(WatchExecutionContext ctx) throws IOException;
+    public abstract R execute(WatchExecutionContext ctx);
 
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {

src/main/java/org/elasticsearch/watcher/condition/always/ExecutableAlwaysCondition.java

@@ -21,7 +21,7 @@ public class ExecutableAlwaysCondition extends ExecutableCondition<AlwaysConditi
     }
 
     @Override
-    public AlwaysCondition.Result execute(WatchExecutionContext ctx) throws IOException {
+    public AlwaysCondition.Result execute(WatchExecutionContext ctx) {
         return AlwaysCondition.Result.INSTANCE;
     }
 

src/main/java/org/elasticsearch/watcher/condition/compare/CompareCondition.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.watcher.condition.compare;
 
+import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.joda.time.DateTime;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -126,19 +127,27 @@ public class CompareCondition implements Condition {
 
     public static class Result extends Condition.Result {
 
-        private final Map<String, Object> resolveValues;
+        private final @Nullable Map<String, Object> resolveValues;
 
         Result(Map<String, Object> resolveValues, boolean met) {
             super(TYPE, met);
             this.resolveValues = resolveValues;
         }
 
+        Result(@Nullable Map<String, Object> resolveValues, Exception e) {
+            super(TYPE, e);
+            this.resolveValues = resolveValues;
+        }
+
         public Map<String, Object> getResolveValues() {
             return resolveValues;
         }
 
         @Override
         protected XContentBuilder typeXContent(XContentBuilder builder, Params params) throws IOException {
+            if (resolveValues == null) {
+                return builder;
+            }
             return builder.startObject(type)
                     .field(Field.RESOLVED_VALUES.getPreferredName(), resolveValues)
                     .endObject();

src/main/java/org/elasticsearch/watcher/condition/compare/ExecutableCompareCondition.java

@@ -7,6 +7,7 @@ package org.elasticsearch.watcher.condition.compare;
 
 import org.elasticsearch.common.joda.time.DateTime;
 import org.elasticsearch.common.logging.ESLogger;
+import org.elasticsearch.watcher.actions.email.DataAttachment;
 import org.elasticsearch.watcher.condition.ExecutableCondition;
 import org.elasticsearch.watcher.execution.WatchExecutionContext;
 import org.elasticsearch.watcher.support.Variables;
@@ -39,10 +40,21 @@ public class ExecutableCompareCondition extends ExecutableCondition<CompareCondi
     }
 
     @Override
-    public CompareCondition.Result execute(WatchExecutionContext ctx) throws IOException {
-        Map<String, Object> model = Variables.createCtxModel(ctx, ctx.payload());
-
+    public CompareCondition.Result execute(WatchExecutionContext ctx) {
         Map<String, Object> resolvedValues = new HashMap<>();
+        try {
+            return doExecute(ctx, resolvedValues);
+        } catch (Exception e) {
+            logger.error("failed to execute [{}] condition for [{}]", CompareCondition.TYPE, ctx.id());
+            if (resolvedValues.isEmpty()) {
+                resolvedValues = null;
+            }
+            return new CompareCondition.Result(resolvedValues, e);
+        }
+    }
+
+    public CompareCondition.Result doExecute(WatchExecutionContext ctx, Map<String, Object> resolvedValues) throws Exception {
+        Map<String, Object> model = Variables.createCtxModel(ctx, ctx.payload());
 
         Object configuredValue = condition.getValue();
 

src/main/java/org/elasticsearch/watcher/condition/never/ExecutableNeverCondition.java

@@ -21,7 +21,7 @@ public class ExecutableNeverCondition extends ExecutableCondition<NeverCondition
     }
 
     @Override
-    public NeverCondition.Result execute(WatchExecutionContext ctx) throws IOException {
+    public NeverCondition.Result execute(WatchExecutionContext ctx) {
         return NeverCondition.Result.INSTANCE;
     }
 

src/main/java/org/elasticsearch/watcher/condition/script/ExecutableScriptCondition.java

@@ -13,7 +13,6 @@ import org.elasticsearch.watcher.execution.WatchExecutionContext;
 import org.elasticsearch.watcher.support.Variables;
 import org.elasticsearch.watcher.support.init.proxy.ScriptServiceProxy;
 
-import java.io.IOException;
 import java.util.Map;
 
 /**
@@ -35,20 +34,25 @@ public class ExecutableScriptCondition extends ExecutableCondition<ScriptConditi
     }
 
     @Override
-    public ScriptCondition.Result execute(WatchExecutionContext ctx) throws IOException {
+    public ScriptCondition.Result execute(WatchExecutionContext ctx) {
         try {
-            Map<String, Object> parameters = Variables.createCtxModel(ctx, ctx.payload());
-            if (condition.script.params() != null && !condition.script.params().isEmpty()) {
-                parameters.putAll(condition.script.params());
-            }
-            ExecutableScript executable = scriptService.executable(compiledScript, parameters);
-            Object value = executable.run();
-            if (value instanceof Boolean) {
-                return (Boolean) value ? ScriptCondition.Result.MET : ScriptCondition.Result.UNMET;
-            }
-            throw new ScriptConditionException("failed to execute [{}] condition for watch [{}]. script [{}] must return a boolean value (true|false) but instead returned [{}]", type(), ctx.watch().id(), condition.script.script(), value);
+            return doExecute(ctx);
         } catch (Exception e) {
-            throw new ScriptConditionException("failed to execute [{}] condition for watch [{}]. script [{}] threw an exception", e, type(), ctx.watch().id(), condition.script.script());
+            logger.error("failed to execute [{}] condition for [{}]", ScriptCondition.TYPE, ctx.id());
+            return new ScriptCondition.Result(e);
         }
     }
+
+    public ScriptCondition.Result doExecute(WatchExecutionContext ctx) throws Exception {
+        Map<String, Object> parameters = Variables.createCtxModel(ctx, ctx.payload());
+        if (condition.script.params() != null && !condition.script.params().isEmpty()) {
+            parameters.putAll(condition.script.params());
+        }
+        ExecutableScript executable = scriptService.executable(compiledScript, parameters);
+        Object value = executable.run();
+        if (value instanceof Boolean) {
+            return (Boolean) value ? ScriptCondition.Result.MET : ScriptCondition.Result.UNMET;
+        }
+        throw new ScriptConditionException("script [{}] must return a boolean value (true|false) but instead returned [{}]", type(), ctx.watch().id(), condition.script.script(), value);
+    }
 }

src/main/java/org/elasticsearch/watcher/condition/script/ScriptCondition.java

@@ -76,6 +76,10 @@ public class ScriptCondition implements Condition {
             super(TYPE, met);
         }
 
+        Result(Exception e) {
+            super(TYPE, e);
+        }
+
         @Override
         protected XContentBuilder typeXContent(XContentBuilder builder, Params params) throws IOException {
             return builder;

src/main/java/org/elasticsearch/watcher/execution/ExecutionService.java

@@ -329,6 +329,9 @@ public class ExecutionService extends AbstractComponent {
             conditionResult = watch.condition().execute(ctx);
             ctx.onConditionResult(conditionResult);
         }
+        if (conditionResult.status() == Condition.Result.Status.FAILURE) {
+            return ctx.abortFailedExecution("failed to execute watch condition");
+        }
 
         if (conditionResult.met()) {
 

src/main/java/org/elasticsearch/watcher/input/Input.java

@@ -27,10 +27,10 @@ public interface Input extends ToXContent {
             SUCCESS, FAILURE
         }
 
-        protected final Status status;
         protected final String type;
-        private final Payload payload;
+        protected final Status status;
         private final String reason;
+        private final Payload payload;
 
         protected Result(String type, Payload payload) {
             this.status = Status.SUCCESS;
@@ -67,8 +67,8 @@ public interface Input extends ToXContent {
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             builder.startObject();
-            builder.field(Field.STATUS.getPreferredName(), status.name().toLowerCase(Locale.ROOT));
             builder.field(Field.TYPE.getPreferredName(), type);
+            builder.field(Field.STATUS.getPreferredName(), status.name().toLowerCase(Locale.ROOT));
             switch (status) {
                 case SUCCESS:
                     assert payload != null;

src/main/resources/watch_history.json

@@ -108,14 +108,14 @@
                   "type" : "string",
                   "index" : "not_analyzed"
                 },
-                "payload" : {
-                  "type" : "object",
-                  "enabled" : false
-                },
                 "status" : {
                   "type" : "string",
                   "index" : "not_analyzed"
                 },
+                "payload" : {
+                  "type" : "object",
+                  "enabled" : false
+                },
                 "search": {
                   "type": "object",
                   "dynamic": true,
@@ -170,6 +170,10 @@
                   "type" : "string",
                   "index" : "not_analyzed"
                 },
+                "status" : {
+                  "type" : "string",
+                  "index" : "not_analyzed"
+                },
                 "met" : {
                   "type" : "boolean"
                 },

src/test/java/org/elasticsearch/watcher/condition/script/ScriptConditionTests.java

@@ -20,6 +20,7 @@ import org.elasticsearch.search.internal.InternalSearchResponse;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.watcher.WatcherException;
+import org.elasticsearch.watcher.condition.Condition;
 import org.elasticsearch.watcher.execution.WatchExecutionContext;
 import org.elasticsearch.watcher.support.Script;
 import org.elasticsearch.watcher.support.init.proxy.ScriptServiceProxy;
@@ -34,7 +35,9 @@ import static org.elasticsearch.common.joda.time.DateTimeZone.UTC;
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.watcher.test.WatcherTestUtils.getScriptServiceProxy;
 import static org.elasticsearch.watcher.test.WatcherTestUtils.mockExecutionContext;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 
 /**
  */
@@ -147,25 +150,28 @@ public class ScriptConditionTests extends ElasticsearchTestCase {
         fail("expected a condition validation exception trying to create an executable with an invalid language");
     }
 
-    @Test(expected = ScriptConditionException.class)
     public void testScriptCondition_throwException() throws Exception {
         ScriptServiceProxy scriptService = getScriptServiceProxy(tp);
         ExecutableScriptCondition condition = new ExecutableScriptCondition(new ScriptCondition(Script.inline("assert false").build()), logger, scriptService);
         SearchResponse response = new SearchResponse(InternalSearchResponse.empty(), "", 3, 3, 500l, new ShardSearchFailure[0]);
         WatchExecutionContext ctx = mockExecutionContext("_name", new Payload.XContent(response));
-        condition.execute(ctx);
-        fail("expected a ScriptConditionException trying to execute a script that throws an exception");
+        ScriptCondition.Result result = condition.execute(ctx);
+        assertThat(result, notNullValue());
+        assertThat(result.status(), is(Condition.Result.Status.FAILURE));
+        assertThat(result.reason(), notNullValue());
+        assertThat(result.reason(), containsString("Assertion"));
     }
 
-    @Test(expected = ScriptConditionException.class)
     public void testScriptCondition_returnObject() throws Exception {
         ScriptServiceProxy scriptService = getScriptServiceProxy(tp);
         ExecutableScriptCondition condition = new ExecutableScriptCondition(new ScriptCondition(Script.inline("return new Object()").build()), logger, scriptService);
         SearchResponse response = new SearchResponse(InternalSearchResponse.empty(), "", 3, 3, 500l, new ShardSearchFailure[0]);
         WatchExecutionContext ctx = mockExecutionContext("_name", new Payload.XContent(response));
-        condition.execute(ctx);
-        fail();
-        fail("expected a ScriptConditionException trying to execute a script that returns an object");
+        ScriptCondition.Result result = condition.execute(ctx);
+        assertThat(result, notNullValue());
+        assertThat(result.status(), is(Condition.Result.Status.FAILURE));
+        assertThat(result.reason(), notNullValue());
+        assertThat(result.reason(), containsString("ScriptConditionException"));
     }
 
     @Test

src/test/java/org/elasticsearch/watcher/execution/ExecutionServiceTests.java

@@ -223,6 +223,75 @@ public class ExecutionServiceTests extends ElasticsearchTestCase {
         verify(action, never()).execute("_action", context, payload);
     }
 
+    @Test
+    public void testExecute_FailedCondition() throws Exception {
+        WatchLockService.Lock lock = mock(WatchLockService.Lock.class);
+        when(watchLockService.acquire("_id")).thenReturn(lock);
+
+        Watch watch = mock(Watch.class);
+        when(watch.id()).thenReturn("_id");
+        when(watchStore.get("_id")).thenReturn(watch);
+
+        ScheduleTriggerEvent event = new ScheduleTriggerEvent("_id", clock.nowUTC(), clock.nowUTC());
+        WatchExecutionContext context = new TriggeredExecutionContext(watch, clock.nowUTC(), event, timeValueSeconds(5));
+
+        ExecutableCondition condition = mock(ExecutableCondition.class);
+        Condition.Result conditionResult = mock(Condition.Result.class);
+        when(conditionResult.status()).thenReturn(Condition.Result.Status.FAILURE);
+        when(conditionResult.reason()).thenReturn("_reason");
+        when(condition.execute(any(WatchExecutionContext.class))).thenReturn(conditionResult);
+
+        // watch level transform
+        Transform.Result watchTransformResult = mock(Transform.Result.class);
+        when(watchTransformResult.payload()).thenReturn(payload);
+        ExecutableTransform watchTransform = mock(ExecutableTransform.class);
+        when(watchTransform.execute(context, payload)).thenReturn(watchTransformResult);
+
+        // action throttler
+        Throttler.Result throttleResult = mock(Throttler.Result.class);
+        when(throttleResult.throttle()).thenReturn(false);
+        ActionThrottler throttler = mock(ActionThrottler.class);
+        when(throttler.throttle("_action", context)).thenReturn(throttleResult);
+
+        // action level transform
+        Transform.Result actionTransformResult = mock(Transform.Result.class);
+        when(actionTransformResult.payload()).thenReturn(payload);
+        ExecutableTransform actionTransform = mock(ExecutableTransform.class);
+        when(actionTransform.execute(context, payload)).thenReturn(actionTransformResult);
+
+        // the action
+        Action.Result actionResult = mock(Action.Result.class);
+        when(actionResult.type()).thenReturn("_action_type");
+        when(actionResult.status()).thenReturn(Action.Result.Status.SUCCESS);
+        ExecutableAction action = mock(ExecutableAction.class);
+        when(action.execute("_action", context, payload)).thenReturn(actionResult);
+
+        ActionWrapper actionWrapper = new ActionWrapper("_action", throttler, actionTransform, action);
+        ExecutableActions actions = new ExecutableActions(Arrays.asList(actionWrapper));
+
+        WatchStatus watchStatus = new WatchStatus(ImmutableMap.of("_action", new ActionStatus(clock.nowUTC())));
+
+        when(watch.input()).thenReturn(input);
+        when(watch.condition()).thenReturn(condition);
+        when(watch.transform()).thenReturn(watchTransform);
+        when(watch.actions()).thenReturn(actions);
+        when(watch.status()).thenReturn(watchStatus);
+
+        WatchRecord watchRecord = executionService.execute(context);
+        assertThat(watchRecord.result().inputResult(), is(inputResult));
+        assertThat(watchRecord.result().conditionResult(), is(conditionResult));
+        assertThat(watchRecord.result().transformResult(), nullValue());
+        assertThat(watchRecord.result().actionsResults(), notNullValue());
+        assertThat(watchRecord.result().actionsResults().count(), is(0));
+
+        verify(historyStore, times(1)).put(watchRecord);
+        verify(lock, times(1)).release();
+        verify(input, times(1)).execute(context);
+        verify(condition, times(1)).execute(context);
+        verify(watchTransform, never()).execute(context, payload);
+        verify(action, never()).execute("_action", context, payload);
+    }
+
     @Test
     public void testExecuteInner() throws Exception {
         DateTime now = DateTime.now(UTC);