add 'strict_decode' option for protocols #227
Conversation
|
@coderabbitai review |
WalkthroughThe changes across various files introduce a Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- tests/test_aio_protocol_binary.py (1 hunks)
- tests/test_aio_protocol_compact.py (1 hunks)
- tests/test_protocol_binary.py (2 hunks)
- tests/test_protocol_compact.py (2 hunks)
- tests/test_protocol_cybinary.py (1 hunks)
- thriftpy2/contrib/aio/protocol/binary.py (7 hunks)
- thriftpy2/contrib/aio/protocol/compact.py (2 hunks)
- thriftpy2/protocol/binary.py (7 hunks)
- thriftpy2/protocol/compact.py (3 hunks)
- thriftpy2/protocol/cybin/cybin.pyx (10 hunks)
Additional comments: 38
tests/test_aio_protocol_binary.py (1)
- 27-33: The test
test_strict_decodeis well-constructed to verify that theUnicodeDecodeErroris raised when strict decoding is enabled and an invalid UTF-8 sequence is encountered. The use ofAsyncBytesIOto wrapBytesIOis appropriate for simulating asynchronous I/O in the test environment.tests/test_aio_protocol_compact.py (1)
- 26-33: The test
test_strict_decodeis well-structured and correctly tests the newstrict_decodefeature by expecting aUnicodeDecodeErrorwhen an invalid UTF-8 sequence is encountered. The use ofAsyncBytesIOandTAsyncCompactProtocolis appropriate for the asynchronous context of the test.tests/test_protocol_binary.py (3)
5-5: The addition of the
pytestimport is appropriate for the new test cases that use it.100-100: The test case
test_unpack_binarycorrectly checks the behavior ofread_valwhen handling binary data without decoding.103-108: The test case
test_strict_decodeis well-designed to verify thatread_valraises aUnicodeDecodeErrorwhenstrict_decodeisTrueand an invalid UTF-8 sequence is encountered.tests/test_protocol_compact.py (2)
120-126: The test
test_strict_decodecorrectly checks forUnicodeDecodeErrorwhenstrict_decodeis set toTrueand an invalid UTF-8 sequence is encountered.117-131: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [2-129]
No further issues found in the rest of the file.
thriftpy2/contrib/aio/protocol/binary.py (9)
73-77: The addition of the
strict_decodeparameter to theread_valfunction aligns with the summary and the pull request's intent to provide strict decoding options.104-109: The implementation of the
strict_decodelogic in theread_valfunction for string decoding is consistent with the pull request's intent and the summary provided.126-131: The
read_valfunction is correctly called with thestrict_decodeparameter when reading lists, ensuring that the strict decoding behavior is propagated.158-165: The
read_valfunction is correctly called with thestrict_decodeparameter when reading maps, ensuring that the strict decoding behavior is propagated.169-170: The
read_structfunction is correctly called with thestrict_decodeparameter, ensuring that the strict decoding behavior is propagated when reading structs.245-252: The
TAsyncBinaryProtocolclass constructor has been updated to accept and store thestrict_decodeparameter, which is in line with the changes described in the summary.274-276: The
read_structmethod in theTAsyncBinaryProtocolclass correctly passes thestrict_decodeparameter, ensuring that the strict decoding behavior is used when reading structs.282-288: The
TAsyncBinaryProtocolFactoryclass constructor has been updated to accept and store thestrict_decodeparameter, which is in line with the changes described in the summary.290-297: The
get_protocolmethod in theTAsyncBinaryProtocolFactoryclass correctly passes thestrict_decodeparameter when creating a newTAsyncBinaryProtocolinstance.thriftpy2/contrib/aio/protocol/compact.py (2)
145-150: The implementation of the
strict_decodefeature in the_read_stringmethod is correct and aligns with the summary provided. It ensures that aUnicodeDecodeErroris raised whenstrict_decodeisTrueand an invalid UTF-8 sequence is encountered.308-318: The addition of the
strict_decodeparameter to theTAsyncCompactProtocolFactoryclass and its propagation to theTAsyncCompactProtocolconstructor is consistent with the changes described in the summary and pull request. This ensures that the newstrict_decodefeature is available when creating protocol instances through the factory.thriftpy2/protocol/binary.py (6)
218-219: The
read_valfunction has been correctly updated to include thestrict_decodeparameter in its signature, aligning with the changes described in the summary.249-254: The implementation of the
strict_decodelogic within theread_valfunction is correct. It attempts to decode the payload as UTF-8 and raises aUnicodeDecodeErrorifstrict_decodeisTrueand a decoding error occurs.316-316: The
read_structfunction has been correctly updated to include thestrict_decodeparameter in its signature, aligning with the changes described in the summary.387-394: The
TBinaryProtocolclass's__init__method has been correctly updated to include thestrict_decodeparameter, and the parameter is stored as an instance variable as intended.423-428: The
TBinaryProtocolFactoryclass's__init__method has been correctly updated to include thestrict_decodeparameter, and the parameter is stored as an instance variable as intended.299-319: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [218-313]
Verify that the
strict_decodeparameter is being passed correctly to all recursive calls ofread_valandread_structwithin thethriftpy2library, and ensure that the default value ofstrict_decodeis set toFalsein all places to maintain backward compatibility.thriftpy2/protocol/compact.py (3)
132-139: The addition of the
strict_decodeparameter to theTCompactProtocolconstructor is consistent with the summary and is implemented correctly.246-251: The changes to the
_read_stringmethod to raise aUnicodeDecodeErrorwhenstrict_decodeisTrueare consistent with the summary and correctly implemented.590-597: The addition of the
strict_decodeparameter to theTCompactProtocolFactoryconstructor and theget_protocolmethod is consistent with the summary and is implemented correctly.thriftpy2/protocol/cybin/cybin.pyx (11)
173-177: The addition of the
strict_decodeparameter to theread_structfunction with a default value ofFalseis correctly implemented and maintains backward compatibility.256-258: The addition of the
strict_decodeparameter to thec_read_stringfunction with a default value ofFalseis correctly implemented and maintains backward compatibility.267-269: The addition of the
strict_decodeparameter to thec_read_valfunction with a default value ofFalseis correctly implemented and maintains backward compatibility.440-442: The addition of the
strict_decodeparameter to theread_valfunction with a default value ofFalseis correctly implemented and maintains backward compatibility.451-462: The addition of the
strict_decodeparameter to theTCyBinaryProtocolclass with a default value ofFalseis correctly implemented and maintains backward compatibility.524-529: The addition of the
strict_decodeparameter to theTCyBinaryProtocolFactoryclass with a default value ofFalseis correctly implemented and maintains backward compatibility.259-264: The use of the
strict_decodeparameter within thec_read_stringfunction correctly implements the intended behavior of raising aUnicodeDecodeErrorwhenstrict_decodeisTrueand a decoding error occurs.203-204: The
strict_decodeparameter is correctly passed to thec_read_valfunction within theread_structfunction, ensuring that the strict decoding behavior is applied when reading struct fields.298-299: The
strict_decodeparameter is correctly passed to thec_read_stringfunction within thec_read_valfunction when the type isT_STRING, ensuring that the strict decoding behavior is applied when reading string values.508-510: The
strict_decodeparameter is correctly used in theread_structmethod of theTCyBinaryProtocolclass, ensuring that the strict decoding behavior is applied when reading messages.531-534: The
strict_decodeparameter is correctly used in theget_protocolmethod of theTCyBinaryProtocolFactoryclass, ensuring that the strict decoding behavior is applied when creating new protocol instances.
| def test_strict_decode(): | ||
| bs = TCyMemoryBuffer(b"\x00\x00\x00\x0c\x00" # there is a redundant '\x00' | ||
| b"\xe4\xbd\xa0\xe5\xa5\xbd\xe4\xb8\x96\xe7\x95\x8c") | ||
| with pytest.raises(UnicodeDecodeError): | ||
| proto.read_val(bs, TType.STRING, decode_response=True, | ||
| strict_decode=True) |
There was a problem hiding this comment.
The test test_strict_decode correctly checks for a UnicodeDecodeError when strict_decode is set to True. However, it might be beneficial to also verify the content of the error message to ensure that it matches the expected error.
There was a problem hiding this comment.
@aisk Understood, we'll proceed without adding a check for the error message content.
🐰
The string field will be encoded into python string (formerly unicode in python2) via UTF-8 encoding, and if the field is not a valid UTF-8 string, current implementatiion will fallback to return the raw bytes.
As a result, there would be a pitfall for users: if someone don't validate the result type and just pass it to database or somewhere else, there would be a dirty data.
This change add a
strict_decodeflag when create protocol (or protocol factory) to progate theUnicodeDecodeErrorto users so the PRC call will be failed as expected.The flag is not enabled by default for compatibility issue.
Protocols needs support:
Summary by CodeRabbit
New Features
Tests
Bug Fixes