search

facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

Fixes to make TJSONProtocol work #215

Closed andrewcox closed 10 years ago

haijunz commented 10 years ago

Code looks good to me. Just a few comments:

  1. Can you add a test with strings containing non-ascii chars?
  2. Do I understand it correctly: before this diff string isn't supported internally so it is coerced to/from binary, after this diff string is supported and will be ser/deser directly just as other internal types.
haijunz commented 10 years ago

With TJSONProtocol, a string will be coerced to binary and TJSONProtocol will base64 encode it. When deserializing it will be base64 decoded and coerced back to string. So data isn't corrupted but just the base64 part is unnecessary?

andrewcox commented 10 years ago

1) Sure. Added a test for encoding/decoding a UTF8 test string. 2) Correct

And you're right, round-trip testing generally would have generally worked, even though we were improperly base64-encoding strings in TJSONProtocol. As long as both sides (read & write) happened using the broken assumption that string should always be handled using writeBinary/readBinary. However, only swift used this assumption so if TJSONProtocol data came from any other thrift implementation to swift, it would have gotten corrupted during decode, and if swift sent TJSONProtocol data to any other thrift implementation, it would have sent corrupted data.

haijunz commented 10 years ago

LGTM