Skip to content

Update rule comparison operators to work with mixed operator types (bytes and strings) #4831

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

Merged
merged 8 commits into from
Dec 10, 2019

Conversation

Kami
Copy link
Member

@Kami Kami commented Dec 9, 2019

This pull request updates all the rule criteria comparison operators which also work with strings (a lot of operators work on multiple types), to try to convert both operators to unicode / string type if either or both of them are of a type bytes.

In ideal world, user would always know a type of each input, but that's rarely the case. Depending on how data is retrieved or defined (e.g. webhook body, etc.) the actual type might be bytes or a string / unicode.

This means that pretty much the only option we are left with for rule comparison operators to work as expected when mixed types come into play is to try to convert both of the operators to the same common type aka unicode / string.

NOTE: This issue / problem only affects Python 3.x deployments.

Kami added 2 commits December 9, 2019 15:48
strings to try to cast both of the operators to string / unicode if
the value is of types bytes.

This works around the issue under Python 3.x when one of the operators
is bytes type and other is string / unicode type which would result in
an exception.
@Kami Kami added the python3 label Dec 9, 2019
@Kami Kami added this to the 3.2.0 milestone Dec 9, 2019
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Dec 9, 2019
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.

👍

As I understand it's somewhat related to py3 issues highlighted in https://forum.stackstorm.com/t/is-python3-7-officially-supported-by-stackstorm/938

@Kami
Copy link
Member Author

Kami commented Dec 10, 2019

That's correct, this issue only affects Python 3. Tests I've added would already pass under Python 2.x without the changes I added.

Having said that, in ideal world, we (and our users) would always be very specific and explicit about the types so we wouldn't need to do this "best effort" type of conversion, but this breaks down in real life for various reasons:

  1. Python is dynamically typed
  2. Our platform relies on dynamic types and lose conversion between types in many scenarios (for better or worse)
  3. Jinja expressions / variables only support string / unicode type
  4. There are many ways for data to come into the system (webhooks, sensors, etc.) and we can't always enforce that this data conforms to a specific schema / type

As far as the forum post goes - yes, I believe this could be a fix for that issue, but I haven't seen any logs yet and that was just my best guess when I checked the operators code (and something which immediately stood out to me). It could also be that there are some other issues as well.

@Kami Kami merged commit ddb5ef8 into master Dec 10, 2019
@Kami Kami deleted the operator_improvements branch December 10, 2019 18:16
@blag blag added the v3.2 label Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python3 size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants