Skip to content

cxp-846 return XML as a generic map: map targets and non-map roots #1061

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.

Sign up for GitHub

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

Open
agustin-conductor wants to merge 2 commits into
base: main
Choose a base branch
from
+207 −11
Open

cxp-846 return XML as a generic map: map targets and non-map roots #1061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3884f0
fix(uhttp): decode XML into a map target in WithAlwaysXMLResponse
agustin-conductor Aug 6, 2026
681b067
fix(uhttp): key a non-map XML root by the root element name
agustin-conductor Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 58 additions & 11 deletions pkg/uhttp/wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,27 @@ func WithXMLResponse(response any) DoOption {
}

// Ignore content type header and always try to parse the response as XML.
//
// A *map[string]any target is decoded through the generic xmlMap decoder, since
// encoding/xml cannot unmarshal into a map and would fail for every input. Any
// other target keeps going straight to xml.Unmarshal, so callers passing a typed
// struct are unaffected.
func WithAlwaysXMLResponse(response any) DoOption {
return func(resp *WrapperResponse) error {
if response == nil && len(resp.Body) == 0 {
return nil
}
if genericResponse, ok := response.(*map[string]any); ok {
// Scoped to the map target so the struct path below keeps its exact
// existing behavior, including for 204 and empty bodies.
if resp.StatusCode == http.StatusNoContent {
return nil
}
if resp.StatusCode >= 200 && resp.StatusCode < 300 && len(resp.Body) == 0 {
return nil
}
return unmarshalXMLToMap(genericResponse, resp)

@github-actions github-actions Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: A typed-nil (*map[string]any)(nil) passes the response == nil check above (interface holds a type), then this assertion succeeds with a nil genericResponse, and unmarshalXMLToMap does *response = vMap → nil-pointer panic on a non-empty body. WithGenericResponse guards this with an explicit nil check; consider mirroring it here. Low confidence — an unusual call pattern, but the map branch is new. (confidence: low)

@agustin-conductor agustin-conductor Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — confirmed, and it's a regression rather than a latent edge case, so fixed in 0cf06e7.

Verified the premise and the prior behavior:

iface == nil?            false                            // typed nil carries a type, so it passes the guard
xml.Unmarshal(typedNil): nil pointer passed to Unmarshal  // old behavior: clean error

So routing map targets through xmlMap turned that clean error into a panic. Reverting just the guard and running the new test reproduces it:

panic: runtime error: invalid memory address or nil pointer dereference

Guarded inside unmarshalXMLToMap rather than in WithAlwaysXMLResponse, so WithGenericResponse and any future caller are covered by the same check and it can't be reintroduced at a new call site. Returns InvalidArgument to match WithGenericResponse's existing nil handling. Test added: should error rather than panic on a typed-nil map target.

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: this newly routes bodies that previously always errored into unmarshalXMLElement, which recurses once per nesting level with no depth cap, on a body read with an unbounded io.ReadAll (wrapper.go:503) and with the content-type check bypassed by design. A few MB of nested open tags from a hostile or broken endpoint is a fatal (unrecoverable) stack overflow rather than a returned error. Pre-existing in the WithGenericResponse path, so not introduced here, but a depth limit in unmarshalXMLElement would cheaply close it. (low confidence on real-world reachability)

}
err := xml.Unmarshal(resp.Body, response)
if err != nil {
return fmt.Errorf("failed to unmarshal xml response: %w. status code: %d", err, resp.StatusCode)
Expand All @@ -230,6 +246,47 @@ func WithAlwaysXMLResponse(response any) DoOption {
}
}

// unmarshalXMLToMap decodes an XML body into a generic map via the xmlMap decoder.
// encoding/xml cannot unmarshal into a map directly, so any caller wanting an
// arbitrary XML document as a map has to route through xmlMap.
func unmarshalXMLToMap(response *map[string]any, resp *WrapperResponse) error {
// A typed-nil target, e.g. WithAlwaysXMLResponse((*map[string]any)(nil)), gets
// past an `any == nil` check because the interface still carries a type. Guard
// here rather than at each call site so assigning through it cannot panic.
// encoding/xml rejected this with "nil pointer passed to Unmarshal".
if response == nil {
return status.Error(codes.InvalidArgument, "response is nil")
}

var xm xmlMap
if err := xml.Unmarshal(resp.Body, &xm); err != nil {
return fmt.Errorf("failed to unmarshal xml response: %w. status code: %d", err, resp.StatusCode)
}
vMap, ok := xm.data.(map[string]any)
if !ok {
// The root's content has no map representation in two cases, and both used
// to fail outright with "unsupported XML structure":
//
// - Its direct children repeat: <Users><User/><User/></Users> decodes to
// a []map[string]any. A root-level list is a common API shape, so this
// was a real gap rather than an edge case.
// - It holds only text: <Code>OK</Code> decodes to a string.
//
// Key it by the root element name, which is otherwise discarded, so the
// document is reachable by a path instead of being an error.
//
// Note the arity seam this leaves: a root holding a *single* <User> decodes
// to a map and keeps the root stripped, so its path is "User" while the
// repeated case is "Users". One config cannot serve both. Closing that
// needs the decoder to group repeated children under their shared name,
// which would also make the slice case here unreachable.
*response = map[string]any{xm.root: xm.data}

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: Because WithGenericResponse now shares this helper, this line also changes that function's documented contract. Its doc comment (line 389) says "if the response is a list, its values will be put into the items field" — the JSON branch still honors that, but an XML root-level list now lands under the root element's own name instead. Worth updating that comment so the public contract matches both branches. (medium confidence)

return nil
}
Comment on lines +265 to +285

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: the comment says the non-map case is "a document whose root holds only text", but unmarshalXMLElement also returns []map[string]any whenever the root's direct children repeat (see xml_test.go:24). So a very common list shape — <Users><User>…</User><User>…</User></Users> — still hard-fails here with Internal: unsupported XML structure: []map[string]interface {}, which is arguably the main case parse_as: xml needs. Pre-existing in WithGenericResponse and not a regression, but worth either handling the slice case (e.g. wrap it under the root element name) or at least correcting the comment and adding a test so the limitation is explicit. (medium confidence)

@agustin-conductor agustin-conductor Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch on both halves — the comment was wrong, and the slice case is the more important one. Fixed in 681b067.

The comment. Corrected to name both ways the root's content can be a non-map, since I'd only documented the string case.

The slice case. Handled as you suggested, keyed by the root element name — xmlMap now records start.Name.Local, which it was discarding:

<users><user><login>a</login></user><user><login>b</login></user></users>
  before: Internal: unsupported XML structure: []map[string]interface {}
  after:  {"users": [{"user":{"login":"a"}}, {"user":{"login":"b"}}]}

I did the text-only root the same way (<Code>OK</Code> → {"Code": "OK"}), so unmarshalXMLToMap can no longer fail on structure at all. Both were errors before, here and on WithGenericResponse, so it stays error → success.

One caveat worth recording, since wrapping is reactive rather than a real fix. It inherits the decoder's arity asymmetry:

document decoded path
<users><user/><user/></users> {"users": [{"user":…},{"user":…}]} users
<users><user/></users> {"user": {…}} user

A single child means nothing repeats, so the content is a map, so the root name is discarded as usual — which means one config can't serve both arities for a root-level list. Measured, not assumed, and pinned by should keep stripping the root when its content is a map so it can't drift silently.

Still a clear win: the ≥2 case is the one every real tenant hits, and it went from an opaque SDK-internal error to something reachable by a path.

What actually closes the seam is grouping repeated children under their shared name — which would also make this slice branch unreachable. That was in an earlier revision of this PR and is deferred (see the PR description) because it changes what existing CEL and template expressions read on paths that never touch items extraction. Your finding is a second argument for it, so it's parked for its own audit rather than dropped.

Verified end-to-end against ConductorOne/baton-http#144 through a Go workspace: root-level lists now sync at both arities (with the path difference above), nested lists sync at both arities from a single path.

*response = vMap
return nil
}

type ErrorResponse interface {
Message() string
}
Expand Down Expand Up @@ -374,17 +431,7 @@ func WithGenericResponse(response *map[string]any) DoOption {
}

if IsXMLContentType(resp.Header.Get(ContentType)) {
var xm xmlMap
err = WithXMLResponse(&xm)(resp)
if err != nil {
return err
}
if vMap, ok := xm.data.(map[string]any); ok {
*response = vMap
} else {
return status.Errorf(codes.Internal, "unsupported XML structure: %T", xm.data)
}
return nil
return unmarshalXMLToMap(response, resp)

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: This is no longer a pure extraction — the shared helper's new root-keying changes WithGenericResponse's observable behavior. <users><user/><user/></users> used to return Internal: unsupported XML structure: []map[string]interface {} and now succeeds as {"users": [...]}; <Code>OK</Code> used to error and now returns {"Code": "OK"}. That direction is error→success so it can't break a working caller, but it means the arity seam documented at lines 278-282 now also applies to this already-shipping API: the same endpoint keys on the root name at 2+ items and on the child name at 1 item, and the 2-item case used to be a loud error rather than a silently different key. The new tests all go through WithAlwaysXMLResponse; TestWrapper_WithGenericResponse has no case pinning either new shape. Worth adding the 1-item/N-item pair there directly, and correcting the PR description, which still says this branch is "same decoder, same error wrapping" and still lists root-text as producing Internal: unsupported XML structure: string. (medium confidence)

}

return status.Error(codes.Unknown, fmt.Sprintf("unsupported content type: %s", resp.Header.Get(ContentType)))
Expand Down
144 changes: 144 additions & 0 deletions pkg/uhttp/wrapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,150 @@ func TestWrapper_WithXMLResponse(t *testing.T) {
})
}

func TestWrapper_WithAlwaysXMLResponse(t *testing.T) {
const userList = `<?xml version="1.0" encoding="UTF-8"?><SERVICE_RESPONSE><USER_LIST>` +
`<USER><LOGIN>john@example.com</LOGIN></USER>` +
`<USER><LOGIN>jane@example.com</LOGIN></USER>` +
`</USER_LIST></SERVICE_RESPONSE>`

newResp := func(body string, contentType string, statusCode int) WrapperResponse {
header := http.Header{}
if contentType != "" {
header.Add("Content-Type", contentType)
}
return WrapperResponse{
Header: header,
Body: []byte(body),
StatusCode: statusCode,
}
}

t.Run("should decode into a map despite a non-XML content type", func(t *testing.T) {
// encoding/xml cannot unmarshal into a map, so before this a *map[string]any
// target failed for every input with "unknown type map[string]interface {}".
resp := newResp(userList, "text/plain", http.StatusOK)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
// The shape is whatever the existing xmlMap decoder produces: a container
// with repeated children becomes a []map[string]any of single-key maps.
// This change only makes the map target reachable; it does not reshape
// anything, so the generic path's output is unchanged.
require.Equal(t, map[string]any{
"USER_LIST": []map[string]any{
{"USER": map[string]any{"LOGIN": "john@example.com"}},
{"USER": map[string]any{"LOGIN": "jane@example.com"}},
},
}, respBody)
})

t.Run("should leave the typed struct path unchanged", func(t *testing.T) {
exampleResponse := example{Name: "John", Age: 30}
buf := new(bytes.Buffer)
require.NoError(t, xml.NewEncoder(buf).Encode(exampleResponse))
resp := newResp(buf.String(), "text/plain", http.StatusOK)

responseBody := example{}
err := WithAlwaysXMLResponse(&responseBody)(&resp)

require.NoError(t, err)
require.Equal(t, exampleResponse, responseBody)
})

t.Run("should key a root-level list by the root element name", func(t *testing.T) {
// A root whose own children repeat decodes to a []map[string]any, which has
// no map representation and used to fail with "unsupported XML structure".
// Keying it by the root name makes this common shape reachable by a path.
resp := newResp(`<users><user><login>a</login></user>`+
`<user><login>b</login></user></users>`, "text/plain", http.StatusOK)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
require.Equal(t, map[string]any{
"users": []map[string]any{
{"user": map[string]any{"login": "a"}},
{"user": map[string]any{"login": "b"}},
},
}, respBody)
})

t.Run("should keep stripping the root when its content is a map", func(t *testing.T) {
// The same document at one item takes the map path, where the root name is
// discarded as it always has been — so the path is "user" here and "users"
// above. Pinning the seam: one config cannot serve both arities until the
// decoder groups repeated children under their shared name.
resp := newResp(`<users><user><login>a</login></user></users>`, "text/plain", http.StatusOK)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
require.Equal(t, map[string]any{
"user": map[string]any{"login": "a"},
}, respBody)
})

t.Run("should key a text-only root by the root element name", func(t *testing.T) {
resp := newResp(`<?xml version="1.0" encoding="UTF-8"?><root>bare text</root>`, "text/plain", http.StatusOK)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
require.Equal(t, map[string]any{"root": "bare text"}, respBody)
})

t.Run("should not decode a map target on 204", func(t *testing.T) {
resp := newResp("", "text/plain", http.StatusNoContent)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
require.Nil(t, respBody)
})

t.Run("should not decode a map target on an empty 200 body", func(t *testing.T) {
resp := newResp("", "text/plain", http.StatusOK)
var respBody map[string]any

err := WithAlwaysXMLResponse(&respBody)(&resp)

require.NoError(t, err)
require.Nil(t, respBody)
})

t.Run("should error rather than panic on a typed-nil map target", func(t *testing.T) {
// (*map[string]any)(nil) gets past an `any == nil` check because the
// interface still carries a type, so the map branch is reached with a nil
// pointer. encoding/xml used to reject this with "nil pointer passed to
// Unmarshal"; assigning through it would panic.
resp := newResp(userList, "text/plain", http.StatusOK)

var respBody *map[string]any
err := WithAlwaysXMLResponse(respBody)(&resp)

require.Error(t, err)
require.Equal(t, codes.InvalidArgument, status.Code(err))
})

t.Run("WithXMLResponse keeps rejecting map targets", func(t *testing.T) {
// Deliberately left alone: it is reached by WithResponse and by three
// connectors that pass typed structs, so its behavior is not widened here.
resp := newResp(userList, "application/xml", http.StatusOK)
var respBody map[string]any

err := WithXMLResponse(&respBody)(&resp)

require.Error(t, err)
require.Contains(t, err.Error(), "unknown type map[string]interface {}")
})
}

func TestWrapper_WithResponse(t *testing.T) {
exampleResponse := example{
Name: "John",
Expand Down
5 changes: 5 additions & 0 deletions pkg/uhttp/xml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ import (
// xmlMap implements xml.Unmarshaler and can unmarshal arbitrary XML into a
// map[string]any structure. Leaf elements become string values, and elements
// with children become nested maps.
// The root element's own name is recorded but not used as a key, so a document's
// paths start at its root's children. Callers need it only when the root content
// has no map representation and has to be keyed by something.
type xmlMap struct {
data any
root string
}

func (x *xmlMap) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
Expand All @@ -18,6 +22,7 @@ func (x *xmlMap) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
return err
}
x.data = result
x.root = start.Name.Local
return nil
}

Expand Down
Loading