BugOrBrokenException when using optional overrides and resolveWith

[tea-dragon]

eg. anything like this:

some-setting = 0
some-setting = ${?maybe.some.other.value}

where you might like to (possibly) resolve maybe.some.other.value without polluting extraneous values (such as you might get with system properties) or doing some kind of janky whitelist. Works fine in 1.2.1, aside from that being the version that still doesn't have my other fix from a million years ago.

[havocp]

I can't fix this right away (patches welcome), but for future reference:

Here is a test case for the end of ConfigTest.scala:

    @Test
    def resolveWithUndefinedOverriding(): Unit = {
        // overriding a value with an optional substitution
        val unresolved = ConfigFactory.parseString("foo = 42, foo = ${?a}")
        val source = ConfigFactory.parseString("b = 14")
        val resolved = unresolved.resolveWith(source)
        assertEquals(42, resolved.getInt("foo"))
    }

Here is how it currently fails:

[error] Test com.typesafe.config.impl.ConfigTest.resolveWithUndefinedOverriding failed: com.typesafe.config.ConfigException$BugOrBroken: replace in parent not possible ConfigDelayedMerge(42,${?a}) with ConfigInt(42) in ResolveSource(root=SimpleConfigObject({"b":14}), pathFromRoot=null), took 0.002 sec
[error]     at com.typesafe.config.impl.ResolveSource.replaceWithinCurrentParent(ResolveSource.java:245)
[error]     at com.typesafe.config.impl.ConfigDelayedMerge.resolveSubstitutions(ConfigDelayedMerge.java:110)
[error]     at com.typesafe.config.impl.ConfigDelayedMerge.resolveSubstitutions(ConfigDelayedMerge.java:59)
[error]     at com.typesafe.config.impl.ResolveContext.realResolve(ResolveContext.java:179)
[error]     at com.typesafe.config.impl.ResolveContext.resolve(ResolveContext.java:142)
[error]     at com.typesafe.config.impl.SimpleConfigObject$ResolveModifier.modifyChildMayThrow(SimpleConfigObject.java:379)
[error]     at com.typesafe.config.impl.SimpleConfigObject.modifyMayThrow(SimpleConfigObject.java:312)
[error]     at com.typesafe.config.impl.SimpleConfigObject.resolveSubstitutions(SimpleConfigObject.java:398)
[error]     at com.typesafe.config.impl.ResolveContext.realResolve(ResolveContext.java:179)
[error]     at com.typesafe.config.impl.ResolveContext.resolve(ResolveContext.java:142)
[error]     at com.typesafe.config.impl.ResolveContext.resolve(ResolveContext.java:231)
[error]     at com.typesafe.config.impl.SimpleConfig.resolveWith(SimpleConfig.java:74)
[error]     at com.typesafe.config.impl.SimpleConfig.resolveWith(SimpleConfig.java:69)
[error]     at com.typesafe.config.impl.SimpleConfig.resolveWith(SimpleConfig.java:37)
[error]     at com.typesafe.config.impl.ConfigTest.resolveWithUndefinedOverriding(ConfigTest.scala:1235)
[error]     ...

I think the problem is that we shouldn't be trying to do a replacement when the substitution we're resolving is not inside the resolve source. The right fix, I don't know without digging in.

[havocp]

thanks for the report btw!

[adamhonen]

I’ve taken a look at this. Unfortunately it would take much more time for me to understand the flow the code is going through, so I have to ask – why does ResolveSource.replaceWithinCurrentParent throw this exception and not just return itself (this) in this case as it once used to? Right out of git, I have 7 failed tests and making this change does not add any failures, yet at the same time, it does allow the above test and the test from issue #339 to pass.

Is there a test that could be added to demonstrate why the exception is needed in this case?

[havocp]

The reason I haven't fixed this of course is that I don't know offhand what the deal is ;-)

Here's a quick scan but I could easily be wrong without digging in more.

It looks like replaceWithinCurrentParent is only called here: https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java#L110

What we're doing is that if we have some stuff that overrides some stuff but requires resolving substitutions, like a=1, a=${b} then that is a "delayed merge." HOCON.md describes the algorithm, but roughly speaking while we are resolving ${b} we want to take ${b} out of the document so we don't get in a circular situation and so we can be self-referential, for example you could do a="hello", a=${a}"world". We replace the delayed merge stack with only the tail of the delayed merge stack in order to resolve the topmost item in the stack.

To allow this replacement, we track ResolveSource.pathFromRoot so we can go up that path (to replace the leaf node, we replace its parent with a parent with the replacement node, then we recursively go up replacing each parent).

The BugOrBroken is thrown when we aren't at the root, and we don't have a pathFromRoot, so we don't know our parent node. That means we can't do the replacement because we can't walk up the tree. The exception here is supposed to catch a bug where we failed to maintain pathFromRoot, I think.

I don't think we could add a test showing why this exception should be thrown because the point of it is to catch a believed-impossible situation. Clearly it isn't actually impossible.

Look at pushParent and the trace-mode checks that can be turned on: https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/ResolveSource.java#L145

In pushParent when we push a node that isn't a child of the root, we seem to simply never build up pathFromRoot. I guess that would happen with resolveWith.

Note also the comment here: https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java#L108 that's talking about resetParents() which is in fact a couple of lines below. But it's saying that after we replace a parent, on this line, we should be doing the equivalent of resolveWith (resolving against a root we aren't underneath): ResolveResult<? extends AbstractConfigValue> result = newContext.resolve(end, sourceForEnd); (link: https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java#L132)

I'm not sure if it's possible for end to have a value that would trigger this bug though because I'm not sure it can be a ConfigDelayedMerge, that would break an invariant that's checked here: https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java#L39

So if that's true, this only happens with resolveWith, which is good since it should be an unusual situation.

We have to think about what should happen with resolveWith. The weird thing about resolveWith is that self-referential substitutions may go haywire... I guess += will just stop working, for example, since it expands to a self-reference. a="hello", a=${a}"world" pretty much changes meaning ... I guess that's just how it is. Doesn't make me a bigger fan of resolveWith, I wonder if we discussed this issue when putting that in... it looks like no: https://github.com/typesafehub/config/issues/95 I suppose you could resolveWith with the allow-unresolved flag, and then resolve a file against itself to catch the self-referential stuff. The resolveWith docs kind-of hint at this.

My initial take then is that you're right, we could return this instead of throwing an exception because pathFromRoot == null means that we aren't a child of the root. The philosophy in pushParent seems to be that if we aren't a child of the root we just silently ignore all the parent-tracking stuff, so we could stick to that.

Might be nice to also add some more text to the resolveWith docs warning about the breakage to self-referential substitutions.

[havocp]

Thanks for digging in to this, btw.

[gkolok]

Any update on that? Will be fixed?

[bluesliverx]

I can confirm that jln-ho's commit fixes this for us, with no noticeable different in functionality as far as I can tell.

[rukgar]

Do you think this issue will be resolved or a workaround is available?

[mvallebr]

I am also interested on a fix / workaround I am getting the same problem with version 1.3.4

[ivanfrolovmd]

I found the following workaround helped in my case, to resolve against system parameters:

    File confFile = new File("...");
    Config config = ConfigFactory.parseFile(confFile);
    Config system = ConfigFactory.systemProperties();
    Config finale = config.withFallback(system).resolve();

In my case this made the following config work as I expect and fallback to explicit defaults as well as interpolate system properties if present:

app {
  component {
    hostname = localhost ; default
    hostname = ${?some.system.property.hostname} ; override if present
  }
}