Hi Matt,
2012/7/3 Matt Benson <mbenson(a)apache.org>:
On Tue, Jul 3, 2012 at 4:02 AM, Hardy Ferentschik
<hardy(a)hibernate.org> wrote:
> Hi,
>
> comments inline
>
> On Jul 2, 2012, at 8:35 PM, Matt Benson wrote:
>
>> Hi all,
>> Sorry for my extreme tardiness in doing the backing research on this
>> issue. I am fine with the standing changes, so far as they go;
>> however it seems like this could also be an opportunity of sorts. If
>> the Configuration object is going to expose the ConfigurationSource
>> (and thereby the data corresponding to that mined from the XML
>> configuration), it would seem to move the responsibility of parsing
>> the XML and eliminate the need for ConfigurationState#getMappingStreams().
>
> I don't think that the changes introduced by ConfigurationSource have an effect
on
> ConfigurationState#getMappingStreams. Yes, this methods provides input streams to
> the configuration files defined in validation.xml, but it is also a general
programmatic
> configuration source to configure Bean Validation.
>
I guess you must mean hypothetical implementation-specific
configuration, but if that's the case it would seem the javadoc for
Configuration#addMapping() and ConfigurationState#getMappingStreams()
could/should be expanded--in any event, the latter should probably
make some mention of the former.
Could you make a suggestion what you'd like to see clarified in the docs?
> Whether it was/is a good choice to use input streams over strings (defining file
paths)
> is debatable. I think the idea was to allow other input sources than just files.
> Generally I think this is a good idea, but of course working with InputStream
creates
> another set of problems. Despite these problems I don't think we should change
anything
> regarding ConfigurationState#getMappingStreams(), since it servers a different
purpose
> than ConfigurationSource and changing it would break backwards compatibility.
>
> Also there are work arounds for the one issue you are referring to (see comment
below).
>
>> This method is AFAICT the
>> place that complicates the building of multiple ValidatorFactory
>> instances from a single Configuration, which was the subject of a
>> thread which I cannot currently find, but whose participants I recall
>> as having included Gunnar and myself
>
> I think you are referring to HV-563 ("Enable
Configuration#buildValidatorFactory() to be callable several times").
This is the issue (I think there was an email thread later on).
The original discussion was back in March (see
http://lists.jboss.org/pipermail/beanvalidation-dev/2012-March/000290.html).
There's also a BVAL issue for this
(
https://hibernate.onjira.com/browse/BVAL-282).
>
> There was indeed an issue with building multiple ValidatorFactory instances from a
single
> Configuration instance (at least in Hibernate Validator). However, we found a way to
solve this, by
> checking whether the provided InputStream supports marking [2]. If it does than there
is no problem and if it doesn't
> we wrap the stream in a BufferedInputStream which does. Once you have a markable
stream you can mark, read
> and finally reset it [3]. This way you can reuse the Configuration multiple times.
>
Yes, I think I remember discussing this--not perfect, but serviceable.
Of course, if Bean Validation v1.1 were to move up to Java SE 6,
javax.activation.DataSource might possibly be more appropriate as an
intermediate type to *provide* an InputStream any number of times.
Thoughts?
While discussing an issue with the spec JAR
(
https://hibernate.onjira.com/browse/BVAL-298), Emmanuel agreed on
basing BV 1.1 on SE 6 (see
https://hibernate.onjira.com/browse/BVAL-299).
That said, I think j.a.DataSource indeed looks nice and I think it
would have been great if it could have been used in BV 1.0. But
introducing it now AFAICS basically means adding an overloaded method
like Configuration#addMapping(DataSource dataSource). As users might
still use the old method passing an input stream, we need to address
BVAL-282 anyways in order to make a configuration re-usable. But then
there is no much advantage by supporting j.a.DataSource IMO.
All in all I think fixing BVAL-282 should be good enough, with the
advantage of an unchanged API. On minor issue is the required
buffering of non-markable streams, but given the typical size of
mapping streams, I don't see that as real problem. And users may still
pass an appropriate stream implementation when working with huge
mapping configurations.
--Gunnar