Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle incomplete errors when parsing escaped string sequences #495

Merged
merged 2 commits into from
Apr 5, 2023

Conversation

nirosys
Copy link
Contributor

@nirosys nirosys commented Mar 28, 2023

Issue #, if available: #494

Description of changes:
This PR adds two changes:

  1. Updates error handling while parsing escaped string sequences in order to bubble up nom's Incomplete errors to the reader, so that they can be communicated further.
  2. Updates read_from to ensure that the amount of data read is limited to the length provided by the caller. Prior to this PR the entire rest of the buffer would be used for reading data. While this could be beneficial (the memory is allocated, using it makes sense) the caller expects to read no more than length, so as a matter of correctness we shouldn't read more than length.

A regression test was also added to mimic the behavior seen in #494.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nirosys nirosys marked this pull request as ready for review March 28, 2023 10:22
Comment on lines +1822 to +1823
reader.append_bytes(source[10..].as_bytes())?;
next_type(&mut reader, IonType::String, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I understanding this correctly? These lines are testing that you can append more data and have the parsing continue successfully, right? If so, it's probably a good idea to assert that we are getting the expected value, not just the expected type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I could add that. The primary purpose of the unit test though is to ensure the incomplete error is returned, since that is what the PR is changing. There are other unit tests that validate the parser's behavior when continuing from an incomplete, so imo the append_bytes, and next_type, could even be removed.

This test might be better off testing the escaped sequence (or string) parser directly rather than the reader.

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Patch coverage: 94.57% and project coverage change: +0.04 🎉

Comparison is base (6a05f30) 89.56% compared to head (3655857) 89.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #495      +/-   ##
==========================================
+ Coverage   89.56%   89.60%   +0.04%     
==========================================
  Files          80       82       +2     
  Lines       13951    14044      +93     
==========================================
+ Hits        12495    12584      +89     
- Misses       1456     1460       +4     
Impacted Files Coverage Δ
src/binary/non_blocking/raw_binary_reader.rs 81.79% <ø> (ø)
src/stream_reader.rs 100.00% <ø> (ø)
src/system_reader.rs 85.42% <ø> (ø)
src/element/mod.rs 89.55% <71.42%> (+0.12%) ⬆️
src/text/non_blocking/raw_text_reader.rs 89.34% <80.00%> (-0.17%) ⬇️
src/binary/raw_binary_reader.rs 90.90% <100.00%> (ø)
src/binary/raw_binary_writer.rs 94.78% <100.00%> (+0.01%) ⬆️
src/element/bytes.rs 100.00% <100.00%> (ø)
src/element/element_stream_reader.rs 86.50% <100.00%> (ø)
src/element/lob.rs 100.00% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nirosys nirosys merged commit ee11982 into amazon-ion:main Apr 5, 2023
@nirosys nirosys deleted the rgiliam/fatal_escaped_string_error branch April 24, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants