[BUG] Very minor type-checking issue in the shutdown() function of the salt.modules.win_system module
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 15.7k
- Forks
- 5.6k
- Avg merge
- 2d 44m
- Merged PRs (30d)
- 80
Description
Description
This is a very minor typing issue in salt.modules.win_system, and should likely be noted for the next person who ends up committing to this file.
HUVp16SeYiPt2L50peVt:
The minion function caused an exception: Traceback (most recent call last):
File "c:\python37\lib\site-packages\salt\minion.py", line 1870, in _thread_return
opts, data, func, args, kwargs
File "c:\python37\lib\site-packages\salt\executors\direct_call.py", line 12, in execute
return func(*args, **kwargs)
File "c:\python37\lib\site-packages\salt\modules\win_system.py", line 315, in shutdown
message = message.decode("utf-8")
AttributeError: 'int' object has no attribute 'decode'
So, this is actually a universal issue and it's mostly because Python's lack of a typesystem forces each implementor to explicitly validate or "cast" the types that they're given as parameters. This is usually fine for internally called functions, but as saltstack exposes these functions directly to the user.. they should be type-checked so that users don't need to know Python in order to resolve these issues.
There's an enforcement of the parameter's type at the top of the function, but unfortunately it's only for Python2 which will soon go away. Whenever it does, that'll likely be a good time to fix issues similar to this.
if six.PY2:
message = _to_unicode(message)
...
However if these types of bugs are incredibly common, a universal issue could likely be discussed (probably as an SEP) after the ports-to-master project is completed and the code-base stabilizes. These issues can likely be remedied with a combination of two decorators. One decorator which validates types passed as parameters (and maybe the result), and another decorator which enforces the types of the parameters that are passed in from the user. If performance is a concern, some of them could be even disabled during production or affected by the loglevel.
I've done something close to the enforcement decorator in another project (https://github.com/arizvisa/ida-minsc/blob/master/base/_utils.py#L968) as the project is similar to salt in that internally defined functions are exposed to the user as part of the interface. This needed implicit string conversions to be done on the user's input before the function used them, and since the function was being directly exposed there wasn't any other way to wrap it before the user would dispatch into it.
An example for a decorator for shutdown() could look like @salt.utils.decorators.verify(message=str, timeout=int), or @salt.utils.decorators.result_dict(ret_code=int, name=str). Then for enforcing a type maybe @salt.utils.decorators.format(message="{!s}".format). Another benefit of doing something like this is that you can parse out the decorator in a test pretty easily with the ast module (or any linter api) in order to extract parameter type information and test all of the functions that are exposed as the "user-interface" get some automated coverage for these types of issues.
This'd squash a large number of my issues such as #55708, #55944, and likely others.
Setup
Just need a windows minion, and it to be running salt w/ Python3.
Steps to Reproduce the behavior
Pretend that you know what you're doing, but instead pass an integral to system.shutdown as the first parameter thinking it's the timeout. Something like the following will do.
$ salt-call system.shutdown 30
Expected behavior
Likely a better error message possibly warning about which parameter is being passed as the incorrect type since it's only being enforced in Python2.
Versions Report
Salt Version:
Salt: 3001
Dependency Versions:
cffi: 1.12.2
cherrypy: 17.4.1
dateutil: 2.8.0
docker-py: Not Installed
gitdb: 2.0.5
gitpython: Not Installed
Jinja2: 2.10.1
libgit2: Not Installed
M2Crypto: Not Installed
Mako: 1.0.7
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: 2.19
pycrypto: 3.9.8
pycryptodome: 3.9.7
pygit2: Not Installed
Python: 3.7.4 (tags/v3.7.4:e09359112e, Jul 8 2019, 19:29:22) [MSC v.1916 32 bit (Intel)]
python-gnupg: 0.4.4
PyYAML: 5.1.2
PyZMQ: 18.0.1
smmap: 2.0.5
timelib: 0.2.4
Tornado: 4.5.3
ZMQ: 4.3.1
System Versions:
dist:
locale: cp1252
machine: x86
release: 8.1
system: Windows
version: 8.1 6.3.9600 SP0
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in salt/modules/win_system.py at the shutdown() function and reproduce the issue with a Windows minion using salt-call system.shutdown 30. Check how the message parameter is handled under Python 3 and define completion as avoiding the AttributeError while reporting an appropriate invalid-parameter error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- operating-systems
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100