search

tafia / quick-xml

Rust high performance xml reader and writer
MIT License
1.22k stars 237 forks source link

Fix DocType closing bracket recognition in buffered reader #802

Closed BlueGreenMagick closed 1 month ago

BlueGreenMagick commented 2 months ago

Fixes #533, fixes #590, fixes #801

codecov-commenter commented 2 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.17%. Comparing base (7558577) to head (3ebe221). Report is 96 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #802 +/- ## ========================================== - Coverage 61.81% 60.17% -1.65% ========================================== Files 41 41 Lines 16798 15958 -840 ========================================== - Hits 10384 9603 -781 + Misses 6414 6355 -59 ``` | [Flag](https://app.codecov.io/gh/tafia/quick-xml/pull/802/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/tafia/quick-xml/pull/802/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `60.17% <100.00%> (-1.65%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

BlueGreenMagick commented 2 months ago

Modified the code so that balance_buf is computed only once.

BlueGreenMagick commented 2 months ago

Thanks for the detailed feedback! I adjusted the PR.

  • start with balance == 1 (it is < in <!DOCTYPE) here

As the balance is calculated in up to chunk excluding current found >, I did not add 1 to balance initially.

add tests for the mentioned case (chunk with < + chunk with > which closes < from the first chunk). It is enough just craft such a string and use appropriate buffer size to slice string to desired chunks

Are regression tests for issues 533, 590, 801 enough? Or are additional tests desired?

try to check if this fixes https://github.com/tafia/quick-xml/issues/533. I think it should because it seems to be a duplicate of these issues, but need to check

This PR does indeed fixes the issue. Added a regression test for the case, and modified the initial post so the issue will be closed when this PR is merged.

Mingun commented 2 months ago

You also can squash all your changes if you wish

BlueGreenMagick commented 2 months ago

Done!

I adjusted regression test for issue801 so that all angle brackets are explicitly in different buffer (by lowering buffer size to 2 bytes).

I think it might be better (and easier with GitHub UI) to squash merge on your end, so others can still follow the conversation in this PR in the future.

Mingun commented 1 month ago

Thanks!