HHH-18693 Hibernate Processor does not handle inner types #9098

cigaly · 2024-10-16T11:48:32Z

See Jira Issue HHH-18693

This is still work in progress, not to be merged with main branch, at least not immediately.

For each (static) inner non-private class one meta-model class will be generated.

Naming is as follows. If we have:

class A {
    ...
    @Entity
    static class B {
        ...
    }
    ...
}

Generated class will be named A_B_

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.

cigaly · 2024-10-16T11:48:51Z

Some existing test classes were changed to make inner classes non-private. Otherwise such class could not be referenced from meta model class.

gavinking · 2024-10-16T12:39:47Z

Thanks for working on this @cigaly !

gavinking · 2024-10-23T18:14:55Z

Some existing test classes were changed to make inner classes non-private. Otherwise such class could not be referenced from meta model class.

I think the Processor is still going to have to be able to elegantly handle private inner classes; we don't want to simply disallow that.

So I'm not quite sure what "elegantly" means here ... perhaps it just means "skip them".

cigaly · 2024-10-24T06:43:38Z

So I'm not quit sure what "elegantly" means here ... perhaps it just means "skip them".

Other than disallowing private classes (either inner or top-level) , excluding them from metamodel generation is only solution that will not cause Java compilation errors. And, of course, there is always possibility that private static class could be referenced by either top level class containing private class or other inner class contained by same top level class.

Now only one test class has to be changed to avoid compilation errors is org.hibernate.orm.test.schematools.PrimaryKeyColumnOrderTest.EntityId. Here metemodel generated from

	public static class EntityId implements Serializable {
		private int A;
		private int B;
	}

will result in

public abstract class PrimaryKeyColumnOrderTest_EntityId_ {
	public static final String A = "A";
	public static final String B = "B";
	public static volatile SingularAttribute<EntityId, Integer> A;
	public static volatile SingularAttribute<EntityId, Integer> B;
	public static volatile EmbeddableType<EntityId> class_;
}

which is definitely something Java is happy with. Perhaps some mechanism can be added to prevent such duplications, but until then this class should be slightly changed to let tests to run.

gavinking · 2024-10-24T06:56:05Z

Now only one test class has to be changed to avoid compilation errors is

Hurm.... so that's actually a hole in the algorithm defined by the spec, isn't it? We should fix that, in both Processor, and in the spec. For example, we could say that A -> _A.

cigaly · 2024-10-24T07:40:27Z

This is more like one of those "tricky" questions for entertainment pages of some Java magazine. Something like "how are named getters and setters in a class with int property named abc and String property named Abc".

It seems that in current code only problematic are properties with name consisting of single uppercase letter. To fix that (at least, until some other solution) I will change method org.hibernate.processor.util.StringUtil#getUpperUnderscoreCaseFromLowerCamelCase by inserting right after method declaration lines

		if (lowerCamelCaseString.length()== 1 && lowerCamelCaseString.toUpperCase().equals( lowerCamelCaseString )) {
			return "_" + lowerCamelCaseString;
		}

(or, maybe, to use underscore as a suffix?)

If nothing else, this will allow implement metamodel class generation for inner classes, without touching any core (main or test) code.

gavinking · 2024-10-24T07:43:34Z

Yeah but you can just use Character.isUpperCase() 😜

cigaly · 2024-10-24T07:44:53Z

Yes, you are right. It seems that I am still sleepy :-)

gavinking · 2024-11-04T16:43:09Z

@cigaly can you give me permission to push to your branch?

gavinking · 2024-11-04T16:43:55Z

Or, whatever, don't worry, I can open a new PR myself, I guess.

cigaly · 2024-11-04T16:50:53Z

@cigaly can you give me permission to push to your branch?

Of course ... only I hope that I did not done something completely different :-)

gavinking · 2024-11-04T20:16:38Z

Or, whatever, don't worry, I can open a new PR myself, I guess.

Heh, I actually typed these comments on the wrong PR. I was meaning to post on the MySQL PR.

...odel-generator/src/main/java/org/hibernate/processor/validation/ProcessorSessionFactory.java

gavinking

Thanks, @cigaly, looks pretty reasonable, but see my comments.

gavinking · 2024-11-17T13:32:13Z

tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java

+			if ( !included( element )
+					|| hasAnnotation( element, Constants.EXCLUDE )
+					|| hasPackageAnnotation( element, Constants.EXCLUDE ) ) {
+				// skip it completely
+			}
+			else if ( isEntityOrEmbeddable( element ) && !element.getModifiers().contains( Modifier.PRIVATE )) {


I think we could just say that the processor completely skips private inner types. WDYT of this @cigaly:

Suggested change

if ( !included( element )

|| hasAnnotation( element, Constants.EXCLUDE )

|| hasPackageAnnotation( element, Constants.EXCLUDE ) ) {

// skip it completely

}

else if ( isEntityOrEmbeddable( element ) && !element.getModifiers().contains( Modifier.PRIVATE )) {

if ( !included( element )

|| hasAnnotation( element, Constants.EXCLUDE )

|| hasPackageAnnotation( element, Constants.EXCLUDE )

|| element.getModifiers().contains( Modifier.PRIVATE ) ) {

// skip it completely

}

else if ( isEntityOrEmbeddable( element ) ) {

Agree. And for maybe will be good idea to keep access modifier for non private classes?

And for maybe will be good idea to keep access modifier for non private classes?

You mean generate the metamodel with the same visibility? Yeah, that makes sense I suppose...

One more thing ... in PR #9220 id class is generated as inner class of parent meta model class. Here meta model class for each inner class is generated as separate class. I guess that now is good time to decide if this should stay that way or will be better idea to generate them as inner classes as well.

I think I would prefer it to be an inner class.

Agree, I will try to refactor this to make things that way some time next week.

gavinking · 2024-11-17T13:34:41Z

tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java

@@ -714,7 +728,7 @@ private void writeIndex() {
 					.createResource(
 							StandardLocation.SOURCE_OUTPUT,
 							ENTITY_INDEX,
-							entityName,
+							URLEncoder.encode(entityName, UTF_8),


Why is this necessary?

Take look at the Person in org.hibernate.orm.test.hql.QuotedIdentifierTest. There entity name is The Person (guess that this is not forbidden by the specification) and processing (and/or compilation) is crashing with message:

java.lang.IllegalArgumentException: Invalid relative name: The Person

If you like, I can write similar test for Hibernate Processor. Of course, if such entity name is allowed.

OK, I see. But then I think we need to make the same change in ProcessorSessionFactory.findEntityByUnqualifiedName()?

if such entity name is allowed.

Well that is a great question. It looks like the JPA spec is silent on the format of entity names, except to say that they are case-sensitive, and unique in a PU.

I think, however, that we can infer that they are not supposed to have spaces in them, because then we would not be able to parse the query language.

I think I should open an issue against the spec.

No, ProcessorSessionFactory.findEntityByUnqualifiedName() will work fine, since entity name is key for entityNameMappings. And such entity can be, despite its really 'ugly' name, used in HQL queries if properly quoted. QuotedIdentifierTest is testing exactly that. I've just tried to name class 'The Person?', and it is working fine.
But, I guess that spec should be changes anyway. Although, I guess that some hardcore Star Trek fans will be disappointed if not able to name entites using Klingon alphabet :-) [It is working now, BTW]

I've removed that change and excluded all "troublemakers". Unfortunately, I can not find wither where this entity index is specified or used, Problem with such "ugly" entity names can not be solved in Hibernate, because finally it is used in com.sun.tools.javac.file.JavacFileManager#getFileForOutput. That name must be encoded to pass as constructor parameter for java.net.URI, but must not be encoded to allow file to be created with proper entity name. Something that Joseph Keller called "Catch 22" :-)

I don't think we need to worry about indexing such pathological entity names. They can't be used in queries anyway (since the query can't be parsed).

jakartaee/persistence#686

For the record.

Can be used if quoted. But I agree that that such names should not be used. Maybe only to put some disclaimer about that for those who like to sue company for mot specifically saying that cat can not be put in microwave oven to dry? :-)

It seems that Hibernate will happily accept non-ASCII letters in entity (and/or class) names (PostgreSQL even in column names), so only it should be not big "sacrifice" to not allow special characters

Can be used if quoted.

Ahhh uuhmmm, yeah, OK sure. I forgot HQL has quoted identifiers (JPQL doesn't).

tooling/metamodel-generator/src/main/java/org/hibernate/processor/util/StringUtil.java

...odel-generator/src/main/java/org/hibernate/processor/validation/ProcessorSessionFactory.java

cigaly · 2024-11-22T14:32:33Z

Code changes to generate metadata for inner class inside other metadata class instead of new metadata class file for each managed class.

Note: This code will surely need some (beauty and other) changes.

...etamodel-generator/src/main/java/org/hibernate/processor/annotation/NonManagedMetamodel.java

+		return metamodel;
+	}
+
+	protected void init() {


gavinking

OK, I've tried this out and it looks like it's working great!

@cigaly could you do me the favor of squashing it down to two or three commits so that I can merge it?

Thanks!!!

...etamodel-generator/src/main/java/org/hibernate/processor/annotation/NonManagedMetamodel.java

Test case for Jakarta Data processing with inner classes Changed existing test class to properly check generated metamodel class @Exclude-ing "troublemakers" with illegal URI character(s) in entity name

Generated metadata for inner class A.B is A_.B_ Path source for inner class is identical to path source for enclosing class

… non-private classes Generaed metadate class for inner class A.B is A_.B_

gavinking · 2024-12-04T19:05:24Z

Thanks so much @cigaly, this is great!

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from 965c5f0 to c20fa62 Compare October 23, 2024 14:14

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from c20fa62 to a8fb6dd Compare October 24, 2024 06:28

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from a8fb6dd to 83e4d1e Compare October 24, 2024 07:40

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from 83e4d1e to 3d9520b Compare October 24, 2024 07:45

cigaly marked this pull request as ready for review October 25, 2024 09:31

cigaly mentioned this pull request Oct 25, 2024

HHH 18705 - Hibernate processor creates bad TypedReferenceQuery when @Entity have name attribute #9064

Merged

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from f920687 to c650224 Compare November 4, 2024 16:28

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from c650224 to 99774e7 Compare November 16, 2024 20:38

github-advanced-security bot found potential problems Nov 16, 2024

View reviewed changes

...odel-generator/src/main/java/org/hibernate/processor/validation/ProcessorSessionFactory.java Fixed Show fixed Hide fixed

gavinking reviewed Nov 17, 2024

View reviewed changes

gavinking reviewed Nov 21, 2024

View reviewed changes

...odel-generator/src/main/java/org/hibernate/processor/validation/ProcessorSessionFactory.java Show resolved Hide resolved

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from 255abee to 68552a8 Compare November 22, 2024 06:27

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from b9a0bcf to 3a5dd6b Compare November 22, 2024 14:44

github-advanced-security bot found potential problems Nov 22, 2024

View reviewed changes

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch 3 times, most recently from 11a8adc to b481c7f Compare November 23, 2024 22:09

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch 3 times, most recently from 78e1fce to 121c85e Compare November 24, 2024 13:12

gavinking approved these changes Dec 4, 2024

View reviewed changes

...etamodel-generator/src/main/java/org/hibernate/processor/annotation/NonManagedMetamodel.java Show resolved Hide resolved

cigaly and others added 3 commits December 4, 2024 17:35

HHH-18693 Test case with inner classes

6cca996

Test case for Jakarta Data processing with inner classes Changed existing test class to properly check generated metamodel class @Exclude-ing "troublemakers" with illegal URI character(s) in entity name

HHH-18693 Changed name generation for metamodel classes and sources

99d39ac

Generated metadata for inner class A.B is A_.B_ Path source for inner class is identical to path source for enclosing class

HHH-18693 Changed code to allow creation of metadata for inner static…

5a6ab87

… non-private classes Generaed metadate class for inner class A.B is A_.B_

cigaly force-pushed the HHH-18693-Hibernate_processor-Inner_classes branch from 121c85e to 5a6ab87 Compare December 4, 2024 16:51

gavinking merged commit d3a1ebd into hibernate:main Dec 4, 2024
22 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HHH-18693 Hibernate Processor does not handle inner types #9098

HHH-18693 Hibernate Processor does not handle inner types #9098

cigaly commented Oct 16, 2024

cigaly commented Oct 16, 2024

gavinking commented Oct 16, 2024

gavinking commented Oct 23, 2024 •

edited

Loading

cigaly commented Oct 24, 2024 •

edited

Loading

gavinking commented Oct 24, 2024 •

edited

Loading

cigaly commented Oct 24, 2024

gavinking commented Oct 24, 2024

cigaly commented Oct 24, 2024

gavinking commented Nov 4, 2024

gavinking commented Nov 4, 2024

cigaly commented Nov 4, 2024

gavinking commented Nov 4, 2024

gavinking left a comment

gavinking Nov 17, 2024

cigaly Nov 17, 2024

gavinking Nov 17, 2024

cigaly Nov 17, 2024

gavinking Nov 17, 2024

cigaly Nov 17, 2024

gavinking Nov 17, 2024

cigaly Nov 17, 2024

gavinking Nov 17, 2024

cigaly Nov 17, 2024

cigaly Nov 25, 2024

gavinking Nov 25, 2024

gavinking Nov 25, 2024

cigaly Nov 25, 2024

cigaly Nov 25, 2024

gavinking Nov 25, 2024

cigaly commented Nov 22, 2024

gavinking left a comment

gavinking commented Dec 4, 2024

HHH-18693 Hibernate Processor does not handle inner types #9098

HHH-18693 Hibernate Processor does not handle inner types #9098

Conversation

cigaly commented Oct 16, 2024

cigaly commented Oct 16, 2024

gavinking commented Oct 16, 2024

gavinking commented Oct 23, 2024 • edited Loading

cigaly commented Oct 24, 2024 • edited Loading

gavinking commented Oct 24, 2024 • edited Loading

cigaly commented Oct 24, 2024

gavinking commented Oct 24, 2024

cigaly commented Oct 24, 2024

gavinking commented Nov 4, 2024

gavinking commented Nov 4, 2024

cigaly commented Nov 4, 2024

gavinking commented Nov 4, 2024

gavinking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cigaly commented Nov 22, 2024

gavinking left a comment

Choose a reason for hiding this comment

gavinking commented Dec 4, 2024

gavinking commented Oct 23, 2024 •

edited

Loading

cigaly commented Oct 24, 2024 •

edited

Loading

gavinking commented Oct 24, 2024 •

edited

Loading