uhttp: capture XML attributes in the XML->map unmarshaler#1038
uhttp: capture XML attributes in the XML->map unmarshaler#1038cvkramer wants to merge 2 commits into
Conversation
unmarshalXMLElement only switched on xml.StartElement / xml.CharData /
xml.EndElement and never read StartElement.Attr, so element attributes
were silently dropped when unmarshaling XML/SOAP responses into
map[string]any.
Thread each element's own attributes through the recursive descent
(the top-level element's Attr comes from xml.Unmarshal's start
parameter, child elements' Attr comes from the StartElement token) and
capture them badgerfish/xml-js style:
- element with children: "@attributes" is added as an additional
sibling key in the resulting map. Purely additive.
- leaf element with attributes: the previously bare string value is
promoted to map[string]any{"#text": <trimmed text>, "@attributes":
{...}}, or just {"@attributes": {...}} if there is no text.
- duplicate child names (the []map[string]any branch): since the
slice has no single map to attach the container's own attributes
to, "@attributes" is added to every entry in the slice.
Attribute names are taken from Attr.Name.Local (namespace prefix
dropped), matching how child/leaf element names are already handled.
BACKWARD COMPATIBILITY: elements with no attributes are byte-for-byte
unchanged (leaf -> string, children -> map, duplicates -> []map), so
all pre-existing tests pass without modification. The one deliberate,
intentional type change is the attributed-leaf promotion: a leaf
element that has attributes now returns map[string]any instead of a
bare string. Any caller currently type-asserting such a leaf's value
directly to string will need to switch to reading `["#text"]`, but
only for elements that actually carry attributes.
Adds table cases in pkg/uhttp/xml_test.go covering: attributed
container elements, attributed leaves with and without text,
namespace-prefixed attributes (local name only), nested attributed
elements, and attributes combined with duplicate siblings. All
existing subtests continue to pass unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… unmarshaler Address panel review findings on the attribute-capture PR: - attrMap no longer collides same-local-name attributes from different namespaces (e.g. a:id vs b:id): colliding entries are now disambiguated with a namespace-qualified key instead of one silently overwriting the other. - attrMap now filters out XML namespace-declaration pseudo-attributes (bare `xmlns="..."` and prefixed `xmlns:foo="..."`), restoring the byte-for-byte "no attributes -> unaffected" invariant for ordinary namespaced XML/SOAP documents that declare a namespace but carry no real attributes. - In the duplicate-sibling branch, each entry's "@attributes" map is now an independent clone (maps.Clone) rather than every sibling aliasing the same underlying map, so mutating one entry's attributes can no longer leak into its siblings. - Added regression tests for all of the above, plus mixed-content and self-closing-tag coverage, and two WithGenericResponse subtests documenting (as intentional) that a root XML element with attributes now succeeds where it previously errored with "unsupported XML structure: string". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if selfAttrs != nil { | ||
| if text == "" { | ||
| return map[string]any{"@attributes": selfAttrs}, nil | ||
| } | ||
| return map[string]any{"#text": text, "@attributes": selfAttrs}, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (compatibility, medium confidence): promoting an attributed leaf element from a bare string to a map[string]any is a silent, non-opt-in default change on a parser many connectors use via WithGenericResponse. A downstream doing resp["user"].(string) on <user active="true">alice</user> previously got "alice"; it now gets a map, so the assertion yields "", false — a silent wrong/empty value that flows into resource IDs and grants (durable c1z output). Attributes were dropped before, so no caller could depend on the attribute data, but callers reading the text of an attributed leaf as a string will break. Since this is deliberate, consider making the shape opt-in (or clearly signaling it with a 0.x minor bump / migration note) rather than changing the default path.
General PR Review: uhttp: capture XML attributes in the XML->map unmarshalerBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryFull PR diff scanned for security and correctness. The change adds XML attribute capture to Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsVerify each finding against the current code and only fix it if needed. SuggestionsIn
Note: the machine-readable review-state marker was omitted because the CI shell sandbox blocks emitting JSON braces; this only affects incremental-review bookkeeping, not the review itself. |
2 participants
Summary
Capture XML attributes in uhttp's XML→map unmarshaler. Previously
unmarshalXMLElementread only element/text tokens and silently dropped all attributes, so data carried in XML attributes (common in SOAP/WS-*) was lost.Changes (
pkg/uhttp/xml.go)@attributessibling key (map of local-name → value). Additive.{#text, @attributes}(or{@attributes}if no text) — a deliberate, documented type change at that path only.xmlns/xmlns:prefixnamespace-declaration pseudo-attributes.Tests
pkg/uhttp/xml_test.go,wrapper_test.go: attributed container, attributed leaf (with/without text), namespace-prefixed attr, nested, attrs+duplicates, namespace collision, xmlns filtering, per-sibling independence; plus aWithGenericResponsecase for an attributed-leaf root. All pre-existing subtests unchanged.go test ./pkg/uhttp/...andgo build ./...pass.Backward-compat
The attributed-leaf string→map promotion (and the related
WithGenericResponseerror→success for an attributed-leaf root) is the one deliberate behavior change; no-attribute paths are unchanged. Validated end-to-end against a live SOAP server (attribute data flowed into resource IDs and grants).Part of CXH-2134. Resolves CXH-2137.