Copilot commented on code in PR #3595:
URL: https://github.com/apache/thrift/pull/3595#discussion_r3407591011
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -22,7 +22,9 @@
import uuid
import _import_local_thrift # noqa
+from thrift.Thrift import TApplicationException
from thrift.protocol.TBinaryProtocol import TBinaryProtocol
+from thrift.protocol.TBinaryProtocol import TBinaryProtocolAcceleratedFactory
Review Comment:
Importing `TBinaryProtocolAcceleratedFactory` at module import time can make
the entire test module fail to import in environments where that symbol isn’t
present, even though the test itself is guarded by `fastbinary` availability.
Consider importing `TBinaryProtocolAcceleratedFactory` inside `setUp()` or the
specific test with a `try/except ImportError` and skipping the test (or the
module) when unavailable.
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -280,6 +299,26 @@ def test_TBinaryProtocol_write_read(self):
print("Assertion fail")
raise e
+ def test_accelerated_utf8_roundtrip_on_application_exception(self):
+ if not self._has_fastbinary:
+ self.skipTest("C extension not available")
+
+ original = TApplicationException(
+ type=TApplicationException.PROTOCOL_ERROR,
+ message=("snowman-\u2603-rocket-\U0001F680-" * 32),
+ )
+
+ otrans = TTransport.TMemoryBuffer()
+ oproto =
TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(otrans)
+ oproto.trans.write(oproto._fast_encode(original,
APPLICATION_EXCEPTION_TYPEARGS))
+
+ itrans = TTransport.TMemoryBuffer(otrans.getvalue())
+ iproto =
TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(itrans)
+ decoded = iproto._fast_decode(None, iproto,
APPLICATION_EXCEPTION_TYPEARGS)
Review Comment:
This test calls private APIs (`_fast_encode` / `_fast_decode`) directly.
That’s brittle across Thrift versions and can fail even when `fastbinary` is
present but the protocol instance doesn’t expose these methods the same way.
Consider guarding with `hasattr(oproto, \"_fast_encode\")` / `hasattr(iproto,
\"_fast_decode\")` and skipping if absent, or using a public accelerated
encode/decode entrypoint if the library provides one.
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -280,6 +299,26 @@ def test_TBinaryProtocol_write_read(self):
print("Assertion fail")
raise e
+ def test_accelerated_utf8_roundtrip_on_application_exception(self):
+ if not self._has_fastbinary:
+ self.skipTest("C extension not available")
+
+ original = TApplicationException(
+ type=TApplicationException.PROTOCOL_ERROR,
+ message=("snowman-\u2603-rocket-\U0001F680-" * 32),
+ )
+
+ otrans = TTransport.TMemoryBuffer()
+ oproto =
TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(otrans)
+ oproto.trans.write(oproto._fast_encode(original,
APPLICATION_EXCEPTION_TYPEARGS))
Review Comment:
This test calls private APIs (`_fast_encode` / `_fast_decode`) directly.
That’s brittle across Thrift versions and can fail even when `fastbinary` is
present but the protocol instance doesn’t expose these methods the same way.
Consider guarding with `hasattr(oproto, \"_fast_encode\")` / `hasattr(iproto,
\"_fast_decode\")` and skipping if absent, or using a public accelerated
encode/decode entrypoint if the library provides one.
##########
lib/py/src/ext/protocol.tcc:
##########
@@ -456,18 +456,32 @@ bool ProtocolBase<Impl>::encodeValue(PyObject* value,
TType type, PyObject* type
case T_STRING: {
ScopedPyObject nval;
+ Py_ssize_t len;
if (PyUnicode_Check(value)) {
+#if PY_VERSION_HEX >= 0x03030000
+ const char* str = PyUnicode_AsUTF8AndSize(value, &len);
+ if (!str) {
+ return false;
+ }
+ if (!detail::check_ssize_t_32(len)) {
+ return false;
+ }
+
+ impl()->writeI32(static_cast<int32_t>(len));
+ return writeBuffer(const_cast<char*>(str), static_cast<size_t>(len));
Review Comment:
Using `const_cast<char*>` here is avoidable and potentially unsafe if
`writeBuffer` ever writes into the buffer (even accidentally). Prefer changing
`writeBuffer` to accept a `const char*` (or adding an overload) and pass `str`
through without casting. If changing the signature isn’t feasible, a safe
alternative is to copy into a mutable buffer before calling `writeBuffer` (with
the tradeoff of allocating).
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -167,6 +169,16 @@ def testField(type, data):
protocol.readStructEnd()
+APPLICATION_EXCEPTION_TYPEARGS = [
+ TApplicationException,
+ (
+ None,
+ (1, 11, "message", "UTF8", None),
+ (2, 8, "type", None, None),
+ ),
+]
Review Comment:
`*_TYPEARGS` in Thrift’s accelerated paths are typically tuples (and may be
used as cache keys or assumed immutable by downstream helpers). Using a list
here is more error-prone and can break if internals assume tuple semantics.
Consider making this a tuple (e.g., `(..., (...))`) to match common Thrift
fastbinary conventions and avoid accidental mutation between tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
