Lists are lost when using Extend

[tiramiseb]

When extending a config with a second Config object which contains lists, all elements in lists in the second object, except the first element, are lost.

I have narrowed it down to the getKeys function, which generates values like [foo bar 0], [foo bar 1], [foo bar 2], etc : when removing the whole case []interface{}: block in this function, extending seems to work perfectly. I have however not done extensive testing...

[olebedev]

Hi @tiramiseb,

Thanks for the report! I will dig into it asap.

[tiramiseb]

Any news?

[olebedev]

Unfirtunatelly, I have no enough time to dig and solve. Could you raise a PR with the solution?

[once168]

cfg.Set(`phones.0`, `1381000000`)  ===>it ok
cfg.Set(`phones.1`, `1381000001`)   ===>it error

because of the code :
case []interface{}:
if i, error := strconv.ParseInt(part, 10, 0); error == nil {
// 1. normalize slice capacity
if int(i) >= cap(c) {
c = append(c, make([]interface{}, int(i)-cap(c)+1, int(i)-cap(c)+1)...)
}
----- append changed variant c ,but is local

[molproduktooo]

Well, you probably did a very bad thing: you made the Extend() function flip config which is getting extended, and the config which does extend. Now all the code where people used this, will stop working. Basically, you broke the lib.

[molproduktooo]

If you did this, you should have left original version as it was, and create a new additional extend. In previous version, it wasn't quite clear what extends what, but quick look at source code was enough to clarify it. At least it was untuitive. Now you made it reversed, and non-intuitive. Maybe it should be ExtendThe() and ExtendBy(), to avoid ambiguity.

And still: breaking the lib API contract, is the very bad move.

[olebedev]

@molproduktooo, hey, thanks for getting back. I haven’t reviewed it properly but breaking API is definitely the thing I would prefer avoiding.

Feel free reverting the change that broke the original behaviour with a short explanation, example and new test case. I will be more than happy reviewing it.

cc @haakonbaa

[molproduktooo]

Additionally, to the above. I suspected this when read the description of new Extend, and now after carefully reviewing the code am certain about this. Look at this https://github.com/olebedev/config/blob/7eaf6b4b0aa2/config.go#L432

Basically, you @haakonbaa undermined the very idea of this extend. You broke the very concept, of why it is (was) useful.

Do you know what the CSS is? Yeah, that "styles" thing? There's this magic word "cascade". Or, maybe you know something about Prototype-based inheritance? Doesn't it ring a bell?

What previous Extend did: added non-existent parts of config, PLUS did overwrite existing with new values. I.e. is was like CSS, you could SPECIALIZE the parent config, more general one, with child config, more specific one. One typical use of it was making two configs, one with general defaults, and one with specific customization values.

What current Extend does: it kind of "enriches" the config with missing fields. It's no longer a cascade, no longer an inheritance. I even don't know what the use of it might be. Looks like a diversion.

[molproduktooo]

Sorry for being toxic.

Would add tests, it's just all about time. I didn't added tests for the PR I submitted a year ago #24

In the meantime, I use this fork of mine, https://github.com/rusriver/config, which now has functions to parse time durations, and some other improvements. Maybe it'd be useful @olebedev

[olebedev]

#26

[haakonbaa]

I'm really sorry. I misunderstood the purpose of the function. I should have read and understood the functionality before i redid the whole thing. @olebedev @molproduktooo

[molproduktooo]

To solve the original issue, as @once168 rightly pointed out, this simple change should be made (if I am correct):

function set(), current code with context:

	for pos, part := range parts {
		switch c := (*point).(type) {
		case []interface{}:
			if i, error := strconv.ParseInt(part, 10, 0); error == nil {
				// 1. normalize slice capacity
				if int(i) >= cap(c) {
					c = append(c, make([]interface{}, int(i)-cap(c)+1, int(i)-cap(c)+1)...)
				}

				// 2. set value or go further
				if pos+1 == len(parts) {
					c[i] = value
				} else {

this line should be added:

	for pos, part := range parts {
		switch c := (*point).(type) {
		case []interface{}:
			if i, error := strconv.ParseInt(part, 10, 0); error == nil {
				// 1. normalize slice capacity
				if int(i) >= cap(c) {
					c = append(c, make([]interface{}, int(i)-cap(c)+1, int(i)-cap(c)+1)...)
					*point = c           <<------------ HERE
				}

				// 2. set value or go further
				if pos+1 == len(parts) {
					c[i] = value
				} else {

Can't make a PR at the moment, using some weird account now. This should fix the problem.

[molproduktooo]

and len() instead of cap() must've been there, also. Normalize slice SIZE, capacity is managed by Go runtime. Currently this may either work, or panic, depending on it's M.O.