ruby-protobuf / protobuf

A pure ruby implementation of Google's Protocol Buffers
https://github.com/ruby-protobuf
MIT License
462 stars 101 forks source link

Implement Enum#to_json to #385

Closed mattnichols closed 6 years ago

mattnichols commented 6 years ago

There is an issue in JrJackson RubyAnySerializer that does not render Protobuf::Enum correctly because it does not implement to_json. This fixes that issue here until the serializer can be patched.

@abrandoned @ah @brianstien

mattnichols commented 6 years ago

@brianstien 's simple repro of the problem in JrJackson:

jruby-9.1.12.0 :001 > class Foo < SimpleDelegator; end
 => nil 
jruby-9.1.12.0 :002 > JrJackson::Json.dump(:foo => Foo.new(1))
 => "{\"foo\":\"#<Foo>\"}"
# defining to_json will fix this, or should we define to_json_data?
jruby-9.1.12.0 :003 > class Foo < SimpleDelegator; def to_json; __getobj__.to_json; end; end
 => :to_json 
jruby-9.1.12.0 :004 > JrJackson::Json.dump(:foo => Foo.new(1))
 => "{\"foo\":1}"
abrandoned commented 6 years ago

@mattnichols I think this same functionality is accounted for in the other PR open https://github.com/ruby-protobuf/protobuf/pull/383

we should add the spec and see if we can merge in this spec with the other branch and make it more comprehensive

film42 commented 6 years ago

@mattnichols I merged #383 and rebased against master for you. Things are passing locally, so if travis is good to go, then I think we're ready to :rocket: .

brianstien commented 6 years ago

@film42 Can you remove your commit please? We did need that to_json method defined.

@mattnichols I think to_json should be to_i, not to_s

mattnichols commented 6 years ago

@film42 This is the code that is requires the explicit .to_json implementation. https://github.com/guyboertje/jrjackson/blob/master/src/main/java/com/jrjackson/RubyAnySerializer.java#L37

The code is looking for an explicit definition of to_json on the metaclass (it's not looking at parent classes)

This is the behavior with your code changes (minus the def to_json)

synchronicity#development >> require "protobuf"
=> false
synchronicity#development >> require "jrjackson"
=> false
synchronicity#development >>
synchronicity?                 class EnumTest < ::Protobuf::Enum; define :ONE, 1; end
=> #<Protobuf::Enum(EnumTest)::ONE=1>
synchronicity#development >> { :value => EnumTest::ONE }.to_json
=> "{\"value\":1}"
synchronicity#development >>
synchronicity?                 class Foo < SimpleDelegator; end
=> nil
synchronicity#development >> JrJackson::Json.dump(:foo => EnumTest::ONE)
=> "{\"foo\":\"#<EnumTest>\"}"
film42 commented 6 years ago

@mattnichols Excellent! I was thinking it was a to_json bug on the root object, not the individual fields, but that makes sense.

@brianstien I removed my commit, and since I was already pushing, made the to_i change as well.

brianstien commented 6 years ago

@film42 That should do it, thanks!

mattnichols commented 6 years ago
synchronicity#development >> require "protobuf"
=> false
synchronicity#development >> require "jrjackson"
=> false
synchronicity#development >> class EnumTest < ::Protobuf::Enum; define :ONE, 1; end
=> #<Protobuf::Enum(EnumTest)::ONE=1>
synchronicity#development >> { :value => EnumTest::ONE }.to_json
=> "{\"value\":1}"
synchronicity#development >> class Foo < SimpleDelegator; end
=> nil
synchronicity#development >> JrJackson::Json.dump(:foo => EnumTest::ONE)
=> "{\"foo\":1}"

Appears to be working now

film42 commented 6 years ago

Looks like travis is struggling. I'm running 2.5.1 locally and specs are passing:

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) Protobuf::Field.build 
     # Not yet implemented
     # ./spec/lib/protobuf/field_spec.rb:8

Finished in 20.07 seconds (files took 0.82061 seconds to load)
659 examples, 0 failures, 1 pending
film42 commented 6 years ago

I think this is good to go. @brianstien ? @abrandoned ?

abrandoned commented 6 years ago

:shipit: