-
Notifications
You must be signed in to change notification settings - Fork 569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HV-1017 javafx detection uses TCCL but JavaFXPropertyValueUnwrapper does not #429
Conversation
An alternative would be to load the unwrapper class via the same loader
used for checking its presence, i.e. reflectively.
|
@DavideD asks for the user feedback (to check that the bug is indeed fixed). Let's give it some time before applying. |
@emmanuelbernard Done: |
I know you did, it was more as a marker on the PR. Also I messed up my English tenses ;) |
@@ -36,13 +36,23 @@ | |||
|
|||
private final ClassLoader classLoader; | |||
|
|||
/** | |||
* when true, it will check the Thread COntext ClassLoader when the class is not found in the provided one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context
I think I like this idea |
Ok :) I don't have a strong preference, it would help in some cases, but I suppose the scenario is very exotic. @DavideD? |
I don't know, this kind of errors seem to happen more often lately. In particular, when people start to use OSGI or JBoss Module on WildFly. Anyway, I can try the classloader approach. |
Ok, cool.
In particular, when people start to use OSGI or JBoss Module on WildFly.
But even then, why should JavaFX classes be visible trough the TCCL but not
the system class loader? It seems like a misconfiguration of that
environment (OSGi runtime with incomplete system package exports). Or
possibly a very old JVM which does not bring JavaFX itself but the user
adds it manually. Anyways, +1 for doing the alternative approach :)
|
I guess that's my question here as well. I am not an JAvaFX expert, but I was wondering whether it would be possible to add JavaFX classes to a JVM which otherwise does not provide them. Either way, using the reflection approach seems to be the safer option in this case, covering more potential scenarios. |
Yes, that would have been the case before JFX was added to the JDK (in 1.7 |
@gunnarmorling |
@@ -113,8 +112,10 @@ public ConfigurationImpl(ValidationProvider<?> provider) { | |||
private ConfigurationImpl() { | |||
this.validationBootstrapParameters = new ValidationBootstrapParameters(); | |||
TypeResolutionHelper typeResolutionHelper = new TypeResolutionHelper(); | |||
if ( isJavaFxInClasspath() ) { | |||
validatedValueHandlers.add( new JavaFXPropertyValueUnwrapper( typeResolutionHelper ) ); | |||
ClassLoader classLoader = getClass().getClassLoader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the intention was to change the isJavaFxInClasspath
in classpath. In my world it would e kept as is. Then, if it returns true
, use the LoadClass
privileged action (which is used under the hood for isJavaFxInClasspath
) to load JavaFXPropertyValueUnwrapper
.
@hferentschik Updated |
@@ -132,6 +133,16 @@ private ConfigurationImpl() { | |||
this.addConstraintDefinitionContributor( defaultConstraintDefinitionContributor ); | |||
} | |||
|
|||
private ValidatedValueUnwrapper<?> createJavaFXUnwrapperClass(TypeResolutionHelper typeResolutionHelper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Right, that's what I had in mind
I would suggest to merge this in this state and deploy a SNAPSHOT the user can test. If it still fails we can re-iterate on this. |
Rebased and merged |
private ValidatedValueUnwrapper<?> createJavaFXUnwrapperClass(TypeResolutionHelper typeResolutionHelper) { | ||
try { | ||
Class<?> jfxUnwrapperClass = run( LoadClass.action( JFX_UNWRAPPER_CLASS, getClass().getClassLoader() ) ); | ||
return (ValidatedValueUnwrapper<?>) ( jfxUnwrapperClass.getConstructor( TypeResolutionHelper.class ).newInstance( typeResolutionHelper ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick thought: If JFX_UNWRAPPER_CLASS
is loaded through the TCCL, will this actually find the right constructor, passing TypeResolutionHelper
obtained through this's class loader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.hibernate.validator
itself will never be loaded via the TCCL, since it is in the org.hibernate.validator
namespace. This is handled in LoadClass
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, so that might not work!? hard to test really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code might just behave like the old code. One would need
- something like a
ClassFoundIn
which instead of aClass
as inLoadClass
returns a classloader (the one which successfully loaded the class or null) - this classloader should then be used to load the unwrapper, but it needs a modified version of
LoadClass
to prevent the special handling of the loading of validator classes
Given 1 + 2, there might then still be the question around the TypeResoltionHelper
@gunnarmorling mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.hibernate.validator itself will never be loaded via the TCCL
LoadClass#loadClassInValidatorNameSpace()
falls back to the TCCL if a class cannot be loaded through its own loader. Now whether that makes sense...
Tbh. the original fix of only loading the JavaFX unwrapper when its visible to HV's loader seems like the best solution to me. Everything else seems very complex for a tiny potential gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the same, Let's use the original fix and hear back from the person with the issue, if he still has problem we will reopen the issue.
I'll send another pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send another pull request
+1
https://hibernate.atlassian.net/browse/HV-1017
I don't know how to test the issue, so I just check that the TCCL is not used when looking for the JavaFX classes. Let me know if it makes sense.