Question about Option reader

[jazott3]

I have a case class that looks like this:

case class RootElement(
    foo: Option[Foo],
    bar: Option[Bar],
    baz: Option[Baz]
)

Foo, Bar, and Baz are all large and deeply-nested types, and I noticed that while writing a parser test, I was parsing a RootElement(None, None, None). Eventually I realized that validation errors way down in the child elements were being turned into None.

I was surprised to see that was the implementation of the default option reader, and am curious how this implementation came about. Would you be open to an alternate implementation? I am thinking something like this, that would convert a ParseFailure with only EmptyErrors to a None:

implicit def optionReader[A](implicit reader: XmlReader[A]): XmlReader[Option[A]] = XmlReader { xml =>
    reader.read(xml) match {
        case ParseSuccess(s) => ParseSuccess(Option(s))
        case PartialParseSuccess(v, e) => PartialParseSuccess(Option(v), e)
        case ParseFailure(f) => if (f.forall {
            case _: EmptyError => true
            case _ => false
        }) ParseSuccess(None) else ParseFailure(f)
    }
}

I realize there's a significant amount of live code using xtract, and this is almost certainly a breaking change. Maybe it would be easier to remove the implicit modifier from the provided reader and/or provide both side-by-side for users to choose from?

Thanks
Joe

[tmccombs]

What would the alternate implementation be? I might be open to changing it in a backwards incompatible way in a future release, but would probably want to group multiple such changes in a single major version bump.

[tmccombs]

Thinking about it some more, I think the behaviour you describe might be a bug, and might not necessarily require a major version release.

[jazott3]

I also did some more thinking- The alternate implementation I was thinking of is listed in the initial issue, but I'm not sure it behaves the way I was hoping. For example:

object RootElement {
    implicit val reader: XmlReader[RootElement] = (
        (__ \ "foo").read[Option[Foo]],
        (__ \ "bar").read[Option[Bar]],
        (__ \ "baz").read[Option[Baz]]
    ).mapN(apply _)
}

case class Foo(a: String, b: Int)
object Foo {
    implicit val reader: XmlReader[Foo] = (
        (__ \ "a").read[String],
        (__ \ "b").read[Int]
    ).mapN(apply _)
}

<RootElement>
    <foo>
        <a>Hello</a>
    </foo>
</RootElement>

<RootElement>
    <foo>
        <a>Hello</a>
        <b>World</b>
    </foo>
</RootElement>

I would expect both of these to return a ParseFailure. With the existing implementation, both will return a ParseSuccess(None) for "foo". With the implementation I proposed, only the second one will.

I'm interested in digging through play-json to see how it handles this. I'm not sure if it's reasonable to expect both to return a ParseError either, although I do think the second one should.

[greatbalin]

Perhaps, instead of returning ParseError it could return PartialParseSuccess with None as value and all errors.
This way it will be backward compatible and you get errors if any.
But I don't see any need in getting those errors, only for debug maybe.

[tmccombs]

I ... am curious how this implementation came about

I honestly can't remember, looking at it now the implementation seems way too simple.

I'm interested in digging through play-json to see how it handles this

It looks like it actually has four options for this: nullable, nullableWithDefault, optionNoError, and optionWithNull. With that I think I might prefer removing the implicit and having multiple optional definitions. But that is definitely a breaking change.

I'm torn on how to handle this. On the one hand the current behavior is unexpected, and while I think it has a place, I don't think the current implementation should be the default for parsing to an Option. On the other hand, I don't see a great way to fix it without making a breaking change, although I'm not sure how much, if any, code relies on the current behavior.

[greatbalin]

So, main intention here is to make option reader fail in case of tag present and it can't be parsed properly but return ParseSuccess(None) in case of tag absence. Am I correct?

[jazott3]

Yes, that is what I'm proposing.

Regarding avoiding the breaking change, I am wondering if we can use @deprecated in the short term, and different XmlReader.read methods (for example, readNullable which will take an implicit XmlReader[A] instead of XmlReader[Option[A]]). This will still break some code in projects using -Yfatal-warnings, but it is slightly less breaking and still allows the multiple types of option readers to coexist.

I am still not sure of all the implementation details, and I'm not sure I'll have a chance to really dig in for a couple of weeks. In the meantime, I've converted all of our option readers the initial implementation I suggested, and implemented this workaround for RootElement:

implicit val reader: XmlReader[RootElement] = (
    (__ \ "foo").read[Option[Foo]],
    (__ \ "bar").read[Option[Bar]],
    (__ \ "baz").read[Option[Baz]]
).mapN(apply _)
.filter(re => re.foo.nonEmpty || re.bar.nonEmpty || re.baz.nonEmpty)

[greatbalin]

I faced the same issue with case class which has all optional fields. And solved the same way as you (filter after mapN)

implicit val reader: XmlReader[RootElement] = (
    (__ \ "foo").read[Option[Foo]],
    (__ \ "bar").read[Option[Bar]],
    (__ \ "baz").read[Option[Baz]]
).mapN(apply _)
.filter(re => re.foo.nonEmpty || re.bar.nonEmpty || re.baz.nonEmpty)

In my opinion, this issue doesn't belong to Option reader. When all fields are empty you will get tuple with all None values and this actually a correct behavior. Imagine situation when tag contains no optional sub-tags at all. Should the case class instance for this tag be generated? I think it depends on desired behavior and there is no correct answer for all possible situations.

As a possible solution new method could be added to a Tuple which will check that tuple contains only None and return a FailureXmlReader in this case. So it will look like

implicit val reader: XmlReader[RootElement] = (
    (__ \ "foo").read[Option[Foo]],
    (__ \ "bar").read[Option[Bar]],
    (__ \ "baz").read[Option[Baz]]
).atLeastOne.mapN(apply _)

And It will be backward compatible.