Make parsing of values syntactically more strict with bad values generating an error #244

azeey · 2020-04-15T21:12:36Z

Currently, libsdformat silently ignores syntax errors when parsing values. For example, <pose>bad 0 0 0 0 0</pose> or <pose>0.1 0.2 0.3 0.4 0.5</pose> are both bad values for the <pose> tag, but the parsed values are <pose>0 0 0 0 0 0</pose> and <pose>0.1 0.2 0.3 0.4 0.5 0</pose> respectively. With this PR, libsdformat will generate an error for such values.

The plan is to make a similar PR for libsdformat9, but printing a warning message instead.

This resolves #228

j-rivero · 2020-04-17T15:01:05Z

@osrf-jenkins run tests

src/Param.cc

EricCousineau-TRI · 2020-05-07T18:35:31Z

meta comment: https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

scpeters · 2020-05-08T19:50:25Z

I think there are some test files missing that should start with bad_syntax_*

scpeters · 2020-05-08T20:09:44Z

src/Param.cc

+
+        ss >> posetmp;
+        this->dataPtr->value = posetmp;
+      }


I think the changes to this block are not needed because Param::SetFromString already calls sdf::trim to remove whitespace and then sets a default value if the trimmed string is empty. I've added tests to confirm this in 9daad39 of branch scpeters/stricter_parsing_10 after reverting the changes to this block in 8f6120c

Sounds good. I see that ValueFromString is also called form the constructor without sdf::trim, but that should be okay because it's using the default string from the spec description.
https://github.com/osrf/sdformat/blob/b80e070dbed613e972acfb81e66b24a248eefd6c/src/Param.cc#L70

I just tried it and it looks like UNIT_Utils_TEST fails with that change.

ok, it sounds like this change is a good idea. Perhaps we could generalize it so it applies to types other than Pose?

I think we discussed this in our last meeting, and this was the last change we expected to make?

I added an overload for sdf::trim that takes a std::string in fdecf77, and used that to trim input values in 32b3d84

azeey · 2020-05-08T20:14:28Z

I think there are some test files missing that should start with bad_syntax_*

Oops! I've added them in 2b42034

scpeters · 2020-05-08T22:13:16Z

there are test failures on macOS and windows; I've investigated the macOS failure, and it seems that the parsing the bad pose file doesn't generate an exception with the CI version of clang / libc++

codecov-commenter · 2020-05-27T18:36:41Z

Codecov Report

Merging #244 into sdf10 will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            sdf10     #244      +/-   ##
==========================================
+ Coverage   86.64%   86.66%   +0.01%     
==========================================
  Files          59       59              
  Lines        9067     9071       +4     
==========================================
+ Hits         7856     7861       +5     
+ Misses       1211     1210       -1

Impacted Files	Coverage Δ
include/sdf/Types.hh	`100.00% <ø> (ø)`
src/Param.cc	`92.15% <100.00%> (+0.09%)`	⬆️
src/Types.cc	`100.00% <100.00%> (ø)`
src/parser.cc	`77.52% <0.00%> (+0.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bb2614...e59c6d7. Read the comment docs.

scpeters · 2020-06-02T22:45:52Z

do you want to target this at sdf10?

…rating an error Signed-off-by: Addisu Z. Taddese <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

libc++ doesn't throw an exception on failbit, so check the bit by calling `fail()` Signed-off-by: Addisu Z. Taddese <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey · 2020-06-02T23:31:23Z

do you want to target this at sdf10?

Done.

…rating an error (#244) Currently, libsdformat silently ignores syntax errors when parsing values. For example, <pose>bad 0 0 0 0 0</pose> or <pose>0.1 0.2 0.3 0.4 0.5</pose> are both bad values for the <pose> tag, but the parsed values are <pose>0 0 0 0 0 0</pose> and <pose>0.1 0.2 0.3 0.4 0.5 0</pose> respectively. With this PR, libsdformat will generate an error for such values.

Signed-off-by: Steve Peters <[email protected]>

* Remove all deprecated *PoseFrame() APIs * Remove deprecated Pose(), SetPose() APIs * Migration guide: update per #244, #276 * Changelog for #308. Signed-off-by: Steve Peters <[email protected]>

…#308) * Remove all deprecated *PoseFrame() APIs * Remove deprecated Pose(), SetPose() APIs * Migration guide: update per gazebosim#244, gazebosim#276 * Changelog for gazebosim#308. Signed-off-by: Steve Peters <[email protected]>

* Remove all deprecated *PoseFrame() APIs * Remove deprecated Pose(), SetPose() APIs * Migration guide: update per #244, #276 * Changelog for #308. Signed-off-by: Steve Peters <[email protected]>

azeey mentioned this pull request Apr 16, 2020

Make alpha optional when parsing a Color from an input stream gazebosim/gz-math#106

Merged

azeey requested a review from scpeters April 16, 2020 21:09

azeey self-assigned this Apr 29, 2020

azeey mentioned this pull request Apr 29, 2020

Parsing of bad values (syntactically and semantically) should be more strict #260

Closed

EricCousineau-TRI reviewed May 7, 2020

View reviewed changes

src/Param.cc Outdated Show resolved Hide resolved

scpeters reviewed May 8, 2020

View reviewed changes

azeey added 8 commits June 2, 2020 18:29

Make parsing of values syntactically more strict with bad values gene…

08dc80c

…rating an error Signed-off-by: Addisu Z. Taddese <[email protected]>

Add missing files

f424210

Signed-off-by: Addisu Z. Taddese <[email protected]>

Update changelog

ff420d7

Signed-off-by: Addisu Z. Taddese <[email protected]>

Fix compiler warning

d917f0c

Signed-off-by: Addisu Z. Taddese <[email protected]>

Fix test failure on macOS

666bf83

libc++ doesn't throw an exception on failbit, so check the bit by calling `fail()` Signed-off-by: Addisu Z. Taddese <[email protected]>

Fix test error on windows

65b14d1

Signed-off-by: Addisu Z. Taddese <[email protected]>

Add an override for sdf::trim that takes a std::string

af89d2a

Signed-off-by: Addisu Z. Taddese <[email protected]>

Trim input before parsing values

e59c6d7

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey force-pushed the stricter_parsing_10 branch from 2fa77df to e59c6d7 Compare June 2, 2020 23:29

azeey changed the base branch from master to sdf10 June 2, 2020 23:29

scpeters approved these changes Jun 4, 2020

View reviewed changes

EricCousineau-TRI approved these changes Jun 4, 2020

View reviewed changes

azeey merged commit 1e21388 into sdf10 Jun 4, 2020

azeey deleted the stricter_parsing_10 branch June 4, 2020 18:47

scpeters added a commit to scpeters/sdformat that referenced this pull request Jun 27, 2020

Migration guide: update per gazebosim#244, gazebosim#276

304a554

Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Jun 27, 2020

Remove deprecated Pose(), PoseFrame() functions from sdf10 #308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make parsing of values syntactically more strict with bad values generating an error #244

Make parsing of values syntactically more strict with bad values generating an error #244

azeey commented Apr 15, 2020 •

edited

Loading

j-rivero commented Apr 17, 2020

EricCousineau-TRI commented May 7, 2020

scpeters commented May 8, 2020

scpeters May 8, 2020

azeey May 14, 2020

azeey May 14, 2020

scpeters May 29, 2020

scpeters Jun 2, 2020

azeey Jun 2, 2020

azeey commented May 8, 2020

scpeters commented May 8, 2020

codecov-commenter commented May 27, 2020 •

edited

Loading

scpeters commented Jun 2, 2020

azeey commented Jun 2, 2020

Make parsing of values syntactically more strict with bad values generating an error #244

Make parsing of values syntactically more strict with bad values generating an error #244

Conversation

azeey commented Apr 15, 2020 • edited Loading

j-rivero commented Apr 17, 2020

EricCousineau-TRI commented May 7, 2020

scpeters commented May 8, 2020

scpeters May 8, 2020

Choose a reason for hiding this comment

azeey May 14, 2020

Choose a reason for hiding this comment

azeey May 14, 2020

Choose a reason for hiding this comment

scpeters May 29, 2020

Choose a reason for hiding this comment

scpeters Jun 2, 2020

Choose a reason for hiding this comment

azeey Jun 2, 2020

Choose a reason for hiding this comment

azeey commented May 8, 2020

scpeters commented May 8, 2020

codecov-commenter commented May 27, 2020 • edited Loading

Codecov Report

scpeters commented Jun 2, 2020

azeey commented Jun 2, 2020

azeey commented Apr 15, 2020 •

edited

Loading

codecov-commenter commented May 27, 2020 •

edited

Loading