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

force string type into str before escaping #3769

Conversation

DanielMarchand
Copy link
Contributor

@DanielMarchand DanielMarchand commented Feb 19, 2020

It is very easy for a user to pass integers as arguments when a string is expected. In the current version, an exception that would be very cryptic to novel users occurs. Forcing a string type should be a harmless way of avoiding this error.

The returned error is

  File "/home/daniel/Src/aiida-core/aiida/engine/processes/calcjobs/tasks.py", line 78, in do_upload
    calc_info, script_filename = process.presubmit(folder)
  File "/home/daniel/Src/aiida-core/aiida/engine/processes/calcjobs/calcjob.py", line 467, in presubmit
    script_content = scheduler.get_submit_script(job_tmpl)
  File "/home/daniel/Src/aiida-core/aiida/schedulers/scheduler.py", line 156, in get_submit_script
    script_lines.append(self._get_run_line(job_tmpl.codes_info, job_tmpl.codes_run_mode))
  File "/home/daniel/Src/aiida-core/aiida/schedulers/scheduler.py", line 227, in _get_run_line
    command_to_exec_list.append(escape_for_bash(arg))
  File "/home/daniel/Src/aiida-core/aiida/common/escaping.py", line 41, in escape_for_bash
    escaped_quotes = str_to_escape.replace("'", """'"'"'""")
AttributeError: 'int' object has no attribute 'replace'

@DanielMarchand
Copy link
Contributor Author

This came up in the context of 'manually' setting the parallelization settings of a AiiDA-QE workchain via the cmdline option in the settings:

nk = 1
'cmdline': ['-nk', nk],

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @DanielMarchand . It would be good to add some unit tests (I can help with this if you like) because the current implementation is not correct.

@@ -35,6 +35,7 @@ def escape_for_bash(str_to_escape):
within triple quotes to make it work, getting finally: the complicated
string found below.
"""
str_to_escape = str(str_to_escape)
Copy link
Contributor

Choose a reason for hiding this comment

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

if str_to_escape is None this will now return 'None' i.e. the literal string 'None' which is not what we want. This should at the very least be moved after the conditional that checks for None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would be great, its definitely a bit more subtle an issue than I thought it was...

@ltalirz
Copy link
Member

ltalirz commented Feb 20, 2020

So, for the tests, create a new file tests/common/test_escaping.py

In there, use regular pytest syntax, e.g.

from aiida.common import escaping

def test_escaping_...():
    escaped = escaping....("test string")
    assert ...

@sphuber
Copy link
Contributor

sphuber commented Mar 27, 2020

Superseded by #3873

@sphuber sphuber closed this Mar 27, 2020
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