HH-563

[hferentschik]

No description provided.

[gunnarmorling]

I think there is a problem if an InputStream is passed in via Configuration#addMapping() which doesn't support the mark/reset contract. In that case the creation of a second factory fails as JAXB will try to read again from the stream which is already consumed.

We could avoid the repeated parsing of input streams by retrieving the configuration represented by each mapping stream only once and storing it for later re-use. If ConfigurationImpl#buildValidatorFactory() is invoked a second time, we would first check from which streams we already have retrieved the configuration and then process only those streams which we haven't parsed yet.

This might be done by using the same XmlMappingParser instance for all calls to buildValidatorFactory() on a given Configuration instance (currently always a new parser is created in the constructor of ValidatorFactoryImpl).

WDYT?

[hferentschik]

I think there is a problem if an InputStream is passed in via Configuration#addMapping() which doesn't support the
mark/reset contract. In that case the creation of a second factory fails as JAXB will try to read again from the stream which > is already consumed.

Sure. That's similar to closing the input stream after the first invocation of buildValidatorFactory(). Because we are using _InputStream_s in Configuration we are getting into a grey area when it comes to reusing the configuration instance. It is our responsibility to cache the input stream data? I am not so sure. In the end the client can always wrap his provided InputStream into a BufferedInputStream. Maybe it is just a question of documenting this properly in the javadocs?

We could also go down the route you are suggesting, but I don't find this so clean (caching against an input stream seems odd).

[gunnarmorling]

In the end the client can always wrap his provided InputStream into a BufferedInputStream.

Or maybe we could do this ourselves in ConfigurationImpl#addMapping() in case the given stream isn't resettable:

validationBootstrapParameters.addMapping( 
    stream.markSupported() ? stream : new BufferedInputStream(stream) 
);

[hferentschik]

Or maybe we could do this ourselves in ConfigurationImpl#addMapping() in case the given stream isn't resettable:

we could. I definitely prefer this over caching the config information. I am still not convinced so that we should do it though

[hferentschik]

@gunnarmorling I added the additional wrapping to validationBootstrapParameters.addMapping (at least for now). I think we are good on this for now. We might need to have another look once BVAL-282 is settled.

[gunnarmorling]

+1 I'll apply the pull request asap. I'll just remove the JavaDoc comment on BufferedInputStream as it isn't required anymore.