Improve analyzePath

[darigaaz]

1. In current implementation there is no support for nested brackets in key/field names

For example:

// pseudocode

// "Map[a[b][c]d]" : "123"
var m map[string]map[string]string
m["a[b"]["c]d"] = "123"
// must be m["a[b][c]d"] = "123"

// Map[a[b[c[[]]]]: "321"
var m map[string]map[string]map[string]map[string]string
m["a[b[c[["][""][""][""] = "321"
// must be m["a[b[c[[]]]"] = "321"

// and so on
// for more examples see tests

2. Decoder panics on "array index out of bounds"

var s struct {
  A [1]string{}
}

vals := url.Values{
  "A[2]": []string{"A"},
}

dec.Decode(vals, &s)
// panics "array index out of bounds"
// must return error instead

3. Nested indirects (pointers/interface{})

var m TestStruct struct {
	PointerToPointer **string
        InterfaceStruct interface{}
}
m.InterfaceStruct = new(struct {
  ID int
  Name string
  Interface interface{}
})

vals := url.Values{
  "PointerToPointer": []string{"PointerToPointer"},
  "InterfaceStruct.Interface": []string{"InterfaceStruct.Interface"},
}

This PR fixes it:
Allow unbalanced and nested brackets
Allow nested indirects (pointer or interface{})
Fix panic on "array index is out of bounds" - return error

I will check benchmark results with these changes and post it here later

[arp242]

Hey, thanks! Sorry if this is a bother, but I think this should really be split out in three different patches:

1. Changes a panic to an error.
2. Allow map keys with [] in them.
3. Allow pointers to pointers.

The problem right now is that it's hard to review the full patch, since it quite large and changes three different things. We need to be sure existing behaviour isn't changed in incompatible ways, or that it doesn't break other things (there are tests, but they may not cover everything). It also makes it harder to discus the patch itself.

Very quickly: 1) seems a good change, for 2) I'm a little bit worried about backwards compatibility with people who already use [] in key names, but I'd have to check in detail, and for 3) I'm not sure if I understand what the use case is? I've never seen anyone use **string, and I don't know if there's any case where that makes sense? It would be helpful if you could expand on real-world use cases for this.

[darigaaz]

Yeah, sure, i can split it in different patches.

Here are some rationale:

2. Allow map keys with brackets.
   We have no control over query params send to us from 3rd party applications.
   In application I am working on I have to save custom data send by clients - best way to do it is via map[string]string. For example: /api/?customdata[key1]=value1&customdata[key2]=value2 and we don't want to apply unnecessary restrictions on key format, but right now its imposible to do customdata[a[b]]=val it will be parsed incorrect like m["a[b"][""] = "val".
   Another example is struct with fieldTag name with brackets.
   Its impossible to do something like struct Data { ID string formam:"[]" } because it will be ambiguous how to parse /?data.[]=123.
   But every other option where there is no pair of open-closed brackets are fine: ex /?data.]]]=123, /?data.[[[=321, /?data.]][[=213.
   And sometimes clients are buggy, real-life example: /?is_numeric($_GET[ - obviously something went wrong on client's side constructing request, but i do not want to get error missing ']' bracket because its ok to have a field named is_numeric($_GET[ and it must be error free with IgnoreUnknownKeys: true option.
   About backward compatibility: no existing test cases were changed so we must be good here.

3. Nested indirects.
   Initially i was fixing bug with map[string]*struct{ ID int } (map to pointer to struct).
   It errors out with unsupported type, because it has to be traversed twice: once for map and once for pointer to struct.
   And I do not want to apply unnecessary restrictions on nesting level, unless we want to be extremely performance oriented.

[arp242]

And I do not want to apply unnecessary restrictions on nesting level, unless we want to be extremely performance oriented.

It's not really about performance but rather about complexity; I'm not really worried about 3 nanosecond of performance or whatnot.

The current code is already rather complex and difficult to follow in places, and if no one is using a feature then IMHO it doesn't really make much sense in adding it. It won't really help anyone and increases maintenance burden in the future, as well as the risk of bugs in getting it wrong in edge cases and the like.

In my experience, most Go libraries (including stdlib ones) don't really work with nested pointers. Unless there's an actual practical problem that this solves, I'm not so sure if we really need to add this.

[darigaaz]

splitted in 3 issues/PRs:
#41
#43
#45

i am closing this one