WFLY-10025 Use new org.jboss.modules.ClassTransformer interface in addition to j.l.i.ClassFileTransformer #11307
Conversation
|
retest this please. |
|
@dmlloyd could you please review? Thanks! We had previously missed that we are supposed to return null, if the JPA persistence provider doesn't enhance the entity class. I made that change for the two #transform methods (byte[] + ByteBuffer), perhaps that will help performance as well. |
| @@ -752,4 +754,8 @@ | |||
| DeploymentUnitProcessingException differentSearchModuleDependencies(String deployment, String searchModuleName1, String searchModuleName2); | |||
|
|
|||
| // id = 72, value = "Could not obtain TransactionListenerRegistry from transaction manager") | |||
|
|
|||
| @Message(id = 73, value = "Invalid class format for deployment class %s.") | |||
There was a problem hiding this comment.
A message like "Transformation of class %s failed" is probably more appropriate. The details will be included in the nested exception.
There was a problem hiding this comment.
easy change, thanks
| @@ -44,13 +46,47 @@ public JPADelegatingClassFileTransformer(PersistenceUnitMetadata pu) { | |||
| public byte[] transform(ClassLoader classLoader, String className, Class<?> aClass, ProtectionDomain protectionDomain, byte[] originalBuffer) throws | |||
| IllegalClassFormatException { | |||
| byte[] transformedBuffer = originalBuffer; | |||
There was a problem hiding this comment.
Since you've introduced the new method, it's probably better to only have the logic in one place. This method can delegate to that one simply by calling transform(classLoader, className, protectionDomain, ByteBuffer.wrap(originalBuffer)). You can factor out a private utility method to 'convert' the ByteBuffer to an array which can be used in both methods.
There was a problem hiding this comment.
Changed as suggested, will push in a minute.
Is it okay that transform(ClassLoader, String, Class, ProtectionDomain, byte[]), will now throw IlegalStateException instead of IllegalClassFormatException?
Will org.jboss.modules.ClassTransformer need to deal with redefining already loaded classes? Currently, j.l.i.ClassFileTransformer#transform is passed "Class<?> classBeingRedefined" for redefinition, which I just pass onto the JPA persistence providers (which they probably ignore).
There was a problem hiding this comment.
No, we do not support redefinition. Changing the exception is probably fine in this case (we're the ones who catch it after all).
| boolean transformed = false; | ||
| byte[] transformedBuffer = bytes; | ||
| for (javax.persistence.spi.ClassTransformer transformer : persistenceUnitMetadata.getTransformers()) { | ||
| byte[] result = new byte[0]; |
There was a problem hiding this comment.
This assignment and allocation is pointless. If the transform call fails, an exception will be thrown; otherwise it will definitely assign result.
scottmarlow
force-pushed
the
WFLY-10025_ClassTransformer_use_modules
branch
from
9611448
to
32f2294
Compare
| public ByteBuffer transform(ClassLoader classLoader, String className, ProtectionDomain protectionDomain, ByteBuffer classBytes) | ||
| throws IllegalArgumentException { | ||
| final byte[] bytes = getBytes(classBytes); | ||
| final Class<?> aClass = null; |
There was a problem hiding this comment.
This variable is useless and can be inlined.
| final byte[] bytes = getBytes(classBytes); | ||
| final Class<?> aClass = null; | ||
| boolean transformed = false; | ||
| byte[] transformedBuffer = bytes; |
There was a problem hiding this comment.
Having both bytes and transformedBuffer is redundant. You can just use bytes directly and drop transformedBuffer.
There was a problem hiding this comment.
true, will drop bytes, as I think we need transformedBuffer.
| if (result != null) { | ||
| transformedBuffer = result; | ||
| transformed = true; |
There was a problem hiding this comment.
This boolean is unnecessary. The check for result is sufficient; see below.
|
|
||
| return transformed ? ByteBuffer.wrap(transformedBuffer) : null; |
There was a problem hiding this comment.
return result != null ? ByteBuffer.wrap(result) : null;
There was a problem hiding this comment.
If there are multiple calls to Transformer#transform and some return null but some return non-null, we need the boolean, to note that we saw at least one transformation, so we can return non-null result.
There was a problem hiding this comment.
Ah right, I missed the loop. Happy Monday
| throws IllegalArgumentException { | ||
| final byte[] bytes = getBytes(classBytes); | ||
| final Class<?> aClass = null; | ||
| boolean transformed = false; |
| @@ -751,5 +753,9 @@ | |||
| @Message(id = 71, value = "Deployment '%s' specified more than one Hibernate Search module name ('%s','%s')") | |||
| DeploymentUnitProcessingException differentSearchModuleDependencies(String deployment, String searchModuleName1, String searchModuleName2); | |||
|
|
|||
| // id = 72, value = "Could not obtain TransactionListenerRegistry from transaction manager") | |||
| // id = 72, value = "Could not obtain TransactionListenerRegistry from transaction manager.") | |||
There was a problem hiding this comment.
removed the unneeded fix.
scottmarlow
force-pushed
the
WFLY-10025_ClassTransformer_use_modules
branch
from
32f2294
to
b1e0aae
Compare
…dition to j.l.i.ClassFileTransformer
scottmarlow
force-pushed
the
WFLY-10025_ClassTransformer_use_modules
branch
from
b1e0aae
to
c7b60a2
Compare
https://issues.jboss.org/browse/WFLY-10025
Note that we should remove the j.l.i.ClassFileTransformer interface after https://issues.jboss.org/browse/WFCORE-3685 changes DelegatingClassFileTransformer to use org.jboss.modules.ClassTransformer.