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 XmlElement::pop_element #239

Closed
wants to merge 3 commits into from
Closed

fix XmlElement::pop_element #239

wants to merge 3 commits into from

Conversation

DenSinH
Copy link
Contributor

@DenSinH DenSinH commented Feb 19, 2025

See the test.

Basically, what was happening before was the following:
If you define a model with ElementT fields, the deserializer would deserialize the field as a verbatim copy of the element, but it would not pop it from the parent element. If your model has extra="forbid", the ModelSerializer would then check for any extra fields, and it would find all of the ElementT fields, as they have not been popped.

In the old situation, pop_element would practically be the same as find_element, not modifying the element, but even judging from the name, this is a bug. The test shows the desired behavior, where extra="forbid" would not mark any ElementT elements as being extra.

@DenSinH
Copy link
Contributor Author

DenSinH commented Feb 19, 2025

Okay apparently it is intended behavior to not actually pop elements in pop_element, I am not sure what you would prefer, a change in the signature for pop_element, a new method pop_and_remove_element or perhaps some private-member access in raw.ElementSerializer::deserialize like so:

    def deserialize(
            self,
            element: Optional[XmlElementReader],
            *,
            context: Optional[Dict[str, Any]],
            sourcemap: Dict[Location, int],
            loc: Location,
    ) -> Optional[str]:
        if self._computed:
            return None

        if element is None:
            return None

        if (sub_element := element.pop_element(self._element_name, self._search_mode)) is not None:
            # private member accesses:
            element._state.next_element_idx -= 1
            element._state.elements.pop(self._state.next_element_idx)
            sourcemap[loc] = sub_element.get_sourceline()
            return sub_element.to_native()
        else:
            return None

It still seems to me like it is in fact a bug that forbid="extra" and native.ElementT interact in the way they do in the current version, but one of these would be the best fix.

@dapper91 dapper91 added bug Something isn't working v2 Version 2 related labels Feb 23, 2025
@dapper91
Copy link
Owner

@DenSinH thanks for the report.

Fixed the bug in the PR #240

@dapper91 dapper91 closed this Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2 Version 2 related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants