-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
[Ruby] #to_h
and #to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
when active_support/core_ext/object/json
loaded
#17037
Comments
.to_h
and .to_json
with Hash/Array/Struct etc returns {"descriptor":[{}{}{}...{}]}
.to_h
and .to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
.to_h
and .to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
.to_h
and .to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
when active_support/core_ext/object/json
loaded
.to_h
and .to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
when active_support/core_ext/object/json
loaded#to_h
and #to_json
with Hash/Array/Struct etc returns {"descriptor":[{}...{}]}
when active_support/core_ext/object/json
loaded
Hi @jufemaiz, can you clarify exactly what the proposed resolution is here? Would that change break any existing use cases? |
There's two options, with one that I would lean towards due to Matz's language design.
Digging into this further, protobuf/ruby/lib/google/protobuf/ffi/internal/convert.rb Lines 190 to 210 in 0302c4c
#to_h implementation (that #to_h sometimes (?) directly calls), however the conventions of ruby are not being adopted, causing issues elsewhere there is an expectation of convention.
|
Please note that the protobuf/ruby/ext/google/protobuf_c/message.c Lines 831 to 840 in 8d8db9c
I don't quite understand how options (1) and (2) are different. What's an example of how we might want |
Ah fair enough on the I am not sure why, but there may be reasons that the core team want a, by convention, internal function to differ from the external function. If there's no reason, aliasing one way or another would resolve the issue – and given the lack of implementation of |
Introducing |
How would you like to proceed on this? Changes to the c code? |
We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment. This issue is labeled |
Ideally this could be done at the Ruby level, with a method alias like some of the ones we have here: https://github.com/protocolbuffers/protobuf/blob/main/ruby/lib/google/protobuf/message_exts.rb Does that seem viable? That would save us from having to duplicate this between the different backends we currently have (C, FFI, JRuby). |
What version of protobuf and what language are you using?
Version: main/v3.27.0+ etc.
Language: Ruby
What operating system (Linux, Windows, ...) and version?
MacOS 14.5
What runtime / compiler are you using (e.g., python version or gcc version) Ruby v3.2
What did you do?
Build a library with it (or anything else). Then include it and build something containing a protobuf message. For example:
A key thing to note is this quote from Matz a decade ago:
ActiveSupport
rightfully makes use of the:to_hash
method for implicitly transforming anObject
to JSON format.Ref: https://github.com/rails/rails/blame/v7.1.3.4/activesupport/lib/active_support/core_ext/object/json.rb#L58-L66
Unfortunately attempting to call
#to_hash
as opposed to#to_h
fails on protobuf messages. Aliasing#to_h
for#to_hash
calls seems a sensible option here in the absence of shifting the#to_json
method to also make use of#to_hash
as Matz implies is correct.What did you expect to see
First, when calling
#to_h
expect to see the marshalled hash even when nested. That is:Second, when calling
#to_json
expect to see the marshalled json, even when nested. That is:What did you see instead?
Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).
When calling
#to_h
:When calling
#to_json
:Anything else we should know about your project / environment
Possibly related to #9500
The text was updated successfully, but these errors were encountered: