Thrift binary type outputs all bytes in TTEXT protocol

[kasecato]

TTEXT parser of Armeria Thrift is calling values of binray type without reference to position and limit of ByteBuffer.

• https://github.com/line/armeria/blob/armeria-1.29.0/thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/TypedParser.java#L195

org.apache.thrift.TDeserializer still has the behavior of returning ByteBuffer with position and limit.

• https://github.com/apache/thrift/blob/v0.20.0/lib/java/src/main/java/org/apache/thrift/protocol/TBinaryProtocol.java#L485-L487
• https://issues.apache.org/jira/browse/THRIFT-3985

When binary type is deserialized with org.apache.thrift.TDeserializer, ByteBuffer has a reference to all bytes of the object with position and limit, and all bytes are retrieved when accessed with val.array().

Input	Method	Output	Expected
ByteBuffer[position=1 limit=11 capacity=12]	val.array()	12 bytes	11 bytes
	TBaseHelper.byteBufferToByteArray(val)	11 bytes	11 bytes

        public void writeValue(JsonGenerator jw, ByteBuffer val) throws IOException {
            jw.writeBinary(val.array()); // As-Is
        }

• https://github.com/apache/thrift/blob/v0.20.0/lib/java/src/main/java/org/apache/thrift/TBaseHelper.java#L235

[ikhoon]

It sounds like a bug. Are you interested in sending a PR?

By the way, how did you find the bug?

[kasecato]

I have no plans to create this PR since I'm not familiar with the scope of impact I was using the Thrift documentation service and found that Thrift binary was large just for TTEXT My use case is for storing data in Thrift binary messages in Redis, and it's deserialized again and responded to by Armeria Thrift service

[ikhoon]

I think you use TTEXT only for documentation services so the impact of the bug is not critical. I will fix the bug and include it in 1.30.0.

If not, please let me know.

[KarboniteKream]

We also use TTEXT for (de)serializing Thrift objects for CMS APIs, but we're aware that it's not really the intended use case of TTEXT. And we don't use binary fields there. So I feel like this kind of change should have minimum impact.