Make ConfigFactory.load* methods throw when encountering a missing required include

[roschlau]

trafficstars

I couldn't find any mention of the include required("foo") syntax in the HOCON specification, I needed to dig through the discussion over on the corresponding issue #121, and even now I'm still confused as to intended behavior, since that issue states that a missing required config should throw an error, but when trying it with a minimal example, it seems to silently fail to load the whole config, and you only notice when the library reports a missing key, even if that key is not related to the missing config.

[havocp]

Does this get at it? https://github.com/lightbend/config/blob/master/HOCON.md#include-semantics-missing-files-and-required-files

[havocp]

I guess what you may be saying is that ConfigFactory.load still succeeds even if an individual file fails due to a required() ? I can imagine that we accidentally made it work that way without realizing, but I'm not actually sure.

[roschlau]

Whoops, forgot that there was a specification separate from the library ReadMe, sorry! So that's covered.

Yes, that's exactly what it seems like. I did a quick test setup like this:

# test.conf
test = "test"

include required("test2")

# test2.conf
test2 = "test2"

// Java code:
Config config = ConfigFactory.load("test");
System.out.println(config.getString("test"));

Run like that, all is well. If I change the include to another file that doesn't exist, the first line of Java code will execute fine, and the second will throw an exception, reporting that the key "test" is missing, even though that one was not even defined in the missing file. Stepping into the debugger, it seems like the config object that gets loaded is filled with the default jvm stuff etc., but any keys defined in test.conf or any included files are just missing. That quickly gets confusing if you have a larger configuration where an unfulfilled include isn't as obvious as in this simple example.

[roschlau]

Judging from the specification, expected behavior would be an Exception thrown on the first line of Java code if any required include can not be resolved.

[havocp]

I think what's happening is that the parseFile or parseResource for the individual files that ConfigFactory.load loads will throw an exception due to the required(), but the overall ConfigFactory.load effectively includes a list of files without required(). The spec is controlling the behavior of the individual file parses but not the entire default search path.

Fixing this could easily break someone in prod unfortunately - and files on the ConfigFactory.load search path are allowed to be missing, so at least some exceptions do need to be ignored.

I think it's probably right to fail the whole load() if a file fails to parse due to a required() but again the trick will be, we do not want to fail the whole load if the file is just missing. I wonder if the whole load should fail if one of the files has bad syntax in it... and I wonder if our existing ConfigParseOptions.ignoreMissing affects load() right now... need to do some research

[roschlau]

Sounds possible. ConfigFactory.load("specific-file.conf") does exhibit the same behavior, though.

I wonder if the whole load should fail if one of the files has bad syntax in it

I would say yes. Bad syntax is very likely not intended, which means at least a part of the application is not configured correctly, and in that case I want a fail-quickly approach so that I don't have to spend time debugging why some seemingly unrelated part of the system does not seem to adhere to my configuration.

[roschlau]

Just to clarify the impact of this, with the current implementation, using required is completely useless to me. With the current implementation, I'd rather use the standard include, because when it fails that will report a missing key from the missing file, so I'm already on the right track to find the actual problem. Whereas with the required include it will more than likely give me some unrelated key (because none of them work), and I'll have to go through the complete application config hoping to find the problem eventually.

That is unless there is a way to get logs from the library that tell me where parsing problems happened - is there something along those lines?

[havocp]

there is a -Dconfig.trace=load (maybe I got that slightly wrong, it’s in the readme tho)

[havocp]

I generally agree with you here and I think it was an oversight / unintentional interaction when we added required. It needs some research and care to add test cases, specify the behavior etc and maybe some judgment about risk of breaking existing setups.

[havocp]

So where required should be working right now is with the parse* family of functions (vs. the load*)

[roschlau]

Yes, the parse* functions seem to work like expected!

[roschlau]

I've changed the issue title to reflect what the discussion has turned out to be actually about.

[aumann]

It seems that this been has fixed by now? At least I'm seeing an exception when using "ConfigFactory.load()", when the -Dconfig.file require-includes a file which does not exist.

But may also be a happy accident :smile:

[gabrieljones]

Still very much a problem in v1.4.2. This is very surprising behavior. It had me questioning my sanity for about an hour.

[gabrieljones]

As a workaround, Would it be possible to create a custom stub ConfigIncluder that throws an exception instead of silently discarding any confs that are missing required includes? What might that look like?

[amanteaux]

As others, I am annoyed that the default behavior allows includes to fail silently.

I did a bit of digging:

• #121: There is actually the required statement that "solves" the issue, so instead of writing include classpath("file.conf"), we should write include required(classpath("file.conf"))
• #683: @radist-nt proposes a solution that is actually working but messy (see below)

Would it be possible to add in the ConfigParseOptions the option allowIncludeMissing that would be true by default to keep the existing behavior in v1.x.x but that will be false in v2.x.x?

While this solution is not accepted, here is the temporary solution:

• To create the config object: ConfigFactory.load(ConfigParseOptions.defaults().setAllowMissing(false).setIncluder(new RequiredIncluder()));
• The full code of RequiredIncluder:

import com.typesafe.config.*;

import java.io.File;
import java.net.URL;
import java.util.Objects;

/**
 * Config includer that forces all includes to resolve or else fail with a ConfigException
 */
public class RequiredIncluder implements ConfigIncluder, ConfigIncluderFile, ConfigIncluderURL,
    ConfigIncluderClasspath {
    private ConfigIncluder delegate;

    @Override
    public ConfigIncluder withFallback(ConfigIncluder fallback) {
        Objects.requireNonNull(fallback);
        if (delegate != fallback) {
            if (delegate == null)
                delegate = fallback;
            else
                delegate = delegate.withFallback(fallback);
        }
        return this;
    }

    @Override
    public ConfigObject include(ConfigIncludeContext context, String what) {
        context = verifyDelegateAndStrictContext(context);
        return delegate.include(context, what);
    }

    @Override
    public ConfigObject includeResources(ConfigIncludeContext context, String what) {
        context = verifyDelegateAndStrictContext(context);
        if (delegate instanceof ConfigIncluderClasspath) {
            return ((ConfigIncluderClasspath) delegate).includeResources(context, what);
        }
        return ConfigFactory.parseResources(what, context.parseOptions()).root();
    }

    @Override
    public ConfigObject includeFile(ConfigIncludeContext context, File what) {
        context = verifyDelegateAndStrictContext(context);
        if (delegate instanceof ConfigIncluderFile) {
            return ((ConfigIncluderFile) delegate).includeFile(context, what);
        }
        return ConfigFactory.parseFile(what, context.parseOptions()).root();
    }

    @Override
    public ConfigObject includeURL(ConfigIncludeContext context, URL what) {
        context = verifyDelegateAndStrictContext(context);
        if (delegate instanceof ConfigIncluderURL) {
            return ((ConfigIncluderURL) delegate).includeURL(context, what);
        }
        return ConfigFactory.parseURL(what, context.parseOptions()).root();
    }

    private ConfigIncludeContext verifyDelegateAndStrictContext(ConfigIncludeContext context) {
        Objects.requireNonNull(delegate, "ConfigIncluder not accessible");
        if (context.parseOptions().getAllowMissing()) {
            return context.setParseOptions(context.parseOptions().setAllowMissing(false));
        }
        return context;
    }
}