Skip to content

Conversation

@mhlakhani
Copy link
Contributor

@mhlakhani mhlakhani commented Jul 20, 2025

This commit fixes two bugs in the Rust Thrift implementation that prevented UUID fields from working correctly with the compact protocol:

  1. Missing UUID case in skip_till_depth function

    • The skip function in lib/rs/src/protocol/mod.rs did not handle TType::Uuid
    • This caused "cannot skip field type Uuid" errors when trying to skip UUID fields
    • Fixed by adding: TType::Uuid => self.read_uuid().map(|_| ())
  2. Incorrect UUID serialization in compact protocol

    • The compact protocol was using read_bytes()/write_bytes() for UUIDs
    • These methods include a length prefix, but UUIDs should be transmitted as raw 16-byte values per the Thrift specification
    • This caused "don't know what type: 15" errors during cross-language tests
    • Fixed by reading/writing UUID bytes directly without length prefix
    • Implementation now matches the binary protocol's approach for consistency

The bug manifested as failures in cross-language tests between Go and Rust when using compact protocol. The error occurred because Go correctly expected raw 16-byte UUID values while Rust was incorrectly adding a length prefix.

Testing:

  • Verified all go-rs and rs-go cross tests now pass with compact protocol
  • Tests confirm UUID values are correctly serialized and deserialized
  • Both buffered and framed transports work correctly

Test command: python3 test/test.py --server go --client rs -R ".compact."

🤖 Generated with Claude Code -- but I as a human reviewed every line, tested it, and ensured this is code I'm happy to put my name on. I used it to help reproduce the failure, figure out how to run the tests, and diagnose the issue - the diagnosis + fix is correct.

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes) --> no, trivial changes
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? --> N/A
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

This commit fixes two bugs in the Rust Thrift implementation that prevented
UUID fields from working correctly with the compact protocol:

1. Missing UUID case in skip_till_depth function
   - The skip function in lib/rs/src/protocol/mod.rs did not handle TType::Uuid
   - This caused "cannot skip field type Uuid" errors when trying to skip UUID fields
   - Fixed by adding: TType::Uuid => self.read_uuid().map(|_| ())

2. Incorrect UUID serialization in compact protocol
   - The compact protocol was using read_bytes()/write_bytes() for UUIDs
   - These methods include a length prefix, but UUIDs should be transmitted as
     raw 16-byte values per the Thrift specification
   - This caused "don't know what type: 15" errors during cross-language tests
   - Fixed by reading/writing UUID bytes directly without length prefix
   - Implementation now matches the binary protocol's approach for consistency

The bug manifested as failures in cross-language tests between Go and Rust
when using compact protocol. The error occurred because Go correctly expected
raw 16-byte UUID values while Rust was incorrectly adding a length prefix.

Testing:
- Verified all go-rs and rs-go cross tests now pass with compact protocol
- Tests confirm UUID values are correctly serialized and deserialized
- Both buffered and framed transports work correctly

Test command: python3 test/test.py --server go --client rs -R ".*compact.*"

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@CJCombrink
Copy link
Contributor

CJCombrink commented Jul 21, 2025

@mhlakhani Nice work.
Would it be possible to look at PR #3144 to ensure that the encoding is correct between at least rust and .net or go when mixing server and client.

@Jens-G made some effort to ensure Delphi .Net is correct: #3144 (comment)

@mhlakhani
Copy link
Contributor Author

@CJCombrink I'll give that a shot -- I have some ideas! Will try and find time at some point

@Jens-G Jens-G added the rust label Jul 22, 2025
@mhlakhani
Copy link
Contributor Author

@CJCombrink I'll try and take a look -- is the main end goal to ensure all the cross tests pass? Sorry I took a look and ti wasn't clear to me what the expected output was.

@CJCombrink
Copy link
Contributor

@mhlakhani

is the main end goal to ensure all the cross tests pass?

No sorry, that post is quite long but the test is rather short/easy.

Run a server of any other language (not Rust) and then connect the Rust client to the non-rust server for the test. The UUID printed on both sides should be the same. Then repeat it for the Rust server and another client.

The first post shows the command to run the C++ server and other commands to run the other clients.

So in summary: For example run the C++ server

/thrift/src/test/cpp/TestServer --port=37471

And then run the rust client connected to the C++ server and confirm that both print the UUID as expected to the console: 00112233-4455-6677-8899-aabbccddeeff

@mhlakhani
Copy link
Contributor Author

thanks @CJCombrink ! I'll take a look in a follow up.

@mhlakhani mhlakhani merged commit 57c5fc1 into apache:master Aug 3, 2025
23 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants