> On Sept. 9, 2017, 7:59 a.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. > > Benjamin Mahler wrote: > 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`?
@Ben, the problem is when `Content-Type` is `application/x-protobuf`, our current implementation is an inexistent enum value will be parsed to the default enum value (i.e., `UNKNOWN`), that is what we have done in MESOS-4997, but when `Content-Type` is `application/json`, the current behavior is different: when parsing an inexistent enum value, we will get an error like `Failed to find enum for 'xxx'` rather than parsing it to the default enum value. So in this patch, I just want to make the two protocols (`application/x-protobuf` and `application/json`) have consistent behavior. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61109/#review185038 ----------------------------------------------------------- On July 25, 2017, 11: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, 11: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 > >
