You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/09/21 01:58:28 UTC
Re: Review Request 61109: Used the default value when parsing an
optional enum field from JSON.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/
-----------------------------------------------------------
(Updated Sept. 21, 2017, 9:58 a.m.)
Review request for mesos, Benjamin Mahler and James Peach.
Changes
-------
Rebased and updated the commit message.
Summary (updated)
-----------------
Used the default value when parsing an optional enum field from JSON.
Bugs: MESOS-7828
https://issues.apache.org/jira/browse/MESOS-7828
Repository: mesos
Description (updated)
-------
Previously in MESOS-4997, we have made any inexistent enum value will
be parsed to the default enum value when parsing protobuf message from
a serialized string. Now in this patch, I made parsing protobuf message
from a JSON have the same behavior.
Diffs (updated)
-----
3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
Diff: https://reviews.apache.org/r/61109/diff/2/
Changes: https://reviews.apache.org/r/61109/diff/1-2/
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
Re: Review Request 61109: Fixed JSON protobuf deserialization to
ignore unrecognized enum values.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/
-----------------------------------------------------------
(Updated Oct. 2, 2017, 11:12 p.m.)
Review request for mesos, Benjamin Mahler and James Peach.
Changes
-------
Addressed review comments.
Summary (updated)
-----------------
Fixed JSON protobuf deserialization to ignore unrecognized enum values.
Bugs: MESOS-7828
https://issues.apache.org/jira/browse/MESOS-7828
Repository: mesos
Description (updated)
-------
Protobuf deserialization will discard any unrecognized enum values.
This patch fixes our custom JSON -> protobuf conversion code to be
consistent with this behavior.
See MESOS-4997 for why this matters when dealing with upgrades.
Fixes MESOS-7828.
Diffs (updated)
-----
3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
Diff: https://reviews.apache.org/r/61109/diff/4/
Changes: https://reviews.apache.org/r/61109/diff/3-4/
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
Re: Review Request 61109: Used the default value when parsing an
optional enum field from JSON.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review186710
-----------------------------------------------------------
Fix it, then Ship it!
Thanks guys, let's just clarify the commit summary and description a bit. The description is inaccurate in that it states that MESOS-4997 consisted of making inexistent enum values get parsed to the default. But this is just the behavior that protobuf de-serialization has always had and MESOS-4997 was just about coming up with pattern for our enums that reflect this when dealing with upgrades.
How about:
```
Fixed JSON protobuf deserialization to ignore unknown enum values.
Protobuf deserialization will discard any unknown enum values. This
patch fixes our custom JSON -> protobuf conversion code to be
consistent with this behavior.
See MESOS-4997 for why this matters when dealing with upgrades.
Fixes MESOS-7828.
```
3rdparty/stout/include/stout/protobuf.hpp
Lines 450-455 (original), 450-458 (patched)
<https://reviews.apache.org/r/61109/#comment263507>
Can you reference the quote from the protobuf documentation, for posterity?
> Notably, unrecognized enum values are discarded when the message is deserialized, which makes the field's has.. accessor return false and its getter return the first value listed in the enum definition.
From: https://developers.google.com/protocol-buffers/docs/proto#updating
- Benjamin Mahler
On Sept. 29, 2017, 12:24 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 12:24 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously in MESOS-4997, we have made any inexistent enum value will
> be parsed to the default enum value when parsing protobuf message from
> a serialized string. Now in this patch, I made parsing protobuf message
> from a JSON have the same behavior.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/3/
>
>
> 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
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field from JSON.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/
-----------------------------------------------------------
(Updated Sept. 29, 2017, 8:24 a.m.)
Review request for mesos, Benjamin Mahler and James Peach.
Changes
-------
Leave the enum field unset if it is not a required field, after parsing, its accessor will return the default enum value.
Bugs: MESOS-7828
https://issues.apache.org/jira/browse/MESOS-7828
Repository: mesos
Description
-------
Previously in MESOS-4997, we have made any inexistent enum value will
be parsed to the default enum value when parsing protobuf message from
a serialized string. Now in this patch, I made parsing protobuf message
from a JSON have the same behavior.
Diffs (updated)
-----
3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
Diff: https://reviews.apache.org/r/61109/diff/3/
Changes: https://reviews.apache.org/r/61109/diff/2-3/
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