Added a note about avoiding ConfigFactory.load()

[staslev]

Added a note about avoiding ConfigFactory.load() to section 3 - Application Architecture.

Let me know if it makes sense :)

[alexandru]

Thanks @staslev,

I like the rule and it's long overdue that we had a rule about configs in this repository. But I'd go much further with this.

First of all calling ConfigFactory.load() is error prone, because it doesn't take care of possible config options and for example Config#resolve() is not called and it should be. For a web service I'd also like to log from where the configuration was loaded (e.g. file path or resource). I prefer to have an utility that is supposed to be called everywhere, instead of calling ConfigFactory.load() directly, like:

sealed trait ConfigSource
object ConfigSource {
  case class Resource(name: String) extends ConfigSource
  case class Path(name: String) extends ConfigSource
  case object Unknown extends ConfigSource
}

object ConfigUtil {
  def getConfigSource: ConfigSource =
    Option(System.getProperty("config.file")) match {
      case Some(path) if new File(path).exists() =>
        ConfigSource.Path(path)

      case _ =>
        val name = Option(System.getProperty("config.resource"))
          .getOrElse("application.conf")
        ConfigSource.Resource(name)
    }

  def loadFromEnv(): (ConfigSource, Config) = {
    getConfigSource match {
      case ref @ ConfigSource.Path(path) =>
        (ref, ConfigFactory.parseFile(new File(path)).resolve())
      case ref @ ConfigSource.Resource(name) =>
        (ref, ConfigFactory.load(name).resolve())
      case ConfigSource.Unknown =>
        (ConfigSource.Unknown, ConfigFactory.load().resolve())
    }
  }
}

But going further, using a Config object directly is not type-safe, which means that using it directly is error prone and (I'd argue) anti-Scala. This is why I like to model a type-safe and immutable configuration object like this:

case class AppConfig(
  configSource: ConfigSource,
  httpClient: HttpConfig,
  other: OtherConfig)

case class HttpClientConfig(
  maxTotalConnections: Int,
  maxConnectionsPerHost: Int,
  requestTimeout: FiniteDuration,
  connectionTimeoutMs: FiniteDuration,
  followRedirects: Boolean,
  compressionEnabled: Boolean,
  userAgent: String)

case class OtherConfig(
  someField: String)

object AppConfig {
  def loadFromEnv(): AppConfig = {
    val (source, config) = ConfigUtil.loadFromEnv()
    load(config).copy(configSource = source)
  }

  def load(config: Config): AppConfig = {
    AppConfig(      
      configSource = ConfigSource.Unknown,
      httpClient = HttpConfig(
        maxTotalConnections = config.getInt("httpClient.maxTotalConnections"),
        maxConnectionsPerHost = config.getInt("httpClient.maxConnectionsPerHost"),
        requestTimeoutMs = config
          .getDuration("httpClient.requestTimeout", TimeUnit.MILLISECONDS).millis,
        connectionTimeoutMs = config
          .getDuration("httpClient.connectionTimeoutMs", TimeUnit.MILLISECONDS).millis,
        followRedirects = ref.followRedirects,
        compressionEnabled = ref.compressionEnabled,
        userAgent = config.getString("httpClient.userAgent")),
      other = OtherConfig(
        someField = config.getString("other.someField")))
    }
}

And then you don't have to tight couple your code from some unsafe, error-prone and mutable config instance, when you could do this instead:

class HttpClient(config: HttpClientConfig) {
  ???
}

And title should be something like: "MUST NOT use ConfigFactory.load parameterless or directly" (imho it should be a mandatory rule, after all what I described above makes the code more clear without downsides).

So what do you say? If you agree, then include this as well and also keep your rationale as it's good.

[staslev]

@alexandru , thanks for taking a look!

• Are you sure about the resolve not being called? "Before using a Config it's necessary to call resolve() to handle substitutions (though ConfigFactory.load() and similar methods will do the resolve for you already)" see https://typesafehub.github.io/config/latest/api/com/typesafe/config/Config.html, "Resolving substitutions" section. I've also taken a look at the sources and it seems like the docs check out.
• Your utilities provide a nice way to deal with some of the common configuration caveats.
  I was wondering if you meant you'd like to see your concrete implementation is this rule?
  Perhaps we could say that it is recommended to have domain specific, type-safe config classes, such that your code operates on them, instead of the general purpose, map-like config. One can think of a number of ways to actually implement this concept, one of them being your implementation, the other for instance Kafka's approach and so on.
• Sure, I'll change it to MUST NOT + directly.

What do you think?

[alexandru]

On resolve not being called, I remember that I had problems, maybe it's only with Config.parseFile, so this needs to be checked. The problem I'm solving in my own stuff with ConfigUtil is that I want the exact same logic for loading the configuration (so for example in a Play application I'm overriding Global.onLoadConfig to use ConfigUtil), so then I don't have any more worries that Play is loading something different than my own code (plus I add a logger.info of the configuration source, which is always useful).

On Kafka's approach, I guess that's fine too, though personally I prefer to work with cases classes of values (immutable stuff). So we could mention both.

People complained about some rules not having examples. I think examples are good, so why not? :-) Though maybe we shouldn't list something like ConfigUtil, since that's too specific to a certain use-case, but something like the case class AppConfig would be useful to let people see what we mean by "domain specific config classes".

[staslev]

@alexandru I've made some changes, let me know what you think.

[alexandru]

Dude, btw, looks good. Will merge when I get home.

[staslev]

Sure thing, let me know if you see anything else.