fix: serialization of undesired content when a flow crashes (#6609)

* chore: update engine to conform transpiler changes #6530 * feat: rework transpiler as per #6530 Signed-off-by: Mustafa Baser <mbaser@mail.com>
JanssenProject · Dec 30, 2023 · fca0e15 · fca0e15
1 parent c17eeba
commit fca0e15
Show file tree

Hide file tree

Showing 7 changed files with 120 additions and 112 deletions.
diff --git a/agama/transpiler/src/main/resources/JSGenerator.ftl b/agama/transpiler/src/main/resources/JSGenerator.ftl
@@ -8,6 +8,11 @@
 //Generated at ${.now?iso_utc}
 <#assign fqname = flow.@fqname>
 function ${flow.@fun}<#recurse flow>
+    <#-- see header macro -->
+} catch (_e) {
+    return _makeJavaException(_fqname, _e)   <#-- See jans#6530 -->
+}
+
 }
 
 <#macro header>
@@ -21,6 +26,8 @@ const _fqname = "${fqname}", _basePath = ${.node.base.STRING}
 let _it = null, _it2 = null
 <#-- idx is accessible to flow writers (it's not underscore-prefixed). It allows to access the status of loops -->
 let idx = [], _items = []
+
+try {
 </#macro>
 
 <#macro statement>
@@ -55,11 +62,7 @@ let idx = [], _items = []
 <#macro action_call>
     <#local catch=.node.preassign_catch?size gt 0>
 
-    <#if catch>
-try {
-    var ${.node.preassign_catch.short_var} = null
-    </#if>
-    <@util_preassign node=.node /> _actionCall(
+    _it = _actionCall(
     <#if .node.static_call?size gt 0>
         null, false, "${.node.static_call.qname}", "${.node.static_call.ALPHANUM}"
         , <@util_argslist node=.node.static_call />
@@ -68,39 +71,44 @@ try {
         , <@util_argslist node=.node.oo_call />
     </#if>    
     )
-
+
+    _it2 = _it.second
     <#if catch>
-} catch (_e) {
-    ${.node.preassign_catch.short_var} = _e.javaException
-}
+        var ${.node.preassign_catch.short_var} = _it2
+    <#else>
+        if (!_isNil(_it2)) return _it2   <#-- "propagate exception" to parent, see jans#6530 -->
     </#if>
+
+    if (_isNil(_it2)) <@util_preassign node=.node /> _it.first
+
+    _it = null      <#-- avoids a later serialization of a Pair -->
 </#macro>
 
 <#macro flow_call>
     <#local catch=.node.preassign_catch?size gt 0>
 
-    <#if catch>
-try {
-    var ${.node.preassign_catch.short_var} = null
-    </#if>
-
     <#if .node.variable?size gt 0>
         _it = ${.node.variable}
     <#else>
         _it = "${.node.qname}"
     </#if>
-_it = _flowCall(_it, _basePath, <@util_url_overrides node=.node.overrides/>, <@util_argslist node=.node />)
 
-if (_it === undefined) throw new Error("No Finish instruction was reached")
+    _it = _flowCall(_it, _basePath, <@util_url_overrides node=.node.overrides/>, <@util_argslist node=.node />)
+
+    if (_it.bubbleUp) return _it.value
+
+    _it = _it.value
+    _it2 = _isJavaException(_it)
 
-if (_it.bubbleUp) return _it.value
-    <@util_preassign node=.node /> _it.value
-
     <#if catch>
-} catch (_e) {
-    ${.node.preassign_catch.short_var} = _makeRhinoException(_e)
-}
+        var ${.node.preassign_catch.short_var} = _it2 ? _it : null
+    <#else>
+        if (_it2) return _it  <#-- "propagate exception" to parent -->
     </#if>
+
+    if (!_it2) <@util_preassign node=.node /> _it
+
+    _it = null
 </#macro>
 
 <#macro rfac>

diff --git a/agama/transpiler/src/main/resources/util.js b/agama/transpiler/src/main/resources/util.js
@@ -2,13 +2,16 @@ let
     _primitiveUtils = Packages.io.jans.agama.engine.misc.PrimitiveUtils,
     _scriptUtils = Packages.io.jans.agama.engine.script.ScriptUtils,
     _logUtils = Packages.io.jans.agama.engine.script.LogUtils,
-    _booleanCls = new Packages.java.lang.Class.forName("java.lang.Boolean"),
-    _numberCls = new Packages.java.lang.Class.forName("java.lang.Number"),
-    _integerCls = new Packages.java.lang.Class.forName("java.lang.Integer"),
-    _stringCls = new Packages.java.lang.Class.forName("java.lang.String"),
-    _collectionCls = new Packages.java.lang.Class.forName("java.util.Collection"),
-    _listCls = new Packages.java.lang.Class.forName("java.util.List"),
-    _mapCls = new Packages.java.lang.Class.forName("java.util.Map")
+    _flowCrashEx = Packages.io.jans.agama.engine.exception.FlowCrashException,
+    _errorUtil = Packages.org.mozilla.javascript.NativeErrorUtil,
+    _booleanCls = Packages.java.lang.Class.forName("java.lang.Boolean"),
+    _numberCls = Packages.java.lang.Class.forName("java.lang.Number"),
+    _integerCls = Packages.java.lang.Class.forName("java.lang.Integer"),
+    _stringCls = Packages.java.lang.Class.forName("java.lang.String"),
+    _collectionCls = Packages.java.lang.Class.forName("java.util.Collection"),
+    _listCls = Packages.java.lang.Class.forName("java.util.List"),
+    _mapCls = Packages.java.lang.Class.forName("java.util.Map"),
+    _exceptionCls = Packages.java.lang.Class.forName("java.lang.Exception")
 
 function _renderReplyFetch(base, page, allowCallbackResume, data) {
     if (_isObject(data))
@@ -19,8 +22,9 @@ function _renderReplyFetch(base, page, allowCallbackResume, data) {
 function _redirectFetchAtCallback(url) {
     if (_isString(url)) {
         let jsonStr = _scriptUtils.pauseForExternalRedirect(url).second
-        //string parsing via JS was preferred over returning a Java Map directly because it feels more
-        //natural for RRF to return a native JS object; it creates the illusion there was no Java involved
+        //string parsing via JS was preferred over returning a Java Map directly
+        //because it feels more natural for RRF to return a native JS object;
+        //it creates the illusion there was no Java involved
         return JSON.parse(jsonStr)
     }
     throw new TypeError("Data passed to RFAC was not a string")
@@ -47,36 +51,43 @@ function _actionCall(instance, instanceRequired, clsName, method, args) {
 function _flowCall(flowName, basePath, urlOverrides, args) {
 
     if (!_isString(flowName))
-        throw new TypeError("Flow name is not a string")
+        return _flowCallErr(new TypeError("Flow name is not a string"))
 
     let mapping = _scriptUtils.templatesMapping(basePath, urlOverrides)
     let p = _scriptUtils.prepareSubflow(flowName, mapping)
     let params = args.map(_scan)
     params.splice(0, 0, p.second)
 
     let f = p.first
-    //Modify p to avoid serializing a Java Pair in the next RRF call
+    //Modify p to avoid serializing a Java Pair in a RRF call (inside function f)
     //wrapping with ArrayList is a workaround for a kryo deserialization exception
     p = new Packages.java.util.ArrayList(mapping.values())
     mapping = null
 
-    let result
-    try {
-        result = f.apply(null, params)
-    } finally {
-        _scriptUtils.closeSubflow()
-    }
+    let result = f.apply(null, params)
+    _scriptUtils.closeSubflow()
 
-    if (_isNil(result)) return     //return undefined
+    if (_isNil(result))
+        return _flowCallErr(new Error(
+                "No Finish instruction was reached during execution of " + flowName))
 
     return { value: result,
         //determines if the parent should handle this returned value
         bubbleUp: result.aborted == true && !_scriptUtils.pathMatching(result.url, p) }
 
 }
 
-function _makeRhinoException(e) {
-    return new Packages.org.mozilla.javascript.SubflowRhinoException(e)
+function _flowCallErr(e) {
+    return { value: _makeJavaException(e), bubbleUp: false }
+}
+
+function _makeJavaException(qname, e) {
+
+    let msg = e.toString()
+    _log2(qname, ["@e An error occurred:", msg])
+    _log2(qname, ["@t \n", _errorUtil.stackTraceOf(e)])
+    return new _flowCrashEx(msg)
+
 }
 
 function _finish(val) {
@@ -90,7 +101,7 @@ function _finish(val) {
     else if (_isObject(val, javaish) && _isBool(val.success))
         return val
 
-    throw new Error ("Cannot determine whether Finish value should be successful or not")
+    throw new Error("Cannot determine whether Finish value should be successful or not")
 
 }
 
@@ -135,6 +146,10 @@ function _ic(val, symbol) {
 
 }
 
+function _isJavaException(val) {
+    return _exceptionCls.isInstance(val)
+}
+
 function _isObject(val, javaish) {
 
     let jish = _isNil(javaish) ? _javaish(val) : javaish

diff --git a/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/script/ScriptUtils.java b/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/script/ScriptUtils.java
@@ -115,15 +115,18 @@ public static Pair<Function, NativeJavaObject> prepareSubflow(String qname,
 
     }
 
-    public static Object callAction(Object instance, String actionClassName, String methodName,
+    public static Pair<Object, Exception> callAction(Object instance, String actionClassName, String methodName,
             Object[] params) throws Exception {
 
+        Object result = null;
+        Exception ex = null;
         try {
-            return CdiUtil.bean(ActionService.class).callAction(instance, actionClassName, methodName, params);
+            result = CdiUtil.bean(ActionService.class).callAction(instance, actionClassName, methodName, params);
         } catch (Exception e) {
-            LOG.warn("Exception raised when executing Call - method {}", methodName);                
-            throw e;
+            LOG.warn("Exception raised when executing Call - class: {}, method: {}", actionClassName, methodName);
+            ex = e;
         }
+        return new Pair<>(result, ex);      //See jans#6530
 
     }
 

diff --git a/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/service/ActionService.java b/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/service/ActionService.java
@@ -15,6 +15,7 @@
 import java.io.File;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
+import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Parameter;
@@ -140,8 +141,12 @@ public Object callAction(Object instance, String className, String methodName, O
         }
         Object[] args = getArgsForCall(javaMethod, arity, rhinoArgs);
 
-        logger.debug("Performing method call");
-        return javaMethod.invoke(instance, args);
+        try {
+            logger.debug("Performing method call");
+            return javaMethod.invoke(instance, args);
+        } catch (InvocationTargetException e) {
+            throw (Exception) e.getCause();     //return the "real" exception, e is just a wrapper here
+        }
 
     }
 

diff --git a/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/service/FlowService.java b/jans-auth-server/agama/engine/src/main/java/io/jans/agama/engine/service/FlowService.java
@@ -41,7 +41,6 @@
 import org.mozilla.javascript.NativeJavaMap;
 import org.mozilla.javascript.NativeJavaObject;
 import org.mozilla.javascript.NativeObject;
-import org.mozilla.javascript.RhinoException;
 import org.mozilla.javascript.Scriptable;
 import org.mozilla.javascript.Undefined;
 import org.slf4j.Logger;
@@ -127,7 +126,7 @@ public FlowStatus startFlow(FlowStatus status) throws FlowCrashException {
             }
 
         } catch (IOException ie) {
-            throw new FlowCrashException(ie.getMessage(), ie);
+            makeCrashException(ie);
         }
         return status;
 
@@ -160,16 +159,17 @@ public FlowStatus continueFlow(FlowStatus status, String jsonParameters, boolean
 
             } catch (ContinuationPending pe) {
                 status = processPause(pe, status);
-                
+
             } catch (FlowTimeoutException te) {
                 terminateFlow();
                 throw te;
+
             } catch (Exception e) {
                 terminateFlow();
                 makeCrashException(e);
             }
         } catch (IOException ie) {
-            throw new FlowCrashException(ie.getMessage(), ie);
+            makeCrashException(ie);
         }
         return status;
 
@@ -253,7 +253,7 @@ private void verifyCode(Flow fl) throws IOException {
     }
 
     private FlowStatus processPause(ContinuationPending pending, FlowStatus status)
-            throws FlowCrashException, IOException {
+            throws IOException {
 
         PendingException pe = null;
         if (pending instanceof PendingRenderException) {
@@ -262,7 +262,7 @@ private FlowStatus processPause(ContinuationPending pending, FlowStatus status)
             String templPath = computeTemplatePath(pre.getTemplatePath(), parentsMappings);
 
             if (!templPath.contains("."))
-                throw new FlowCrashException(
+                throw new IOException(
                         "Expecting file extension for the template to render: " + templPath);
 
             status.setTemplatePath(templPath);
@@ -350,32 +350,32 @@ private NativeJavaObject wrapListOrMap(Object obj) {
 
     private NativeObject checkJSReturnedValue(Object obj) throws Exception {
 
-        try {
+        try {      
             //obj is not null
-            return NativeObject.class.cast(obj);
-        } catch (ClassCastException e) {
             if (Undefined.isUndefined(obj)) {
-                //This may only happen for a top-level flow. A subflow that returns undefined
-                //is handled in the generated javascript for a Trigger directive
-                throw new Exception("No Finish instruction was reached");
-            } else throw e;
+                //This may only happen at a top-level flow. For subflows see util.js#_flowCall
+                throw new FlowCrashException("No Finish instruction was reached during execution of flow");
+
+            } else return NativeObject.class.cast(obj);
+
+        } catch (ClassCastException e) {
+
+            Object ex = NativeJavaObject.class.cast(obj).unwrap();
+
+            if (Exception.class.isInstance(ex))
+                throw (Exception) ex;
+
+            throw new RuntimeException("Unexpected Java value received upon flow termination");
         }
 
     }
 
     private void makeCrashException(Exception e) throws FlowCrashException {
-
-        String msg;
-        if (e instanceof RhinoException) {            
-            RhinoException re = (RhinoException) e;
-            msg = re.details();
-            logger.error(msg + re.getScriptStackTrace());
-            //logger.error(re.getMessage());
-            msg = "Error executing flow's code - " + msg;
-        } else 
-            msg = e.getMessage();
 
-        throw new FlowCrashException(msg, e);
+        if (FlowCrashException.class.isInstance(e))
+            throw (FlowCrashException) e;
+
+        throw new FlowCrashException(e.getMessage(), e);
 
     }
 

diff --git a/jans-auth-server/agama/engine/src/main/java/org/mozilla/javascript/NativeErrorUtil.java b/jans-auth-server/agama/engine/src/main/java/org/mozilla/javascript/NativeErrorUtil.java
@@ -0,0 +1,16 @@
+package org.mozilla.javascript;
+
+public class NativeErrorUtil {
+
+    public static String stackTraceOf(Object err) {
+
+        try {
+            return NativeError.class.cast(err).getStackDelegated().toString();
+        } catch (Exception ex) {
+            return "Stacktrace not available. " + ex.getMessage();
+        }
+
+    }
+
+
+}