-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
HHH-16058 Removing Environment#getBytecodeProvider #5977
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,10 +7,13 @@ | |
| package org.hibernate.jpa.internal.enhance; | ||
|
|
||
| import java.security.ProtectionDomain; | ||
| import java.util.Objects; | ||
|
|
||
| import org.hibernate.bytecode.enhance.spi.EnhancementContext; | ||
| import org.hibernate.bytecode.enhance.spi.EnhancementContextWrapper; | ||
| import org.hibernate.bytecode.enhance.spi.Enhancer; | ||
| import org.hibernate.bytecode.internal.BytecodeProviderInitiator; | ||
| import org.hibernate.bytecode.spi.BytecodeProvider; | ||
| import org.hibernate.bytecode.spi.ClassTransformer; | ||
| import org.hibernate.cfg.Environment; | ||
|
|
||
|
|
@@ -23,9 +26,12 @@ | |
| public class EnhancingClassTransformerImpl implements ClassTransformer { | ||
|
|
||
| private final EnhancementContext enhancementContext; | ||
| private final BytecodeProvider bytecodeProvider; | ||
|
|
||
| public EnhancingClassTransformerImpl(EnhancementContext enhancementContext) { | ||
| Objects.requireNonNull( enhancementContext ); | ||
| this.enhancementContext = enhancementContext; | ||
| this.bytecodeProvider = BytecodeProviderInitiator.buildDefaultBytecodeProvider(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't that possibly introducing another problem? You won't be using the same bytecode provider as Hibernate ORM... But I must admit I don't know what this class is used for anyway :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is used by JPA's "enhance at deployment time" capability, used on EE servers. And yes I'd like to make the BytecodeProvider implementation pluggable, but it really isn't today: choices are exclusively (there are other areas too in which it's implied that it needs to be the ByteBuddy one - I'd certainly like to improve that but I'm considering that a different issue) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was thinking more that you're not using the same instance of |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -41,12 +47,15 @@ public byte[] transform( | |
| // It also assumed that all calls come from the same class loader, which is fair, but this makes it more robust. | ||
|
|
||
| try { | ||
| Enhancer enhancer = Environment.getBytecodeProvider().getEnhancer( new EnhancementContextWrapper( enhancementContext, loader ) ); | ||
| Enhancer enhancer = bytecodeProvider.getEnhancer( new EnhancementContextWrapper( enhancementContext, loader ) ); | ||
| return enhancer.enhance( className, classfileBuffer ); | ||
| } | ||
| catch (final Exception e) { | ||
| throw new TransformerException( "Error performing enhancement of " + className, e ); | ||
| } | ||
| finally { | ||
| bytecodeProvider.resetCaches(); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,15 +265,16 @@ public final void initializeWithoutLoadIfPossible() { | |
| /** | ||
| * Initialize internal state based on the currently attached session, | ||
| * in order to be ready to load data even after the proxy is detached from the session. | ||
| * | ||
| * This method only has any effect if | ||
| * {@link SessionFactoryOptions#isInitializeLazyStateOutsideTransactionsEnabled()} is {@code true}. | ||
| */ | ||
| protected void prepareForPossibleLoadingOutsideTransaction() { | ||
| if ( session != null ) { | ||
| allowLoadOutsideTransaction = session.getFactory().getSessionFactoryOptions().isInitializeLazyStateOutsideTransactionsEnabled(); | ||
|
|
||
| if ( allowLoadOutsideTransaction && sessionFactoryUuid == null ) { | ||
| if ( sessionFactoryUuid == null ) { | ||
| //we're going to need the UUID even if we the SessionFactory configuration doesn't | ||
| // allow any operations on it, as we need it to match deserialized objects with | ||
| // the originating SessionFactory: at very least it's useful to actually get | ||
| // such configuration, so to know if such operation isn't allowed or configured otherwise. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you've changed the purpose of this method; maybe it should be renamed? I don't know if this class is API or "looks like API but actually is not", though... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, I've amended the javadoc to clarify. |
||
| sessionFactoryUuid = session.getFactory().getUuid(); | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
There is something weird going on.
This constructor is only ever called from here:
hibernate-orm/hibernate-core/src/main/java/org/hibernate/jpa/boot/internal/PersistenceUnitInfoDescriptor.java
Lines 111 to 114 in a986a38
But then all implementations of
PersistenceUnitInfo#addTransformerare no-ops...This component is probably not being tested. I'd suggest trying your patch in WildFly before merging?
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.
Good idea, yes WildFly has a similar implementation but it's a different one.