THRIFT-3364 Fix ruby binary field encoding in TJSONProtocol

Client: Ruby
Patch: Nobuaki Sukegawa <nsukeg@gmail.com>

This closes #633

6 files changed, 35 insertions, 18 deletions

diff --git a/lib/rb/lib/thrift/processor.rb b/lib/rb/lib/thrift/processor.rb
index b96fb43b9..fd312eee7 100644
--- a/lib/rb/lib/thrift/processor.rb
+++ b/lib/rb/lib/thrift/processor.rb
@@ -57,12 +57,10 @@ module Thrift
     end
 
     def write_error(err, oprot, name, seqid)
-      p 'write_error'
       oprot.write_message_begin(name, MessageTypes::EXCEPTION, seqid)
       err.write(oprot)
       oprot.write_message_end
       oprot.trans.flush
-      p 'write_error end'
     end
   end
 end
diff --git a/lib/rb/lib/thrift/protocol/json_protocol.rb b/lib/rb/lib/thrift/protocol/json_protocol.rb
index 9f0069d68..4d6186c60 100644
--- a/lib/rb/lib/thrift/protocol/json_protocol.rb
+++ b/lib/rb/lib/thrift/protocol/json_protocol.rb
@@ -18,6 +18,7 @@
 # under the License.
 # 
 
+require 'base64'
 
 module Thrift
   class LookaheadReader
@@ -310,7 +311,7 @@ module Thrift
     def write_json_base64(str)
       @context.write(trans)
       trans.write(@@kJSONStringDelimiter)
-      write_json_string([str].pack("m"))
+      trans.write(Base64.strict_encode64(str))
       trans.write(@@kJSONStringDelimiter)
     end
 
@@ -546,7 +547,15 @@ module Thrift
 
     # Reads a block of base64 characters, decoding it, and returns via str
     def read_json_base64
-      read_json_string.unpack("m")[0]
+      str = read_json_string
+      m = str.length % 4
+      if m != 0
+        # Add missing padding
+        (4 - m).times do
+          str += '='
+        end
+      end
+      Base64.strict_decode64(str)
     end
 
     # Reads a sequence of characters, stopping at the first one that is not
diff --git a/lib/rb/spec/binary_protocol_spec_shared.rb b/lib/rb/spec/binary_protocol_spec_shared.rb
index c615b5852..7a9d02872 100644
--- a/lib/rb/spec/binary_protocol_spec_shared.rb
+++ b/lib/rb/spec/binary_protocol_spec_shared.rb
@@ -423,9 +423,9 @@ shared_examples_for 'a binary protocol' do
     clientproto = protocol_class.new(clientside)
     serverproto = protocol_class.new(serverside)
 
-    processor = Srv::Processor.new(SrvHandler.new)
+    processor = Thrift::Test::Srv::Processor.new(SrvHandler.new)
 
-    client = Srv::Client.new(clientproto, clientproto)
+    client = Thrift::Test::Srv::Client.new(clientproto, clientproto)
 
     # first block
     firstblock.call(client)
diff --git a/lib/rb/spec/compact_protocol_spec.rb b/lib/rb/spec/compact_protocol_spec.rb
index daad5838e..8a1a228d6 100644
--- a/lib/rb/spec/compact_protocol_spec.rb
+++ b/lib/rb/spec/compact_protocol_spec.rb
@@ -75,11 +75,11 @@ describe Thrift::CompactProtocol do
     trans = Thrift::MemoryBufferTransport.new
     proto = Thrift::CompactProtocol.new(trans)
 
-    struct = CompactProtoTestStruct.new
+    struct = Thrift::Test::CompactProtoTestStruct.new
     # sets and maps don't hash well... not sure what to do here.
     struct.write(proto)
 
-    struct2 = CompactProtoTestStruct.new
+    struct2 = Thrift::Test::CompactProtoTestStruct.new
     struct2.read(proto)    
     struct2.should == struct
   end
@@ -91,9 +91,9 @@ describe Thrift::CompactProtocol do
     client_in_trans = Thrift::MemoryBufferTransport.new
     client_in_proto = Thrift::CompactProtocol.new(client_in_trans)
 
-    processor = Srv::Processor.new(JankyHandler.new)
+    processor = Thrift::Test::Srv::Processor.new(JankyHandler.new)
 
-    client = Srv::Client.new(client_in_proto, client_out_proto)
+    client = Thrift::Test::Srv::Client.new(client_in_proto, client_out_proto)
     client.send_Janky(1)
     # puts client_out_trans.inspect_buffer
     processor.process(client_out_proto, client_in_proto)
@@ -101,9 +101,9 @@ describe Thrift::CompactProtocol do
   end
   
   it "should deal with fields following fields that have non-delta ids" do
-    brcp = BreaksRubyCompactProtocol.new(
+    brcp = Thrift::Test::BreaksRubyCompactProtocol.new(
       :field1 => "blah", 
-      :field2 => BigFieldIdStruct.new(
+      :field2 => Thrift::Test::BigFieldIdStruct.new(
         :field1 => "string1", 
         :field2 => "string2"), 
       :field3 => 3)
@@ -111,18 +111,18 @@ describe Thrift::CompactProtocol do
     bytes = ser.serialize(brcp)
 
     deser = Thrift::Deserializer.new(Thrift::CompactProtocolFactory.new)
-    brcp2 = BreaksRubyCompactProtocol.new
+    brcp2 = Thrift::Test::BreaksRubyCompactProtocol.new
     deser.deserialize(brcp2, bytes)
     brcp2.should == brcp
   end
   
   it "should deserialize an empty map to an empty hash" do
-    struct = SingleMapTestStruct.new(:i32_map => {})
+    struct = Thrift::Test::SingleMapTestStruct.new(:i32_map => {})
     ser = Thrift::Serializer.new(Thrift::CompactProtocolFactory.new)
     bytes = ser.serialize(struct)
 
     deser = Thrift::Deserializer.new(Thrift::CompactProtocolFactory.new)
-    struct2 = SingleMapTestStruct.new
+    struct2 = Thrift::Test::SingleMapTestStruct.new
     deser.deserialize(struct2, bytes)
     struct.should == struct2
   end
diff --git a/lib/rb/spec/json_protocol_spec.rb b/lib/rb/spec/json_protocol_spec.rb
index 2f7f1e6b2..9fb6b7bfd 100644
--- a/lib/rb/spec/json_protocol_spec.rb
+++ b/lib/rb/spec/json_protocol_spec.rb
@@ -57,7 +57,7 @@ describe 'JsonProtocol' do
 
     it "should write json base64" do
       @prot.write_json_base64("this is a base64 string")
-      @trans.read(@trans.available).should == "\"\"dGhpcyBpcyBhIGJhc2U2NCBzdHJpbmc=\\n\"\""
+      @trans.read(@trans.available).should == "\"dGhpcyBpcyBhIGJhc2U2NCBzdHJpbmc=\""
     end
 
     it "should write json integer" do
@@ -244,7 +244,12 @@ describe 'JsonProtocol' do
 
     it "should write binary" do
       @prot.write_binary("this is a base64 string")
-      @trans.read(@trans.available).should == "\"\"dGhpcyBpcyBhIGJhc2U2NCBzdHJpbmc=\\n\"\""
+      @trans.read(@trans.available).should == "\"dGhpcyBpcyBhIGJhc2U2NCBzdHJpbmc=\""
+    end
+
+    it "should write long binary" do
+      @prot.write_binary((0...256).to_a.pack('C*'))
+      @trans.read(@trans.available).should == "\"AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5fYGFiY2RlZmdoaWprbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6PkJGSk5SVlpeYmZqbnJ2en6ChoqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/wMHCw8TFxsfIycrLzM3Oz9DR0tPU1dbX2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v8PHy8/T19vf4+fr7/P3+/w==\""
     end
 
     it "should get type name for type id" do
@@ -503,6 +508,11 @@ describe 'JsonProtocol' do
       @trans.write("\"dGhpcyBpcyBhIHRlc3Qgc3RyaW5n\"")
       @prot.read_binary.should == "this is a test string"
     end
+
+    it "should read long binary" do
+      @trans.write("\"AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5fYGFiY2RlZmdoaWprbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6PkJGSk5SVlpeYmZqbnJ2en6ChoqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/wMHCw8TFxsfIycrLzM3Oz9DR0tPU1dbX2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v8PHy8/T19vf4+fr7/P3+/w==\"")
+      @prot.read_binary.bytes.to_a.should == (0...256).to_a
+    end
   end
 
   describe Thrift::JsonProtocolFactory do
diff --git a/lib/rb/spec/spec_helper.rb b/lib/rb/spec/spec_helper.rb
index 3672bf065..5bf98d077 100644
--- a/lib/rb/spec/spec_helper.rb
+++ b/lib/rb/spec/spec_helper.rb
@@ -54,7 +54,7 @@ require 'thrift_spec_types'
 require 'nonblocking_service'
 
 module Fixtures
-  COMPACT_PROTOCOL_TEST_STRUCT = COMPACT_TEST.dup
+  COMPACT_PROTOCOL_TEST_STRUCT = Thrift::Test::COMPACT_TEST.dup
   COMPACT_PROTOCOL_TEST_STRUCT.a_binary = [0,1,2,3,4,5,6,7,8].pack('c*')
   COMPACT_PROTOCOL_TEST_STRUCT.set_byte_map = nil
   COMPACT_PROTOCOL_TEST_STRUCT.map_byte_map = nil