Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. #191

Merged

Conversation

userlocalhost
Copy link
Member

@userlocalhost userlocalhost commented Mar 24, 2020

Abstract

This fixes a problem not to be able to parse data which is typed dict(etc) in array in some YAQL operators by converting input data-structure from unhashable value to hashable one. (c.f. #176)

Problem

Orquesta couldn't handle dict in array typed value in some yqal operators (e.g. distinct) as below.

username@host:~$ cd st2
username@host:~/st2$ source virtualenv/bin/activate; python

>>> from orquesta.expressions import base as expr_base
>>>

# It's OK when an array that has string values is passed to distinct()
>>> data = {'val': ['foo', 'bar', 'foo']}
>>> expr_base.evaluate("<% ctx(val) %>", data)
['foo', 'bar', 'foo']
>>> expr_base.evaluate("<% ctx(val).distinct() %>", data)
['foo', 'bar']
>>>

# It'll be failed when an array that has dict values is passed to distinct()
>>> data = {'val': [{'a': 1}, {'b': 2}, {'a': 1}]}
>>> expr_base.evaluate("<% ctx(val) %>", data)
[{'a': 1}, {'b': 2}, {'a': 1}]
>>> expr_base.evaluate("<% ctx(val).distinct() %>", data) 
(TypeError exception will be occurred)
Execution Result スクリーンショット 2020-04-01 15 09 11

Cause & Measure

In some YAQL operators (e.g. distinct) refer to hash value of input data to identify it. And the openstack/YAQL library that Orquesta uses wraps those input value using utils.convert_output_data to prevent TypeError exception when these operator's implementation try to refer to input hash value. Thus, we can handle these value in the distinct operator like following. And this PR applies it to the Orquesta.

(c.f. openstack/yaql@67d58bc)

username@host:~$ cd st2
username@host:~/st2$ source virtualenv/bin/activate; python

>>> import yaql
>>> 
>>> engine = yaql.language.factory.YaqlFactory().create()
>>> 
>>> data = {'val': [{'a': 1}, {'b': 2}, {'a': 1}]}
>>> 
>>> engine('$.val').evaluate(data=data)
[{'a': 1}, {'b': 2}, {'a': 1}]
>>> engine('$.val.distinct()').evaluate(data=data)
[{'a': 1}, {'b': 2}]
>>> 
Execution Result スクリーンショット 2020-03-24 15 22 37

userlocalhost and others added 2 commits March 23, 2020 19:33
…dict in array in some YAQL operators.

In some YAQL operators (e.g. distinct) refer to hash value of input
value by calling __hash__ method. But some basic data structures (list,
dict and set) don't implement this method.
This commit adds a feature that may convert input datastructure from
unhashable one to hashable one by using YAQL library that orquesta uses.
Auto generate and upate the json schemas for the workflow spec
@userlocalhost userlocalhost changed the title Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. (WIP) Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Mar 24, 2020
@userlocalhost
Copy link
Member Author

I realized some other problems (other than #193) have to be cared so I amended title as WIP.

@arm4b arm4b requested a review from m4dcoder March 24, 2020 13:25
@userlocalhost userlocalhost changed the title (WIP) Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Apr 1, 2020
Copy link
Collaborator

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add unit test(s) of a workflow with variables of various data types, publish those variables in a task transition into the context, write them out to the workflow output, and then check data and type is the same?

The various data types should be those touched by https://github.com/openstack/yaql/blob/master/yaql/language/utils.py#L67.

Please put the unit test(s) under https://github.com/StackStorm/orquesta/blob/master/orquesta/tests/unit/conducting/test_workflow_conductor_data_flow.py.

@userlocalhost
Copy link
Member Author

Thank you very much for your review and I appreciate your clear instructions!

@codecov-io
Copy link

codecov-io commented Apr 9, 2020

Codecov Report

Merging #191 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #191   +/-   ##
=======================================
  Coverage   94.03%   94.04%           
=======================================
  Files          41       41           
  Lines        2731     2735    +4     
  Branches      544      545    +1     
=======================================
+ Hits         2568     2572    +4     
  Misses        100      100           
  Partials       63       63           
Impacted Files Coverage Δ
orquesta/expressions/functions/workflow.py 95.83% <100.00%> (+0.05%) ⬆️
orquesta/expressions/yql.py 89.89% <100.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc85df7...4bc5575. Read the comment docs.

@userlocalhost userlocalhost changed the title Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. (WIP) Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Apr 9, 2020
@userlocalhost userlocalhost changed the title (WIP) Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Fixed a problem not to be able to parse input value which is type of dict in array in some YAQL operators. Apr 9, 2020
@userlocalhost userlocalhost force-pushed the bugfix/issue-176/evaluate-yaql-context branch from ea04b50 to 8532901 Compare April 9, 2020 06:09
Copy link
Collaborator

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a separate set of tests similar to WorkflowConductorDataFlowTest but then the workflow is using Jinja expressions instead of YAQL expressions? Since this change is converting dict to YAQL types, we have to make sure that it doesn't break Jinja users.

Copy link
Collaborator

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@userlocalhost LGTM. I made some minor cleanup. Thanks for the contribution and being diligent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants