Skip to content

Fixing pdu.generate() encoding#132

Open
vazir wants to merge 2 commits intopython-smpplib:masterfrom
vazir:master
Open

Fixing pdu.generate() encoding#132
vazir wants to merge 2 commits intopython-smpplib:masterfrom
vazir:master

Conversation

@vazir
Copy link

@vazir vazir commented Feb 1, 2021

pdu.generate()
will lead to the following exception, so this is the proposed fix for it.

      File "/opt/smpp_proxy_server/smpplib/pdu.py", line 136, in generate
        body = self.generate_params()
      File "/opt/smpp_proxy_server/smpplib/command.py", line 144, in generate_params
        value = self._generate_string(field)
      File "/opt/smpp_proxy_server/smpplib/command.py", line 180, in _generate_string
        value = field_value + chr(0)
    TypeError: can't concat str to bytes

@code-of-kpp
Copy link
Member

I think, all chr calls should be replaced with six.b(chr(...)), all isinstance checks are redundant.

Good catch anyway! Perhaps add a test for that?

value = chr(0).encode()

setattr(self, field, field_value)
if isinstance(value, bytes):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(value, bytes):
if isinstance(value, six.binary_type):

hm, this check is required though

Copy link
Author

Choose a reason for hiding this comment

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

Probably right :) You are trying to keep compatibility with 2.7

Copy link
Author

@vazir vazir Feb 2, 2021

Choose a reason for hiding this comment

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

for the test that was happening for me for every received pdu, as I was testing the generated PDU equals to the received.

pdu = parse_pdu(raw_pdu, client=self)
gen_bin = pdu.generate()

And so, the exception occurs. Seems nobody had long not using that part, as I was fixing that over a year ago for myself and realized the bug is still there just yesterday after checking in a new version :)

Copy link
Member

Choose a reason for hiding this comment

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

can you please add a new function or a new file with a new function to the tests/ directory?

Co-authored-by: Konstantin <kpp.live+github@gmail.com>
Copy link
Member

@code-of-kpp code-of-kpp left a comment

Choose a reason for hiding this comment

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

It also worth looking on line 174 and in general, why field_value is ever not bytes. Looks like the bug is actually happens because self contains a string in the field-attribute, while it should have bytes

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.

2 participants