When marshal with map[string]string, it gave me ">UNKNOWN/>"

[gqcn]

Hi there, mxj is a great project and I've been using it in my gf project in XML marshal/unmarshal. It worked quite well with map[string]interface{} type, but when the parameter contains other map type (eg: map[string]string/map[string]int etc...), it failed working.

Here's the simple codes to reproduce this issue:

package main

import (
    "fmt"
    "github.com/clbanning/mxj"
)

func main() {
    m := make(map[string]interface{})
    m["m"] = map[string]string {
        "k" : "v",
    }
    b, _ := mxj.Map(m).Xml()
    fmt.Println(string(b))
}

It's supposed to give me {"m":{"k":"v"}} , but actually I got >UNKNOWN/>.

Would you please take a look, and kindly give me a nice feedback, thanks!

[clbanning]

I assume you want <m><k>v</k></m>.

I'll look into it. There's not a quick fix, since the encoder doesn't use reflection. (It was originally written to re-encode values that were the result of calling mxj.NewMapXml().)

[gqcn]

@clbanning
Sorry, yes, I want <m><k>v</k></m>.
Can we just take a check and turn all map type to map[string]interface{} before passing it to mxj.Map()? It's simple , although it has bad performance...

[clbanning]

Addressed with release v1.8. (Note: also handles map[int]string values, etc.)

[gqcn]

Well done, I pulled the latest codes and this issue was gone, thanks a lot.

But hey I noticed that you solved this using reflection, compulsively convert any map type to map[string]interface{}(no matter it's originally map[string]interface{} or not) and also make a new copy variable of that. This will affect the performance a little bit (especially when the map is big).

if reflect.ValueOf(value).Kind() == reflect.Map {
	if _, ok := value.(map[string]interface{}); !ok {
		val := make(map[string]interface{})
		keys := reflect.ValueOf(value).MapKeys()
		vv := reflect.ValueOf(value)
		for _, k := range keys {
			val[fmt.Sprint(k)] = vv.MapIndex(k).Interface()
		}
		value = val
	}
}

Is it better have a original type check, and using type assertion instead of reflection?

[clbanning]

First, note that only map values that are NOT map[string]interface{} are processed -

   if _, ok := value.(map[string]interface{}); !ok {
      ...
   }

Using type check logic:

   switch value.(type) {
   case map[string]interface{}:
   default:
      if reflect.ValueOf(value).Kind() == reflect.Map {
         val := make(map[string]interface})
         ...
         value = val
      }
   }

requires at least similar work to be performed (under the covers) for map values and, I suspect, more processing of non-map values.

In general, users of this package are encoding Map values that were decoded from it or are dealing with simple non-nested Map values they've created. So lots, if not most, values - the 'value' argument passed to mapToXmlIndent() - are not map[string]interface{}; they are string, int, float64, []interface{}, etc. The current logic lets all these pass with a simple, light-weight interrogation of their 'Kind()' - completely skipping the case map[string]interface{}: test that would be required for every value by the type check logic. (And then they are still subjected to the == reflect.Map test.) Only reflect.Map 'values are processed further, and then only those map values that are not map[string]interface{} require a new allocation, which is the real performance impact.

But I'm not sure the overhead for type assertion - I think it's minimal, since it must first check that the assertion can be made, then just creates a new 'value' but doesn't create a new hash map itself. (Note: map values are like slices and pointers in that regard.) But just in case the compiler isn't smart enough to just perform the type check and not create a new value, I've changed that logic from performing a type assertion to a type check.

[gqcn]

I looked into the codes in detail, yes you're right. Thanks for your detailed explanation, and also your great project.