You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/10/11 05:05:19 UTC
Review Request 39211: Check for trailing characters in JSON::parse().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/
-----------------------------------------------------------
Review request for mesos, Adam B and Joseph Wu.
Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698
Repository: mesos
Description
-------
Check for trailing characters in JSON::parse().
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
Diff: https://reviews.apache.org/r/39211/diff/
Testing
-------
Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
Thanks,
Greg Mann
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review102283
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 741)
<https://reviews.apache.org/r/39211/#comment159873>
Nit: s/picojson/PicoJson/
(For consistency with other comments about PicoJson)
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)
<https://reviews.apache.org/r/39211/#comment159878>
Nit: We don't put periods in error messages.
- Joseph Wu
On Oct. 10, 2015, 8:05 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2015, 8:05 p.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review103368
-----------------------------------------------------------
Ship it!
Committing..
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 757)
<https://reviews.apache.org/r/39211/#comment161385>
Okay, I finally did the math too, and this checks out now. Tricky off-by-ones and pointer arithmetic. If this was anything more than an error message, I'd ask you to break this into a couple of intermediate variable assignments with comments.
- Adam B
On Oct. 20, 2015, 10:13 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 10:13 a.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 5:13 p.m.)
Review request for mesos, Adam B and Joseph Wu.
Changes
-------
Adjusted index.
Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698
Repository: mesos
Description
-------
Check for trailing characters in JSON::parse().
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
Diff: https://reviews.apache.org/r/39211/diff/
Testing
-------
Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
Thanks,
Greg Mann
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 4:22 p.m.)
Review request for mesos, Adam B and Joseph Wu.
Changes
-------
Addressed comments.
Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698
Repository: mesos
Description
-------
Check for trailing characters in JSON::parse().
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
Diff: https://reviews.apache.org/r/39211/diff/
Testing
-------
Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
Thanks,
Greg Mann
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 20, 2015, 7:28 a.m., Adam B wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, lines 744-748
> > <https://reviews.apache.org/r/39211/diff/3/?file=1097521#file1097521line744>
> >
> > I'm getting confused between `last_char` and `end`, since `end` isn't really the `end`. Also, we don't use camel_case. How about: `lastVisibleChar` and `parseEnd`? Maybe `parseBegin` too?
Yea good call, I like your suggested names. Thanks!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review103223
-----------------------------------------------------------
On Oct. 20, 2015, 4:22 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 4:22 p.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review103223
-----------------------------------------------------------
Ship it!
Sorry for the delay. Just a concern about confusing names and then I'm ready to commit.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 744 - 748)
<https://reviews.apache.org/r/39211/#comment161213>
I'm getting confused between `last_char` and `end`, since `end` isn't really the `end`. Also, we don't use camel_case. How about: `lastVisibleChar` and `parseEnd`? Maybe `parseBegin` too?
- Adam B
On Oct. 13, 2015, 10:51 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 10:51 a.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/
-----------------------------------------------------------
(Updated Oct. 13, 2015, 5:51 p.m.)
Review request for mesos, Adam B and Joseph Wu.
Changes
-------
Addressed comments.
Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698
Repository: mesos
Description
-------
Check for trailing characters in JSON::parse().
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
Diff: https://reviews.apache.org/r/39211/diff/
Testing
-------
Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
Thanks,
Greg Mann
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 1:13 a.m., Adam B wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp, line 252
> > <https://reviews.apache.org/r/39211/diff/2/?file=1096253#file1096253line252>
> >
> > But parsing this as an array should succeed without error, correct? Can we validate that in the test too?
Well this string is missing the outer square brackets, so strictly speaking I don't think it's a valid JSON array. PicoJSON will happily parse the first object, ignoring the rest of the string, and I would argue that it should throw an error since it's missing the [].
> On Oct. 13, 2015, 1:13 a.m., Adam B wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 754
> > <https://reviews.apache.org/r/39211/diff/2/?file=1096251#file1096251line754>
> >
> > Worth including s.substring(end..last_char) in the error message, so the user knows what trailing chars?
Good call!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review102372
-----------------------------------------------------------
On Oct. 13, 2015, 5:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 5:51 p.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/#review102372
-----------------------------------------------------------
Looks great! Just a couple of suggestions to expand unit testing & logging.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)
<https://reviews.apache.org/r/39211/#comment159997>
Worth including s.substring(end..last_char) in the error message, so the user knows what trailing chars?
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 232)
<https://reviews.apache.org/r/39211/#comment159999>
Could also test the positive case that we correctly parse (no error) a jsonString with a valid element and only whitespace trailing.
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 252)
<https://reviews.apache.org/r/39211/#comment159998>
But parsing this as an array should succeed without error, correct? Can we validate that in the test too?
- Adam B
On Oct. 12, 2015, 11:29 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 11:29 a.m.)
>
>
> Review request for mesos, Adam B and Joseph Wu.
>
>
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Check for trailing characters in JSON::parse().
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
>
> Diff: https://reviews.apache.org/r/39211/diff/
>
>
> Testing
> -------
>
> Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39211: Check for trailing characters in
JSON::parse().
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39211/
-----------------------------------------------------------
(Updated Oct. 12, 2015, 6:29 p.m.)
Review request for mesos, Adam B and Joseph Wu.
Changes
-------
Addressed comments.
Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698
Repository: mesos
Description
-------
Check for trailing characters in JSON::parse().
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0870300c2d372722b4850155cb2a8e848b986ed5
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp b95667f4c1a1c170016da024a5e4af02ccaa064e
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 131d39b87e571f16c774a133eb3148e98d8b9dd4
Diff: https://reviews.apache.org/r/39211/diff/
Testing
-------
Added tests to make sure that JSON::parse() is successfully returning errors when passed invalid JSON objects. Ran `make check`.
Thanks,
Greg Mann