BVTCK-188 Allow bootstrap exceptions to be thrown at deployment#152
BVTCK-188 Allow bootstrap exceptions to be thrown at deployment#152gsmet merged 6 commits intojakartaee:2.0from
Conversation
|
|
||
| @Override | ||
| protected Class<? extends Exception> acceptedDeploymentExceptionType() { | ||
| return ValidationException.class; |
There was a problem hiding this comment.
Does it actually work in the case of the OP? It seems the ValidationException was wrapped there? Or are you trying to find the root exception?
There was a problem hiding this comment.
I suppose you have your answer now :).
I browse through the causes to try to find a valid exception. It might not be perfect but I hope it's going to work for the OP.
There was a problem hiding this comment.
Ok. Once we got the list of tests, it'd be nice to deploy this as a snapshot and ask them to give it a spin.
There was a problem hiding this comment.
I think I will ask them to build my branch and test if it's OK for the tests I fixed.
I agree my extraction might not be perfect especially if the exception wrapping uses a non standard way of doing it. We'll see.
| } | ||
| } | ||
|
|
||
| if ( exception.getCause() != null ) { |
There was a problem hiding this comment.
Apologies, I should have included the complete stack trace in my OP. Unfortunately this wont' work for the use case I am running into, because Weld sets any deployment exceptions (there may be >1) as the message of the DeploymentException, instead of as a cause.
So I think in addition to looking for chained exceptions, this method should also check if the exception message contains acceptedDeploymentExceptionType.getName()
Here is the full stack trace, to illustrate what I'm talking about:
org.jboss.weld.exceptions.DefinitionException: Exception List with 1 exceptions:
Exception 0 :
javax.validation.ValidationException: HV000201: Unable to instantiate clock provider class org.hibernate.beanvalidation.tck.tests.xmlconfiguration.XmlDefinedClockProviderWithNoDefaultConstructor.
at org.hibernate.validator.internal.xml.ValidationBootstrapParameters.setClockProvider(ValidationBootstrapParameters.java:241)
at org.hibernate.validator.internal.xml.ValidationBootstrapParameters.<init>(ValidationBootstrapParameters.java:61)
at org.hibernate.validator.internal.engine.ConfigurationImpl.parseValidationXml(ConfigurationImpl.java:512)
at org.hibernate.validator.internal.engine.ConfigurationImpl.buildValidatorFactory(ConfigurationImpl.java:317)
at org.hibernate.validator.cdi.ValidationExtension.<init>(ValidationExtension.java:119)
at com.ibm.ws.beanvalidation.v20.cdi.internal.LibertyHibernateValidatorExtension.delegate(LibertyHibernateValidatorExtension.java:112)
at com.ibm.ws.beanvalidation.v20.cdi.internal.LibertyHibernateValidatorExtension.beforeBeanDiscovery(LibertyHibernateValidatorExtension.java:132)
at sun.reflect.GeneratedMethodAccessor124.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.jboss.weld.injection.StaticMethodInjectionPoint.invoke(StaticMethodInjectionPoint.java:95)
at org.jboss.weld.injection.MethodInvocationStrategy$SpecialParamPlusBeanManagerStrategy.invoke(MethodInvocationStrategy.java:144)
at org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:330)
at org.jboss.weld.event.ExtensionObserverMethodImpl.sendEvent(ExtensionObserverMethodImpl.java:123)
at org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:308)
at org.jboss.weld.event.ObserverMethodImpl.notify(ObserverMethodImpl.java:286)
at javax.enterprise.inject.spi.ObserverMethod.notify(ObserverMethod.java:124)
at org.jboss.weld.util.Observers.notify(Observers.java:166)
at org.jboss.weld.event.ObserverNotifier.notifySyncObservers(ObserverNotifier.java:285)
at org.jboss.weld.event.ObserverNotifier.notify(ObserverNotifier.java:273)
at org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:177)
at org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:171)
at org.jboss.weld.bootstrap.events.AbstractContainerEvent.fire(AbstractContainerEvent.java:53)
at org.jboss.weld.bootstrap.events.AbstractDefinitionContainerEvent.fire(AbstractDefinitionContainerEvent.java:44)
at org.jboss.weld.bootstrap.events.BeforeBeanDiscoveryImpl.fire(BeforeBeanDiscoveryImpl.java:54)
at org.jboss.weld.bootstrap.WeldStartup.startInitialization(WeldStartup.java:391)
at org.jboss.weld.bootstrap.WeldBootstrap.startInitialization(WeldBootstrap.java:76)
at com.ibm.ws.cdi.impl.CDIContainerImpl$2.run(CDIContainerImpl.java:142)
at com.ibm.ws.cdi.impl.CDIContainerImpl$2.run(CDIContainerImpl.java:139)
at java.security.AccessController.doPrivileged(Native Method)
at com.ibm.ws.cdi.impl.CDIContainerImpl.applicationStarting(CDIContainerImpl.java:139)
at com.ibm.ws.cdi.liberty.CDIRuntimeImpl.applicationStarting(CDIRuntimeImpl.java:383)
at com.ibm.ws.container.service.state.internal.ApplicationStateManager.fireStarting(ApplicationStateManager.java:28)
at com.ibm.ws.container.service.state.internal.StateChangeServiceImpl.fireApplicationStarting(StateChangeServiceImpl.java:50)
at com.ibm.ws.app.manager.module.internal.DeployedAppInfoBase.preDeployApp(DeployedAppInfoBase.java:383)
at com.ibm.ws.app.manager.module.internal.DeployedAppInfoBase.deployApp(DeployedAppInfoBase.java:412)
at com.ibm.ws.app.manager.war.internal.WARApplicationHandlerImpl.install(WARApplicationHandlerImpl.java:65)
at com.ibm.ws.app.manager.internal.statemachine.StartAction.execute(StartAction.java:140)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.enterState(ApplicationStateMachineImpl.java:1258)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.performAction(ApplicationStateMachineImpl.java:1106)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.run(ApplicationStateMachineImpl.java:881)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
Caused by: javax.validation.ValidationException: HV000065: Unable to load class: org.hibernate.beanvalidation.tck.tests.xmlconfiguration.XmlDefinedClockProviderWithNoDefaultConstructor from org.eclipse.osgi.internal.loader.EquinoxClassLoader@169014c7[com.ibm.ws.org.hibernate.validator.6.0.4.Final:1.0.19.master(id=117)].
at org.hibernate.validator.internal.util.privilegedactions.LoadClass.loadNonValidatorClass(LoadClass.java:132)
at org.hibernate.validator.internal.util.privilegedactions.LoadClass.run(LoadClass.java:64)
at org.hibernate.validator.internal.util.privilegedactions.LoadClass.run(LoadClass.java:29)
at org.hibernate.validator.internal.xml.ValidationBootstrapParameters.run(ValidationBootstrapParameters.java:298)
at org.hibernate.validator.internal.xml.ValidationBootstrapParameters.setClockProvider(ValidationBootstrapParameters.java:234)
... 43 more
Caused by: java.lang.ClassNotFoundException: org.hibernate.beanvalidation.tck.tests.xmlconfiguration.XmlDefinedClockProviderWithNoDefaultConstructor cannot be found by com.ibm.ws.org.hibernate.validator.6.0.4.Final_1.0.19.master
at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:484)
at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:395)
at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:387)
at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:150)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:348)
at org.hibernate.validator.internal.util.privilegedactions.LoadClass.loadNonValidatorClass(LoadClass.java:129)
... 47 more
at org.jboss.weld.bootstrap.events.AbstractDefinitionContainerEvent.fire(AbstractDefinitionContainerEvent.java:46)
at org.jboss.weld.bootstrap.events.BeforeBeanDiscoveryImpl.fire(BeforeBeanDiscoveryImpl.java:54)
at org.jboss.weld.bootstrap.WeldStartup.startInitialization(WeldStartup.java:391)
at org.jboss.weld.bootstrap.WeldBootstrap.startInitialization(WeldBootstrap.java:76)
at com.ibm.ws.cdi.impl.CDIContainerImpl$2.run(CDIContainerImpl.java:142)
at com.ibm.ws.cdi.impl.CDIContainerImpl$2.run(CDIContainerImpl.java:139)
at java.security.AccessController.doPrivileged(Native Method)
at com.ibm.ws.cdi.impl.CDIContainerImpl.applicationStarting(CDIContainerImpl.java:139)
at com.ibm.ws.cdi.liberty.CDIRuntimeImpl.applicationStarting(CDIRuntimeImpl.java:383)
at com.ibm.ws.container.service.state.internal.ApplicationStateManager.fireStarting(ApplicationStateManager.java:28)
at com.ibm.ws.container.service.state.internal.StateChangeServiceImpl.fireApplicationStarting(StateChangeServiceImpl.java:50)
at com.ibm.ws.app.manager.module.internal.DeployedAppInfoBase.preDeployApp(DeployedAppInfoBase.java:383)
at com.ibm.ws.app.manager.module.internal.DeployedAppInfoBase.deployApp(DeployedAppInfoBase.java:412)
at com.ibm.ws.app.manager.war.internal.WARApplicationHandlerImpl.install(WARApplicationHandlerImpl.java:65)
at com.ibm.ws.app.manager.internal.statemachine.StartAction.execute(StartAction.java:140)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.enterState(ApplicationStateMachineImpl.java:1258)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.performAction(ApplicationStateMachineImpl.java:1106)
at com.ibm.ws.app.manager.internal.statemachine.ApplicationStateMachineImpl.run(ApplicationStateMachineImpl.java:881)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
There was a problem hiding this comment.
Hum. Can't say I'm excited about it :).
The issue is that I will have to build a fake exception so we won't be able to test the real content of the exception. But, thinking about it, I don't think we test it. I'll push something shortly.
There was a problem hiding this comment.
Agreed that it's a bit odd, but in a real world situation deployment exceptions aren't actually catchable by java code, so it doesn't really matter if this data is in the exception message or as a proper chained exception from the user perspective.
Thanks for looking into this so promptly btw! I'm working on collecting the full set of classes effected now.
There was a problem hiding this comment.
BTW, in your case, you only have one exception in the list? Wondering if we should open an issue against Weld to better deal with this case.
There was a problem hiding this comment.
@aguibert pushed an additional commit to work around the Weld limitation
Didn't have the time to test it properly though (I have an appointment and need to go). Feel free to test it if you have time and it helps. Otherwise, I'll test it tomorrow.
If it doesn't work, feel free to adjust the approach. I think it's the right one, even if not perfect.
|
@gsmet here is the full set of classes I found that had this issue: Also, I noticed a few other tests were failing for me because the test apps are missing some required classes: |
|
I pulled this change in locally and it seems to be working. I need to make a few additional tweaks on top of it, which I can contribute via another PR. |
|
@aguibert please wait for a bit. I'm working on it and will push a squashed version soon. Will let you know. |
|
I already updated all the tests you mentioned. I'm checking the issue you reported with the missing classes (I'm a bit surprised because we are running the TCK tests in WildFly with proper isolation but will check anyway). |
|
@aguibert so indeed, you're right on the XML tests but I have a hard time understanding the issue with this one: As we include the test, the inner class should be copied with the test. |
530c092 to
b7d9f5a
Compare
|
@aguibert I just force-pushed a new version with all the tests you mentioned fixed (and tested everything in WF container mode - so I consider this version OK for us). Would appreciate if you could validate it works OK for you too. The only issue I haven't fixed so far is this one: as I don't understand why it wouldn't work as is. I'm finished with my changes for now so feel free to contribute additional work on top of it. |
|
@gsmet you're correct, I double checked |
|
@aguibert so do you pass the TCK with the version in this PR, now? Are other changes required? Thanks for your feedback! |
| } | ||
| } | ||
|
|
||
| // in the case of Weld, the wrapped exceptions are simply stored in the message, so we check for the exception |
There was a problem hiding this comment.
No big fan of this. Not sure thought what could be done alternatively... .
There was a problem hiding this comment.
As you can imagine, I'm no fan either.
Unfortunately, we don't have much of a choice as the exception is lost in CDI. See https://github.com/weld/core/blob/master/impl/src/main/java/org/jboss/weld/exceptions/DeploymentException.java#L52 . We don't have a way to get back the cause.
I asked @aguibert if he was in the case where there was only one exception in the list because, if so, we could see with the Weld people if they would accept to create a static method taking the list instead of a constructor and special case the "List with only one cause" case. @aguibert still interested in the answer :).
We would still have to wait for a Weld release though so we can hope for better in the future, but for now, we will have to accept that.
There was a problem hiding this comment.
Yes, all the cases I saw were just one exception in the message.
However, I don't think this is worth opening an issue with the Weld folks because, in practice, no java code (aside from test frameworks) are going to care if the exception is a proper chained exception or simply in the exception message.
| catch ( ValidationException e ) { | ||
| // success | ||
| } | ||
| TestUtil.getValidatorUnderTest(); |
There was a problem hiding this comment.
Is it on purpose that there's no fail() call here as with the others?
There was a problem hiding this comment.
I removed the fail() calls when they were totally useless as it's just an old pattern. They were even counter productive.
I kept them when the message was interesting and conveyed some information as it was better than the error generated by TestNG.
So, yes, it's consistent.
| catch ( ValidationException e ) { | ||
| // success | ||
| } | ||
| TestUtil.getValidatorUnderTest(); |
There was a problem hiding this comment.
Looking further, it seems to be handled a bit inconsistently. I don't think the fail() calls would be needed, would they?
|
@gsmet All of the TCK-side issues appear to be resolved now. Thanks! There was one minor tweak I had to make so the TCK builds on Windows, but I can contribute that separately. I've got OpenLiberty almost passing the TCK now with your most recent set of changes from this PR. Currently 1036 tests pass. Still need to get the JavaFX stuff sorted out, and then there is one additional issue with OpenLiberty where specifying an app-defined |
|
@aguibert I integrated a trimmed down version of your patch for the Windows support in this PR. Could you please validate that everything is OK for you with this branch? Thanks! |
Let's be on the safe side.
|
I added a few commits to be on the safe side. I'm finished on this. Here is what I suggest:
If you really need a new version, we could release earlier but I really think it would be better to be sure all your issues are sorted out before making a new release. Feedback welcome! |
|
hi @gsmet, it will probably be at least a week before I have time to work through the remaining TCK issues with OpenLiberty. Until then I can work off of a local snapshot with these changes |
|
If it works for you to work with the snapshot, let's do that. And we'll release once we are totally sure we fixed all your issues. |
|
hi @gsmet just wanted to confirm that this PR plus the exclusion of the JavaFX tests gets OpenLiberty 100% passing for the BeanVal 2.0 TCK. Thanks for all of your help on this! |
|
@aguibert by JavaFX tests exclusion, you mean the other PR? Would be nice if you could just get the branch from the other PR, build it and test the TCK (just checking, it's what you're done so that we are sure everything is OK before releasing 2.0.2.Final). |
https://hibernate.atlassian.net/browse/BVTCK-188
https://hibernate.atlassian.net/browse/BVTCK-190
https://hibernate.atlassian.net/browse/BVTCK-191