mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 61109: Used the default value when parsing an optional enum field.
Date Sun, 10 Sep 2017 14:48:53 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message