HV-1604 Fix instantiation of the JPATraversableResolver

[gsmet]

Also raise the log message to WARN as it's important to be warned if
there is an issue instantiating it.

• https://hibernate.atlassian.net/browse/HV-1604

[marko-bekhta]

Hi @gsmet I was just thinking ... this change of visibility to public will solve the current problem, but we might encounter the similar issue with for example ScriptEvaluatorFactory or any other class, passed to us by the user, that we want to instantiate, right? So maybe we should also take care of that and change the NewInstance action a bit. For example to something like:

public T run() {
    try {
        Constructor<T> declaredConstructor = clazz.getDeclaredConstructor();
        if ( makeAccessible && !declaredConstructor.isAccessible() ) {
            declaredConstructor.setAccessible( true );
        }
        return declaredConstructor.newInstance();
    }
    // all the catches ...
}

similar to what we have for fields and getters. WDYT ?

[gsmet]

Hi @marko-bekhta ,

There are 2 cases for these extensions:

• programmatic API: the user has already instantiated the class;
• XML: the spec mandates that these classes must have a public no-arg constructor.

In the latter case, we have this sentence in the spec that will defeat any attempt to improve the situation:
If a public no-arg constructor is missing on any of the classes referenced by the relevant XML elements, a ValidationException is raised during the Configuration.buildValidatorFactory() call.

[marko-bekhta]

ahh.. I see, it's in the spec :) should we clarify that the class should be also public (in a future spec update)? Or something like If an accessible no-arg constructor is missing ... because as I would read it right now, something like:

class BVConfigClasses{
    private static class MyTraversableResolver implements TraversableResolver {
        public MyTraversableResolver(){ }
        //... 
    }
    private static class MyClockProvider implements ClockProvider { 
        public MyClockProvider(){ }
        //... 
    }
}

should not throw an exception, as constructors are public, but the classes themselves, not that much :) ...

[gsmet]

Hi Marko, thanks for taking a look.

should we clarify that the class should be also public (in a future spec update)?

I don't think it's strictly necessary, a constructor of a private class, even if marked as public, is not public. And in any case, it looks obvious that the class should be public.

Apart from that, do you see any issue with the PR? Would like to merge it soon. Thanks!

[marko-bekhta]

Hi, nothing else that I can see. I'd say it's good to go.

[gsmet]

Merged, thanks for the review @marko-bekhta .