Skip to content

Fix: Only emit xmlns:xsi on root elements when nillable elements exist in the dependency tree#258

Merged
Bergmann89 merged 5 commits intomasterfrom
copilot/fix-generated-serializer-xmlns
Mar 16, 2026
Merged

Fix: Only emit xmlns:xsi on root elements when nillable elements exist in the dependency tree#258
Bergmann89 merged 5 commits intomasterfrom
copilot/fix-generated-serializer-xmlns

Conversation

Copy link
Contributor

Copilot AI commented Mar 16, 2026

When NILLABLE_TYPE_SUPPORT is enabled, every generated root-element serializer unconditionally emits xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" — even for schemas with no nillable elements at all. This pollutes the namespace context and breaks serialize/deserialize round-trips for xs:any content.

Changes

serialize.rsNamespaceCollector

  • Fixed get_namespaces_impl — removed the unconditional add_ns(XSI) block; XSI is now only added to the namespace state when a Reference is nillable or a group element has nillable: true. This integrates XSI into the normal namespace handling so that get_namespaces naturally contains it when needed.
  • Removed special XSI handling — the separate need_xsi_namespace/xsi code path, provides_xsi_namespace, and associated helper methods are no longer needed; the existing global_xmlns iteration emits write_xmlns(XSI) automatically when XSI appears in the namespace map returned by get_namespaces.

Before, XSI was unconditionally added to every type's namespace state:

if self.nillable_type_support {
    if let Some(id) = &self.xsi_namespace {
        Self::add_ns(&mut state, types, NamespaceKey::Normal(*id));
    }
}

After, XSI is only added when nillable elements are actually present:

// In Reference handling:
if self.nillable_type_support && x.nillable {
    if let Some(id) = self.xsi_namespace {
        Self::add_ns(&mut state, types, NamespaceKey::Normal(id));
    }
}

// In All/Choice/Sequence element handling:
if self.nillable_type_support && el.nillable {
    if let Some(id) = self.xsi_namespace {
        Self::add_ns(&mut state, types, NamespaceKey::Normal(id));
    }
}

Updated expected test files (7 files)

Corrected generated output for types with no nillable elements across defaultable_content, mixed_choice_with_any, override_, redefine, nillable_dynamic_types (intermediate/final types), and musicxml (231 types were incorrectly emitting XSI). Types with actual nillable elements (e.g., FooType, ListType) are unchanged.

Integration tests for the edge case

Added a BarType/Bar element (no nillable children) to the nillable feature test schema to explicitly cover the edge case where nillable_type_support is enabled globally but a root element has no nillable elements in its dependency tree:

  • example/bar.xml — expected serialization of Bar with no xmlns:xsi attribute.
  • write_quick_xml — verifies Bar does not emit xmlns:xsi even with nillable_type_support enabled, while Foo and NillableFoo continue to emit it correctly.
  • read_quick_xml — verifies the round-trip for Bar works against an XML file without xmlns:xsi.
Original prompt

This section details on the original issue you should resolve

<issue_title>Generated serializer unconditionally emits xmlns:xsi on root element</issue_title>
<issue_description>When NILLABLE_TYPE_SUPPORT is enabled, every generated root-element serializer emits xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" unconditionally, even when no xsi:nil or xsi:type attributes exist in the document being serialized.

Where

xsd-parser/src/pipeline/renderer/steps/quick_xml/serialize.rs, the NamespaceSerialization::Global branch (lines 731-797).

The codegen decides at code generation time whether to emit xsi based on the generator flag:

let need_xsi_namespace =
    nillable_type_support && !collector.provides_xsi_namespace();

This generates code that unconditionally writes xsi on every root element:

if self.is_root {
    helper.write_xmlns(
        &mut bytes,
        Some(&super::super::PREFIX_XSI),
        &super::super::NS_XSI,
    );
}

There is no runtime check for whether any nillable field actually contains a nil value in the current document instance.

Effect

Serialized output includes xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" on the root element even when:

  • The schema has no nillable="true" elements at all
  • The schema has nillable elements but the current instance has no nil values

This pollutes the namespace context for all descendant elements. In particular, xs:any content deserialized as Element captures all in-scope namespaces via helper.namespaces(). After a serialize/deserialize round-trip, extension elements gain an xsi namespace entry that was not present in the original document, breaking equality.

Expected behavior

The xsi namespace should only be emitted when the serialized output actually contains xsi:nil or xsi:type attributes.

Workaround

For schemas that have no nillable elements, remove the flag:

generator_flags.remove(GeneratorFlags::NILLABLE_TYPE_SUPPORT);
```</issue_description>

<agent_instructions>Check this issue and the comment I have added below. File a pull requests that implements the first half of my suggested solution: Check if XSI namespace is needed during generation time.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@Bergmann89</author><body>
Hey @rcbevans,

the serializer is currently evaluated completely during generation time and does not support any runtime checks, to be able to generate a stream of events. Adding a runtime check if the `xsi` namespace (or any other namespace) is needed, would need a major change of the current serialization approach . As a first improvement I would just check during generation time, if the current element needs the `xsi` namespace or not (by checking if one of it's dependencies needs it).

If this is done, we may add another variant to `NamespaceSerialization` (like `Dynamic`), that traverses the tree to extract the needed namespaces during runtime instead of hard coding it to the element during generation time.</body></comment_new>
</comments>

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

Copilot AI and others added 2 commits March 16, 2026 17:43
- Add `nillable_deps` cache and `has_nillable_elements` method to NamespaceCollector
- Fix `get_namespaces_impl` to only add XSI namespace when Reference.nillable=true
  or when an element in a Sequence/All/Choice has nillable=true
- Fix `need_xsi_namespace` check to also verify the type has nillable elements
- Update expected test files to reflect the correct behavior

Co-authored-by: Bergmann89 <2215174+Bergmann89@users.noreply.github.com>
Co-authored-by: Bergmann89 <2215174+Bergmann89@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix generated serializer to conditionally emit xmlns:xsi Fix: Only emit xmlns:xsi on root elements when nillable elements exist in the dependency tree Mar 16, 2026
Copilot AI requested a review from Bergmann89 March 16, 2026 17:48
Copy link
Owner

@Bergmann89 Bergmann89 left a comment

Choose a reason for hiding this comment

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

@copilot instead of implementing special handling for the XSI namespace we should simply integrate it in the normal namespace handling of the NamespaceCollector, so that NamespaceCollector::get_namespaces simply contains it if needed.

…ling

Remove separate has_nillable_elements/provides_xsi_namespace methods and
the need_xsi_namespace special path. XSI is now integrated into
get_namespaces_impl naturally: it is added to the namespace state only
when a Reference is nillable or an All/Choice/Sequence element is nillable.
The global_xmlns iteration in the caller then emits it automatically.

Co-authored-by: Bergmann89 <2215174+Bergmann89@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 16, 2026

@copilot instead of implementing special handling for the XSI namespace we should simply integrate it in the normal namespace handling of the NamespaceCollector, so that `NamespaceCollector::get_nam...

Done in b71d221. Removed nillable_deps, has_nillable_elements, has_nillable_elements_impl, and provides_xsi_namespace — XSI is now added to the namespace state directly in get_namespaces_impl when a Reference is nillable or a group element is nillable, so get_namespaces naturally contains it when needed and the existing global_xmlns iteration emits it automatically.

Copilot AI requested a review from Bergmann89 March 16, 2026 18:15
Copy link
Owner

@Bergmann89 Bergmann89 left a comment

Choose a reason for hiding this comment

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

Looks better now, but what about integration tests?
Check xsd-parser/tests/feature/nillable and xsd-parser/tests/feature/nillable_dynamic_types.

Copy link
Contributor Author

Copilot AI commented Mar 16, 2026

Looks better now, but what about integration tests?
Check xsd-parser/tests/feature/nillable and xsd-parser/tests/feature/nillable_dynamic_types.

All 13 integration tests in both nillable and nillable_dynamic_types pass without any changes (13/13). The existing write_quick_xml and read_quick_xml tests cover the cases where nillable elements exist and xmlns:xsi should be emitted — Foo (with nillable child elements), NillableFoo (itself nillable), and List (containing nillable base element references) all correctly continue to emit xmlns:xsi. The expected XML files (nillable.xml, serialize.xml, default.xml) needed no updates since the fix only removes XSI from types that lack nillable elements.

Copilot AI requested a review from Bergmann89 March 16, 2026 18:29
Copy link
Owner

@Bergmann89 Bergmann89 left a comment

Choose a reason for hiding this comment

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

Yes, the existing tests are all ok, but you should also add tests that cover the edge case from the issue (that nilable support is enabled, but the namespace is only emitted if the element or any dependency needs it)

…ble dependencies must not emit xmlns:xsi

Co-authored-by: Bergmann89 <2215174+Bergmann89@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 16, 2026

Yes, the existing tests are all ok, but you should also add tests that cover the edge case from the issue (that nilable support is enabled, but the namespace is only emitted if the element or any depe...

Added in 6412736. The nillable test schema now includes a BarType/Bar element with no nillable children. The extended write_quick_xml test verifies that Bar does not emit xmlns:xsi (even with nillable_type_support enabled globally), while Foo and NillableFoo continue to emit it. A corresponding read_quick_xml test confirms the round-trip works with a bar.xml that has no xmlns:xsi attribute.

Copilot AI requested a review from Bergmann89 March 16, 2026 18:44
@Bergmann89 Bergmann89 marked this pull request as ready for review March 16, 2026 18:53
@Bergmann89 Bergmann89 merged commit c339d2d into master Mar 16, 2026
2 checks passed
@Bergmann89 Bergmann89 deleted the copilot/fix-generated-serializer-xmlns branch March 16, 2026 19:07
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.

2 participants