feat(c14n): add xml:* attr inheritance, C14N 1.1 mode, xml:base URI resolution#12
Merged
feat(c14n): add xml:* attr inheritance, C14N 1.1 mode, xml:base URI resolution#12
Conversation
…esolution - xml:* attribute inheritance for document subsets: propagate xml:lang/xml:space/xml:base/xml:id from ancestors when parent is outside node set (W3C C14N §2.4) - C14N 1.1 mode enabled with xml:id propagation and xml:base relative URI resolution via RFC 3986 (new xml_base.rs module) - Cow<str> attr values for zero-alloc in common (non-1.1) path - 45 new tests across unit and integration suites Closes #11
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds C14N 1.1 support: Changes
Sequence DiagramsequenceDiagram
participant Client as C14N Processor
participant Serialize as serialize_canonical
participant AttrCollector as Attribute Collector
participant AncestorWalker as Ancestor Walker
participant URIResolver as xml_base Resolver
participant Renderer as NS Renderer
Client->>Serialize: serialize_canonical(doc, node_set, with_comments, ns_renderer, fixup_xml_base=true, out)
Serialize->>AttrCollector: collect element's own attrs
AttrCollector->>AncestorWalker: if parent not in node_set -> gather inheritable xml:* attrs
AncestorWalker-->>AttrCollector: returned inherited xml:lang/xml:space/xml:base/xml:id
AttrCollector->>URIResolver: compute_effective_xml_base(start, node_set)
URIResolver->>URIResolver: resolve relative bases per RFC‑3986 (parse/merge/remove_dot_segments)
URIResolver-->>AttrCollector: resolved absolute xml:base values
AttrCollector->>Serialize: merge own + inherited attrs (canonical order)
Serialize->>Renderer: render namespaces and emit element + attrs in canonical order
Renderer-->>Client: canonicalized output bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the C14N implementation to support Inclusive C14N 1.1 behavior for document subsets, including xml:* attribute inheritance and xml:base URI fixups.
Changes:
- Enable
C14nMode::Inclusive1_1by routing through the inclusive serializer with C14N 1.1-specific behavior. - Add
xml:baseeffective-URI computation + RFC 3986-style resolution for subset fixup (C14N 1.1). - Add/expand unit + integration tests covering 1.1 behavior, subset inheritance, and xml:base resolution.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/c14n/mod.rs |
Enables Inclusive C14N 1.1 by invoking the inclusive serializer with an xml:base fixup flag. |
src/c14n/serialize.rs |
Implements xml:* inheritance for subsets and threads fixup_xml_base through serialization. |
src/c14n/xml_base.rs |
Introduces minimal RFC 3986 URI resolution utilities for xml:base fixup in subsets. |
src/c14n/ns_inclusive.rs |
Updates serializer call sites to pass the new fixup_xml_base parameter. |
src/c14n/ns_exclusive.rs |
Updates serializer call sites to pass the new fixup_xml_base parameter. |
tests/c14n_integration.rs |
Adds integration tests for Inclusive C14N 1.1, xml:id propagation, and xml:base fixup behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/c14n/serialize.rs`:
- Around line 206-258: The current logic unconditionally collects and merges
ancestor xml:* attributes using collect_inherited_xml_attrs and the
fixup_xml_base flag, which causes Exclusive C14N and C14N 1.1 semantics to be
wrong; change the API to pass an explicit canonicalization mode/strategy (e.g.,
enum C14nMode or InheritanceStrategy) into collect_inherited_xml_attrs and the
surrounding logic (where fixup_xml_base, node_set, inherited_xml, and
effective_parent_base are used) and use that mode to (1) skip copying ancestor
xml:* for Exclusive1_0, (2) for C14N 1.1 only import xml:lang and xml:space
(treat xml:id as ordinary attribute), and (3) continue handling xml:base
resolution only when the mode indicates C14N 1.1 fixup; update
compute_effective_xml_base/resolve_uri usage and any call-sites (including the
other similar block) to propagate the new mode parameter so the inherit/copy
decisions are mode-aware rather than driven solely by fixup_xml_base.
In `@src/c14n/xml_base.rs`:
- Around line 22-44: compute_effective_xml_base currently walks to the document
root and joins all xml:base values; change it to only collect xml:base values
from the start node up the ancestor chain until (but not including) the first
ancestor that is actually included in the canonical output (i.e. stop at the
nearest included ancestor), so only the contiguous omitted-ancestor run is
merged. Implement this by adding an inclusion check (either use an existing Node
inclusion predicate or add a parameter like is_included: impl Fn(Node) -> bool),
stop the while-traversal when that predicate returns true for an ancestor, still
use xml_base_value to collect bases and resolve_uri to join them, and leave
logic that reverses and resolves bases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2972211c-4e41-4b13-9433-b025f110db3b
📒 Files selected for processing (6)
src/c14n/mod.rssrc/c14n/ns_exclusive.rssrc/c14n/ns_inclusive.rssrc/c14n/serialize.rssrc/c14n/xml_base.rstests/c14n_integration.rs
…expect() - Gate xml:id inheritance on C14N 1.1 only (C14N 1.0 inherits xml:lang/xml:space/xml:base per §2.4, NOT xml:id) - Replace expect() calls with if-let pattern matching to comply with crate-level deny(clippy::unwrap_used) - Stop xml:base ancestor walk at nearest included ancestor (contiguous omitted chain only, not full walk to root) - Add is_inheritable_xml_attr() whitelist function - Update XML_NS doc comment to describe mode-specific behavior - Add RFC 3986 spec reference comment on resolve_uri
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/c14n/serialize.rs (1)
562-756: Comprehensive inheritance tests, but missing edge case coverage.The tests thoroughly cover:
- Basic
xml:langandxml:spaceinheritance- Multiple attribute inheritance with sort order verification
- Own attribute precedence over ancestors
- Closer ancestor precedence
- No inheritance when parent is in set / full document
Consider adding a test for the intermediate-included-ancestor scenario to verify the fix for the issue flagged above:
📝 Suggested test case
#[test] fn no_inheritance_past_included_ancestor() { // A (in set, xml:lang="en") -> B (not in set) -> C (in set) // C should NOT inherit xml:lang from A because A is in the set // and renders its own attributes. let xml = r#"<a xml:lang="en"><b><c>text</c></b></a>"#; let doc = Document::parse(xml).unwrap(); let a = doc.root_element(); let b = a.first_element_child().unwrap(); let c = b.first_element_child().unwrap(); // Include only a and c (not b) let mut ids = HashSet::new(); ids.insert(a.id()); ids.insert(c.id()); for child in c.children() { ids.insert(child.id()); } let pred = subset_predicate(ids); let renderer = InclusiveNsRenderer; let mut out = Vec::new(); serialize_canonical(&doc, Some(&pred), false, &renderer, false, &mut out).unwrap(); let result = String::from_utf8(out).unwrap(); // xml:lang should appear on <a> only, not inherited onto <c> assert!( result.contains(r#"<a xml:lang="en">"#), "a should have xml:lang; got: {result}" ); assert!( !result.contains(r#"<c xml:lang"#), "c should NOT inherit xml:lang from a; got: {result}" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c14n/serialize.rs` around lines 562 - 756, Add a new unit test called no_inheritance_past_included_ancestor that constructs a document A(xml:lang="en")->B->C, builds a subset including A and C but not B (use a HashSet of node ids and subset_predicate(ids)), calls serialize_canonical (as in the other tests) with InclusiveNsRenderer, and asserts that xml:lang appears on A but not on C (no <c xml:lang>). Reference symbols to modify/include: Document::parse, subset_predicate, serialize_canonical, InclusiveNsRenderer, and ensure HashSet from std::collections is in scope for the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/c14n/serialize.rs`:
- Around line 382-398: The ancestor-walk loop that builds inherited (starting
from ancestor = node.parent()) must stop when it encounters an ancestor that is
itself in the node set so that included ancestors render their own xml:*
attributes and block further inheritance; update the while-let block to check
whether the current anc is included (e.g., via the same membership test used
elsewhere such as compute_effective_xml_base — use the project’s node-set
predicate like anc_is_in_set(anc) or included_nodes.contains(&anc) /
anc.is_in_node_set()) and break out of the loop when that test is true before
collecting attributes; keep the existing checks (anc.is_element(),
attr.namespace() == Some(XML_NS), is_inheritable_xml_attr(local,
include_xml_id), seen.insert(local)) and then push (attr.name(), attr.value())
as before.
---
Nitpick comments:
In `@src/c14n/serialize.rs`:
- Around line 562-756: Add a new unit test called
no_inheritance_past_included_ancestor that constructs a document
A(xml:lang="en")->B->C, builds a subset including A and C but not B (use a
HashSet of node ids and subset_predicate(ids)), calls serialize_canonical (as in
the other tests) with InclusiveNsRenderer, and asserts that xml:lang appears on
A but not on C (no <c xml:lang>). Reference symbols to modify/include:
Document::parse, subset_predicate, serialize_canonical, InclusiveNsRenderer, and
ensure HashSet from std::collections is in scope for the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0868f161-5978-40a7-b5ad-f0d343f1d86c
📒 Files selected for processing (2)
src/c14n/serialize.rssrc/c14n/xml_base.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/c14n/xml_base.rs
- collect_inherited_xml_attrs now breaks when encountering an ancestor that IS in the node set (same pattern as compute_effective_xml_base) — prevents incorrect propagation past included elements that render their own attributes - Add test: no_inheritance_past_included_ancestor (A→B→C scenario)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/c14n/serialize.rs (1)
16-22:⚠️ Potential issue | 🔴 CriticalGate
collect_inherited_xml_attrsby canonicalization mode (skip in Exclusive).
collect_inherited_xml_attrsis still invoked unconditionally on subset elements (Line 218), so Exclusive mode will import ancestorxml:*attributes. This should be disabled for Exclusive canonicalization and enabled only for Inclusive modes.🔧 Suggested change (mode-aware inheritance gate)
pub(crate) fn serialize_canonical( doc: &Document, node_set: Option<&dyn Fn(Node) -> bool>, with_comments: bool, ns_renderer: &dyn NsRenderer, + inherit_xml_attrs: bool, fixup_xml_base: bool, output: &mut Vec<u8>, ) -> Result<(), C14nError> { @@ serialize_children( root, node_set, with_comments, ns_renderer, + inherit_xml_attrs, fixup_xml_base, &HashMap::new(), output, ); @@ fn serialize_children( parent: Node, node_set: Option<&dyn Fn(Node) -> bool>, with_comments: bool, ns_renderer: &dyn NsRenderer, + inherit_xml_attrs: bool, fixup_xml_base: bool, parent_rendered: &HashMap<String, String>, output: &mut Vec<u8>, ) { @@ serialize_element( child, node_set, with_comments, ns_renderer, + inherit_xml_attrs, fixup_xml_base, parent_rendered, output, ); @@ serialize_children( child, node_set, with_comments, ns_renderer, + inherit_xml_attrs, fixup_xml_base, parent_rendered, output, ); @@ fn serialize_element( node: Node, node_set: Option<&dyn Fn(Node) -> bool>, with_comments: bool, ns_renderer: &dyn NsRenderer, + inherit_xml_attrs: bool, fixup_xml_base: bool, parent_rendered: &HashMap<String, String>, output: &mut Vec<u8>, ) { @@ - let inherited_xml = collect_inherited_xml_attrs(node, node_set, fixup_xml_base); + let inherited_xml = if inherit_xml_attrs { + collect_inherited_xml_attrs(node, node_set, fixup_xml_base) + } else { + Vec::new() + };Based on learnings: “Exclusive XML Canonicalization ... omits ancestor xml:* attribute search-and-copy;
collect_inherited_xml_attrsmust be skipped entirely in Exclusive mode.”Also applies to: 218-218, 352-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c14n/serialize.rs` around lines 16 - 22, The call to collect_inherited_xml_attrs is currently unconditional and causes xml:* attributes to be imported during Exclusive canonicalization; update every invocation (notably the calls around the subset element handling and the block referenced at 352-356) to only run when the canonicalization mode is Inclusive by gating it with a mode check (e.g., if canonicalization_mode != CanonicalizationMode::Exclusive or match on CanonicalizationMode and only call collect_inherited_xml_attrs for Inclusive variants); use the existing mode/config variable (e.g., canonicalization_mode or mode) and ensure no other code paths call collect_inherited_xml_attrs when in CanonicalizationMode::Exclusive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/c14n/serialize.rs`:
- Around line 16-22: The call to collect_inherited_xml_attrs is currently
unconditional and causes xml:* attributes to be imported during Exclusive
canonicalization; update every invocation (notably the calls around the subset
element handling and the block referenced at 352-356) to only run when the
canonicalization mode is Inclusive by gating it with a mode check (e.g., if
canonicalization_mode != CanonicalizationMode::Exclusive or match on
CanonicalizationMode and only call collect_inherited_xml_attrs for Inclusive
variants); use the existing mode/config variable (e.g., canonicalization_mode or
mode) and ensure no other code paths call collect_inherited_xml_attrs when in
CanonicalizationMode::Exclusive.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds C14N 1.1 support to the canonicalization pipeline, including required xml:* attribute inheritance for document subsets and xml:base fixup via a small RFC 3986 resolver, with expanded unit/integration test coverage.
Changes:
- Enable
Inclusive1_1mode by routing through inclusive serialization with xml:id propagation and optionalxml:basefixup. - Implement
xml:*attribute inheritance for document subsets and thread afixup_xml_baseflag through serialization. - Add
xml_base.rsminimal URI resolver + extensive unit/integration tests for C14N 1.1 andxml:basebehaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/c14n_integration.rs |
Adds integration tests covering C14N 1.1 parity/idempotency, xml:id inheritance, and xml:base fixup vs 1.0. |
src/c14n/xml_base.rs |
Introduces minimal RFC 3986-style resolver and effective-base computation for subset fixup. |
src/c14n/serialize.rs |
Implements inherited xml:* attribute collection and applies C14N 1.1 xml:base resolution during attribute rendering. |
src/c14n/ns_inclusive.rs |
Updates tests for new serialize_canonical signature. |
src/c14n/ns_exclusive.rs |
Updates tests for new serialize_canonical signature. |
src/c14n/mod.rs |
Enables Inclusive1_1 in canonicalize() and adds unit tests for 1.1 behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- parse_base: split scheme at ':', handle optional //authority, support file: URIs and bases with ?query/#fragment - xml_base_value: treat xml:base="" as absent per RFC 3986 - Add include_xml_id named binding at call site for clarity - Add tests: file: scheme, query/fragment base, empty xml:base
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/c14n/serialize.rs`:
- Around line 262-273: collect_inherited_xml_attrs is currently adding ancestor
xml:base="" entries into inherited_xml while compute_effective_xml_base treats
empty xml:base as absent via xml_base_value, causing spurious emitted
xml:base=""; fix by normalizing/filtering empty xml:base when building
inherited_xml (skip adding entries where name == "base" and value.is_empty() or
use the same xml_base_value normalization used in compute_effective_xml_base) so
effective_parent_base and inherited_xml are consistent; update
collect_inherited_xml_attrs to skip or convert empty xml:base values accordingly
so the later logic using effective_parent_base doesn't emit an empty xml:base.
- Around line 218-222: serialize_element currently decides to collect ancestor
xml:* attributes using only the fixup_xml_base flag, which causes Exclusive C14N
to incorrectly inherit xml:lang/xml:space/xml:base; modify serialize_element to
accept the C14nMode enum (or an explicit skip_inheritance bool) and skip calling
collect_inherited_xml_attrs when the mode is Exclusive1_0 (i.e., only call
collect_inherited_xml_attrs when mode is Inclusive1_0 or when inheritance is
allowed), ensuring include_xml_id remains based on fixup_xml_base but the
ancestor-walking step is disabled for Exclusive mode.
In `@src/c14n/xml_base.rs`:
- Around line 102-105: The authority-override branch that checks
reference.starts_with("//") must normalize the path per RFC 3986 §5.2.2: extract
the authority and the path from the reference (after the leading "//"), apply
the existing remove_dot_segments function to the path component, then
reconstruct the reference with the authority and normalized path before
returning (keep the same return style used in the branch, i.e. return
format!("{scheme}:{...}")). Update the block that currently returns
format!("{scheme}:{reference}") (the reference.starts_with("//") branch) to
parse out authority vs path, call remove_dot_segments(path), and reassemble
"{scheme}://{authority}{normalized_path}" (ensuring any empty path is handled
consistently).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4671d065-6b21-4431-a656-773bd45e356e
📒 Files selected for processing (2)
src/c14n/serialize.rssrc/c14n/xml_base.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Canonical XML (C14N) 1.1 support to the crate, focusing on document-subset correctness by inheriting xml:* attributes and performing xml:base fixup via a minimal RFC 3986 resolver.
Changes:
- Enable Inclusive C14N 1.1 mode (previously errored) and thread a
fixup_xml_baseflag through serialization. - Implement
xml:*attribute inheritance for document subsets (xml:lang,xml:space,xml:base, plusxml:idin 1.1). - Add an
xml:baseresolver module and expand unit/integration test coverage for 1.1 behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration tests for C14N 1.1 parity/idempotency, xml:id subset propagation, and xml:base fixup scenarios. |
| src/c14n/xml_base.rs | New RFC 3986-based (minimal) resolver and helpers to compute effective xml:base across ancestor chains. |
| src/c14n/serialize.rs | Implements xml:* inheritance merge and 1.1 xml:base fixup during attribute serialization; adds subset-focused unit tests. |
| src/c14n/ns_inclusive.rs | Updates tests for new serialize_canonical signature (threads fixup_xml_base). |
| src/c14n/ns_exclusive.rs | Updates tests for new serialize_canonical signature (threads fixup_xml_base). |
| src/c14n/mod.rs | Enables Inclusive1_1 algorithm routing to serializer and adds C14N 1.1 unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…lues - compute_effective_xml_base: collect included ancestor's xml:base as resolution seed before breaking (prevents incomplete relative base) - collect_inherited_xml_attrs: skip empty xml:base="" (no-op per RFC 3986) - resolve_uri: normalize path on authority override per RFC 3986 §5.2.2 - Update docs: effective URI may be relative when no absolute base exists - Add code comment: xml:* inheritance applies to all C14N modes per spec
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/c14n/serialize.rs (1)
222-227:⚠️ Potential issue | 🔴 CriticalBlocker: Exclusive mode still imports ancestor
xml:*attributes.Line 227 always runs
collect_inherited_xml_attrs(...), and Line 222–225 explicitly documents this for Exclusive too. That makes Exclusive subset output inheritxml:lang/xml:space/xml:base, which should be omitted in Exclusive C14N.🐛 Proposed fix (mode-gate inheritance, keep C14N 1.1 fixup behavior)
- // NOTE: xml:* inheritance applies to ALL C14N modes (inclusive, exclusive, - // 1.0, 1.1) per W3C specs. Exclusive C14N §4.4 modifies only namespace - // node handling; attribute rules (including xml:* inheritance from §2.4) - // are inherited unchanged from the inclusive spec. - let include_xml_id = fixup_xml_base; - let inherited_xml = collect_inherited_xml_attrs(node, node_set, include_xml_id); + // xml:* inheritance is enabled only for inclusive canonicalization modes. + // Exclusive canonicalization must not import ancestor xml:* attrs. + let include_xml_id = fixup_xml_base; + let inherited_xml = if inherit_xml_attrs { + collect_inherited_xml_attrs(node, node_set, include_xml_id) + } else { + Vec::new() + };RFC 3741 Exclusive XML Canonicalization section 1.2 and 4.4: does Exclusive C14N omit ancestor xml:lang/xml:space search-and-copy for orphan nodes in a subset?Based on learnings, Exclusive XML Canonicalization in this repo must skip
collect_inherited_xml_attrsentirely and require explicit inclusion of neededxml:*attributes in the subset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c14n/serialize.rs` around lines 222 - 227, The current code always calls collect_inherited_xml_attrs(node, node_set, include_xml_id) causing Exclusive C14N to import ancestor xml:* attributes; modify the logic so collect_inherited_xml_attrs is only invoked when the mode is not Exclusive (i.e., gate the call on a non-exclusive canonicalization mode), while preserving the existing fixup_xml_base behavior for C14N 1.1 when applicable; update the conditional around include_xml_id/fixup_xml_base and remove the unconditional call to collect_inherited_xml_attrs so Exclusive mode requires explicit xml:* inclusion in the subset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/c14n/serialize.rs`:
- Around line 222-227: The current code always calls
collect_inherited_xml_attrs(node, node_set, include_xml_id) causing Exclusive
C14N to import ancestor xml:* attributes; modify the logic so
collect_inherited_xml_attrs is only invoked when the mode is not Exclusive
(i.e., gate the call on a non-exclusive canonicalization mode), while preserving
the existing fixup_xml_base behavior for C14N 1.1 when applicable; update the
conditional around include_xml_id/fixup_xml_base and remove the unconditional
call to collect_inherited_xml_attrs so Exclusive mode requires explicit xml:*
inclusion in the subset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d455f5d1-f49d-4f3e-936a-98164a2e9a59
📒 Files selected for processing (2)
src/c14n/serialize.rssrc/c14n/xml_base.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/c14n/xml_base.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Canonical XML 1.1 support to the C14N module, including required xml:* inheritance for document subsets and xml:base URI fixup per RFC 3986, plus expanded unit/integration coverage.
Changes:
- Enable
C14nMode::Inclusive1_1by routing it through the inclusive serializer with 1.1-specific behaviors. - Implement
xml:*attribute inheritance for document subsets (xml:lang,xml:space,xml:base, andxml:idin 1.1). - Add an RFC 3986-based
xml:baseresolver and integrate it into the serialization pipeline (1.1 mode), with new unit + integration tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration tests covering C14N 1.1 parity/idempotency, xml:id propagation, and xml:base fixup in subsets. |
| src/c14n/xml_base.rs | New module implementing xml:base effective-base computation + RFC 3986-like reference resolution with unit tests. |
| src/c14n/serialize.rs | Threads fixup_xml_base through the serializer, adds xml:* inheritance, and applies xml:base fixup when enabled. |
| src/c14n/ns_inclusive.rs | Updates test call sites for the new serialize_canonical signature. |
| src/c14n/ns_exclusive.rs | Updates test call sites for the new serialize_canonical signature. |
| src/c14n/mod.rs | Enables Inclusive1_1 mode and wires it to the serializer with 1.1 behavior flags; adds xml_base module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Exclusive C14N explicitly omits ancestor xml:* attribute import per W3C Exc-C14N §3: "This search and copying are omitted" - Add inherit_xml_attrs flag: true for Inclusive (1.0/1.1), false for Exclusive — bundled into C14nConfig struct to avoid clippy::too_many_arguments - Remove incorrect code comment claiming all modes inherit xml:*
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the C14N implementation to support Inclusive C14N document subsets’ required xml:* attribute inheritance and enables C14N 1.1, including xml:base fixup via an RFC 3986-style resolver.
Changes:
- Add inclusive subset inheritance for whitelisted
xml:*attributes (andxml:idfor C14N 1.1), while explicitly skipping this behavior for Exclusive C14N. - Enable C14N 1.1 mode by routing through the inclusive serializer with new configuration flags.
- Introduce an
xml:baseresolver module and add new unit/integration tests covering C14N 1.1 behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration tests for C14N 1.1 output parity/idempotency, xml:id inheritance, and xml:base fixup behavior. |
| src/c14n/xml_base.rs | New RFC 3986-inspired resolver used to compute effective xml:base in omitted-ancestor subset scenarios. |
| src/c14n/serialize.rs | Threads a new C14nConfig through serialization and implements inclusive subset xml:* inheritance + C14N 1.1 xml:base fixup. |
| src/c14n/ns_inclusive.rs | Updates tests to pass the new serialization config. |
| src/c14n/ns_exclusive.rs | Updates tests to pass the new serialization config (and disables inheritance for exclusive mode). |
| src/c14n/mod.rs | Enables Inclusive C14N 1.1 mode and wires the per-mode serializer configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Distinguish absent vs empty authority (Some("") vs None) for
correct recomposition of file:/// and urn: URIs
- Handle query-only (?q) and fragment-only (#f) references
- Strip query/fragment from base path before merge_paths
- Skip resolving empty xml:base="" on elements (emit unchanged)
- Add recompose() and strip_query_fragment() helpers
- remove_dot_segments: preserve unmatched ".." in relative paths per RFC 3986 §5.2.4 (foo/../../bar -> ../bar) - Update mod.rs module docs: C14N 1.1 now implemented (was "URI parsing only; returns UnsupportedAlgorithm") - Update compute_effective_xml_base docs: clarify included ancestor seed behavior and None return conditions - Clarify xml:base="" comment (no-op, not "remove the base") - Document include_xml_id coupling with rationale
Contributor
There was a problem hiding this comment.
Pull request overview
Adds full support for Inclusive C14N 1.1 document-subset semantics in the c14n module, including xml:* attribute inheritance and xml:base fixup via an internal RFC 3986 resolver.
Changes:
- Introduces
C14nConfigto thread mode-specific flags (xml:* inheritance and xml:base fixup) through the serialization pipeline. - Implements Inclusive subset inheritance for whitelisted
xml:*attributes (andxml:idfor 1.1), and enables C14N 1.1 mode instead of returningUnsupportedAlgorithm. - Adds
xml_base.rswith an RFC 3986 resolver and extensive unit/integration tests for xml:base resolution and C14N 1.1 behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration tests for C14N 1.1 behavior (xml:id propagation + xml:base fixup) and parity with 1.0 for full documents. |
| src/c14n/xml_base.rs | New RFC 3986-based resolver and effective xml:base computation used for C14N 1.1 subset fixup. |
| src/c14n/serialize.rs | Adds config plumbing, xml:* inheritance collection, and xml:base fixup during attribute emission. |
| src/c14n/ns_inclusive.rs | Updates tests to pass the new C14nConfig into serialize_canonical. |
| src/c14n/ns_exclusive.rs | Updates tests to pass C14nConfig (with inheritance disabled) into serialize_canonical. |
| src/c14n/mod.rs | Enables Inclusive C14N 1.1 by routing through the inclusive renderer with appropriate config; updates module docs and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Canonical XML 1.1 support in the C14N module, adding spec-required xml:* attribute inheritance for inclusive document subsets and introducing xml:base URI fixup logic for C14N 1.1.
Changes:
- Add inclusive-subset inheritance for whitelisted
xml:*attributes (xml:lang,xml:space,xml:base, andxml:idin 1.1). - Enable C14N 1.1 mode (previously errored) by routing through inclusive serialization with C14N 1.1 behaviors.
- Introduce an RFC 3986-based
xml:baseresolver and integration/unit tests covering inheritance and fixup scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration coverage for C14N 1.1 parity/idempotency, xml:id inheritance, and xml:base fixup behavior. |
| src/c14n/xml_base.rs | New module providing xml:base effective-base computation and URI resolution utilities with unit tests. |
| src/c14n/serialize.rs | Threads a new C14nConfig through serialization; implements xml:* inheritance and xml:base fixup in element attribute emission. |
| src/c14n/ns_inclusive.rs | Updates tests to pass the new C14nConfig parameter into serialization. |
| src/c14n/ns_exclusive.rs | Updates tests to pass the new C14nConfig parameter into serialization (inheritance disabled). |
| src/c14n/mod.rs | Enables Inclusive C14N 1.1 mode and wires mode → C14nConfig mapping; updates module docs and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Split reference into path and ?query#fragment suffix before applying remove_dot_segments (path-only operation) - Reattach suffix after recomposition - Add split_path_suffix() helper - Add 4 tests: reference with query, fragment, both, absolute+query
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing Canonical XML (C14N) 1.1 behaviors to the c14n module, focusing on correct handling of document subsets (xml:* inheritance) and xml:base fixup via RFC 3986 resolution.
Changes:
- Thread a
C14nConfig { inherit_xml_attrs, fixup_xml_base }through the serializer and enable Inclusive C14N 1.1 mode. - Implement xml:* attribute inheritance for inclusive document subsets (and explicitly skip it for exclusive).
- Add a minimal RFC 3986 resolver for
xml:basefixup and expand unit/integration coverage for 1.1 behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration tests for C14N 1.1 equivalence/idempotency, xml:id inheritance, and xml:base fixup behavior. |
| src/c14n/xml_base.rs | New RFC 3986-based URI resolver + effective xml:base computation for subset fixup. |
| src/c14n/serialize.rs | Implements xml:* inheritance + optional xml:base fixup; introduces C14nConfig and updates attribute serialization pipeline. |
| src/c14n/ns_inclusive.rs | Updates tests to pass C14nConfig into serialization. |
| src/c14n/ns_exclusive.rs | Updates tests to pass C14nConfig into serialization (inheritance disabled). |
| src/c14n/mod.rs | Enables Inclusive1_1 canonicalization path (no longer returns UnsupportedAlgorithm) and wires in xml_base module/config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…bases - parse_base: validate via has_scheme() before treating ':' as scheme delimiter — prevents mis-parsing relative paths like "a/b:c" - Schemeless base: perform path-merge + dot removal so chained relative xml:base values collapse correctly (a/b/ + c/ -> a/b/c/)
Contributor
There was a problem hiding this comment.
Pull request overview
Adds full support for Inclusive C14N 1.1 document-subset behavior by inheriting whitelisted xml:* attributes, enabling C14N 1.1 mode in the public API, and implementing RFC 3986 xml:base resolution/fixup for subset canonicalization.
Changes:
- Introduce
C14nConfigto thread per-mode flags (xml:* inheritance + xml:base fixup) through the serialization pipeline. - Implement RFC 3986-based
xml:baseresolver (xml_base.rs) and integrate it into attribute rendering for C14N 1.1 subsets. - Add/extend unit + integration tests for C14N 1.1 behavior, xml:* inheritance, and xml:base fixup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/c14n/mod.rs |
Enables Inclusive C14N 1.1 by routing through serializer with a new C14nConfig; updates tests accordingly. |
src/c14n/serialize.rs |
Implements xml:* inheritance for subsets and hooks in C14N 1.1 xml:base fixup during attribute emission. |
src/c14n/xml_base.rs |
New minimal RFC 3986 resolver + effective xml:base computation used by C14N 1.1 subset fixup. |
src/c14n/ns_inclusive.rs |
Updates tests for new serialize_canonical signature/config threading. |
src/c14n/ns_exclusive.rs |
Updates tests for new serialize_canonical signature/config threading (ensuring no xml:* inheritance for exclusive). |
tests/c14n_integration.rs |
Adds integration tests covering C14N 1.1 mode, xml:id propagation, and xml:base resolution behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Canonical XML 1.1 support and document-subset correctness for Inclusive C14N by implementing spec-required xml:* inheritance and RFC 3986 xml:base resolution, wiring this behavior through the serialization pipeline and validating via new unit/integration tests.
Changes:
- Introduce
C14nConfigto parameterize xml:* inheritance and C14N 1.1xml:basefixup, and route Inclusive1_1 through the inclusive renderer. - Implement xml:* attribute inheritance for inclusive document subsets (and explicitly skip it for exclusive mode).
- Add an RFC 3986 resolver (
xml_base.rs) plus integration tests covering C14N 1.1 subset behavior andxml:basefixup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/c14n/mod.rs |
Enables C14N 1.1 mode by selecting config + renderer instead of returning UnsupportedAlgorithm. |
src/c14n/serialize.rs |
Implements xml:* inheritance + C14N 1.1 xml:base fixup during attribute serialization; threads C14nConfig through. |
src/c14n/xml_base.rs |
New minimal RFC 3986 resolver and effective xml:base computation for subset fixup. |
src/c14n/ns_inclusive.rs |
Updates tests to pass C14nConfig into serialize_canonical. |
src/c14n/ns_exclusive.rs |
Updates tests to pass C14nConfig into serialize_canonical with inheritance disabled. |
tests/c14n_integration.rs |
Adds integration coverage for C14N 1.1 behavior (idempotency, xml:id inheritance, xml:base fixup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the C14N subsystem to fully support C14N 1.1 behavior for document subsets, including spec-required xml:* attribute inheritance (inclusive only) and xml:base URI fixup via RFC 3986 resolution.
Changes:
- Add
C14nConfigto thread mode-specific flags (xml:* inheritance and xml:base fixup) through the serialization pipeline. - Implement
xml:*inheritance for inclusive subsets and enable C14N 1.1 mode (xml:id propagation + xml:base fixup). - Introduce an RFC 3986-based relative URI resolver for C14N 1.1
xml:basefixup and add extensive unit/integration tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c14n_integration.rs | Adds integration coverage for C14N 1.1 output parity, xml:id propagation, and xml:base fixup behaviors. |
| src/c14n/xml_base.rs | New RFC 3986-oriented resolver + effective xml:base computation used for C14N 1.1 subset fixup. |
| src/c14n/serialize.rs | Threads C14nConfig, implements xml:* inheritance in subsets, and applies xml:base fixup for C14N 1.1. |
| src/c14n/ns_inclusive.rs | Updates tests to pass C14nConfig into serialize_canonical. |
| src/c14n/ns_exclusive.rs | Updates tests to pass C14nConfig into serialize_canonical (with inheritance disabled). |
| src/c14n/mod.rs | Enables Inclusive1_1 mode by routing through inclusive serialization with proper config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This was referenced Mar 16, 2026
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
xml:*attributes are propagated from ancestors onto the topmost included element (W3C C14N §2.4). Restricted to spec whitelist:xml:lang,xml:space,xml:basefor C14N 1.0; C14N 1.1 addsxml:idUnsupportedAlgorithm, now routes through inclusive renderer withxml:idpropagation andxml:baseURI resolutionxml_base.rs) — resolves relativexml:basevalues against the effective ancestor chain in document subsets (C14N 1.1 only)Technical Details
C14nConfigstruct bundlesinherit_xml_attrs+fixup_xml_baseflags, threaded through serialize pipeline{inherit: true, fixup: false}, Inclusive 1.1:{inherit: true, fixup: true}, Exclusive:{inherit: false, fixup: false}collect_inherited_xml_attrs()walks ancestor chain, stops at nearest included ancestor;is_inheritable_xml_attr()whitelist gates which attrs per modecompute_effective_xml_base()resolves omitted ancestor chain, seeds with included ancestor'sxml:base. Emptyxml:base=""treated as absent per RFC 3986parse_base()follows RFC 3986 structure (scheme at:, optional//authority)Cow<str>— zero allocation in common pathKnown Limitations
Test Plan
Closes #11