From 38f01311602f115ccec23f9ee8faf4b2d69d636d Mon Sep 17 00:00:00 2001 From: Chris Cranford Date: Fri, 6 Dec 2019 15:04:07 -0500 Subject: [PATCH] HHH-10844 Resolve columnDefinition to appropriate sql-type for audit mappings --- .../metadata/AuditMetadataGenerator.java | 8 +- .../metadata/BasicMetadataGenerator.java | 9 +- .../metadata/IdMetadataGenerator.java | 2 +- .../internal/metadata/MetadataTools.java | 65 ++++++-- .../basic/BasicTypeColumnDefinitionTest.java | 156 ++++++++++++++++++ 5 files changed, 223 insertions(+), 17 deletions(-) create mode 100644 hibernate-envers/src/test/java/org/hibernate/envers/test/integration/basic/BasicTypeColumnDefinitionTest.java diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/AuditMetadataGenerator.java b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/AuditMetadataGenerator.java index 4de1e90b8b..89d69df8ad 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/AuditMetadataGenerator.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/AuditMetadataGenerator.java @@ -110,7 +110,7 @@ public final class AuditMetadataGenerator { this.auditStrategy = auditStrategy; this.revisionInfoRelationMapping = revisionInfoRelationMapping; - this.basicMetadataGenerator = new BasicMetadataGenerator(); + this.basicMetadataGenerator = new BasicMetadataGenerator( this ); this.componentMetadataGenerator = new ComponentMetadataGenerator( this ); this.idMetadataGenerator = new IdMetadataGenerator( this ); this.toOneRelationMetadataGenerator = new ToOneRelationMetadataGenerator( this ); @@ -460,7 +460,7 @@ public final class AuditMetadataGenerator { } final Element joinKey = joinElement.addElement( "key" ); - MetadataTools.addColumns( joinKey, join.getKey().getColumnIterator() ); + MetadataTools.addColumns( joinKey, join.getKey().getColumnIterator(), metadata ); MetadataTools.addColumn( joinKey, verEntCfg.getRevisionFieldName(), null, null, null, null, null, null ); } } @@ -509,7 +509,7 @@ public final class AuditMetadataGenerator { if ( pc.getDiscriminator() != null ) { final Element discriminatorElement = classMapping.addElement( "discriminator" ); // Database column or SQL formula allowed to distinguish entity types - MetadataTools.addColumnsOrFormulas( discriminatorElement, pc.getDiscriminator().getColumnIterator() ); + MetadataTools.addColumnsOrFormulas( discriminatorElement, pc.getDiscriminator().getColumnIterator(), metadata ); discriminatorElement.addAttribute( "type", pc.getDiscriminator().getType().getName() ); } @@ -633,7 +633,7 @@ public final class AuditMetadataGenerator { // Adding the "key" element with all id columns... final Element keyMapping = mappingData.getFirst().addElement( "key" ); - MetadataTools.addColumns( keyMapping, pc.getTable().getPrimaryKey().columnIterator() ); + MetadataTools.addColumns( keyMapping, pc.getTable().getPrimaryKey().columnIterator(), metadata ); // ... and the revision number column, read from the revision info relation mapping. keyMapping.add( (Element) cloneAndSetupRevisionInfoRelationMapping().element( "column" ).clone() ); diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/BasicMetadataGenerator.java b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/BasicMetadataGenerator.java index b5c5f5edc6..7bd79f533c 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/BasicMetadataGenerator.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/BasicMetadataGenerator.java @@ -8,6 +8,7 @@ package org.hibernate.envers.configuration.internal.metadata; import java.util.Properties; +import org.hibernate.boot.Metadata; import org.hibernate.envers.configuration.internal.metadata.reader.PropertyAuditingData; import org.hibernate.envers.internal.entities.PropertyData; import org.hibernate.envers.internal.entities.mapper.SimpleMapperBuilder; @@ -28,6 +29,12 @@ import org.dom4j.Element; */ public final class BasicMetadataGenerator { + private final Metadata metadata; + + public BasicMetadataGenerator(AuditMetadataGenerator mainGenerator) { + this.metadata = mainGenerator.getMetadata(); + } + boolean addBasic( Element parent, PropertyAuditingData propertyAuditingData, @@ -109,7 +116,7 @@ public final class BasicMetadataGenerator { key ); - MetadataTools.addColumns( propMapping, value.getColumnIterator() ); + MetadataTools.addColumns( propMapping, value.getColumnIterator(), metadata ); return propMapping; } diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/IdMetadataGenerator.java b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/IdMetadataGenerator.java index 0047dd6a69..fdcb7aec5b 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/IdMetadataGenerator.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/IdMetadataGenerator.java @@ -313,7 +313,7 @@ public final class IdMetadataGenerator { // schema and the base table schema when a @ManyToOne is present in an identifier. manyToOneElement.addAttribute( "foreign-key", "none" ); - MetadataTools.addColumns( manyToOneElement, value.getColumnIterator() ); + MetadataTools.addColumns( manyToOneElement, value.getColumnIterator(), mainGenerator.getMetadata() ); return true; } diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/MetadataTools.java b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/MetadataTools.java index 77f86d6c84..4999800909 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/MetadataTools.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/configuration/internal/metadata/MetadataTools.java @@ -9,6 +9,10 @@ package org.hibernate.envers.configuration.internal.metadata; import java.util.Iterator; import javax.persistence.JoinColumn; +import org.hibernate.boot.Metadata; +import org.hibernate.dialect.Dialect; +import org.hibernate.engine.spi.Mapping; +import org.hibernate.envers.internal.EnversMessageLogger; import org.hibernate.envers.internal.tools.StringTools; import org.hibernate.mapping.Column; import org.hibernate.mapping.Formula; @@ -18,12 +22,20 @@ import org.dom4j.Attribute; import org.dom4j.Document; import org.dom4j.Element; +import org.jboss.logging.Logger; + /** * @author Adam Warski (adam at warski dot org) * @author Lukasz Antoniak (lukasz dot antoniak at gmail dot com) * @author Michal Skowronek (mskowr at o2 dot pl) */ public final class MetadataTools { + + private static final EnversMessageLogger LOG = Logger.getMessageLogger( + EnversMessageLogger.class, + MetadataTools.class.getName() + ); + private MetadataTools() { } @@ -285,38 +297,68 @@ public final class MetadataTools { return joinMapping; } - public static void addColumns(Element anyMapping, Iterator selectables) { + public static void addColumns(Element anyMapping, Iterator selectables, Metadata metadata) { + addColumns( anyMapping, selectables, metadata, metadata.getDatabase().getDialect() ); + } + + public static void addColumns(Element anyMapping, Iterator selectables, Mapping mapping, Dialect dialect) { while ( selectables.hasNext() ) { final Selectable selectable = (Selectable) selectables.next(); if ( selectable.isFormula() ) { throw new FormulaNotSupportedException(); } - addColumn( anyMapping, (Column) selectable ); + addColumn( anyMapping, (Column) selectable, mapping, dialect ); } } /** - * Adds column element with the following attributes (unless empty): name, - * length, scale, precision, sql-type, read - * and write. + * Adds {@code column} element with the following attributes (unless empty): + * + * + * @param anyMapping parent element + * @param column column descriptor + * @param mapping the metadata mapping + * @param dialect the dialect */ - public static void addColumn(Element anyMapping, Column column) { + public static void addColumn(Element anyMapping, Column column, Mapping mapping, Dialect dialect) { addColumn( anyMapping, column.getName(), column.getLength(), column.getScale(), column.getPrecision(), - column.getSqlType(), + resolveSqlType( column, mapping, dialect ), column.getCustomRead(), column.getCustomWrite(), column.isQuoted() ); } + private static String resolveSqlType(Column column, Mapping mapping, Dialect dialect) { + String columnDefinition = column.getSqlType(); + if ( !StringTools.isEmpty( columnDefinition ) ) { + final int sqlTypeCode = column.getSqlTypeCode( mapping ); + final String sqlType = dialect.getTypeName( sqlTypeCode, column.getLength(), column.getPrecision(), column.getScale() ); + LOG.infof( + "Column [%s] uses a column-definition of [%s], resolved sql-type as [%s].", + column.getName(), + columnDefinition, + sqlType + ); + columnDefinition = sqlType; + } + return columnDefinition; + } + @SuppressWarnings({"unchecked"}) private static void changeNamesInColumnElement(Element element, ColumnNameIterator columnNameIterator) { final Iterator properties = element.elementIterator(); @@ -392,12 +434,13 @@ public final class MetadataTools { * @param element Parent element. * @param columnIterator Iterator pointing at {@link org.hibernate.mapping.Column} and/or * {@link org.hibernate.mapping.Formula} objects. + * @param metadata The boot-time entity model metadata */ - public static void addColumnsOrFormulas(Element element, Iterator columnIterator) { + public static void addColumnsOrFormulas(Element element, Iterator columnIterator, Metadata metadata) { while ( columnIterator.hasNext() ) { final Object o = columnIterator.next(); if ( o instanceof Column ) { - addColumn( element, (Column) o ); + addColumn( element, (Column) o, metadata, metadata.getDatabase().getDialect() ); } else if ( o instanceof Formula ) { addFormula( element, (Formula) o ); diff --git a/hibernate-envers/src/test/java/org/hibernate/envers/test/integration/basic/BasicTypeColumnDefinitionTest.java b/hibernate-envers/src/test/java/org/hibernate/envers/test/integration/basic/BasicTypeColumnDefinitionTest.java new file mode 100644 index 0000000000..25e78f3e6c --- /dev/null +++ b/hibernate-envers/src/test/java/org/hibernate/envers/test/integration/basic/BasicTypeColumnDefinitionTest.java @@ -0,0 +1,156 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.envers.test.integration.basic; + +import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.Id; + +import org.hibernate.annotations.Generated; +import org.hibernate.annotations.GenerationTime; +import org.hibernate.dialect.H2Dialect; +import org.hibernate.envers.Audited; +import org.hibernate.envers.test.BaseEnversJPAFunctionalTestCase; +import org.hibernate.envers.test.Priority; +import org.hibernate.mapping.Table; +import org.junit.Test; + +import org.hibernate.testing.RequiresDialect; +import org.hibernate.testing.TestForIssue; + +import static org.hibernate.boot.model.naming.Identifier.toIdentifier; +import static org.hibernate.mapping.Column.DEFAULT_LENGTH; +import static org.hibernate.mapping.Column.DEFAULT_PRECISION; +import static org.hibernate.mapping.Column.DEFAULT_SCALE; +import static org.hibernate.testing.transaction.TransactionUtil.doInJPA; +import static org.junit.Assert.assertEquals; + +/** + * This test verifies that resolving a column mapping's {@code sql-type} for HBM XML is performed + * correctly such that when a column supplies a {@code columnDefinition}, Envers properly builds + * its schema based on the right type rather than directly using the column definition as-is. + * + * The following illustrate some examples of expected transformations: + * + *
  • {@code @Column(columnDefinition = "varchar(10) not null")} => {@code sql-type = "varchar(255)"}
    • + *
  • {@code @Column(length = 10, columnDefinition = "varchar(10) not null")} => {@code sql-type = "varchar(10)"}
    • + *
  • {@code @Column(columnDefinition = "integer not null auto_increment")} => {@code sql-type = "integer"}
    • + * + * It is important to point out that resolving the sql-types length/precision/scale is all based on the + * values supplied as part of the {@link Column} annotation itself and not what is in the definition text. + * + * @author Chris Cranford + */ +@TestForIssue(jiraKey = "HHH-10844") +@RequiresDialect(value = H2Dialect.class) +public class BasicTypeColumnDefinitionTest extends BaseEnversJPAFunctionalTestCase { + @Override + protected Class[] getAnnotatedClasses() { + return new Class[] { BasicTypeContainer.class }; + } + + @Test + @Priority(10) + public void testMetadataBindings() { + final Table auditTable = metadata().getEntityBinding( BasicTypeContainer.class.getName() + "_AUD" ).getTable(); + + final org.hibernate.mapping.Column caseNumber = auditTable.getColumn( toIdentifier( "caseNumber" ) ); + assertEquals( "integer", caseNumber.getSqlType() ); + assertEquals( DEFAULT_LENGTH, caseNumber.getLength() ); + assertEquals( DEFAULT_PRECISION, caseNumber.getPrecision() ); + assertEquals( DEFAULT_SCALE, caseNumber.getScale() ); + + final org.hibernate.mapping.Column colDef = auditTable.getColumn( toIdentifier( "columnWithDefinition" ) ); + assertEquals( "varchar(10)", colDef.getSqlType() ); + assertEquals( 10, colDef.getLength() ); + assertEquals( DEFAULT_PRECISION, colDef.getPrecision() ); + assertEquals( DEFAULT_SCALE, colDef.getScale() ); + } + + @Test + @Priority(10) + public void initData() { + final BasicTypeContainer detachedEntity = doInJPA( this::entityManagerFactory, entityManager -> { + final BasicTypeContainer entity = new BasicTypeContainer(); + entity.setData( "test" ); + entity.setColumnWithDefinition( "1234567890" ); + entityManager.persist( entity ); + return entity; + } ); + + doInJPA( this::entityManagerFactory, entityManager -> { + final BasicTypeContainer entity = entityManager.find( BasicTypeContainer.class, detachedEntity.getId() ); + entity.setData( "test2" ); + entityManager.merge( entity ); + } ); + } + + @Test + public void testRevisionHistory() { + assertEquals( 2, getAuditReader().getRevisions( BasicTypeContainer.class, 1 ).size() ); + + final BasicTypeContainer rev1 = getAuditReader().find( BasicTypeContainer.class, 1, 1 ); + assertEquals( "test", rev1.getData() ); + assertEquals( "1234567890", rev1.getColumnWithDefinition() ); + assertEquals( Integer.valueOf( 1 ), rev1.getCaseNumber() ); + + final BasicTypeContainer rev2 = getAuditReader().find( BasicTypeContainer.class, 1, 2 ); + assertEquals( "test2", rev2.getData() ); + assertEquals( "1234567890", rev2.getColumnWithDefinition() ); + assertEquals( Integer.valueOf( 1 ), rev2.getCaseNumber() ); + } + + @Entity(name = "BasicTypeContainer") + @Audited + public static class BasicTypeContainer { + @Id + @GeneratedValue + private Integer id; + + @Generated(GenerationTime.INSERT) + @Column(name = "caseNumber", columnDefinition = "integer not null auto_increment") + private Integer caseNumber; + + @Column(name = "columnWithDefinition", length = 10, nullable = false, columnDefinition = "varchar(10) not null") + private String columnWithDefinition; + + private String data; + + public Integer getId() { + return id; + } + + public void setId(Integer id) { + this.id = id; + } + + public Integer getCaseNumber() { + return caseNumber; + } + + public void setCaseNumber(Integer caseNumber) { + this.caseNumber = caseNumber; + } + + public String getColumnWithDefinition() { + return columnWithDefinition; + } + + public void setColumnWithDefinition(String columnWithDefinition) { + this.columnWithDefinition = columnWithDefinition; + } + + public String getData() { + return data; + } + + public void setData(String data) { + this.data = data; + } + } +}