[MGNLUI-5415] Break ChainedAction execution sequence on failure gracefully Created: 09/Oct/19 Updated: 10/Mar/21 Resolved: 10/Mar/21 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | None |
| Affects Version/s: | 6.2 |
| Fix Version/s: | None |
| Type: | Task | Priority: | Neutral |
| Reporter: | Rishab Dhar | Assignee: | Rishab Dhar |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | UI | ||
| Remaining Estimate: | 5h | ||
| Time Spent: | 22m | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Task DoR: |
Empty
|
||||||||
| Date of First Response: | |||||||||
| Description |
|
The info.magnolia.ui.contentapp.action.ChainedAction#ChainedAction currently keeps executing actions irrespective of whether there is a failure (validation) in an Action. In order to prevent further validation checks on dependent actions, failing actions have two choices - either duplicate validation logic or throw an Exception. Throwing Exception propagates all the way to the parent UI as RpcInvocationException for a very simple thing such as form validation failure. In order to avoid this behavior, the action sequence has to duplicate this validation in subsequent actions, which has it's own side effects of code duplication and suboptimal performance due to dependent ActionDefinition objects created and executed each time the chain sequence is executed. |
| Comments |
| Comment by Roman Kovařík [ 09/Oct/19 ] |
|
Hard to believe this as exceptions are not swallowed anywhere, so the other actions can't be executed. Maybe the "failure" in your case is just failed validation which is not a "real" failure? |
| Comment by Rishab Dhar [ 09/Oct/19 ] |
|
No the follow up Actions do not execute, but the problem is if I throw an Exception on validation failure, it shows RpcExecutionException on UI, which is quite misleading from UX perspective. Duplication of validation logic to prevent dependent action executions to prevent RpcInvocationException seems not optimal. Not to mention it leads to code duplication in a long chain of actions just for mere validation check and performance drop (n loop iterations where n is the number of chained actions defined). |
| Comment by Roman Kovařík [ 09/Oct/19 ] |
Doesn't this statement contradict with the ticket description?
so it's a general problem, not a chained action problem right?
every action would probably validate different things (e.g. first action validates the form, whereas second validates e.g. if there is new item created to operate on...) |
| Comment by Rishab Dhar [ 09/Oct/19 ] |
What I meant is validation failure in the chain, not exception failure. My assumption was that let's say I have validation check in an Action, but not in dependent Actions, the chain continues executing, dependent actions. This like I said in my previous comment can be solved in two ways currently which I mentioned. Neither one of which is currently optimal.
Partially yes, because the ChainedAction is trying to execute ActionDefinitions but doesn't have the API necessary from info.magnolia.ui.contentapp.browser.ActionExecutionService because of which it has a custom ActionExecutor defined which directly invokes the ActionExecutor API to execute an Action, which does not handle Exceptions gracefully. As the ActionExecutionService was being delegated to do this graceful handling, ChainedAction should probably do this as well. @Override public final void execute(String actionName, Object... args) throws ActionExecutionException { try { Action action = createAction(actionName, args); action.execute(); } catch (RuntimeException e) { throw new ActionExecutionException("Action execution failed for action: " + actionName, e); } }
If the form validation itself failed, it is redundant to check whether an item has been created. This just adds more complexity unnecessarily to mask the problem of non graceful handling at ChainedAction. |
| Comment by Roman Kovařík [ 09/Oct/19 ] |
|
It looks like you are trying to use the very simple implementation chained action for your use case but nobody prevents you to implement/extend your custom chained action or implement one action instead of a chain. Wouldn't that be better...or why do you have to use the chained action while one actions depends on result of another other? |