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

Warn if a sequence type is passed when expecting str #38309

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@willthames
Contributor

willthames commented Apr 4, 2018

SUMMARY

_check_type_str happily converts all values to str.
More validation would be helpful to avoid errors when
accidentally passing the wrong thing.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

basic.py

ANSIBLE VERSION
ansible 2.6.0 (warn_on_complex_types 3902b60d97) last updated 2018/04/05 09:12:11 (GMT +1000)
  config file = None
  configured module search path = [u'/home/will/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/will/src/ansible/lib/ansible
  executable location = /home/will/src/ansible/bin/ansible
  python version = 2.7.14 (default, Mar 14 2018, 13:36:31) [GCC 7.3.1 20180303 (Red Hat 7.3.1-5)]
ADDITIONAL INFORMATION

Helps to avoid problems like #38233

@willthames willthames requested a review from bcoca Apr 4, 2018

@willthames willthames force-pushed the willthames:warn_on_complex_types branch Apr 5, 2018

@samdoran samdoran removed the needs_triage label Apr 5, 2018

@willthames

This comment has been minimized.

Contributor

willthames commented Apr 11, 2018

@bcoca, the change to to_text from str causes unit test failures:

>       assert isinstance(am.params['arg'], type_)
E       AssertionError: assert False
E        +  where False = isinstance('42', <type 'str'>)

Basically, am.params['arg'] is of type unicode, not type str, so this test fails. This is presumably because str(value) converts to str and to_text(value) keeps it as unicode.

I'm not sure what the best approach here is. From my point of view, removing the to_text change would still meet my needs but I imagine we want to accept unicode module arguments.

lib/ansible/module_utils/basic.py Outdated
return str(value)
if isinstance(value, (list, dict)):
self.warn("Parameter value %s is expected to be string but was %s" % (value, value.__class__.__name__))
return to_text(value)

This comment has been minimized.

@abadger

abadger Apr 12, 2018

Member

@willthames bcoca asked me to look at your problem. I think that we want to use to_native() here and we probably want to use errors='surrogate_or_strict' as well. to_native() converts the string into the type which an undecorated string literal has on that version of python. That type is called "str" on both python2 and python3 but it's a byte string on python2 and a text string on python3. It will have the following benefits:

  • It's the most compatible since it's the same type as str on both platforms. (It just has better defaults and more flexible error handling than using str()).
  • For most module_utils code, we use native string types right now so that a module author can mostly write idiomatic python and it mostly just works. They don't run into a problem combining byte and text type. (They do have to worry about it when they deal with an external API which needs a specific type but those are not as common so it's easier to deal with those when the time comes and not worry the rest of the time).

This comment has been minimized.

@willthames

willthames Apr 12, 2018

Contributor

@abadger that just leaves the question of what to do with the test - it's failing because the result of to_text isn't an instance of type _type, which defaults to str.

This comment has been minimized.

@abadger

abadger Apr 12, 2018

Member

to_native() should make it be str on both Python2 and Python3.

Warn if a sequence type is passed when expecting str
`_check_type_str` happily converts all values to `str`.
More validation would be helpful to avoid errors when
accidentally passing the wrong thing.
@abadger

This comment has been minimized.

Member

abadger commented Nov 29, 2018

@willthames were you going to add errors='surrogate_or_strict?

@bcoca

bcoca approved these changes Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment