HHH-16058 Removing Environment#getBytecodeProvider #5977

Sanne · 2023-01-18T22:57:48Z

https://hibernate.atlassian.net/browse/HHH-16058

cc/ @sebersole and @DavideD for awareness

Sanne · 2023-01-19T15:43:09Z

@yrodiere would you like to review this one?

yrodiere

Well that does looks dangerous, but I guess we like to live dangerously :) A few comments below.

By the way, does this also solve https://hibernate.atlassian.net/browse/HHH-12193 ?

hibernate-core/src/main/java/org/hibernate/cfg/Environment.java

yrodiere · 2023-01-20T09:00:55Z

...ate-core/src/main/java/org/hibernate/jpa/internal/enhance/EnhancingClassTransformerImpl.java

 	public EnhancingClassTransformerImpl(EnhancementContext enhancementContext) {
+		Objects.requireNonNull( enhancementContext );
 		this.enhancementContext = enhancementContext;
+		this.bytecodeProvider = BytecodeProviderInitiator.buildDefaultBytecodeProvider();


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 :/

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 bytebuddy or none. Clearly this use case can't use none.

(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)

And yes I'd like to make the BytecodeProvider implementation pluggable

I was thinking more that you're not using the same instance of BytecodeProvider, whereas before this patch, you were. I have no idea what kind of impact this can have, but since the bytecode provider is stateful, it might be a problem?

yrodiere · 2023-01-20T09:03:50Z

...ate-core/src/main/java/org/hibernate/jpa/internal/enhance/EnhancingClassTransformerImpl.java

 	private final EnhancementContext enhancementContext;
+	private final BytecodeProvider bytecodeProvider;

 	public EnhancingClassTransformerImpl(EnhancementContext enhancementContext) {


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

@Override

public void pushClassTransformer(EnhancementContext enhancementContext) {

persistenceUnitInfo.addTransformer( new EnhancingClassTransformerImpl( enhancementContext ) );

}

But then all implementations of PersistenceUnitInfo#addTransformer are no-ops...

This component is probably not being tested. I'd suggest trying your patch in WildFly before merging?

Good idea, yes WildFly has a similar implementation but it's a different one.

yrodiere · 2023-01-20T09:09:28Z

hibernate-core/src/main/java/org/hibernate/proxy/AbstractLazyInitializer.java

+				//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.


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...

Good catch, I've amended the javadoc to clarify.

yrodiere · 2023-01-20T09:12:08Z

hibernate-core/src/main/java/org/hibernate/proxy/pojo/bytebuddy/SerializableProxy.java

 	}
+
+	private static SessionFactoryImplementor retrieveMatchingSessionFactory(final String sessionFactoryUuid) {
+		Objects.requireNonNull( sessionFactoryUuid );


We won't be able to deserialize entities serialized before this patch, since the uuid will be missing.

Not sure what the backwards compatibility expectations of serialization are, though.

The UUID was always supposed to be there in practice - I only noticed this wasn't happening consistently because we have some proxy serialization tests which were performing this while in the odd configuration of not having set the sessionFactoryUuid first.
However in a "real" scenario this would have failed as the proxy can't be initialized.

hibernate-core/src/main/java/org/hibernate/proxy/pojo/bytebuddy/SerializableProxy.java

...testing/src/main/java/org/hibernate/testing/bytecode/enhancement/BytecodeEnhancerRunner.java

Sanne · 2023-01-20T09:36:33Z

By the way, does this also solve https://hibernate.atlassian.net/browse/HHH-12193 ?

Right, thanks for finding that one.

hibernate-github-bot · 2023-01-20T09:42:36Z

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

Sanne · 2023-01-20T09:59:39Z

pushed a reviewed version

yrodiere

Thanks. LGTM as long as you're confident about the impact on EnhancingClassTransformerImpl.java and WildFly.
EDIT: And serialization :)

Sanne · 2023-01-20T10:57:20Z

👍 still checking WildFly, will merge if I spot no issues there.

Sanne · 2023-01-20T14:31:02Z

All good on the WildFly side - if we bypass the lack of an upodated Infinispan 2LC module.

Merging

Sanne force-pushed the HHH-16058 branch from 08b09e8 to 9651494 Compare January 18, 2023 23:05

yrodiere reviewed Jan 20, 2023

View reviewed changes

Sanne force-pushed the HHH-16058 branch from d982f5c to 0991dec Compare January 20, 2023 09:59

HHH-16058 Removing Environment#getBytecodeProvider

418acc2

Sanne force-pushed the HHH-16058 branch from 0991dec to 418acc2 Compare January 20, 2023 10:05

yrodiere approved these changes Jan 20, 2023

View reviewed changes

Sanne merged commit 800873c into hibernate:main Jan 20, 2023

Sanne deleted the HHH-16058 branch January 20, 2023 14:31

ctabin mentioned this pull request Mar 31, 2023

Bug Report: Deployment crash when Hibernate 6.2 is used/FISH-7338 payara/Payara#6248

Open

ctabin mentioned this pull request May 13, 2023

[7.0.4] Deployment fail with Hibernate 6.2.2.Final eclipse-ee4j/glassfish#24414

Closed

	@Override
	public void pushClassTransformer(EnhancementContext enhancementContext) {
	persistenceUnitInfo.addTransformer( new EnhancingClassTransformerImpl( enhancementContext ) );
	}

Uh oh!

HHH-16058 Removing Environment#getBytecodeProvider #5977

HHH-16058 Removing Environment#getBytecodeProvider #5977

Uh oh!

Conversation

Sanne commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sanne commented Jan 19, 2023

Uh oh!

yrodiere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrodiere Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sanne commented Jan 20, 2023

Uh oh!

hibernate-github-bot bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sanne commented Jan 20, 2023

Uh oh!

yrodiere left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanne commented Jan 20, 2023

Uh oh!

Sanne commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sanne commented Jan 18, 2023 •

edited

Loading

yrodiere Jan 20, 2023 •

edited

Loading

hibernate-github-bot bot commented Jan 20, 2023 •

edited

Loading

yrodiere left a comment •

edited

Loading