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

Fix couple of bugs in serde deserializer #523

Merged
merged 5 commits into from Dec 12, 2022
Merged

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Dec 11, 2022

This PR fixes two bugs in serde deserializer that I found during work on #521:

  • text of CDATA content before the element was silently skipped when attempting to deserialize structs. This should not happen
  • unescaping of escaped entities is performed before applying rules to split elements in xs:list. That means, that you cannot encode spaces in xs:list elements using entity references   and   -- that escape sequences should be threated as list delimiters. The Java's mature XmlBeans library confirm that.

failures (2):
    struct_::excess_data_before::cdata
    struct_::excess_data_before::text_non_spaces
…uctures

in serde deserializer, like

  ```xml
  unwanted text<struct>...</struct>
  ```
@Mingun Mingun added bug serde Issues related to mapping from Rust types to XML labels Dec 11, 2022
@Mingun Mingun requested a review from dralley December 11, 2022 17:15
Comment on lines +315 to +316
// Comment to prevent auto-formatting
e.decode(true)?,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I keep this formatting because it highly likely that I'll replace e.decode() with more complicated expression in #521

@codecov-commenter
Copy link

Codecov Report

Merging #523 (2b9228d) into master (6347f3b) will increase coverage by 0.07%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   61.70%   61.77%   +0.07%     
==========================================
  Files          33       33              
  Lines       15466    15425      -41     
==========================================
- Hits         9543     9529      -14     
+ Misses       5923     5896      -27     
Flag Coverage Δ
unittests 61.77% <72.72%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/de/map.rs 72.36% <60.86%> (+1.41%) ⬆️
src/de/mod.rs 67.15% <82.35%> (-0.09%) ⬇️
src/de/simple_type.rs 92.87% <100.00%> (+1.68%) ⬆️
src/events/mod.rs 72.38% <0.00%> (-0.50%) ⬇️
src/reader/mod.rs 94.94% <0.00%> (-0.11%) ⬇️
src/lib.rs 13.25% <0.00%> (+0.07%) ⬆️

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

@dralley
Copy link
Collaborator

dralley commented Dec 11, 2022

IMO we should probably push a release after this

@Mingun
Copy link
Collaborator Author

Mingun commented Dec 11, 2022

I hope I finish #521 this year and cut a release after that

Changelog.md Outdated
@@ -38,6 +38,8 @@
```xml
unwanted text<struct>...</struct>
```
- [#523]: Fix incorrect handling of `xs:list`s with encoded spaces: their are still
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean "they still act as delimiters"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

Mingun and others added 2 commits December 12, 2022 19:16
…act as delimiters

Co-authored-by: Daniel Alley <dalley@redhat.com>
@Mingun Mingun merged commit e877f4f into tafia:master Dec 12, 2022
@Mingun Mingun deleted the fix-bugs branch December 12, 2022 14:18
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 29, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [quick-xml](https://github.com/tafia/quick-xml) | dependencies | minor | `0.26.0` -> `0.27.1` |

---

### Release Notes

<details>
<summary>tafia/quick-xml</summary>

### [`v0.27.1`](https://github.com/tafia/quick-xml/blob/HEAD/Changelog.md#&#8203;0271----2022-12-28)

[Compare Source](tafia/quick-xml@v0.27.0...v0.27.1)

##### Bug Fixes

-   [#&#8203;530]: Fix an infinite loop leading to unbounded memory consumption that occurs when
    skipping events on malformed XML with the `overlapped-lists` feature active.
-   [#&#8203;530]: Fix an error in the `Deserializer::read_to_end` when `overlapped-lists`
    feature is active and malformed XML is parsed

[#&#8203;530]: tafia/quick-xml#530

### [`v0.27.0`](https://github.com/tafia/quick-xml/blob/HEAD/Changelog.md#&#8203;0270----2022-12-25)

[Compare Source](tafia/quick-xml@v0.26.0...v0.27.0)

##### New Features

-   [#&#8203;521]: Implement `Clone` for all error types. This required changing `Error::Io` to contain
    `Arc<std::io::Error>` instead of `std::io::Error` since `std::io::Error` does not implement
    `Clone`.

##### Bug Fixes

-   [#&#8203;490]: Ensure that serialization of map keys always produces valid XML names.
    In particular, that means that maps with numeric and numeric-like keys (for
    example, `"42"`) no longer can be serialized because [XML name] cannot start
    from a digit
-   [#&#8203;500]: Fix deserialization of top-level sequences of enums, like
    ```xml
    <?xml version="1.0" encoding="UTF-8"?>
    <!-- list of enum Enum { A, B, С } -->
    <A/>
    <B/>
    <C/>
    ```
-   [#&#8203;514]: Fix wrong reporting `Error::EndEventMismatch` after disabling and enabling
    `.check_end_names`
-   [#&#8203;517]: Fix swapped codes for `\r` and `\n` characters when escaping them
-   [#&#8203;523]: Fix incorrect skipping text and CDATA content before any map-like structures
    in serde deserializer, like
    ```xml
    unwanted text<struct>...</struct>
    ```
-   [#&#8203;523]: Fix incorrect handling of `xs:list`s with encoded spaces: they still
    act as delimiters, which is confirmed also by mature XmlBeans Java library
-   [#&#8203;473]: Fix a hidden requirement to enable serde's `derive` feature to get
    quick-xml's `serialize` feature for `edition = 2021` or `resolver = 2` crates

##### Misc Changes

-   [#&#8203;490]: Removed `$unflatten=` special prefix for fields for serde (de)serializer, because:

    -   it is useless for deserializer
    -   serializer was rewritten and does not require it anymore

    This prefix allowed you to serialize struct field as an XML element and now
    replaced by a more thoughtful system explicitly indicating that a field should
    be serialized as an attribute by prepending `@` character to its name
-   [#&#8203;490]: Removed `$primitive=` prefix. That prefix allowed you to serialize struct
    field as an attribute instead of an element and now replaced by a more thoughtful
    system explicitly indicating that a field should be serialized as an attribute
    by prepending `@` character to its name
-   [#&#8203;490]: In addition to the `$value` special name for a field a new `$text`
    special name was added:

    -   `$text` is used if you want to map field to text content only. No markup is
        expected (but text can represent a list as defined by `xs:list` type)
    -   `$value` is used if you want to map elements with different names to one field,
        that should be represented either by an `enum`, or by sequence of `enum`s
        (`Vec`, tuple, etc.), or by string. Use it when you want to map field to any
        content of the field, text or markup

    Refer to [documentation] for details.
-   [#&#8203;521]: MSRV bumped to 1.52.
-   [#&#8203;473]: `serde` feature that used to make some types serializable, renamed to `serde-types`
-   [#&#8203;528]: Added documentation for XML to `serde` mapping

[#&#8203;473]: tafia/quick-xml#473

[#&#8203;490]: tafia/quick-xml#490

[#&#8203;500]: tafia/quick-xml#500

[#&#8203;514]: tafia/quick-xml#514

[#&#8203;517]: tafia/quick-xml#517

[#&#8203;521]: tafia/quick-xml#521

[#&#8203;523]: tafia/quick-xml#523

[#&#8203;528]: tafia/quick-xml#528

[XML name]: https://www.w3.org/TR/xml11/#NT-Name

[documentation]: https://docs.rs/quick-xml/0.27.0/quick_xml/de/index.html#difference-between-text-and-value-special-names

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43My4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1692
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug serde Issues related to mapping from Rust types to XML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants