From 6fb17d8266ab6cc6d6389a5fec1282d1094658c1 Mon Sep 17 00:00:00 2001
From: Renat Akhmerov <renat.akhmerov@gmail.com>
Date: Wed, 8 Apr 2020 13:22:49 +0700
Subject: [PATCH] Add YAQL sanitizing for iterators

* This patch also adds handling for iterators in the sanitizing
  function for YAQL results. JSON serialization for iterators now
  works correctly but the issue is that they can be used only once.
  So, for example, if we got a result from YAQL and performed JSON
  serialization for it, then the iterator is already empty and
  the data structure is just corrupted. So we can't further pass
  to executors and other subsystems. The solution is just to sanitize
  iterators after they're just returned from YAQL.
* Added a test to make sure that action input reaches an executor
  not corrupted, although it gets saved into DB and hence serialized
  into JSON.
* Changed the Sphinx entry in requirements.txt so that the version
  3.0.0 is now excluded (it breaks the build).

Closes-Bug: #1871567
Change-Id: I47abe0904b49d72e33eb10080c71fb81980d44da
---
 doc/requirements.txt                          |  2 +-
 mistral/expressions/yaql_expression.py        |  3 +-
 .../engine/test_disabled_yaql_conversion.py   | 74 +++++++++++++++++++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/doc/requirements.txt b/doc/requirements.txt
index 9bc364519..a918df6bc 100644
--- a/doc/requirements.txt
+++ b/doc/requirements.txt
@@ -1,4 +1,4 @@
-sphinx>=1.8.0,!=2.1.0;python_version>='3.4' # BSD
+sphinx>=1.8.0,!=2.1.0,!=3.0.0;python_version>='3.4' # BSD
 sphinxcontrib-httpdomain>=1.3.0 # BSD
 sphinxcontrib-pecanwsme>=0.8.0 # Apache-2.0
 openstackdocstheme>=1.30.0 # Apache-2.0
diff --git a/mistral/expressions/yaql_expression.py b/mistral/expressions/yaql_expression.py
index 1a467bfcc..d6059e236 100644
--- a/mistral/expressions/yaql_expression.py
+++ b/mistral/expressions/yaql_expression.py
@@ -14,6 +14,7 @@
 #    See the License for the specific language governing permissions and
 #    limitations under the License.
 
+import collections
 import inspect
 import re
 
@@ -135,7 +136,7 @@ def _sanitize_yaql_result(result):
     if isinstance(result, yaql_utils.FrozenDict):
         return result._d
 
-    if inspect.isgenerator(result):
+    if inspect.isgenerator(result) or isinstance(result, collections.Iterator):
         return list(result)
 
     return result
diff --git a/mistral/tests/unit/engine/test_disabled_yaql_conversion.py b/mistral/tests/unit/engine/test_disabled_yaql_conversion.py
index fe640b3a7..493c1567d 100644
--- a/mistral/tests/unit/engine/test_disabled_yaql_conversion.py
+++ b/mistral/tests/unit/engine/test_disabled_yaql_conversion.py
@@ -13,6 +13,8 @@
 #    See the License for the specific language governing permissions and
 #    limitations under the License.
 
+import mock
+
 from mistral.db.v2 import api as db_api
 from mistral.engine import engine_server
 from mistral import exceptions as exc
@@ -167,3 +169,75 @@ class DisabledYAQLConversionTest(engine_test_base.EngineTestCase):
             self.assertTrue(len(action_ex.input) > 0)
             self.assertIn('output', action_ex.input)
             self.assertIn('param', action_ex.input['output'])
+
+    def test_iterators_in_yaql_result(self):
+        # Both input and output data conversion in YAQL need to be disabled
+        # so that we're sure that there won't be any surprises from YAQL
+        # like some YAQL internal types included in expression results.
+        self.override_config('convert_input_data', False, 'yaql')
+        self.override_config('convert_output_data', False, 'yaql')
+
+        # Setting YAQL engine to None so it reinitialized again with the
+        # right values upon the next use.
+        yaql_expression.YAQL_ENGINE = None
+
+        wf_text = """---
+        version: '2.0'
+
+        wf:
+          input:
+            - params: null
+
+          tasks:
+            task1:
+              action: std.echo
+              input:
+                output:
+                  param1:
+                    <% switch($.params = null => [],
+                    $.params != null =>
+                    $.params.items().select({k => $[0], v => $[1]})) %>
+        """
+
+        wf_service.create_workflows(wf_text)
+
+        wf_input = {
+            'params': {
+                'k1': 'v1',
+                'k2': 'v2'
+            }
+        }
+
+        with mock.patch.object(self.executor, 'run_action',
+                               wraps=self.executor.run_action) as mocked:
+            # Start workflow.
+            wf_ex = self.engine.start_workflow('wf', wf_input=wf_input)
+
+            self.await_workflow_success(wf_ex.id)
+
+            with db_api.transaction():
+                # Note: We need to reread execution to access related tasks.
+                wf_ex = db_api.get_workflow_execution(wf_ex.id)
+
+                t_ex = self._assert_single_item(
+                    wf_ex.task_executions,
+                    name='task1'
+                )
+
+                action_ex = t_ex.action_executions[0]
+
+                self.assertTrue(len(action_ex.input) > 0)
+
+            mocked.assert_called_once()
+
+            # We need to make sure that the executor got the right action
+            # input regardless of an iterator (that can only be used once)
+            # present in the YAQL expression result. Let's check first 4
+            # actual arguments with the executor was called, including the
+            # action parameters.
+            args = mocked.call_args[0]
+
+            self.assertEqual(action_ex.id, args[0])
+            self.assertEqual('mistral.actions.std_actions.EchoAction', args[1])
+            self.assertDictEqual({}, args[2])
+            self.assertDictEqual(action_ex.input, args[3])