-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[6.2] Package-private members access fixes for bytecode enhancement #10959
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
Conversation
...rnate-core/src/main/java/org/hibernate/bytecode/internal/bytebuddy/BytecodeProviderImpl.java
Outdated
Show resolved
Hide resolved
b651802
to
a15630a
Compare
...core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/optimizer/AncestorEntity.java
Dismissed
Show dismissed
Hide dismissed
...core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/optimizer/AncestorEntity.java
Dismissed
Show dismissed
Hide dismissed
hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/foreignpackage/ConcreteEntity.java
Dismissed
Show dismissed
Hide dismissed
hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/foreignpackage/ConcreteEntity.java
Dismissed
Show dismissed
Hide dismissed
hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/foreignpackage/ConcreteEntity.java
Dismissed
Show dismissed
Hide dismissed
if ( declaringType.isVisibleTo( type ) ) { | ||
if ( fieldDescription.isPublic() ) { | ||
return true; |
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.
Technically a package-protected declaring type would pass the check, but would likely end up hitting the problem we're trying to fix in case of split packages.
Though in practice I doubt package-protected types would work well with Hibernate ORM, so I suppose we can ignore this.
// We explicitly consider package-private fields as not visible, as the classes | ||
// might have the same package name but be loaded by different class loaders. | ||
// (see https://hibernate.atlassian.net/browse/HHH-19784) |
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 think we don't have a test for that, correct? That's fine, but I just wanted to check that it's simply because it's too hard to set up.
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.
You are correct, it's quite hard to test mappings in different EARs or even EJB jars using separate class loaders - we tried in the past and didn't get anywhere. We might give it another try, but it might be much simpler to do in something like a Wildfly integration test.
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.
In case it helps, I believe the attempt to simulate an EAR deploying with Hibernate ORM was defeated by the test framework library loading the test entity classes before they could be loaded by (EAR like) test classloader.
84c64a0
to
53573c7
Compare
…erent class loaders
53573c7
to
3b77f44
Compare
Fixes https://hibernate.atlassian.net/browse/HHH-19784 in the first commit.
The rest of the commits are a backport of #10192, which are required to make access optimizers work in similar cases (package-private fields in the same hierarchy but with different classloaders).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-19369
https://hibernate.atlassian.net/browse/HHH-19372