8000 #4804 Handle action parameters of type array correctly by winem · Pull Request #4861 · StackStorm/st2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

#4804 Handle action parameters of type array correctly #4861

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Apr 8, 2020
Merged

#4804 Handle action parameters of type array correctly #4861

merged 19 commits into from
Apr 8, 2020

Conversation

winem
Copy link
Contributor
@winem winem commented Feb 11, 2020 •
edited by arm4b
Loading

This closes #4804 and actually affects more than just remote shell runner actions.

In the beginning this looked like a simple issue with the serialization and parsing of the parameter but it actually looks like this did not work at all.

So I address 2 issues with this PR:

  1. elif argument_type == 'list': actually did not match when the type was array. This means that the final else matched and the parameter was just transformed to a unicode string. Although according to the documentation, array is the correct value (See https://docs.stackstorm.com/actions.html#action-metadata) I see some potential how this could break something in existing environments where the unicode string is already handled by the script and users did not notice that this is actually a bug.
    Question 1: Do we want to address this and if yes, how? This change should at least be mentioned in the changelog once it's released.
    Question 2: Is there any reason (i.e. backward compatibility) I am not aware of to check if the parameter type is either list or array and not just array?
  2. Its required to map the values and cast them to strings to handle the case where not all the values of the array are either strings or integers. join on an array with strings and integers or other values for example would fail terribly.

The solution provided with this PR is the pragmatic approach. It's actually a trade-off and may apply unnecessary casting operations when applying str() to the item.

Alternative approach

The alternative would look like this:

    elif argument_type == 'list' or argument_type == 'array':
        # Lists are serialized a comma delimited string (foo,bar,baz)
        argument_value_types = set([type(value) for value in argument_value])
        if len(argument_value_types) == 1 and int in argument_value_types or str in argument_value_types:
            # join arrays that hold just strings or integers
            argument_value = ','.join(argument_value) if argument_value else ''
        else:
            # join arrays with items of any other type and transform these items to strings
            argument_value = ','.join(map(str, argument_value)) if argument_value else ''

Here we check the type of all the items in an array first and just join the items if they are either just strings or just integers. In all other cases, str() would be applied to all the items in the array.

But I think this just shifts the overhead to another place and is not worth it. I did not do any performance tests with real large arrays yet but I doubt that this matters here. Any input by more experienced experts is appreciated!

@winem
- match for argument_type 'array' instead of 'list' to be compliant w…
ce0b448 
…ith the documentation

- handle the parameter and transform the array to a string of comma-separated values
@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Feb 11, 2020
@winem
Copy link
Contributor Author
winem commented Feb 11, 2020

I will take a look at the failed test. I was actually reviewing the current tests already and was wondering why that tests did not fail in the past. So, let's see... Investigation ongoing.

@winem
Copy link
Contributor Author
winem commented Feb 11, 2020 •
edited
Loading

So the failed test was caused by:

st2/st2common/tests/unit/test_action_db_utils.py

Line 466 in cdfeeaf

'actionlist': {'type': 'list', 'required': False, 'position': 4},
where the used type was also 'list' and not 'array'.

So, since I don't believe in coincidence this seems to confirm the concerns I mentioned in the PR: just checking the type for array instead of list will break existing stuff and their might be historical reasons why st2 should support both types.

I pushed a commit that checks if the type is in [ 'array', 'list' ] and assume that the test will be fixed by this commit. In the end, I'm open to adapt the test to use type: array as well and revert the recent commit to stick with just array but I would like to get some feedback from the "old hands" first.

@blag blag requested a review from m4dcoder February 12, 2020 19:03
blag
blag reviewed Feb 12, 2020
View reviewed changes
@blag
Copy link
Contributor
blag commented Feb 13, 2020

I reran the failing Travis test and it passed. Solving the intermittent failures on Travis is out of the scope of this PR.

At this point the only failing Travis build is the linting run, so fixing that as I've suggested should solve it. @winem If you're still interested in merging this, just make that fix and I'll approve it and merge it in! 😃

@winem
Copy link
Contributor Author
winem commented Feb 13, 2020

Hi @blag, do you have any opinion on the array vs. list vs. support both topic? Or maybe even some insights from the history of stackstorm?

@blag blag modified the milestones: 3.2.0, 3.1.1 Feb 13, 2020
@blag
Copy link
Contributor
blag commented Feb 13, 2020

I reran all of the failing tests - everything passes now!

At this point I think we'll need to support both list and array. I'm skeptical that argument_type is ever list in production, but I don't think it would hurt to support both.

However, this PR also needs at least a unit test for this function so nobody breaks it in the future. You should be able to extend the test for get_args to include tests for arrays.

@winem
Copy link
Contributor Author
winem commented Feb 13, 2020

Thanks for re-running the tests and and the feedback!

I just added the tests. Let's just wait for the unit tests to be done.

@blag
Copy link
Contributor
blag commented Feb 21, 2020

Sorry I should have mentioned this before: please add a changelog entry for this fix.

@blag blag mentioned this pull request Feb 22, 2020
Closed
@winem
Copy link
Contributor Author
winem commented Feb 22, 2020

No worries, @blag. Could have though of it as well. Anyway, done. 👍

cognifloyd
cognifloyd approved these changes Feb 23, 2020
View reviewed changes
Copy link
Member
@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

👍

@blag blag added v3.2 labels Feb 26, 2020
m4dcoder
m4dcoder requested changes Feb 27, 2020
View reviewed changes
Copy link
Contributor
@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.

I don't see any unit tests that show the difference made by the fix. Looking at just the changes to existing unit tests, I only see that it performs the same function when elif argument_type == 'list':. Please clarify or add appropriate unit test that show what this PR fixes.

@blag blag removed v3.1.1 labels Feb 27, 2020
@winem
Copy link
Contributor Author
winem commented Mar 3, 2020 •
edited
Loading

Hi @m4dcoder,

I'm sorry but to be honest, I can't follow. So I will just provide as much details as possible and hope that it helps.

Without the change from this PR an array would be passed to the shell command as a unicode string if the parameter type is array (as the documentation recommends).
So the parameter to the script was

[u'foo'
 u'bar'
 u'baz']

instead of the expected foo,bar,baz.

What this PR does is to handle parameters of both types, list and array, the same way and pass the list values as comma separated argument to the shell command runner.

I added unit tests which use the params.actionarray in addition to params.actionlist to ensure that we test both types of list parameters and have the same tests for both.

So I think I can just come up with tests that prove that the solution works but I can't provide one that reproduces the old behavior and would fail expectedly.

Please let me know if I miss anything or got something wrong.

@cognifloyd
Copy link
Member
cognifloyd commented Mar 21, 2020 •
edited
Loading

This is primarily an issue with documented vs actual functionality.

Part 1 of the PR: adding array parameter type

In the docs:

parameters - A dictionary of parameters and optional metadata describing type and default. The metadata is structured data following the JSON Schema specification draft 4. The common parameter types allowed are string, boolean, number (...), object, integer (...) and array.

Which references the JSON Schema Spec 4. This section is particularly relevant:

JSON Schema defines seven primitive types for JSON values:

  • array A JSON array.
  • boolean A JSON boolean.
  • integer A JSON number without a fraction or exponent part.
  • number Any JSON number. Number includes integer.
  • null The JSON null value.
  • object A JSON object.
  • string A JSON string.

So, array is called out in the docs as "the" way to get a array/list parameter into an action, and cites an external spec as the source for the naming convention. However, only list was implemented, not array. So, this PR adds proper array support but maintains backwards compatibility for list.

Part 2 of the PR: the serialization of arrays/lists

In the docs:

If your script only uses positional arguments (which is often the case for existing scripts), you simply need to declare parameters with the correct value for the position attribute in the metadata file. Positional arguments are serialized based on the simple rules described below:

  • string, integer, float - Serialized as a string.
  • boolean - Serialized as a string 1 (true) or 0 (false).
  • array - Serialized as a comma delimited string (e.g. foo,bar,baz).
  • object - Serialized as JSON.

This part of the PR is the part that I think could probably still use another test:

st2/st2common/st2common/util/action_db.py

Line 288 in 0725178

argument_value = ','.join(map(str, argument_value)) if argument_value else ''

That is the implementation of @winem's second point:

  1. Its required to map the values and cast them to strings to handle the case where not all the values of the array are either strings or integers. join on an array with strings and integers or other values for example would fail terribly.

Maybe this needs one or more tests that demonstrate a list that includes something that is neither int nor string to show when the casting to string is necessary.

@winem
Copy link
Contributor Author
winem commented Apr 4, 2020

Hi @cognifloyd, thank you for the extensive answer. Now I got it and totally agree that this kind of test is missing.

I added a test for arrays with values of different types and might have found another issue. But this may also be a matter of expectation. So when someone enters a complex number as value it will be interpreted as complex number first and st2 would just pass the result.

This is probably obvious for developers but may cause issues in the rare case that a user wants to pass a complex number as parameter to a script. Do we wanna add this to the documentation?

@winem winem requested a review from m4dcoder April 4, 2020 22:35
cognifloyd
cognifloyd approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member
@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM

cognifloyd
cognifloyd reviewed Apr 6, 2020
View reviewed changes
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Apr 6, 2020
@arm4b
Copy link
Member
arm4b commented Apr 7, 2020 •
edited
Loading

@cognifloyd Thanks for the review and feedback, that was helpful! 👍

@m4dcoder this PR is up to you for the final decision/review.

m4dcoder
m4dcoder approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor
@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.

LGTM

9E81
@arm4b arm4b merged commit ac98365 into StackStorm:master Apr 8, 2020
@arm4b arm4b mentioned this pull request Apr 17, 2020
Closed
@arm4b arm4b mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blag blag blag left review comments

@cognifloyd cognifloyd cognifloyd approved these changes

@m4dcoder m4dcoder m4dcoder approved these changes

Assignees
No one assigned
Labels
size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

Shell script action not serializing array properly.
6 participants
@winem @blag @cognifloyd @arm4b @m4dcoder @armab
0