Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thrift binary type outputs all bytes in TTEXT protocol #5768

Open
kasecato opened this issue Jun 17, 2024 · 4 comments
Open

Thrift binary type outputs all bytes in TTEXT protocol #5768

kasecato opened this issue Jun 17, 2024 · 4 comments
Labels
Milestone

Comments

@kasecato
Copy link

kasecato commented Jun 17, 2024

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

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

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
        }
@ikhoon ikhoon added the defect label Jun 17, 2024
@ikhoon ikhoon added this to the 1.30.0 milestone Jun 17, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Jun 17, 2024

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

By the way, how did you find the bug?

@kasecato
Copy link
Author

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
Copy link
Contributor

ikhoon commented Jun 17, 2024

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
Copy link
Member

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.

@ikhoon ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants