Open jazott3 opened 4 years ago
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.
Thinking about it some more, I think the behaviour you describe might be a bug, and might not necessarily require a major version release.
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.
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.
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.
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?
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)
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.
I have a case class that looks like this:
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 intoNone
.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
EmptyError
s to a None: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