> On Sept. 8, 2017, 11:59 p.m., James Peach wrote: > > This looks pretty reasonable to me. It's unfortunate that this will convert > > all invalid enum names into the default value, but AFAICT that is > > unavoidable.
Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61109/#review185038 ----------------------------------------------------------- On July 25, 2017, 3:17 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61109/ > ----------------------------------------------------------- > > (Updated July 25, 2017, 3:17 p.m.) > > > Review request for mesos, Benjamin Mahler and James Peach. > > > Bugs: MESOS-7828 > https://issues.apache.org/jira/browse/MESOS-7828 > > > Repository: mesos > > > Description > ------- > > Used the default value when parsing an optional enum field. > > > Diffs > ----- > > 3rdparty/stout/include/stout/protobuf.hpp > 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 > > > Diff: https://reviews.apache.org/r/61109/diff/1/ > > > Testing > ------- > > With this patch, when accessing master endpoint with an inexistent enum `xxx` > in a JSON: > ``` > curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' > 127.0.0.1:5050/api/v1 > ``` > The master log will be: > ``` > I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from > 127.0.0.1:49566 with User-Agent='curl/7.47.0' > I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN > ``` > This proves when parsing an inexistent enum the default enum value (i.e., > `UNKNOWN`) will be used. > > > Thanks, > > Qian Zhang > >
