-
Notifications
You must be signed in to change notification settings - Fork 4.1k
THRIFT-5910: Add UUID support in Ruby #3254
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
base: master
Are you sure you want to change the base?
Conversation
Starting with Ruby 3.4, base64 is no longer a bundled gem, and now Thrift does not load in modern Ruby versions:
An error occurred while loading ./spec/types_spec.rb.
Failure/Error: require 'base64'
LoadError:
cannot load such file -- base64
# ./lib/thrift/protocol/json_protocol.rb:21:in '<top (required)>'
# ./lib/thrift.rb:45:in '<top (required)>'
# ./spec/spec_helper.rb:30:in '<top (required)>'
# ./spec/types_spec.rb:20:in '<top (required)>'
Ruby already has ability to serialize and deserialize base64 without requiring the base64 gem,
which is a thin wrapper for syntactic sugar.
Additionally, the code throws a warning at the moment, which will become an error in Ruby 3.5:
/code/lib/thrift/processor.rb:20: warning: logger was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.
Added logger as an explicit dependency to avoid this warning.
Fixnum type has been deprecated since Ruby 2.4 and removed in Ruby 3.0,
which makes BaseProtocol incompatible with modern Ruby versions.
This change removes the Fixnum reference, as well as a monkey-patch that
introduces Fixnum#ord() to Ruby versions below 1.8.7.
**This change essentially makes it officially required to use Ruby 2.4 or newer.**
Fixes the following test failures:
1) BaseProtocol Thrift::BaseProtocol should write out the different types (deprecated write_type signature)
Failure/Error: if field_info.is_a? Fixnum
NameError:
uninitialized constant Thrift::BaseProtocol::Fixnum
# ./lib/thrift/protocol/base_protocol.rb:256:in 'Thrift::BaseProtocol#write_type'
# ./spec/base_protocol_spec.rb:65:in 'block (3 levels) in <top (required)>'
2) BaseProtocol Thrift::BaseProtocol should write out the different types
Failure/Error: if field_info.is_a? Fixnum
NameError:
uninitialized constant Thrift::BaseProtocol::Fixnum
# ./lib/thrift/protocol/base_protocol.rb:256:in 'Thrift::BaseProtocol#write_type'
# ./spec/base_protocol_spec.rb:90:in 'block (3 levels) in <top (required)>'
3) BaseProtocol Thrift::BaseProtocol should read the different types (deprecated read_type signature)
Failure/Error: if field_info.is_a? Fixnum
NameError:
uninitialized constant Thrift::BaseProtocol::Fixnum
# ./lib/thrift/protocol/base_protocol.rb:296:in 'Thrift::BaseProtocol#read_type'
# ./spec/base_protocol_spec.rb:113:in 'block (3 levels) in <top (required)>'
4) BaseProtocol Thrift::BaseProtocol should read the different types
Failure/Error: if field_info.is_a? Fixnum
NameError:
uninitialized constant Thrift::BaseProtocol::Fixnum
# ./lib/thrift/protocol/base_protocol.rb:296:in 'Thrift::BaseProtocol#read_type'
# ./spec/base_protocol_spec.rb:136:in 'block (3 levels) in <top (required)>'
…ation
This merge request addresses the following test failures:
```
1) Thrift::ThinHTTPServer::RackApplication 404 response receives a non-POST
Failure/Error: expect(last_response.status).to be 404
expected #<Integer:809> => 404
got #<Integer:1001> => 500
Compared using equal?, which compares object identity,
but expected and actual are not the same object. Use
`expect(actual).to eq(expected)` if you don't care about
object identity in this example.
# ./spec/thin_http_server_spec.rb:102:in 'block (3 levels) in <top (required)>'
2) Thrift::ThinHTTPServer::RackApplication 404 response receives a header other than application/x-thrift
Failure/Error: expect(last_response.status).to be 404
expected #<Integer:809> => 404
got #<Integer:1001> => 500
Compared using equal?, which compares object identity,
but expected and actual are not the same object. Use
`expect(actual).to eq(expected)` if you don't care about
object identity in this example.
# ./spec/thin_http_server_spec.rb:108:in 'block (3 levels) in <top (required)>'
3) Thrift::ThinHTTPServer::RackApplication 200 response status code 200
Failure/Error: expect(last_response.ok?).to be_truthy
expected: truthy value
got: false
# ./spec/thin_http_server_spec.rb:135:in 'block (3 levels) in <top (required)>'
```
From the [Rack documentation](https://rack.github.io/rack/2.2/Rack/Response.html),
> Your application’s call should end returning [Response#finish](https://rack.github.io/rack/2.2/Rack/Response.html#method-i-finish).
Additionally:
* using identity checks on integers produces weird expectation "expected #<Integer:809> => 404", which is not necessary. Switched to using `eq` matcher
* `be_truthy` matcher is overly generic and identity matching on `true`/`false` is preferred: "expected true" output will be displayed on failure
|
Please look at my findings here #3144 to ensure that when the ruby code (your implementation) sends a UUID to the .NET client/server that the values match on the other side. Not an echo test since that only ensures that the client and server has the same understanding, this must specifically be one implementation sending it and the other implementation correctly extracting it There still is no unit tests to confirm this but it would be great if you can do a manual check to ensure that it corresponds to ensure there is a common understanding of the underlying data layout. |
|
I did a full cross-test suite verification with C++, Java, Rust, and Go. Will review your findings as well Edit: I see what you mean, will check what needs to be updated on Ruby cross-tests side. Thank you! Edit2: And you were right: Great finding, thank you! |
|
Unfortunately it is a manual test, run the test and manually check the output of the client and server. And assume the .net implementation is correct |
|
Found another inconsistency while testing constants THRIFT-5911: produces GEN_UUID = %q"00000000-4444-CCCC-ffff-0123456789ab"
GEN_GUID = %q"00112233-4455-6677-8899-aaBBccDDeeFF"
MY_UUID = %q"00000000-4444-CCCC-ffff-0123456789ab"
MY_GUID = %q"{00112233-4455-6677-8899-aaBBccDDeeFF}"Seems like a bit of an issue with the compiler. |
Currently in draft state until CI fixes are merged in my other pull requests.
This PR adds UUID support to Ruby. The UUID is stored as a string, as this is a common format returned by
SecureRandom.uuidin Ruby. This adds a little bit overhead on writing, as the format of the UUID is verified there.[skip ci]anywhere in the commit message to free up build resources.