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

Fix a bug with "serialize_positional_argument" not correctly handling unicode values and fix unicode support in core.sendmail action #4533

Merged
merged 15 commits into from Feb 10, 2019

Conversation

Kami
Copy link
Member

@Kami Kami commented Feb 4, 2019

This pull request fixes a bug with serialize_positional_argument function not correctly handling unicode values.

This would prevent actions such as core.sendmail from working when a non-ascii (unicode) value was provided.

Reported by @johandahlberg.

Resolves StackStorm/st2contrib#597.

unicode values.

This would cause actions which rely on positions arguments (e.g.
core.sendmail) not to work when an unicode value was provided.

Reported by @johandahlberg.
@Kami Kami added the python3 label Feb 4, 2019
@Kami Kami added this to the 2.10.2 milestone Feb 4, 2019
@Kami Kami requested a review from bigmstone February 4, 2019 14:23
@Kami Kami added the unicode label Feb 4, 2019
@johandahlberg
Copy link
Contributor

I've tried this, and I can verify that the action works. However, the email that I get does look a little bit funky. Instead of the swedish letter "Å" I get the result below when I run: st2 run core.sendmail to='johan.dahlberg@medsci.uu.se' body='Test Åsa' subject='Test Åsa'

test_asa_pic

I'm unsure if this is directly related to the st2 action, or it might have something to do with the locale settings of the system. Any ideas?

@Kami
Copy link
Member Author

Kami commented Feb 5, 2019

I also confirmed that with those changes unicode characters are correctly passed to the script so it could indeed be something with the bash locale setting for stanley user.

If you run the following command:

st2 run core.sendmail to='johan.dahlberg@medsci.uu.se' body='Test Åsa' subject='Test Åsa'

The action runner process will run the following script (you can see the command in the action runner log file under DEBUG log level).

chmod +x /opt/stackstorm/packs/core/actions/send_mail/send_mail ; sudo -E -H -u stanley -- bash -c '/opt/stackstorm/packs/core/actions/send_mail/send_mail stanley johan.dahlberg@medsci.uu.se '"'"'Test Åsa'"'"' 1 text/html '"'"'Test Åsa'"'"' '"'"''"'"''

I would recommend you to test / play with this script directly. Locally I tried to modify it to modify it to print the values out directly instead of calling sendmail and it seems to work fine:

(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ python st2client/st2client/shell.py run core.sendmail to='johan.dahlberg@medsci.uu.se' body='Test Åsa' subject='Test Åsa'
.
id: 5c595110076129037f50d54c
status: failed
parameters: 
  body: "Test Åsa"
  subject: "Test Åsa"
  to: johan.dahlberg@medsci.uu.se
result: 
  failed: true
  return_code: 1
  stderr: ''
  stdout: "johan.dahlberg@medsci.uu.se
Test Åsa
Test Åsa"
  succeeded: false

@Kami
Copy link
Member Author

Kami commented Feb 5, 2019

Discussed this with @johandahlberg on Slack and we managed to get it work end to end with one additional change / fix - 4784d94.

We now force utf8 charset for the email body.

In theory, we could also make this configurable as par of the action parameter (either as part of content_type one or by adding new one), but I think it's not worth the complexity - you would pretty much almost always want to default to utf8 anyway.

@johandahlberg
Copy link
Contributor

I've added a PR into this branch here #4534 with some additional fixes. Feel free to close it if you don't think that the changes make sense.

Johan Dahlberg and others added 3 commits February 5, 2019 14:56
@Kami Kami changed the title Fix a bug with "serialize_positional_argument" not correctly handling unicode values Fix a bug with "serialize_positional_argument" not correctly handling unicode values and fix unicode support in core.sendmail action Feb 5, 2019
@Kami
Copy link
Member Author

Kami commented Feb 5, 2019

While at it, I will also work on some unit tests since I don't think we have any right now.

@Kami
Copy link
Member Author

Kami commented Feb 5, 2019

I added some unit / integration tests in ded040f.

To be able to test the script better, I need to modify the action a bit. I added a new "sendmail_binary" argument which is set to cat by the tests. This way the whole raw email data is output to stdout instead of piped to the sendmail binary.

This allows us to test 90% of the original sendmail script code and works much better than alternative mock approaches or similar (in a lot of cases like that, people tend to mock too much of the original code / action and your tests end up mostly testing the mocks and not the actual code itself).

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@Kami Kami merged commit f45200c into master Feb 10, 2019
@Kami Kami deleted the fix_unicode_issue_in_serialize_positional_arguments branch February 10, 2019 13:20
Kami added a commit that referenced this pull request Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core.sendmail: Unable to handle non-ascii char in body or subject
3 participants