Appending within an array could work if we could reference array elements in path expressions

[hrj]

I am facing a situation that I don't understand. In my config file if I write at the top level:

exports += {
  type = xml
  output = pqr
}
exports += {
  type = ledger
  output = xyz
}

and if I do config.getConfigList("exports"), I get a List of two objects as expected.

However, this doesn't work at the second level of nesting:

exports += {
  type = xml
  output = pqr
}
exports += {
  type = ledger
  output = xyz
  closures += {
    source = [Income]
    destination = Assets
  }
  closures += {
    source = [Expenses]
    destination = Assets
  }
}

Then I don't get two objects when I call export.getConfigList("closures"). What I get is a list with a single object that corresponds to the last one specified in the config file.

Not able to figure out why the syntax that works at the root level doesn't work at the nested level.

[havocp]

try config.root.render() to see what's generated vs. what you expected, maybe? It isn't instantly obvious to me what's going wrong. maybe += does the wrong thing if the thing you're adding to is completely unset to begin with... you could try starting with closures = [] ? just a wild guess. I don't remember offhand the specified behavior for that so don't know if we are off-spec. See https://github.com/typesafehub/config/blob/master/HOCON.md

I would need to dig in and make a test case, I can't do it right away but would like to if you don't figure it out.

[hrj]

config.root.render() shows the same problem (an array with a single object instead of two).

Using closures = [] at start doesn't change the behaviour.

The funny thing is this works perfectly well at the root level.

[havocp]

I suspect this is just a bug and it should work but doesn't. So just need to make a minimal junit case in the test suite and then fix it. The problem could be that when you have an object inside a list we get confused about resolving substitutions, I don't know. (a += b expands to a = ${?a} [b] as the spec describes, so it boils down to a substitution ${?a} and that thing presumably is not resolving properly).

[hrj]

Are you not sure what the behaviour should be in this case?

I am now a little worried about the usability of HOCON. Are the rules getting complex to the point it is confusing the authors too?

That's a genuine question; please don't take it otherwise.

[havocp]

I guess I'm just hedging because I haven't had time to look into it. I believe it's supposed to work as you expect but I might be missing some detail of what's going on.

HOCON is complex to implement, but it's (mostly) in the direction of trying to do what humans mean. You can always stick to the JSON subset and then incrementally use other features as you feel comfortable.

[hrj]

You can always stick to the JSON subset and then incrementally use other features as you feel comfortable.

It's not just about me. The config files will be written by my app's users. Fortunately the app isn't that popular yet :)

IMO, this is a major bug, because it silently consumes all except the last entry in the array. In my accounting application it results in an erroneous report.

But more than the bug, it is the ambiguity that I was worried about.

Aside, thanks for all the time you have been spending in supporting this library.

[havocp]

ok I made some test cases to reproduce. The problem seems to be specifically if you nest one use of += inside another use of +=. All of these tests pass except the last one. I will look into it.

    @Test
    def plusEqualsMultipleTimes() {
        val conf = parseConfig(""" a += 1, a += 2, a += 3 """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getIntList("a").asScala.toList)
    }

    @Test
    def plusEqualsMultipleTimesNested() {
        val conf = parseConfig(""" x { a += 1, a += 2, a += 3 } """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getIntList("x.a").asScala.toList)
    }

    @Test
    def plusEqualsAnObjectMultipleTimes() {
        val conf = parseConfig(""" a += { b: 1 }, a += { b: 2 }, a += { b: 3 } """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getObjectList("a").asScala.toList.map(_.toConfig.getInt("b")))
    }

    @Test
    def plusEqualsAnObjectMultipleTimesNested() {
        val conf = parseConfig(""" x { a += { b: 1 }, a += { b: 2 }, a += { b: 3 } } """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getObjectList("x.a").asScala.toList.map(_.toConfig.getInt("b")))
    }

    @Test
    def plusEqualsMultipleTimesNestedInPlusEquals() {
        val conf = parseConfig("""x += { a += 1, a += 2, a += 3 } """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList)
    }

[havocp]

A simpler test case doesn't nest += but just puts += inside a list:

    @Test
    def plusEqualsMultipleTimesNestedInArray() {
        val conf = parseConfig("""x = [ { a += 1, a += 2, a += 3 } ] """).resolve()
        assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList)
    }

The problem is pretty simple to explain but will require some gymnastics to fix. All substitution references are paths starting from the root. Currently, there is no way to refer to an array element. So there's actually nothing we can expand += to when inside an array. The code wrongly expands it to this:

  "x" : [
        {
            "a" : ${?x.a}[1],
            "a" : ${?x.a}[2],
            "a" : ${?x.a}[3]
        }
    ]

But ${?x.a} does not exist. What exists is something like ${?x[0].a} but we don't support that array indexing in a path expression.

In short we are just ignoring that we're inside a list, but we can't do that.

This behavior is probably what the spec says, but it doesn't make sense and I doubt anyone is relying on it. So I think we should fix it.

Here are the options I can think of right now:

1. Throw an error "+= impossible when nested inside a list"
2. Implement lexically-scoped substitutions maybe with another syntax like ^{?a} (see maybe search lexical scope for substitutions? (was: list entry local variable resolution) #40 ) and expand to that
3. Implement array indexes in path expressions like ${x[0].a} (see path expression to access array elements directly #30 ) and expand to that
4. Implement 2 or 3, but without any syntax - have a way to represent lexical scope or array indexing in the internal representation of a reference, but don't expose it.

Option 1 is trivial to do, but sucks. Options 2-4 are probably the right path and have been requested anyway, but are sort of involved. Option 1 is probably better than silently doing the wrong thing as we do now, though, so I might throw it in short-term.

[havocp]

I implemented throwing an error (which is better than incorrect results) but leaving this issue open because we should make this case work.

[dukejansen]

Just came across this fix because it began throwing exceptions on an existing config which was working previously. Based on what I'm reading here, it seems likely this is a case where the code is surfacing something that was silently not working before, so I'm confident I'll be able to avoid the exception by revisiting my nested += usage.

Nevertheless, it was a surprise for config which "worked" (ran) against 1.2.0 to fail against 1.2.1 with an exception. Perhaps this would have been better as a logged warning in 1.2.1 rather than an exception? Then you could throw the exception in a 1.3.0 release. This might have been a bit less disruptive. Just something to think about.

Just sharing my experience in case it's helpful in making changes like this in the future. Obviously I love the library and am in no place to nitpick on a project I have yet to contribute to. :)

For context: We were using this to drive some environment-specific logging configuration, so the silent failure was simply resulting in some missing debug logs and therefore never detected, but the exception throwing was quite dramatic. So your exception is doing its job informing me of the misconfiguration, which is why I think it's reasonable to throw it -- just a bit jarring in a dot release. I may be a little too religiously attached to http://semver.org/.

[dukejansen]

I have confirmed that my use case was one where I was adding a single value to the nested list, and that list was empty, so it was easy to rework my config to just set a list value rather attempting to add. Presumably my case was working before because it was only a single value being added to an empty list, which actually worked correctly, unless I'm mistaken. Maybe the exception case could be updated to only throw if there's actually going to be a problem, although with the expansion magic you described maybe not. Good luck with the more thorough fix, which sounds challenging.

[havocp]

Thanks for understanding and letting me know. You are probably right that a warning would be better in situations like this.