Lack of consistency when decoding optional lists.

[FrancisToth]

Hi, I've noticed that in some cases a decoder may not produce the same result:

import ru.tinkoff.phobos.decoding.*

case class Foo(i: Int)                  derives ElementDecoder
case class Bar(foo: List[Foo] = Nil)    derives ElementDecoder
case class Qux(bar: Option[Bar] = None) derives ElementDecoder

println(XmlDecoder.fromElementDecoder[Qux]("qux").decode("<qux><bar></bar></qux>"))
println(XmlDecoder.fromElementDecoder[Qux]("qux").decode("<qux><bar/></qux>"))
println(XmlDecoder.fromElementDecoder[Bar]("bar").decode("<bar></bar>"))
println(XmlDecoder.fromElementDecoder[Bar]("bar").decode("<bar/>"))

outputs

Right(Qux(Some(Bar(List())))) // <qux><bar></bar></qux>
Right(Qux(None))              // <qux><bar/></qux>
Right(Bar(List()))            // <bar></bar>
Right(Bar(List()))            // <bar/>

Is there any reason why a decoder outputs different result depending on if the value is optional or not? I would have expected "<qux><bar/></qux>" to be decoded like "<qux><bar></bar></qux>". Is there anything I can do at the decoder level to make this behavior consistent? I am using 15.1.

[FrancisToth]

I've looked at the code, and this is feels odd to me:

implicit def optionDecoder[A](implicit decoder: ElementDecoder[A]): ElementDecoder[Option[A]] =
    new ElementDecoder[Option[A]] {
      def decodeAsElement(c: Cursor, localName: String, namespaceUri: Option[String]): ElementDecoder[Option[A]] = {
        if (c.isStartElement) {
          ElementDecoder.errorIfWrongName[Option[A]](c, localName, namespaceUri).getOrElse {
            // Why does it consider an empty element with no attribute to be a None?
            if (ElementDecoder.isNil(c) || (c.isEmptyElement && c.getAttributeInfo.getAttributeCount == 0)) {
              c.next()
              new ConstDecoder(None)
            } else {
              decoder.map[Option[A]](a => Some(a)).decodeAsElement(c, localName, namespaceUri)
            }
          }
        } else {
          new FailedDecoder[Option[A]](c.error(s"Unexpected event: '${c.getEventType}'"))
        }
      }

If we think about it, an empty element with not attribute can be used as a marker tag. Its presence or its absence could mean two different things. It seems to me that the right part of the condition (right after the ||) is a business decision rather than a technical one which should be up to the user, but I may be wrong. Do you know the context which led to this decision?

[valentiay]

Hi!

Thanks for the report. Current behavior of option decoder is strange. I too think that there should be no difference in processing <foo/> and <foo></foo>. We should remove the part of condition that you were pointing at. As far as I remember, there was no particular reason for it.

I am currently on vacation and I have no opportunity to make and publish this change. I'll get to it as soon as I come back to my laptop on September 14th

[FrancisToth]

Thank you very much for your reply, I'll push a PR next week to address this :)

[valentiay]

Hi! I'm back home, and now ready to review and publish the fix

[FrancisToth]

Hey @valentiay, here's a PR for this issue: https://github.com/Tinkoff/phobos/pull/227

[valentiay]

Hi!

Excuse me for the long absence. Had some personal issues to solve. I hope, I will respond regularly from now on.

I've merged the PR and I'll publish the fix in 0.17.0, it should happen today