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(set_message): encode strings if field is bytes #30

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

russkel
Copy link

@russkel russkel commented Nov 2, 2024

I was trying to load yaml that was output from ros2 topic echo and then convert them back into msgs for fixtures, and I came across this issue:

Traceback (most recent call last):
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 85, in set_message_fields_internal
    value = field_type(field_value)
            ^^^^^^^^^^^^^^^^^^^^^^^
TypeError: string argument without an encoding

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 58, in set_message_fields_internal
    items = values.items()
            ^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'items'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 101, in set_message_fields
    set_message_fields_internal(msg, values, timestamp_fields)
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 97, in set_message_fields_internal
    set_message_fields_internal(
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 88, in set_message_fields_internal
    set_message_fields_internal(
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rosidl_runtime_py/set_message.py", line 60, in set_message_fields_internal
    raise TypeError(
TypeError: Value '' is expected to be a dictionary but is a str

With the following yaml:

header:
  stamp:
    sec: 1730513229
    nanosec: 6167372
  frame_id: ''
status:
- level: "\x02"
  name: 'geofence_node: Go GeoFences'
  message: No Geofences
  hardware_id: none
  values: []
- level: "\0"
  name: 'geofence_node: No Go GeoFences'
  message: Outside All
  hardware_id: none
  values: []

The level value is a string \x02 but set_message is trying to convert it to the field_type bytes. This patch fixes this issue by encoding the string to bytes.

@russkel russkel requested a review from sloretz as a code owner November 2, 2024 05:08
@russkel russkel force-pushed the feature/fix-byte-load branch from ac20f17 to faf19f3 Compare November 2, 2024 05:09
@russkel
Copy link
Author

russkel commented Nov 2, 2024

If this could be backported to jazzy that would be lovely too.

@russkel russkel force-pushed the feature/fix-byte-load branch from faf19f3 to 6670ff4 Compare November 2, 2024 05:19
@russkel russkel force-pushed the feature/fix-byte-load branch from 6670ff4 to 9483332 Compare November 2, 2024 05:21
Copy link
Contributor

@fujitatomoya fujitatomoya 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 to me but i would like to have 2nd review just in case something missing.

@fujitatomoya
Copy link
Contributor

Pulls: #30
Gist: https://gist.githubusercontent.com/fujitatomoya/451def5286e30198246798264ec9e99a/raw/5973684a8f6b8eb319279f18f63da329c50b5e5e/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_runtime_py
TEST args: --packages-above rosidl_runtime_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14779

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@russkel russkel force-pushed the feature/fix-byte-load branch from 0c3c97c to ee6c327 Compare November 3, 2024 05:29
Signed-off-by: Russ Webber <[email protected]>
@russkel russkel force-pushed the feature/fix-byte-load branch from ee6c327 to ad5a8b2 Compare November 3, 2024 05:29
@russkel
Copy link
Author

russkel commented Nov 3, 2024

I added a basic smoke test for conversion from yaml using one of the test message fixtures and it appears not good enough with a string containing \xff. Although maybe this is an issue with the way some bytes are encoded into yaml/strings and become irrecoverable...

In [1]: moo = '\xff'

In [2]: moo
Out[2]: 'ÿ'

In [3]: moo.encode()
Out[3]: b'\xc3\xbf'

In [9]: moo = chr(0xff)

In [10]: moo
Out[10]: 'ÿ'

In [11]: moo.encode(encoding='utf8')
Out[11]: b'\xc3\xbf'

In [12]: moo.encode(encoding='utf16')
Out[12]: b'\xff\xfe\xff\x00'

In [13]: moo.encode(encoding='ascii')
---------------------------------------------------------------------------
UnicodeEncodeError                        Traceback (most recent call last)
Cell In[13], line 1
----> 1 moo.encode(encoding='ascii')

UnicodeEncodeError: 'ascii' codec can't encode character '\xff' in position 0: ordinal not in range(128)

In [17]: for c in moo: print(ord(c))
255

In [19]: import yaml

In [20]: yaml.dump('\xff')
Out[20]: '"\\xFF"\n'

In [21]: from rosidl_runtime_py.convert import _convert_value

In [22]: _convert_value('\xff')
Out[22]: 'ÿ'

In [27]: bytes(moo, 'ascii', 'backslashreplace')
Out[27]: b'\\xff'

@russkel
Copy link
Author

russkel commented Nov 3, 2024

The only way I am able to get this to pass is using value = bytes([ord(c) for c in field_value]).
I suspect this isn't terribly correct.... 🤔

@cottsay
Copy link
Member

cottsay commented Nov 14, 2024

Does passing canonical=True to yaml.dump() do what you need (possibly here)?

@MichaelOrlov MichaelOrlov added the more information needed Further information is requested label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more information needed Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants