Skip to content

Conversation

@gustavs
Copy link
Contributor

@gustavs gustavs commented May 6, 2025

Added support to encode maps as parameters, simplifies integrating with Odoo XMLRPC API for example.

@alexejk alexejk requested a review from Copilot June 29, 2025 14:32
@alexejk alexejk self-assigned this Jun 29, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for encoding Go maps into XML-RPC <struct> elements, enabling richer parameter serialization.

  • Added a new encodeMap method and hooked it into encodeValue for reflect.Map kinds
  • Introduced tests in encode_test.go covering empty maps, basic types, nested maps, and non-string keys
  • Defined XMLRPC* helper types in tests (currently unused)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
encode.go Added case reflect.Map in encodeValue and new encodeMap method to serialize maps as XML-RPC structs
encode_test.go Added Test_encodeMap with various map scenarios and helper XMLRPC* types

Comment on lines +133 to +160
case reflect.Map:
if err := e.encodeMap(w, value); err != nil {
return fmt.Errorf("cannot encode map value: %w", err)
}
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

The reflect.Map branch in encodeValue calls encodeMap without wrapping the output in tags, causing nested maps to be encoded incorrectly. Consider adding ... around encodeMap's output to match XML-RPC format.

Suggested change
case reflect.Map:
if err := e.encodeMap(w, value); err != nil {
return fmt.Errorf("cannot encode map value: %w", err)
}
case reflect.Map:
_, _ = fmt.Fprint(w, "<value>")
if err := e.encodeMap(w, value); err != nil {
return fmt.Errorf("cannot encode map value: %w", err)
}
_, _ = fmt.Fprint(w, "</value>")

Copilot uses AI. Check for mistakes.
_, _ = fmt.Fprint(w, "<struct>")

mapValue := reflect.ValueOf(val)
iter := mapValue.MapRange()
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

Iteration over maps in Go is non-deterministic, which may produce unstable XML output. Consider collecting and sorting the keys before encoding to ensure deterministic ordering.

Copilot uses AI. Check for mistakes.
}

func (e *StdEncoder) encodeMap(w io.Writer, val interface{}) error {
_, _ = fmt.Fprint(w, "<struct>")
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

Write operations on io.Writer ignore errors here. It’s better to check and handle potential write errors instead of discarding them.

Suggested change
_, _ = fmt.Fprint(w, "<struct>")
if _, err := fmt.Fprint(w, "<struct>"); err != nil {
return fmt.Errorf("failed to write struct start tag: %w", err)
}

Copilot uses AI. Check for mistakes.
encode_test.go Outdated
Comment on lines 407 to 423
// XMLRPCValue represents a value in XML-RPC format
type XMLRPCValue struct {
Type string `xml:"type,attr"`
Value interface{} `xml:",chardata"`
}

// XMLRPCMember represents a member in XML-RPC struct
type XMLRPCMember struct {
Name string `xml:"name"`
Value XMLRPCValue `xml:"value"`
}

// XMLRPCStruct represents an XML-RPC struct
type XMLRPCStruct struct {
Members []XMLRPCMember `xml:"member"`
}

Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

The XMLRPCValue, XMLRPCMember, and XMLRPCStruct types are declared but not used in this test file. Consider removing them to reduce clutter.

Suggested change
// XMLRPCValue represents a value in XML-RPC format
type XMLRPCValue struct {
Type string `xml:"type,attr"`
Value interface{} `xml:",chardata"`
}
// XMLRPCMember represents a member in XML-RPC struct
type XMLRPCMember struct {
Name string `xml:"name"`
Value XMLRPCValue `xml:"value"`
}
// XMLRPCStruct represents an XML-RPC struct
type XMLRPCStruct struct {
Members []XMLRPCMember `xml:"member"`
}

Copilot uses AI. Check for mistakes.
@alexejk
Copy link
Owner

alexejk commented Jun 30, 2025

Thanks for the contribution. As I had to adjust a few parts of the implementation, I've rebased your commits and pushed them as part of #92 instead. I will be closing this one while wrapping up the implementation and cleanup in the other branch 🙇🏼

@alexejk alexejk closed this Jun 30, 2025
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