zio / zio-config

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

`orElse` on ConfigSource does not work properly with empty collection #1336

Open strokyl opened 9 months ago

strokyl commented 9 months ago

Here is a test:

import zio.Scope
import zio.test.{Spec, TestEnvironment, ZIOSpecDefault, assertTrue}
import zio.config.{ConfigDescriptor, ConfigSource, read}
import zio.config.ConfigDescriptor.{list, string}
import zio.config.yaml.YamlConfigSource

object ConfigSpec extends ZIOSpecDefault {
  override def spec: Spec[TestEnvironment with Scope, Any] = suite("spec")(
    test("work") {
      val listDescriptor: ConfigDescriptor[List[String]] = list("some_data")(string)
      val source                                         = YamlConfigSource.fromYamlString("some_data: []")

      for {
        result <- read[List[String]](listDescriptor from source)
      } yield assertTrue(result == List.empty)
    },
    test("does not work but should") {
      val listDescriptor: ConfigDescriptor[List[String]] = list("some_data")(string)
      val source                                         = YamlConfigSource.fromYamlString("some_data: []")

      val secondarySource = ConfigSource.fromMap(Map.empty)
      for {
        result <- read[List[String]](listDescriptor from (source orElse secondarySource))
      } yield assertTrue(result == List.empty)
    },
    test("work (2)") {
      val listDescriptor: ConfigDescriptor[List[String]] = list("some_data")(string)
      val source                                         = YamlConfigSource.fromYamlString("""some_data: ["a"]""")

      val secondarySource = ConfigSource.fromMap(Map.empty)
      for {
        result <- read[List[String]](listDescriptor from (source orElse secondarySource))
      } yield assertTrue(result == List("a"))
    }
  )
}

Here is the output:

+ spec
  + work (2)
  + work
  - does not work but should
    Exception in thread "zio-fiber-130" zio.config.ReadError$MissingValue: ReadError:
    MissingValue
    path: some_data
        at zio.config.ReadModule.read.loopSequence(ReadModule.scala:288)
        at zio.config.ReadModule.read.loopSequence(ReadModule.scala:285)
        at zio.config.ReadModule.read.loopAny(ReadModule.scala:305)
        at zio.config.ReadModule.read(ReadModule.scala:354)
        at zio.config.ReadModule.read(ReadModule.scala:355)
        at zio.config.ReadModule.read(ReadModule.scala:352)
        at io.conduktor.admin.license.onpremise.ConfigSpec.spec(ConfigSpec.scala:25)
        at io.conduktor.admin.license.onpremise.ConfigSpec.spec(ConfigSpec.scala:19)
2 tests passed. 1 tests failed. 0 tests ignored.

  - spec - does not work but should
    Exception in thread "zio-fiber-130" zio.config.ReadError$MissingValue: ReadError:
    MissingValue
    path: some_data
        at zio.config.ReadModule.read.loopSequence(ReadModule.scala:288)
        at zio.config.ReadModule.read.loopSequence(ReadModule.scala:285)
        at zio.config.ReadModule.read.loopAny(ReadModule.scala:305)
        at zio.config.ReadModule.read(ReadModule.scala:354)
        at zio.config.ReadModule.read(ReadModule.scala:355)
        at zio.config.ReadModule.read(ReadModule.scala:352)
        at io.conduktor.admin.license.onpremise.ConfigSpec.spec(ConfigSpec.scala:25)
        at io.conduktor.admin.license.onpremise.ConfigSpec.spec(ConfigSpec.scala:19)
Executed in 257 ms
[info] Completed tests
[error] Failed tests:
[error]     io.conduktor.admin.license.onpremise.ConfigSpec
[error] (Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 2 s, completed 22 déc. 2023, 09:43:11

I would expect that when the first datasource have an empty list for the given path and there is nothing on the second datasource, we get an empty list and not an error.

IvanFinochenko commented 5 months ago

Hi If we look implementation of orElse then we see that secondary ConfigSource will be used if we get exception while parsing config from first ConfigSource. And now ConfigSource is renamed to ConfigProvider. I consider that it's correct behaviour. For your case, we should create, for example, a merge method.

strokyl commented 4 months ago

If we look implementation of orElse then we see that secondary ConfigSource will be used if we get exception while parsing config from first ConfigSource

In test("does not work but should") the first ConfigSource does not fail and return a valid result with is an empty list