Skip to content

remove the second Python2 vs Py3 compat.py#3105

Closed
a-detiste wants to merge 2 commits intoapache:masterfrom
a-detiste:master
Closed

remove the second Python2 vs Py3 compat.py#3105
a-detiste wants to merge 2 commits intoapache:masterfrom
a-detiste:master

Conversation

@a-detiste
Copy link
Copy Markdown
Contributor

No description provided.

@Jens-G Jens-G added the python label Feb 19, 2025
raise TProtocolException(type=TProtocolException.BAD_VERSION,
message='No protocol version header')
name = binary_to_str(self.trans.readAll(sz))
name = self.trans.readAll(sz).decode('utf8')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it looks like utf8 is some compatibility alias for utf-8 defined in python:

Python 3.13.2 (main, Feb  5 2025, 01:23:35) [GCC 14.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> "foobar".encode('utf8')
b'foobar'
>>> "foobar".encode('foobar')
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    "foobar".encode('foobar')
    ~~~~~~~~~~~~~~~^^^^^^^^^^
LookupError: unknown encoding: foobar
>>> "foobar".encode('utf-8')
b'foobar'

so this still works, but I think we probably want to use utf-8 instead as that's the correct term (that's also the encoding used in the examples on https://docs.python.org/3/howto/unicode.html)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(this is an issue already existing in today's code, not introduced in this PR) also this logic kind of assumes that the name in message begin need to be utf-8 encoded. if it's not, this python code will throw some errors, for example:

>>> b'\xe7\x8e'.decode('utf-8')
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    b'\xe7\x8e'.decode('utf-8')
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^
UnicodeDecodeError: 'utf-8' codec can't decode bytes in position 0-1: unexpected end of data

and I don't believe "the name of a message needs to be utf-8 encoded" is part of thrift spec so this is potentially a violation to the spec? cc @Jens-G

(again, this is not a new issue introduced by this PR, so I don't think it's a blocking issue of merging this PR)

@Jens-G Jens-G closed this in 3494e1c Mar 8, 2025
vzhd1701 added a commit to vzhd1701/thrift that referenced this pull request Apr 17, 2025
vzhd1701 added a commit to vzhd1701/thrift that referenced this pull request Apr 17, 2025
fishy pushed a commit that referenced this pull request Apr 17, 2025
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.

3 participants