> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/protobuf.hpp > > Lines 391-487 (patched) > > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line391> > > > > It looks like we can avoid this logic since protobuf's JSON conversion > > allows the protobuf key types (bool, integers, strings) to be converted > > from JSON strings. This means we could just recurse for both key and value > > here: > > > > ``` > > // Some comment explaining what map is equivalent to with > > // a reference to the google documentation. > > google::protobuf::Message* entry = > > reflection->AddMessage(message, field); > > > > const google::protobuf::FieldDescriptor* key_field = > > entry->GetDescriptor()->FindFieldByNumber(1); > > > > // Maybe passing just 'key' works due to implicit > > construction? > > // TODO(...): This copies the key, consider avoiding this. > > Try<Nothing> apply = > > boost::apply_visitor(Parser(entry, key_field), > > JSON::String(key)); > > > > if (apply.isError()) { > > return Error(apply.error()); > > } > > > > const google::protobuf::FieldDescriptor* value_field = > > entry->GetDescriptor()->FindFieldByNumber(2); > > > > Try<Nothing> apply = > > boost::apply_visitor(Parser(entry, value_field), value); > > > > if (apply.isError()) { > > return Error(apply.error()); > > } > > ``` > > > > Now, to make this simplification work, we need to update our JSON > > conversion in a separate patch to allow bools and integers to be parsed > > from JSON strings to match google's logic for conversion: > > > > > > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203 > > > > Documentation here: > > https://developers.google.com/protocol-buffers/docs/proto3#json > > Qian Zhang wrote: > > // Maybe passing just 'key' works due to implicit construction? > > We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` > because it cannot pass compilation: > ``` > > ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: > error: no member named 'apply_visitor' in 'std::__1::basic_string<char>' > ``` > or > ``` > > ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: > error: no member named 'apply_visitor' in 'JSON::String' > ``` > That is because `std::string` and `JSON::String` do not have a member > `apply_visitor`, so I think we have to use `JSON::Value` like this: > ``` > google::protobuf::Message* entry = > reflection->AddMessage(message, field); > > const google::protobuf::FieldDescriptor* key_field = > entry->GetDescriptor()->FindFieldByNumber(1); > > JSON::Value key(name); > > Try<Nothing> apply = > boost::apply_visitor(Parser(entry, key_field), key); > > if (apply.isError()) { > return Error(apply.error()); > } > > const google::protobuf::FieldDescriptor* value_field = > entry->GetDescriptor()->FindFieldByNumber(2); > > apply = boost::apply_visitor(Parser(entry, value_field), > value); > if (apply.isError()) { > return Error(apply.error()); > } > ``` > > > we need to update our JSON conversion in a separate patch to allow > bools and integers to be parsed from JSON strings to match google's logic for > conversion > > Did you mean we should improve the method below by adding more cases for > converting `JSON::String` to bools and integers? > ``` > Try<Nothing> operator()(const JSON::String& string) const > { > switch (field->type()) { > case google::protobuf::FieldDescriptor::TYPE_STRING: > ... > ``` > If so, then that means moving the code here (see below) into the above > method. But I think those code are specific for map support, however > `Try<Nothing> operator()(const JSON::String& string) const` is a generic > method for JSON string conversion, so I would still like to keep those code > as where they are now in this patch (i.e., the code path to handle map). > ``` > case google::protobuf::FieldDescriptor::TYPE_INT64: > case google::protobuf::FieldDescriptor::TYPE_SINT64: > case google::protobuf::FieldDescriptor::TYPE_SFIXED64: > { > Try<int64_t> key = numify<int64_t>(name); > ... > } > case google::protobuf::FieldDescriptor::TYPE_UINT64: > case google::protobuf::FieldDescriptor::TYPE_FIXED64: > { > Try<uint64_t> key = numify<uint64_t>(name); > ... > } > ... > ``` > > Chun-Hung Hsiao wrote: > Re: `JSON::Value`: It seems that there's no need to declare a new `key` > variable, and just pass `JSON::Value(name)` into `apply_visitor`. > > Re: `JSON::String` <-> bools and ints: Yes. Ben's actually suggesting > that we should allow this conversion in general, an an enhancement of stout's > JSON support, so these conversions need not to be specific for map support. > For example, if we implement this enhancement, we could parse JSON outputed > from Google's JSON function. You could probably prepend another patch for > this enhancement. > > Qian Zhang wrote: > > It seems that there's no need to declare a new key variable, and just > pass JSON::Value(name) into apply_visitor. > > I tried it before, but it can not pass the compilation. > ``` > ../../3rdparty/stout/include/stout/protobuf.hpp:408:15: error: no > matching function for call to 'apply_visitor' > boost::apply_visitor(Parser(entry, key_field), > JSON::Value(name)); > ^~~~~~~~~~~~~~~~~~~~ > > ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:82:1: > note: candidate function [with Visitor = protobuf::internal::Parser, > Visitable = JSON::Value] not > viable: expects an l-value for 2nd argument > apply_visitor(const Visitor& visitor, Visitable& visitable) > ``` > > > Re: JSON::String <-> bools and ints: Yes. Ben's actually suggesting > that we should allow this conversion in general, an an enhancement of stout's > JSON support, so these conversions need not to be specific for map support. > For example, if we implement this enhancement, we could parse JSON outputed > from Google's JSON function. You could probably prepend another patch for > this enhancement. > > OK, so we need this enhancement to make our stout JSON -> protobuf > conversion can handle more valid JSONs. The followings are valid in JSON > which can be successfully parsed by Google's protobuf utility function > `JsonStringToMessage()` but can NOT be parsed our stout (i.e., call > `JSON::parse()` to convert the JSON to a stout JSON object and then call > `protobuf::parse()` to parse the JSON object to a protobuf message). > ``` > "int32": "-2147483647" > "int64": "-9223372036854775807" > "bool": "true" > ``` > The second step (`protobuf::parse()`) will fail with an error like this: > ``` > Not expecting a JSON string for field 'int32' > ``` > This error comes from `Try<Nothing> operator()(const JSON::String& > string)`, so we need to enhance it to convert `JSON::String` to bools and > integers. I created a separate ticket > (https://issues.apache.org/jira/browse/MESOS-8656) for it, let's handle it > there.
Here is the patch (https://reviews.apache.org/r/66025/) for MESOS-8656, and I have rebased this patch chain on top of that patch. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59987/#review198429 ----------------------------------------------------------- On March 12, 2018, 3:15 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59987/ > ----------------------------------------------------------- > > (Updated March 12, 2018, 3:15 p.m.) > > > Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, > and Zhitao Li. > > > Bugs: MESOS-7656 > https://issues.apache.org/jira/browse/MESOS-7656 > > > Repository: mesos > > > Description > ------- > > Map is a feature of proto2 syntax, but it can only be compiled > with 3.0.0+ protobuf compiler, see the following discussion for > details: > > https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4 > > We have already upgraded the compiler from 2.6.1 to 3.3.0 in > MESOS-7228. This patch adds map support in the json <-> protobuf > conversion in stout. > > > Diffs > ----- > > 3rdparty/stout/include/stout/protobuf.hpp > 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a > > > Diff: https://reviews.apache.org/r/59987/diff/8/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
