Python runner should throw / log an error when failing to de-serialize action result #3489
Conversation
action result (e.g. due to non-simple type in the result or similar).
|
Managed to track down old behavior @tonybaloney was described to v1.5 - The different was that at that point in time, we only tried to de-serialize it if user returned JSON from action, otherwise we always simply returned a string (which wasn't all that useful). This behavior was introduced in v1.6 when we introduced result tuple and ability to return status from Python runner actions. I will look if we can replicate pre v1.6 behavior, but I still think correct long term approach would be throwing. /cc @humblearner |
(serialized as string) in case the result contains non-simple type and we can't de-serialize it.
|
I pushed a change to utilize pre-v1.6 behavior in - 1de143a. Now if we fail to de-serialize the result because it contains a non-simple type we simply return result as is aka serialized as a string. Before this change (post v1.6): id: 5947967e0640fd7382f43278
status: succeeded
parameters: None
result:
exit_code: 0
result: None
stderr: 'No handlers could be found for logger "st2.st2common.util.loader"
'
stdout: ''After this change (pre v1.6 and going forward): id: 59479dea0640fd7382f4328d
status: succeeded
parameters: None
result:
exit_code: 0
result: '[{''a'': ''1''}, {''b'': ''2''}, {''h'': 5, ''c'': 3}, {''e'': <print_config.Test object at 0x7f45c758fed0>}]}'
stderr: 'No handlers could be found for logger "st2.st2common.util.loader"
'
stdout: '' |
| else: | ||
| result = 'None' | ||
| # Failed to de-serialize return result as is aka as a string | ||
| match = re.search("'result': (.*?)$", action_result).groups() |
There was a problem hiding this comment.
Yes, this is horrible but we have a safe-guard below - if there are no results we simply return result as is.
And as mentioned in other comment, I believe that correct long term approach would be to throw.
|
P.S. Thanks again for @tonybaloney for catching and reporting this. |
Kami
changed the title
|
👍 |
CHANGELOG.rst
Outdated
| non-simple types (e.g. class instances) which can't be serialized. | ||
|
|
||
| In v1.6 we introduced a change when in such instances, we simply returned ``None`` as result | ||
| and didn't log anything which was confusing. (improvement) |
There was a problem hiding this comment.
let's please include the PR# in the changelog.
Could be a good tradition to follow, since it's easier to find the change later.
There was a problem hiding this comment.
Will do - I agree it's a good idea and we should do it for all the changes :)
There was a problem hiding this comment.
Done - 21b8fa8.
@lakshmi-kannan @m4dcoder @bigmstone let's all try to go doing going forward :)
arm4b
left a comment
arm4b
left a comment
There was a problem hiding this comment.
Besides of the changelog, looks good 👍
…kStorm/st2 into python_runner_non_simple_type_error
@tonybaloney reported this issue (user unfriendly behavior) on Slack.
Right now, if Python runner actions returns a result which can't be serialized (e.g. non complex type such as class instance, datetime object or similar), Python runner will "silently" swallow this issue and simply return
Noneas the result.This is obviously very unfriendly and confusing.
The code change I included just logs a message and still returns
None, but I think a better and more user-friendly approach is to simply throw a very explicit and user-friendly error message - something along the lines of "Failed to de-serialize action result, it most likely contains a non-simple type or similar, see docs at YYY on info on Python runner action results".The thing is if we can't serialize and de-serialize the result user can't do anything with it anyway so throw makes sense because user / developer interaction is required (updating the action code so it only returns simple types).
@tonybaloney asked if we recently made a change which affects that, but AFAIK this behavior has been there since the beginning - I checked the code and we did the same thing in v1.6 as well (https://github.com/StackStorm/st2/blob/v1.6/st2actions/st2actions/runners/pythonrunner.py#L190).Example action which reproduces this issue - https://gist.github.com/Kami/7e654186f95eda654589a171c9f90d12