Introduction of bug related to optional as part of RC28 commits

[afsalthaj]

The commits for RC28 has introduced bugs in zio-config when dealing with optional values.

final case class RawTableConfig(
    options: Option[Options],
  )

  final case class Options(
    header: String,
    separator: String,
    quoteAll: String,
  )

  val string =
    s"""
       |      {
       |        "options" : {
       |            "quoteAll" : true
       |        }
       |      }
       |""".stripMargin

read(descriptor[RawTableConfig] from TypesafeConfigSource.fromHoconString(string).loadOrThrow)

returns Right(RawTableConfig(None)) while it should have complained about missing separator and header.

Note: These functionalities were in-tact at RC27 and it is seem to broken by RC28. The corresponding test cases were also changed as part of RC28, and hence this wasn't picked up

[afsalthaj]

@vigoo When you get time, can u take a look at why. I tried to identify myself on whats going on, but couldn't track down.
There were a few changes in the logic behind optional parameters from RC27 to RC28 breaking some behaviour.

Another change in behaviour which I noticed is reduced view of the error. For example

╥
╠══╗
║  ║
║  ╠─MissingValue
║  ║ path: database
║  ║ Details: optional value
║  ║
║  ╠─MissingValue
║  ║ path: database
║  ║ Details: optional value, value of type string
║  ▼
▼) 

In this case, first database is an optional product, but we don't see the details of the product. This was available before and its important to give out this info to the user.

[vigoo]

I believe the recursive config support changed the error reporting for this and we discussed this in that PR too, I think here: #409 (comment)
(if I understand correctly)

I don't know why your first example on the top behaves like that, I agree that should report the missing two fields. In the second one if database is an optional product, I think it is normal that only the missing database is reported and if I remember correctly this is what we discussed in the above linked thread; I think this was necessary to be able to terminate the recursion. There is no way to know if that is a recursive node or not, so you cannot go deeper to report the sub-nodes.

[afsalthaj]

For sure. The first example did work at RC27 and broke at RC28. I am not entirely sure out of the big change, which part made that bug. I thought, you will have a quick idea on where it went wrong.

I agree, with the second case we did have a discussion and thanks for reminding me about it.

[vigoo]

Unfortunately I haven't got any quick ideas :(

[afsalthaj]

No worries.

Came in with this logic anyway

final def requiredZipAndOrFields[A](config: ConfigDescriptor[A]): Int = {
    def countZipSize[A](config: ConfigDescriptor[A]): Int =
      config match {
        case ConfigDescriptorAdt.Zip(left, right) =>
          countZipSize(left.get()) + countZipSize(right.get())
        case _ => 1
      }
    def countOrElseSize[A](config: ConfigDescriptor[A]): Int =
      config match {
        case ConfigDescriptorAdt.OrElse(left, right) =>
          countOrElseSize(left.get()) + countOrElseSize(right.get())
        case _ => 1
      }
    def countOrElseEitherSize[A](config: ConfigDescriptor[A]): Int =
      config match {
        case ConfigDescriptorAdt.OrElseEither(left, right) =>
          countOrElseEitherSize(left.get()) + countOrElseEitherSize(right.get())
        case _ => 1
      }

    def loop[B](count: List[K], config: ConfigDescriptor[B]): Int =
      config match {
        case ConfigDescriptorAdt.Zip(_, _)             => countZipSize(config)
        case ConfigDescriptorAdt.XmapEither(cfg, _, _) => loop(count, cfg.get())
        case ConfigDescriptorAdt.Describe(cfg, _)      => loop(count, cfg.get())
        case ConfigDescriptorAdt.Nested(_, _, next)    => loop(count, next.get())
        case ConfigDescriptorAdt.Source(_, _)          => 1
        case ConfigDescriptorAdt.Optional(_)           => 0
        case ConfigDescriptorAdt.OrElse(_, _)          => countOrElseSize(config)
        case ConfigDescriptorAdt.OrElseEither(_, _)    => countOrElseEitherSize(config)
        case ConfigDescriptorAdt.Default(_, _)         => 0
        case ConfigDescriptorAdt.Sequence(_, _)        => 1
        case ConfigDescriptorAdt.DynamicMap(_, _)      => 1
      }

    loop(Nil, config)
  }

[afsalthaj]

@vigoo Apologies that we had to revert back some of the changes as part of RC28 in the process of fixing the bug (which I think was not looking good).

We will definitely follow up on the changes and the tests related to recursion as a later commit. The issue will be reopened.

[vigoo]

Don't really undertsand the motivation here, if there is a bug why not fix it with keeping/improving the already implemented other features? Looks like there is some hidden deadline here?

[afsalthaj]

Sorry I didn't get u when u said "Why not fix bug?"
Yes i fixed the bug. It's removing a logic that introduced the bug involving wild card pattern matching. The wrong logic is pasted above as well, and removed the short circuit of errors, which was also a reason of the bug above.

Motivation is "not putting users into risk of losing a basic functionality mentioned in the issue."

So its not really a revert. It is a bug fix.
It will take a bit more time for us to redesign recursion properly and hence merged only the bug fix. The components such as Lazy nodes are already available and we will see how it goes. May be that Lazy node can be renamed as recursion (but yea , still sound a bit odd)

[afsalthaj]

Just pointing out here that we might need to bring back the explicit node as well that @vigoo implemented (may be along with Lazy constructor) to make things work. More discussions here : #407