EH type must match step type for node-oriented wf

for #218

rundeckapp/grails-app/controllers/rundeck/controllers/WorkflowController.groovy

@@ -441,9 +441,6 @@ class WorkflowController {
             def WorkflowStep item = editwf.commands.get(numi)
             def ehitem= createItemFromParams(input.params)
             _validateCommandExec(ehitem, params.newitemtype)
-            if(item.nodeStep && !ehitem.nodeStep){
-                ehitem.errors.rejectValue('nodeStep', 'Workflow.stepErrorHandler.nodeStep.invalid')
-            }
             if (ehitem.errors.hasErrors()) {
                 return [error: ehitem.errors.allErrors.collect {g.message(error: it)}.join(","), item: ehitem]
             }

rundeckapp/grails-app/i18n/messages.properties

@@ -110,7 +110,8 @@ commandExec.adhocExecution.adhocFilepath.blank.message=A Script file path must b
 commandExec.adhocExecution.adhocRemoteString.blank.message=A Command string must be specified
 commandExec.adhocExecution.adhocLocalString.blank.message=Script must be specified
 commandExec.jobName.blank.message=Job Name must be specified
-Workflow.stepErrorHandler.nodeStep.invalid=A Node Step's Error Handler must also be a Node Step
+Workflow.stepErrorHandler.nodeStep.invalid=The Error Handler for Step {0} is of the wrong type. Error Handlers for Node Steps must also be Node Steps when the Workflow is Node-oriented.
+WorkflowStep.errorHandler.nodeStep.invalid=The Error Handler must be a Node Step.
 scheduledExecution.adhocString.duplicate.message=Please specify only one of Local script or Remote command or Script file
 scheduledExecution.options.invalid.message=Invalid Option definition: {0}
 scheduledExecution.notifications.invalid.message=Invalid Notification definition: {0}

rundeckapp/grails-app/services/rundeck/services/ScheduledExecutionService.groovy

@@ -1061,6 +1061,11 @@ class ScheduledExecutionService /*implements ApplicationContextAware*/{
             failed = true
             scheduledExecution.errors.rejectValue('workflow', 'scheduledExecution.workflow.empty.message')
         }
+
+        //validate error handler types
+        if (!validateWorkflow(scheduledExecution.workflow,scheduledExecution)) {
+            failed = true
+        }
         if (( params.options || params['_nooptions']) && scheduledExecution.options) {
             def todelete = []
             scheduledExecution.options.each {
@@ -1498,6 +1503,11 @@ class ScheduledExecutionService /*implements ApplicationContextAware*/{
             failed = true
             scheduledExecution.errors.rejectValue('workflow', 'scheduledExecution.workflow.empty.message')
         }
+
+        //validate error handler types
+        if (!validateWorkflow(scheduledExecution.workflow,scheduledExecution)) {
+            failed = true
+        }
         if (scheduledExecution.options) {
             def todelete = []
             scheduledExecution.options.each {
@@ -1657,6 +1667,41 @@ class ScheduledExecutionService /*implements ApplicationContextAware*/{
         }
     }
 
+    /**
+     * Validate workflow command error handler types, return true if valid
+     * @param workflow
+     * @param scheduledExecution
+     * @return
+     */
+    private boolean validateWorkflow(Workflow workflow, ScheduledExecution scheduledExecution){
+        def failed=false
+        //validate error handler types
+        if (workflow?.strategy == 'node-first') {
+            //if a step is a Node step and has an error handler
+            def cmdi = 1;
+            workflow.commands
+                    .findAll { WorkflowStep step -> step.errorHandler }
+            .findAll { it.instanceOf(CommandExec) || it.instanceOf(PluginStep) }
+            .each { WorkflowStep step ->
+                def isNodeStepEh = false;
+                if (step.errorHandler.instanceOf(PluginStep)) {
+                    PluginStep pseh = step.errorHandler.asType(PluginStep)
+                    isNodeStepEh = pseh.nodeStep
+                } else {
+                    isNodeStepEh = step.errorHandler.instanceOf(CommandExec)
+                }
+                //reject if the Error Handler is not a node step
+                if (!isNodeStepEh) {
+                    step.errors.rejectValue('errorHandler', 'WorkflowStep.errorHandler.nodeStep.invalid', [cmdi] as Object[], "Step {0}: Must have a Node Step as an Error Handler")
+                    scheduledExecution?.errors.rejectValue('workflow', 'Workflow.stepErrorHandler.nodeStep.invalid', [cmdi] as Object[], "Step {0}: Must have a Node Step as an Error Handler")
+                    failed = true
+                }
+                cmdi++
+            }
+        }
+        return !failed
+    }
+
     def _dovalidate (Map params, user, String roleList, Framework framework ){
         log.debug("ScheduledExecutionController: save : params: " + params)
         boolean failed = false;
@@ -1849,6 +1894,11 @@ class ScheduledExecutionService /*implements ApplicationContextAware*/{
             scheduledExecution.errors.rejectValue('workflow', 'scheduledExecution.workflow.empty.message')
         }
 
+        //validate error handler types
+        if(!validateWorkflow(scheduledExecution.workflow,scheduledExecution)){
+            failed = true
+        }
+
         if (scheduledExecution.argString) {
             try {
                 scheduledExecution.argString.replaceAll(/\$\{DATE:(.*)\}/, { all, tstamp ->

rundeckapp/grails-app/views/execution/_execDetailsWorkflow.gsp

@@ -36,7 +36,7 @@
     <span class="label" title="Strategy for iteration">Strategy:</span>
     <g:if test="${edit}">
         <label title="Execute the full workflow on each node before the next node">
-            <input type="radio" name="workflow.strategy" value="node-first" ${!workflow?.strategy||workflow?.strategy=='node-first'?'checked':''}/>
+            <input id="wf_strat_node_first" type="radio" name="workflow.strategy" value="node-first" ${!workflow?.strategy||workflow?.strategy=='node-first'?'checked':''}/>
             <g:message code="Workflow.strategy.label.node-first"/>
         </label>
         <label title="Execute each step on all nodes before the next step">

rundeckapp/grails-app/views/execution/_wflistContent.gsp

@@ -28,10 +28,18 @@
         <g:render template="/execution/wflistitemContent" model="${[i:i,stepNum: i,item:item,workflow:workflow,edit:edit,highlight:highlight,noimgs:noimgs, project: project]}"/>
         </span>
         <g:if test="${item.errorHandler}">
-            <ul class="wfhandleritem">
+            <ul class="wfhandleritem ${item.errors?.hasFieldErrors('errorHandler') ? 'fieldError' : ''}">
 
                 <li id="wfli_eh_${i}" ><g:render template="/execution/wflistitemContent"
-                          model="${[i: 'eh_' + i, stepNum:i, item: item.errorHandler, workflow: workflow, edit: edit, highlight: highlight, noimgs: noimgs, isErrorHandler:true]}"/></li>
+                          model="${[i: 'eh_' + i, stepNum:i, item: item.errorHandler, workflow: workflow, edit: edit, highlight: highlight, noimgs: noimgs, isErrorHandler:true]}"/>
+                    <g:hasErrors bean="${item}" field="errorHandler">
+                        <span class="info error">
+                            <g:eachError field="errorHandler" bean="${item}" var="err">
+                                <g:message error="${err}" encodeAs="HTML"/>
+                            </g:eachError>
+                        </span>
+                    </g:hasErrors>
+                </li>
 
             </ul>
         </g:if>

rundeckapp/test/integration/ScheduledExServiceTests.groovy

@@ -1,24 +1,8 @@
 import grails.test.GrailsUnitTestCase
-
-import org.quartz.Scheduler
-import org.quartz.SchedulerContext
-import org.quartz.SchedulerMetaData
+import org.quartz.*
 import org.quartz.spi.JobFactory
-import org.quartz.JobDetail
-import org.quartz.Trigger
-import org.quartz.JobDataMap
-import org.quartz.Calendar
-import org.quartz.JobListener
-import org.quartz.TriggerListener
-import org.quartz.SchedulerListener
-
 import org.springframework.context.MessageSource
-import rundeck.ScheduledExecution
-import rundeck.CommandExec
-import rundeck.Workflow
-import rundeck.JobExec
-import rundeck.Option
-import rundeck.Notification
+import rundeck.*
 import rundeck.services.FrameworkService
 import rundeck.services.ScheduledExecutionService
 /*
@@ -371,6 +355,84 @@ class ScheduledExServiceTests extends GrailsUnitTestCase {
         }
     }
 
+    public void testDoValidateWorkflowStepFirstErrorhandlers() {
+        def testService = new ScheduledExecutionService()
+        def fwkControl = mockFor(FrameworkService, true)
+        fwkControl.demand.getFrameworkFromUserSession { session, request -> return null }
+        fwkControl.demand.existsFrameworkProject { project, framework ->
+            assertEquals 'testProject', project
+            return true
+        }
+        testService.frameworkService = fwkControl.createMock()
+
+        //step first allows any combination of step and error handler types
+        def params = [jobName: 'monkey1', project: 'testProject', description: 'blah',
+                workflow: new Workflow(keepgoing: true,strategy: 'step-first',commands: [
+                        new CommandExec(adhocRemoteString: 'test command1', adhocExecution: true, errorHandler:
+                                new JobExec(jobGroup: 'test1',jobName: 'blah')),
+                        new CommandExec(adhocRemoteString: 'test command2', adhocExecution: true,errorHandler:
+                                new CommandExec(adhocRemoteString: 'test ehcommand', adhocExecution: true)),
+                        new JobExec(jobGroup: 'test1', jobName: 'blah2', errorHandler:
+                                new CommandExec(adhocRemoteString: 'test command1', adhocExecution: true)),
+                        new JobExec(jobGroup: 'test1', jobName: 'blah3', errorHandler:
+                                new JobExec(jobGroup: 'test1', jobName: 'blah4')),
+                ])
+        ]
+        def results = testService._dovalidate(params, 'test', 'test', null)
+        if (results.scheduledExecution.errors.hasErrors()) {
+            results.scheduledExecution.errors.allErrors.each {
+                System.err.println(it);
+            }
+        }
+        assertFalse results.failed
+
+        assertFalse results.scheduledExecution.workflow.commands[0].errors.hasErrors()
+        assertFalse results.scheduledExecution.workflow.commands[1].errors.hasErrors()
+        assertFalse results.scheduledExecution.workflow.commands[2].errors.hasErrors()
+        assertFalse results.scheduledExecution.workflow.commands[3].errors.hasErrors()
+    }
+    public void testDoValidateWorkflowNodeFirstErrorhandlers() {
+        def testService = new ScheduledExecutionService()
+        def fwkControl = mockFor(FrameworkService, true)
+        fwkControl.demand.getFrameworkFromUserSession { session, request -> return null }
+        fwkControl.demand.existsFrameworkProject { project, framework ->
+            assertEquals 'testProject', project
+            return true
+        }
+        testService.frameworkService = fwkControl.createMock()
+
+        //Node first rejects non-Node error handler steps for Node steps.
+        def params = [jobName: 'monkey1', project: 'testProject', description: 'blah',
+                workflow: new Workflow(keepgoing: true,strategy: 'node-first',commands: [
+                        new CommandExec(adhocRemoteString: 'test command1', adhocExecution: true, errorHandler:
+                                new JobExec(jobGroup: 'test1',jobName: 'blah')),
+                        new CommandExec(adhocRemoteString: 'test command2', adhocExecution: true,errorHandler:
+                                new CommandExec(adhocRemoteString: 'test ehcommand', adhocExecution: true)),
+                        new JobExec(jobGroup: 'test1', jobName: 'blah2', errorHandler:
+                                new CommandExec(adhocRemoteString: 'test command1', adhocExecution: true)),
+                        new JobExec(jobGroup: 'test1', jobName: 'blah3', errorHandler:
+                                new JobExec(jobGroup: 'test1', jobName: 'blah4')),
+                ])
+        ]
+        def results = testService._dovalidate(params, 'test', 'test', null)
+        results.scheduledExecution.workflow.commands.each{ cmd->
+            if (cmd.errors.hasErrors()) {
+                cmd.errors.allErrors.each {
+                    System.out.println("command: "+cmd+", error: "+it);
+                }
+            }
+        }
+        assertTrue results.failed
+        assertTrue results.scheduledExecution.workflow.commands[0].errors.hasErrors()
+        assertTrue results.scheduledExecution.workflow.commands[0].errors.hasFieldErrors('errorHandler')
+
+        //no error in other commands
+        assertFalse results.scheduledExecution.workflow.commands[1].errors.hasErrors()
+        assertFalse results.scheduledExecution.workflow.commands[2].errors.hasErrors()
+        assertFalse results.scheduledExecution.workflow.commands[3].errors.hasErrors()
+
+    }
+
     public void testDoValidateWorkflowOptions() {
         def testService = new ScheduledExecutionService()
 
@@ -1733,6 +1795,114 @@ class ScheduledExServiceTests extends GrailsUnitTestCase {
             assertEquals 'err command', ehexec.adhocRemoteString
         }
     }
+    public void testDoUpdateJobErrorHandlersStepFirst() {
+        def sec = new ScheduledExecutionService()
+        if (true) {//test update basic job details
+            def se = new ScheduledExecution(jobName: 'monkey1', project: 'testProject', description: 'blah', adhocExecution: true, adhocRemoteString: 'test command',
+                    workflow: new Workflow(strategy: 'step-first',
+                            commands: [
+                                    new CommandExec(adhocRemoteString: 'test command', adhocExecution: true),
+                                    new CommandExec(adhocRemoteString: 'test command', adhocExecution: true),
+                                    new JobExec(jobName: 'test1',jobGroup: 'test'),
+                                    new JobExec(jobName: 'test1', jobGroup: 'test'),
+                            ]))
+            se.save()
+
+            assertNotNull se.id
+
+            //try to do update of the ScheduledExecution
+            def fwkControl = mockFor(FrameworkService, true)
+            fwkControl.demand.getFrameworkFromUserSession {session, request -> return null }
+            fwkControl.demand.existsFrameworkProject {project, framework ->
+                assertEquals 'testProject2', project
+                return true
+            }
+            fwkControl.demand.getCommand {project, type, command, framework ->
+                assertEquals 'testProject2', project
+                assertEquals 'aType2', type
+                assertEquals 'aCommand2', command
+                return null
+            }
+            sec.frameworkService = fwkControl.createMock()
+
+            def eh1 = new CommandExec(adhocRemoteString: 'err command')
+            def eh2 = new CommandExec(adhocRemoteString: 'err command')
+            def eh3 = new JobExec(jobGroup: 'eh',jobName: 'eh1')
+            def eh4 = new JobExec(jobGroup: 'eh',jobName: 'eh2')
+            def params = new ScheduledExecution(jobName: 'monkey2', project: 'testProject2', description: 'blah', adhocExecution: true, adhocRemoteString: 'test command',
+                workflow: new Workflow(strategy: 'step-first',
+                       commands: [
+                               new CommandExec(adhocRemoteString: 'test command', adhocExecution: true,errorHandler: eh1),
+                               new CommandExec(adhocRemoteString: 'test command', adhocExecution: true,errorHandler:eh3),
+                               new JobExec(jobName: 'test1', jobGroup: 'test',errorHandler: eh2),
+                               new JobExec(jobName: 'test1', jobGroup: 'test',errorHandler: eh4),
+                       ]
+                )
+            )
+            def results = sec._doupdateJob(se.id.toString(), params, 'test', 'test', null)
+            def succeeded = results[0]
+            assertTrue succeeded
+
+        }
+    }
+    public void testDoUpdateJobErrorHandlersNodeFirst() {
+        def sec = new ScheduledExecutionService()
+        if (true) {//test update basic job details
+            def se = new ScheduledExecution(jobName: 'monkey1', project: 'testProject', description: 'blah', adhocExecution: true, adhocRemoteString: 'test command',
+                    workflow: new Workflow(strategy: 'node-first',
+                            commands: [
+                                    new CommandExec(adhocRemoteString: 'test command', adhocExecution: true),
+                                    new CommandExec(adhocRemoteString: 'test command', adhocExecution: true),
+                                    new JobExec(jobName: 'test1',jobGroup: 'test'),
+                                    new JobExec(jobName: 'test1', jobGroup: 'test'),
+                            ]))
+            se.save()
+
+            assertNotNull se.id
+
+            //try to do update of the ScheduledExecution
+            def fwkControl = mockFor(FrameworkService, true)
+            fwkControl.demand.getFrameworkFromUserSession {session, request -> return null }
+            fwkControl.demand.existsFrameworkProject {project, framework ->
+                assertEquals 'testProject2', project
+                return true
+            }
+            fwkControl.demand.getCommand {project, type, command, framework ->
+                assertEquals 'testProject2', project
+                assertEquals 'aType2', type
+                assertEquals 'aCommand2', command
+                return null
+            }
+            sec.frameworkService = fwkControl.createMock()
+
+            def eh1 = new CommandExec(adhocRemoteString: 'err command')
+            def eh2 = new CommandExec(adhocRemoteString: 'err command')
+            def eh3 = new JobExec(jobGroup: 'eh',jobName: 'eh1')
+            def eh4 = new JobExec(jobGroup: 'eh',jobName: 'eh2')
+            def params = new ScheduledExecution(jobName: 'monkey2', project: 'testProject2', description: 'blah', adhocExecution: true, adhocRemoteString: 'test command',
+                workflow: new Workflow(strategy: 'node-first',
+                       commands: [
+                               new CommandExec(adhocRemoteString: 'test command', adhocExecution: true,errorHandler: eh1),
+                               new CommandExec(adhocRemoteString: 'test command', adhocExecution: true,errorHandler:eh3),
+                               new JobExec(jobName: 'test1', jobGroup: 'test',errorHandler: eh2),
+                               new JobExec(jobName: 'test1', jobGroup: 'test',errorHandler: eh4),
+                       ]
+                )
+            )
+            def results = sec._doupdateJob(se.id.toString(), params, 'test', 'test', null)
+            def succeeded = results[0]
+            def ScheduledExecution rese=results[1]
+            assertFalse succeeded
+
+            assertFalse rese.workflow.commands[0].errors.hasErrors()
+
+            assertTrue rese.workflow.commands[1].errors.hasErrors()
+            assertTrue rese.workflow.commands[1].errors.hasFieldErrors('errorHandler')
+            assertFalse rese.workflow.commands[2].errors.hasErrors()
+            assertFalse rese.workflow.commands[3].errors.hasErrors()
+
+        }
+    }
 
     public void testDoUpdateJobShouldReplaceFilters() {
         def sec = new ScheduledExecutionService()

rundeckapp/web-app/js/jobedit.js

@@ -367,8 +367,10 @@ function _wfishownewErrorHandler(key,num,nodeStep){
     newehdiv.parentNode.removeChild(newehdiv);
     wfiehli.appendChild(newehdiv);
 
-//    $(newehdiv).select('.node_step_section').each(nodeStep?Element.show:Element.hide);
-    $(newehdiv).select('.step_section').each(!nodeStep ? Element.show : Element.hide);
+    var nodeFirstWfStrat = $('wf_strat_node_first').checked;
+    var allowedWfStepEh=!(nodeStep && nodeFirstWfStrat);
+    //WF step error handler not allowed if strategy is "node-first" and the step is a node step
+    $(newehdiv).select('.step_section').each(allowedWfStepEh ? Element.show : Element.hide);
 
     newehdiv.show();
     $(wfiehli.parentNode).show();