7:56 a.m.
Thanks for your input, some comments inline.
On Sun 16-09-25 11:22, Christian Kaltepoth wrote:
Hey Emmanuel, Hey Gunnar,
sorry for my late reply. Thank you very much for the enormous amount of
work you already spent on this proposal. This topic seems to be quite
complex and it looks like you spent many hours thinking about all this. It
is a bit difficult to give good feedback without being so deep into this
topic. But I'll try. ;-)
1. I was wondering whether the distinction between
SingleContainerValueExtractor and ManyContainerValuesExtractor is really
necessary. I'm not sure if the unnecessary instantiation of an iterator in
the single value case is really a problem in practice. I think extractor
implementations will provide some special implementation of the Iterator/
Iterable contract in any case. Not sure how others feel about that.
I thought about that but even with a special implementation of Iterator,
you are stuck with one extra object created for each single object
validated with no purpose.
We don't have in Java way to decorate an object without extra
invocation.
The Bean Validation design is very sensible to performance in the
critical path of object validation.
But yes we should check how things react once we have a RI prototyle,
I'll add a note to that effect.
2. Section 2.1 states that BV will support Iterable and Map by
default.
What about Optional? We will support the JDK8 date/time API, so why
don't we plan to support Optional?
Yes that's an oversight. Fixed now.
3. I like the idea that users can provide custom extractors for
other
container types. Not sure if this is something the average developer will
do very often, but defining an SPI here seems like a good idea.
Yes, it felt a bit over engineered at first but with the multitude of
new languages on the VM, such a contract is very useful.
4. I'm not completely sure if the "one extractor per
container" or the
"one extractor per container + value" approach makes more sense. That's
a
very difficult questions. I think I need some more time to think about that.
Gunnar explained that he prefers "one extractor per container" to avoid
the need for for @ExtractedValue on the type parameter.
I personally find the "one extractor per container + value" nicer for a
couple of reasons:
* the actual implementations are straight to the point of extracting
values instead of also handling type parameter references and having
to implement `getNode` which is a bit of a mystery to me now (I used
to know why).
* it leaves the side responsibilities (type param / node) to the BV
engine
* it is inherently more composable. I can add a new extractor after the
fact
* it only extracts objects that need to be validated (constrained ones)
vs returning all objects (i.e. only return Map.value vs Map.key and
value)
It's main drawback I think is that to read a Map which is constrained
on both key and values, it requires to walk through it twice. You can
work around it in the BV engine but that requires to be slight off the
standard semantic of this proposed SPI.