Skip to content

Commit a75a4d8

Browse files
gavinkinggavinking
committed
HHH-19252 detect unsupported @id overriding
1 parent 19a5c93 commit a75a4d8

File tree

4 files changed

+129
-63
lines changed

4 files changed

+129
-63
lines changed

‎hibernate-core/src/main/java/org/hibernate/boot/model/internal/GeneratorAnnotationHelper.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ public static void handleIdGeneratorType(
250250
MetadataBuildingContext buildingContext) {
251251
final IdGeneratorType markerAnnotation =
252252
generatorAnnotation.annotationType().getAnnotation( IdGeneratorType.class );
253-
idValue.setCustomIdGeneratorCreator( (creationContext) -> {
253+
idValue.setCustomIdGeneratorCreator( creationContext -> {
254254
final Generator identifierGenerator =
255255
instantiateGenerator( beanContainer( buildingContext ), markerAnnotation.value() );
256256
prepareForUse(

‎hibernate-core/src/main/java/org/hibernate/boot/model/internal/InheritanceState.java

Lines changed: 20 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import org.hibernate.AnnotationException;
1313
import org.hibernate.boot.spi.AccessType;
14+
import org.hibernate.boot.spi.InFlightMetadataCollector;
1415
import org.hibernate.boot.spi.MetadataBuildingContext;
1516
import org.hibernate.boot.spi.PropertyData;
1617
import org.hibernate.mapping.Component;
@@ -185,19 +186,16 @@ else if ( classDetails.hasDirectAnnotationUsage( IdClass.class ) ) {
185186
return classDetails;
186187
}
187188
else {
188-
final long count = Stream.concat(
189-
classDetails.getFields().stream(),
190-
classDetails.getMethods().stream()
191-
).filter( t -> t.hasDirectAnnotationUsage( Id.class ) ).count();
189+
final long count =
190+
Stream.concat( classDetails.getFields().stream(), classDetails.getMethods().stream() )
191+
.filter( member -> member.hasDirectAnnotationUsage( Id.class ) )
192+
.count();
192193
if ( count > 1 ) {
193194
return classDetails;
194195
}
195-
final InheritanceState state = getSuperclassInheritanceState( classDetails, inheritanceStatePerClass );
196-
if ( state != null ) {
197-
return state.getClassWithIdClass( true );
198-
}
199196
else {
200-
return null;
197+
final InheritanceState state = getSuperclassInheritanceState( classDetails, inheritanceStatePerClass );
198+
return state == null ? null : state.getClassWithIdClass( true );
201199
}
202200
}
203201
}
@@ -209,8 +207,7 @@ public Boolean hasIdClassOrEmbeddedId() {
209207
hasIdClassOrEmbeddedId = true;
210208
}
211209
else {
212-
final ElementsToProcess process = getElementsToProcess();
213-
for ( PropertyData property : process.getElements() ) {
210+
for ( PropertyData property : getElementsToProcess().getElements() ) {
214211
if ( property.getAttributeMember().hasDirectAnnotationUsage( EmbeddedId.class ) ) {
215212
hasIdClassOrEmbeddedId = true;
216213
break;
@@ -228,7 +225,7 @@ public Boolean hasIdClassOrEmbeddedId() {
228225
*/
229226
private ElementsToProcess getElementsToProcess() {
230227
if ( elementsToProcess == null ) {
231-
InheritanceState inheritanceState = inheritanceStatePerClass.get( classDetails );
228+
final InheritanceState inheritanceState = inheritanceStatePerClass.get( classDetails );
232229
assert !inheritanceState.isEmbeddableSuperclass();
233230

234231
getMappedSuperclassesTillNextEntityOrdered();
@@ -237,20 +234,11 @@ private ElementsToProcess getElementsToProcess() {
237234

238235
final ArrayList<PropertyData> elements = new ArrayList<>();
239236
int idPropertyCount = 0;
240-
241237
for ( ClassDetails classToProcessForMappedSuperclass : classesToProcessForMappedSuperclass ) {
242-
PropertyContainer propertyContainer = new PropertyContainer(
243-
classToProcessForMappedSuperclass,
244-
classDetails,
245-
accessType
246-
);
247-
idPropertyCount = addElementsOfClass(
248-
elements,
249-
propertyContainer,
250-
buildingContext,
251-
idPropertyCount );
238+
final PropertyContainer container =
239+
new PropertyContainer( classToProcessForMappedSuperclass, classDetails, accessType );
240+
idPropertyCount = addElementsOfClass( elements, container, buildingContext, idPropertyCount );
252241
}
253-
254242
if ( idPropertyCount == 0 && !inheritanceState.hasParents() ) {
255243
throw new AnnotationException( "Entity '" + classDetails.getName() + "' has no identifier"
256244
+ " (every '@Entity' class must declare or inherit at least one '@Id' or '@EmbeddedId' property)" );
@@ -278,15 +266,11 @@ private AccessType determineDefaultAccessType() {
278266
if ( candidate.hasDirectAnnotationUsage( Entity.class )
279267
|| candidate.hasDirectAnnotationUsage( MappedSuperclass.class ) ) {
280268
for ( MethodDetails method : candidate.getMethods() ) {
281-
if ( method.getMethodKind() != MethodDetails.MethodKind.GETTER ) {
282-
continue;
283-
}
284-
285-
if ( hasIdAnnotation( method ) ) {
269+
if ( method.getMethodKind() == MethodDetails.MethodKind.GETTER
270+
&& hasIdAnnotation( method ) ) {
286271
return AccessType.PROPERTY;
287272
}
288273
}
289-
290274
for ( FieldDetails field : candidate.getFields() ) {
291275
if ( hasIdAnnotation( field ) ) {
292276
return AccessType.FIELD;
@@ -314,7 +298,6 @@ private void getMappedSuperclassesTillNextEntityOrdered() {
314298
while ( superClass != null
315299
&& !Object.class.getName().equals( superClass.getClassName() )
316300
&& superclassState == null );
317-
318301
currentClassInHierarchy = superClass;
319302
}
320303
while ( superclassState != null && superclassState.isEmbeddableSuperclass() );
@@ -337,11 +320,12 @@ private void addMappedSuperClassInMetadata(PersistentClass persistentClass) {
337320
private org.hibernate.mapping.MappedSuperclass processMappedSuperclass(Table implicitTable) {
338321
//add @MappedSuperclass in the metadata
339322
// classes from 0 to n-1 are @MappedSuperclass and should be linked
323+
final InFlightMetadataCollector metadataCollector = buildingContext.getMetadataCollector();
340324
final InheritanceState superEntityState = getInheritanceStateOfSuperEntity( classDetails, inheritanceStatePerClass );
341325
final PersistentClass superEntity =
342-
superEntityState != null ?
343-
buildingContext.getMetadataCollector().getEntityBinding( superEntityState.getClassDetails().getName() ) :
344-
null;
326+
superEntityState != null
327+
? metadataCollector.getEntityBinding( superEntityState.getClassDetails().getName() )
328+
: null;
345329
final int lastMappedSuperclass = classesToProcessForMappedSuperclass.size() - 1;
346330
org.hibernate.mapping.MappedSuperclass mappedSuperclass = null;
347331
for ( int index = 0; index < lastMappedSuperclass; index++ ) {
@@ -351,11 +335,11 @@ private org.hibernate.mapping.MappedSuperclass processMappedSuperclass(Table imp
351335
final ClassDetails mappedSuperclassDetails = classesToProcessForMappedSuperclass.get( index );
352336
final Class<?> mappedSuperclassJavaType = mappedSuperclassDetails.toJavaClass();
353337
//add MappedSuperclass if not already there
354-
mappedSuperclass = buildingContext.getMetadataCollector().getMappedSuperclass( mappedSuperclassJavaType );
338+
mappedSuperclass = metadataCollector.getMappedSuperclass( mappedSuperclassJavaType );
355339
if ( mappedSuperclass == null ) {
356340
mappedSuperclass = new org.hibernate.mapping.MappedSuperclass( parentSuperclass, superEntity, implicitTable );
357341
mappedSuperclass.setMappedClass( mappedSuperclassJavaType );
358-
buildingContext.getMetadataCollector().addMappedSuperclass( mappedSuperclassJavaType, mappedSuperclass );
342+
metadataCollector.addMappedSuperclass( mappedSuperclassJavaType, mappedSuperclass );
359343
}
360344
}
361345
return mappedSuperclass;

‎hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.util.List;
1111
import java.util.Map;
1212

13+
import jakarta.persistence.GeneratedValue;
1314
import jakarta.persistence.MapKey;
1415
import jakarta.persistence.MapKeyClass;
1516
import jakarta.persistence.MapKeyColumn;
@@ -25,6 +26,7 @@
2526
import org.hibernate.annotations.Any;
2627
import org.hibernate.annotations.AttributeBinderType;
2728
import org.hibernate.annotations.CompositeType;
29+
import org.hibernate.annotations.IdGeneratorType;
2830
import org.hibernate.annotations.Immutable;
2931
import org.hibernate.annotations.LazyGroup;
3032
import org.hibernate.annotations.ManyToAny;
@@ -596,20 +598,22 @@ private static int addProperty(
596598
List<PropertyData> inFlightPropertyDataList,
597599
MetadataBuildingContext context,
598600
int idPropertyCounter) {
601+
final InFlightMetadataCollector collector = context.getMetadataCollector();
602+
599603
// see if inFlightPropertyDataList already contains a PropertyData for this name,
600604
// and if so, skip it...
601605
for ( PropertyData propertyData : inFlightPropertyDataList ) {
602606
if ( propertyData.getPropertyName().equals( property.resolveAttributeName() ) ) {
603-
checkIdProperty( property, propertyData );
607+
checkIdProperty( property, propertyData, collector.getSourceModelBuildingContext() );
604608
// EARLY EXIT!!!
605609
return idPropertyCounter;
606610
}
607611
}
608612

609-
final ClassDetails declaringClass = propertyContainer.getDeclaringClass();
610613
final TypeVariableScope ownerType = propertyContainer.getTypeAtStake();
614+
611615
final PropertyData propertyAnnotatedElement = new PropertyInferredData(
612-
declaringClass,
616+
propertyContainer.getDeclaringClass(),
613617
ownerType,
614618
property,
615619
propertyContainer.getClassLevelAccessType().getType(),
@@ -622,31 +626,51 @@ private static int addProperty(
622626
if ( hasIdAnnotation( element ) ) {
623627
inFlightPropertyDataList.add( idPropertyCounter, propertyAnnotatedElement );
624628
if ( hasToOneAnnotation( element ) ) {
625-
context.getMetadataCollector()
626-
.addToOneAndIdProperty( ownerType.determineRawClass(), propertyAnnotatedElement );
629+
collector.addToOneAndIdProperty( ownerType.determineRawClass(), propertyAnnotatedElement );
627630
}
628631
idPropertyCounter++;
629632
}
630633
else {
631634
inFlightPropertyDataList.add( propertyAnnotatedElement );
632635
}
633636
if ( element.hasDirectAnnotationUsage( MapsId.class ) ) {
634-
context.getMetadataCollector()
635-
.addPropertyAnnotatedWithMapsId( ownerType.determineRawClass(), propertyAnnotatedElement );
637+
collector.addPropertyAnnotatedWithMapsId( ownerType.determineRawClass(), propertyAnnotatedElement );
636638
}
637639

638640
return idPropertyCounter;
639641
}
640642

641-
private static void checkIdProperty(MemberDetails property, PropertyData propertyData) {
642-
final Id incomingIdProperty = property.getDirectAnnotationUsage( Id.class );
643+
private static void checkIdProperty(MemberDetails property, PropertyData propertyData, SourceModelBuildingContext context) {
644+
final boolean incomingIdProperty = hasIdAnnotation( property );
643645
final MemberDetails attributeMember = propertyData.getAttributeMember();
644-
final Id existingIdProperty = attributeMember.getDirectAnnotationUsage( Id.class );
645-
if ( incomingIdProperty != null && existingIdProperty == null ) {
646-
throw new MappingException(
647-
"Attribute '" + attributeMember.getName()
648-
+ "' is declared by '" + attributeMember.getDeclaringType().getName()
649-
+ "' and may not be redeclared as an '@Id' by '" + property.getDeclaringType().getName() + "'" );
646+
final boolean existingIdProperty = hasIdAnnotation( attributeMember );
647+
if ( incomingIdProperty ) {
648+
if ( existingIdProperty ) {
649+
if ( property.hasDirectAnnotationUsage( GeneratedValue.class )
650+
|| !property.getMetaAnnotated( IdGeneratorType.class, context ).isEmpty() ) {
651+
//TODO: it would be nice to allow a root @Entity to override an
652+
// @Id field declared by a @MappedSuperclass and change the
653+
// generator, but for now we don't seem to be able to detect
654+
// that case here
655+
throw new AnnotationException(
656+
"Attribute '" + attributeMember.getName()
657+
+ "' is declared as an '@Id' or '@EmbeddedId' property by '"
658+
+ attributeMember.getDeclaringType().getName()
659+
+ "' and so '" + property.getDeclaringType().getName()
660+
+ "' may not respecify the generation strategy" );
661+
}
662+
}
663+
else {
664+
//TODO: it would be nice to allow a root @Entity to override a
665+
// field declared by a @MappedSuperclass, redeclaring it
666+
// as an @Id field, but for now we don't seem to be able
667+
// to detect that case here
668+
throw new AnnotationException(
669+
"Attribute '" + attributeMember.getName()
670+
+ "' is declared by '" + attributeMember.getDeclaringType().getName()
671+
+ "' and may not be redeclared as an '@Id' or '@EmbeddedId' by '"
672+
+ property.getDeclaringType().getName() + "'" );
673+
}
650674
}
651675
}
652676

@@ -721,14 +745,17 @@ private static void buildProperty(
721745
MetadataBuildingContext context,
722746
Map<ClassDetails, InheritanceState> inheritanceStatePerClass,
723747
MemberDetails property) {
748+
749+
if ( isPropertyOfRegularEmbeddable( propertyHolder, isComponentEmbedded )
750+
&& property.hasDirectAnnotationUsage(Id.class)) {
751+
throw new AnnotationException("Member '" + property.getName()
752+
+ "' of embeddable class " + propertyHolder.getClassName() + " is annotated '@Id'");
753+
}
754+
724755
final TypeDetails attributeTypeDetails =
725756
inferredData.getAttributeMember().isPlural()
726757
? inferredData.getAttributeMember().getType()
727758
: inferredData.getClassOrElementType();
728-
final ClassDetails attributeClassDetails = attributeTypeDetails.determineRawClass();
729-
final ColumnsBuilder columnsBuilder =
730-
new ColumnsBuilder( propertyHolder, nullability, property, inferredData, entityBinder, context )
731-
.extractMetadata();
732759

733760
final PropertyBinder propertyBinder = propertyBinder(
734761
propertyHolder,
@@ -741,17 +768,14 @@ private static void buildProperty(
741768
attributeTypeDetails
742769
);
743770

744-
if ( isPropertyOfRegularEmbeddable( propertyHolder, isComponentEmbedded )
745-
&& property.hasDirectAnnotationUsage(Id.class)) {
746-
throw new AnnotationException("Member '" + property.getName()
747-
+ "' of embeddable class " + propertyHolder.getClassName() + " is annotated '@Id'");
748-
}
749-
750771
final LazyGroup lazyGroupAnnotation = property.getDirectAnnotationUsage( LazyGroup.class );
751772
if ( lazyGroupAnnotation != null ) {
752773
propertyBinder.setLazyGroup( lazyGroupAnnotation.value() );
753774
}
754775

776+
final ColumnsBuilder columnsBuilder =
777+
new ColumnsBuilder( propertyHolder, nullability, property, inferredData, entityBinder, context )
778+
.extractMetadata();
755779
final AnnotatedJoinColumns joinColumns = columnsBuilder.getJoinColumns();
756780
final AnnotatedColumns columns = propertyBinder.bindProperty(
757781
propertyHolder,
@@ -762,7 +786,7 @@ private static void buildProperty(
762786
isComponentEmbedded,
763787
inSecondPass,
764788
property,
765-
attributeClassDetails,
789+
attributeTypeDetails.determineRawClass(),
766790
columnsBuilder
767791
);
768792
addNaturalIds( inSecondPass, property, columns, joinColumns, context );

‎hibernate-core/src/test/java/org/hibernate/IdGeneratorOverridingTest.java

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
* Copyright Red Hat Inc. and Hibernate Authors
4+
*/
5+
package org.hibernate;
6+
7+
import jakarta.persistence.Entity;
8+
import jakarta.persistence.GeneratedValue;
9+
import jakarta.persistence.Id;
10+
import jakarta.persistence.MappedSuperclass;
11+
import jakarta.persistence.SequenceGenerator;
12+
import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
13+
import org.hibernate.testing.orm.junit.FailureExpected;
14+
import org.hibernate.testing.orm.junit.Jpa;
15+
import org.junit.jupiter.api.Test;
16+
17+
@Jpa(annotatedClasses = {IdGeneratorOverridingTest.A.class, IdGeneratorOverridingTest.B.class})
18+
public class IdGeneratorOverridingTest {
19+
20+
@FailureExpected(reason = "We don't (yet) allow overriding of ids declared by mapped superclasses")
21+
@Test void test(EntityManagerFactoryScope scope) {
22+
scope.inTransaction( em -> em.persist( new B() ) );
23+
}
24+
25+
@MappedSuperclass
26+
static abstract class A {
27+
private Long id;
28+
29+
@Id
30+
@GeneratedValue(generator = "a_sequence")
31+
@SequenceGenerator(name = "a_sequence", sequenceName = "a_sequence")
32+
public Long getId() {
33+
return id;
34+
}
35+
36+
public void setId(Long id) {
37+
this.id = id;
38+
}
39+
}
40+
41+
@Entity
42+
static class B extends A {
43+
private Long id;
44+
45+
@Id
46+
@GeneratedValue(generator = "b_sequence")
47+
@SequenceGenerator(name = "b_sequence", sequenceName = "b_sequence")
48+
@Override
49+
public Long getId() {
50+
return id;
51+
}
52+
53+
@Override
54+
public void setId(Long id) {
55+
this.id = id;
56+
}
57+
}
58+
}

0 commit comments

Comments
 (0)