zio / zio-config

Easily use and document any config from anywhere in ZIO apps
https://zio.dev/zio-config
Apache License 2.0
231 stars 112 forks source link

Typesafe-conf sequence injection is not supported #557

Open broij opened 3 years ago

broij commented 3 years ago

I am using java properties to inject the values of an array in an HOCON configuration file (https://github.com/lightbend/config#set-array-values-outside-configuration-files).

Here is a simple code snippet reproducing the issue I am facing:

package be.broij

import zio.ZIO
import zio.config._
import zio.config.magnolia.DeriveConfigDescriptor._
import zio.config.typesafe._

object MyApp extends zio.App {
  final case class Config(items: Set[String])

  val configDescriptor = descriptor[Config]

  def run(args: List[String]) = {
    System.setProperty("ITEMS.0", "a")
    System.setProperty("ITEMS.1", "b")
    System.setProperty("ITEMS.2", "c")
    oopsie.exitCode
  }

  val oopsie =
    TypesafeConfigSource.fromDefaultLoader.map { s =>
      read(configDescriptor from s)
    }.flatMap(ZIO.fromEither(_))
}

Content of application.conf:

items = ${ITEMS}

Dependencies:

val zio = "dev.zio" %% "zio" % "1.0.8"
val zioConfig = "dev.zio" %% "zio-config" % "1.0.6"
val zioConfigMagnolia = "dev.zio" %% "zio-config-magnolia" % "1.0.6"
val zioConfigTypesafe = "dev.zio" %% "zio-config-typesafe" % "1.0.6"

ZIO config fails to load the configuration:

Fiber failed. A checked error was not handled. ReadError: FormatError cause: Provided value is of type Record, expecting the type Sequence path: items

This error seems to come from zio.config.ReadModule.read.loopSequence line 233. I checked the content of the config resolved by ConfigFactory.load.resolve. Typesafe-conf indeed loads this as a record ("items" : {...}) instead of a sequence ("items" : [...]).

I can get around the issue by updating my config definition final case class Config(items: Set[String]) to final case class Config(items: Map[String, String]). However, from the lib perspective, I think it is a bad idea to keep it as such. Forcing the users to write their configuration in a particular way would pollute their domain and over-complexify it. Moreover, supporting sequences injection would provide a better experience for the users as they wouldn't have to investigate the why it doesn't work.

I don't expect typesafe-conf to change the behavior of its resolver. A more elaborate version of case PropertyTree.Record(values) => fromTrees(values.values.toList) could make it (should make sure all keys are integers and sort on them first). That way the lib would be able to parse any record with integer keys as a sequence. I don't see this as a bad thing. The decision of whether you want to support this or not is up to you. I also think that providing support for the opposite operation (sequence to record) would be nice although I fail to see a use case for this atm. If you decide that you don't want to support parsing records as sequences, you can also fix this by post-processing the resolved config along with the config schema to convert records to sequences in such cases.

afsalthaj commented 3 years ago

Thanks for reporting the issue.

It is fairly solveable issue with the fix you suggested. However there might be users who would really want to see them as a map itself (may not be users of typesafe Config).

I am definitely open to making these changes to support the idea, of “sequence of numbers under a parent node could indidicate it’s a list”.

afsalthaj commented 3 years ago

Following my previous comment:

Your suggestion on adding the behaviour into PropertyTree sounds very reasonable (given that this use-case of numerically listed map is applicable for other sources too).

However, for the time being, given the current version of zio-config, I would create a method similar to the below one (not tested, as it is a quick snippet) that supports this behaviour in general.

   //  Although, it could be a temporary solution, 
   //  I think this is composing quite well, and could be even part of the `ConfigDescriptorModule` 
   // as a hot-fix if you are interested. This is regardless of whether we are making the changes in PropertyTree in future or not.

   // Not tested, intentionally skipped some details too, and could be made even better.
    def listFromNumericalMap[A](
      configDescriptor: ConfigDescriptor[A]
    ): ConfigDescriptor[List[A]] =
      map(configDescriptor)
        .transformOrFailLeft(map => {
            map.toList.traverse[Either[String, *], (Int, A)]({
            case (k, v) =>
              PropertyType.IntType.read(k).bimap(_.typeInfo, r => (r, v))
          }).map(_.sortBy(_._1).map(_._2))
        })(_.zipWithIndex.map({ case (v, index) => (index.toString, v) }).toMap)

Also, if we are using auto derivation, then I will create a new type.

     import zio.config._, ConfigDescriptor._
     import zio.config.magnolia.DeriveConfigDescriptor.Descriptor

    final case class NumericallyListedMap(set: Set[String])

    object NumericallyListedMap {
      implicit val descriptorForCustomType =
          Descriptor(listFromNumericalMap(string))
    }

// Usage

  final case class Config(items: NumericallyListedMap)

  object Config {
     val config = descriptor[Config]
  }