Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HHH-16531 HHH-18004 ColumnDefinitions should respect @Column(columnDefinition) #8137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+188 −19
Open

HHH-16531 HHH-18004 ColumnDefinitions should respect @Column(columnDefinition) #8137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public class AnnotatedColumn {
private boolean updatable = true;
private String explicitTableName; // the JPA @Column annotation lets you specify a table name
private boolean isImplicit;
public String sqlType;
private String columnDefinition;
protected String sqlType;
private Long length;
private Integer precision;
private Integer scale;
Expand Down Expand Up @@ -109,6 +110,10 @@ public String getLogicalColumnName() {
return logicalColumnName;
}

public String getColumnDefinition() {
return columnDefinition;
}

public String getSqlType() {
return sqlType;
}
Expand Down Expand Up @@ -169,6 +174,10 @@ public void setImplicit(boolean implicit) {
isImplicit = implicit;
}

public void setColumnDefinition(String columnDefinition) {
this.columnDefinition = nullIfEmpty(columnDefinition);
}

public void setSqlType(String sqlType) {
this.sqlType = sqlType;
}
Expand Down Expand Up @@ -290,6 +299,7 @@ protected void initMappingColumn(
mappingColumn = new Column();
mappingColumn.setExplicit( !isImplicit );
redefineColumnName( columnName, propertyName, applyNamingStrategy );
mappingColumn.setColumnDefinition( columnDefinition );
mappingColumn.setLength( length );
if ( precision != null && precision > 0 ) { //relevant precision
mappingColumn.setPrecision( precision );
Expand Down Expand Up @@ -769,6 +779,7 @@ private static AnnotatedColumn buildColumn(
final AnnotatedColumn annotatedColumn = new AnnotatedColumn();
annotatedColumn.setLogicalColumnName( columnName );
annotatedColumn.setImplicit( false );
annotatedColumn.setColumnDefinition( column.columnDefinition() );
annotatedColumn.setSqlType( sqlType );
annotatedColumn.setLength( (long) column.length() );
if ( fractionalSeconds != null ) {
Expand Down
1 change: 1 addition & 0 deletions ...te-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedDiscriminatorColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ else if ( discriminatorColumn != null ) {
discriminatorType = discriminatorColumn.discriminatorType();
column.setImplicit( false );
if ( !discriminatorColumn.columnDefinition().isEmpty() ) {
column.setColumnDefinition( discriminatorColumn.columnDefinition() );
column.setSqlType( discriminatorColumn.columnDefinition() );
}
if ( !discriminatorColumn.name().isEmpty() ) {
Expand Down
2 changes: 2 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedJoinColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ public void applyJoinAnnotation(JoinColumn joinColumn, String defaultName) {
else {
setImplicit( false );
if ( !joinColumn.columnDefinition().isEmpty() ) {
setColumnDefinition( joinColumn.columnDefinition() );
setSqlType( getBuildingContext().getObjectNameNormalizer()
.applyGlobalQuoting( joinColumn.columnDefinition() ) );
}
Expand Down Expand Up @@ -426,6 +427,7 @@ public void overrideFromReferencedColumnIfNecessary(Column column) {
}

// these properties can only be applied on the referenced column - we can just take them over
mappingColumn.setColumnDefinition( column.getColumnDefinition() );
mappingColumn.setLength( column.getLength() );
mappingColumn.setPrecision( column.getPrecision() );
mappingColumn.setScale( column.getScale() );
Expand Down
1 change: 1 addition & 0 deletions ...re/src/main/java/org/hibernate/boot/model/internal/CopyIdentifierComponentSecondPass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ private Property createSimpleProperty(

private void applyComponentColumnSizeValueToJoinColumn(Column column, AnnotatedJoinColumn joinColumn) {
final Column mappingColumn = joinColumn.getMappingColumn();
mappingColumn.setColumnDefinition( column.getColumnDefinition() );
mappingColumn.setLength( column.getLength() );
mappingColumn.setPrecision( column.getPrecision() );
mappingColumn.setScale( column.getScale() );
Expand Down
1 change: 1 addition & 0 deletions hibernate-core/src/main/java/org/hibernate/boot/model/internal/IndexColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ public static IndexColumn buildColumnFromAnnotation(
: orderColumn.name();
final IndexColumn column = new IndexColumn();
column.setLogicalColumnName( name );
column.setColumnDefinition( orderColumn.columnDefinition() );
column.setSqlType( sqlType );
column.setNullable( orderColumn.nullable() );
// column.setJoins( secondaryTables );
Expand Down
1 change: 1 addition & 0 deletions hibernate-core/src/main/java/org/hibernate/mapping/AggregateColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class AggregateColumn extends Column {
private final Component component;

public AggregateColumn(Column column, Component component) {
setColumnDefinition( column.getColumnDefinition() );
setLength( column.getLength() );
setPrecision( column.getPrecision() );
setScale( column.getScale() );
Expand Down
9 changes: 9 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/mapping/Column.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
*/
public class Column implements Selectable, Serializable, Cloneable, ColumnTypeInformation {

private String columnDefinition; // column definition specified by user
private Long length;
private Integer precision;
private Integer scale;
Expand Down Expand Up @@ -88,6 +89,14 @@ public Column(String columnName) {
setName( columnName );
}

public String getColumnDefinition() {
return columnDefinition;
}

public void setColumnDefinition(String columnDefinition) {
this.columnDefinition = columnDefinition;
}

public Long getLength() {
return length;
}
Expand Down
1 change: 1 addition & 0 deletions hibernate-core/src/main/java/org/hibernate/mapping/ForeignKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ public void alignColumns() {
for ( int i = 0; i<columnSpan; i++ ) {
Column referencedColumn = primaryKey.getColumn(i);
Column referencingColumn = getColumn(i);
referencingColumn.setColumnDefinition( referencingColumn.getColumnDefinition() );
referencingColumn.setLength( referencedColumn.getLength() );
referencingColumn.setScale( referencedColumn.getScale() );
referencingColumn.setPrecision( referencedColumn.getPrecision() );
Expand Down
51 changes: 48 additions & 3 deletions hibernate-core/src/main/java/org/hibernate/tool/schema/internal/ColumnDefinitions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.hibernate.boot.Metadata;
import org.hibernate.boot.model.relational.SqlStringGenerationContext;
import org.hibernate.dialect.Dialect;
import org.hibernate.dialect.OracleDialect;
import org.hibernate.engine.jdbc.Size;
import org.hibernate.mapping.CheckConstraint;
import org.hibernate.mapping.Column;
Expand All @@ -27,8 +28,11 @@
class ColumnDefinitions {

static boolean hasMatchingType(Column column, ColumnInformation columnInformation, Metadata metadata, Dialect dialect) {
if ( column.getColumnDefinition() != null ) {
return isExpected( column.getColumnDefinition(), columnInformation, dialect );
}
gavinking marked this conversation as resolved.
Show resolved Hide resolved
final boolean typesMatch = dialect.equivalentTypes( column.getSqlTypeCode( metadata ), columnInformation.getTypeCode() )
|| normalize( stripArgs( column.getSqlType( metadata ) ) ).equals( normalize( columnInformation.getTypeName() ) );
|| isExpected( column.getSqlType( metadata ), columnInformation, dialect );
if ( typesMatch ) {
return true;
}
Expand Down Expand Up @@ -92,8 +96,11 @@ static String getFullColumnDeclaration(
return definition.toString();
}


static String getColumnDefinition(Column column, Metadata metadata, Dialect dialect) {
String explicitColumnDefinition = column.getColumnDefinition();
if ( explicitColumnDefinition != null ) {
return explicitColumnDefinition;
}
StringBuilder definition = new StringBuilder();
appendColumnDefinition( definition, column, metadata, dialect );
appendComment( definition, column, dialect );
Expand Down Expand Up @@ -227,6 +234,8 @@ private static String normalize(String typeName) {
switch (lowercaseTypeName) {
case "int":
return "integer";
quaff marked this conversation as resolved.
Show resolved Hide resolved
case "int unsigned":
return "integer unsigned";
case "character":
return "char";
case "character varying":
Expand All @@ -249,13 +258,49 @@ private static String normalize(String typeName) {
}
}

private static boolean isExpected(String columnDefinition, ColumnInformation columnInformation, Dialect dialect) {
String expectingTypeName = extractTypeName( columnDefinition );
if ( dialect instanceof OracleDialect && expectingTypeName.equals( "timestamp with time zone" ) ) {
// Fix: found [timestamp (Types#UNKNOWN(-101))], but expecting [timestamp(6) with time zone (Types#TIMESTAMP_UTC)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works fine previously because stripArgs just keep string before (, now it drop (*) for keep unsigned from int(6) unsigned.

expectingTypeName = "timestamp";
}
return expectingTypeName.equals( normalize( columnInformation.getTypeName() ) );
}

static String extractTypeName(String typeExpression) {
String sqlType = extractType(typeExpression);
String typeName = stripArgs( sqlType );
typeName = normalize( typeName );
return typeName;
}

static String extractType(String typeExpression) {
String sqlType = typeExpression.toLowerCase(Locale.ROOT);
sqlType = keepBefore( sqlType, " generated " ); // Strip generated value
sqlType = keepBefore( sqlType, " as " ); // Strip generated value
sqlType = keepBefore( sqlType, " primary " );
sqlType = keepBefore( sqlType, " unique " );
sqlType = keepBefore( sqlType, " key " );
sqlType = keepBefore( sqlType, " check " );
sqlType = keepBefore( sqlType, " default " ); // Strip default value
sqlType = keepBefore( sqlType, " not null" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, there really are a lot more possibilities than just default and not null. What about generated? What about unique? What about constraint? primary key? etc, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a really bad idea that columnDefinition allow mixing type and constrains, what's your suggestion here? writing a sophisticated parser or simply using substring to cover that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I 100% agree that it's bad. columnDefinition was a TopLink thing that I never really liked but at the time of JPA 1.0 didn't have a good reason to object to.

As I've said, we need to support it because it's in the spec, but there's no requirement that migration/validation handle it elegantly.

sqlType = keepBefore( sqlType, " null" );
sqlType = keepBefore( sqlType, " comment " );
return sqlType.trim();
}

private static String keepBefore(String input, String token) {
int i = input.indexOf( token );
return i > 0 ? input.substring( 0, i ).trim() : input;
}

private static String stripArgs(String typeExpression) {
if ( typeExpression == null ) {
return null;
}
else {
int i = typeExpression.indexOf('(');
return i>0 ? typeExpression.substring(0,i).trim() : typeExpression;
return i>0 ? typeExpression.replaceFirst( "\\(.*\\)", "" ).trim() : typeExpression;
}
}
}
4 changes: 3 additions & 1 deletion hibernate-core/src/main/java/org/hibernate/tool/schema/internal/StandardTableMigrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.List;

import static org.hibernate.internal.util.collections.ArrayHelper.EMPTY_STRING_ARRAY;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.extractType;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.getColumnDefinition;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.hasMatchingLength;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.hasMatchingType;
Expand Down Expand Up @@ -93,9 +94,10 @@ public static List<String> sqlAlterStrings(
else if ( dialect.supportsAlterColumnType() ) {
if ( !hasMatchingType( column, columnInformation, metadata, dialect )
|| !hasMatchingLength( column, columnInformation, metadata, dialect ) ) {
final String explicitColumnDefinition = column.getColumnDefinition();
final String alterColumn = dialect.getAlterColumnTypeString(
column.getQuotedName( dialect ),
column.getSqlType(metadata),
explicitColumnDefinition != null ? extractType( explicitColumnDefinition ) : column.getSqlType(metadata),
getColumnDefinition( column, metadata, dialect )
);
results.add( alterTable + alterColumn );
Expand Down
19 changes: 5 additions & 14 deletions ...nate-core/src/test/java/org/hibernate/orm/test/tool/schema/MySQLColumnValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,19 +194,10 @@ public SchemaFilter getSchemaFilter() {
);
}

try {
schemaValidator.doValidation( scope.getMetadataImplementor(), executionOptions,
contributed -> {
return "test_data3".equalsIgnoreCase( contributed.getExportIdentifier() );
} );
fail( "SchemaManagementException expected" );
}
catch (SchemaManagementException e) {
assertEquals(
"Schema-validation: wrong column type encountered in column [integral1] in table [TEST_DATA3]; found [tinyint unsigned (Types#TINYINT)], but expecting [tinyint unsigned default '0' (Types#INTEGER)]",
e.getMessage()
);
}
schemaValidator.doValidation( scope.getMetadataImplementor(), executionOptions,
contributed -> {
return "test_data3".equalsIgnoreCase( contributed.getExportIdentifier() );
} );
}

@Entity(name = "test_entity1")
Expand All @@ -217,7 +208,7 @@ public static class TestEntity1 {
private Integer id;

@JdbcTypeCode( Types.TINYINT )
@Column(name = "integral1", columnDefinition = "tinyint")
@Column(name = "integral1", columnDefinition = "tinyint UNSIGNED")
private int integral1;

@JdbcTypeCode( Types.TINYINT )
Expand Down
104 changes: 104 additions & 0 deletions hibernate-core/src/test/java/org/hibernate/tool/schema/internal/ColumnDefinitionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* 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 <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.tool.schema.internal;

import org.hibernate.boot.Metadata;
import org.hibernate.boot.MetadataSources;
import org.hibernate.boot.model.TruthValue;
import org.hibernate.mapping.Column;
import org.hibernate.mapping.Value;
import org.hibernate.testing.util.ServiceRegistryUtil;
import org.hibernate.tool.schema.extract.internal.ColumnInformationImpl;
import org.hibernate.tool.schema.extract.spi.ColumnInformation;
import org.hibernate.type.JavaObjectType;
import org.hibernate.type.SqlTypes;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.extractType;
import static org.hibernate.tool.schema.internal.ColumnDefinitions.extractTypeName;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;

/**
* @author Yanming Zhou
*/
public class ColumnDefinitionsTest {

@Test
public void testExtractType() {
assertThat(extractType("varchar(255)"), is("varchar(255)"));
assertThat(extractType("varchar(255) primary key"), is("varchar(255)"));
assertThat(extractType("varchar(255) unique key"), is("varchar(255)"));
assertThat(extractType("varchar(255) null"), is("varchar(255)"));
assertThat(extractType("varchar(255) not null"), is("varchar(255)"));
assertThat(extractType("varchar(255) generated as 'test' not null"), is("varchar(255)"));
assertThat(extractType("varchar(255) default 'test' not null"), is("varchar(255)"));
assertThat(extractType("varchar(255) comment 'test'"), is("varchar(255)"));
assertThat(extractType("varchar(255) check ()"), is("varchar(255)"));
}

@Test
public void testExtractTypeName() {
assertThat(extractTypeName("INT(10) NOT NULL"), is("integer"));
assertThat(extractTypeName("INT(10) UNSIGNED NOT NULL"), is("integer unsigned"));
}

@Test
public void matchIntegerType() {
assertHasMatchingType("integer", SqlTypes.INTEGER, "integer", SqlTypes.INTEGER, true);
assertHasMatchingType("integer not null", SqlTypes.INTEGER, "integer", SqlTypes.INTEGER, true);
assertHasMatchingType("integer default 0", SqlTypes.INTEGER, "integer", SqlTypes.INTEGER, true);
assertHasMatchingType("integer not null default 0", SqlTypes.INTEGER, "integer", SqlTypes.INTEGER, true);
assertHasMatchingType("integer", SqlTypes.INTEGER, "int", SqlTypes.INTEGER, true);
assertHasMatchingType("int", SqlTypes.INTEGER, "integer", SqlTypes.INTEGER, true);
assertHasMatchingType("integer", SqlTypes.INTEGER, "bigint", SqlTypes.BIGINT, false);
assertHasMatchingType("bigint", SqlTypes.BIGINT, "integer", SqlTypes.INTEGER, false);
}

@Test
public void matchDecimalType() {
assertHasMatchingType("decimal(10,2)", SqlTypes.DECIMAL, "decimal", SqlTypes.DECIMAL, true);
assertHasMatchingType("decimal( 10 , 2 )", SqlTypes.DECIMAL, "decimal", SqlTypes.DECIMAL, true);
}

@Test
public void matchComplexCharacterVaryingType() {
assertHasMatchingType("character varying(255) not null default '-'", SqlTypes.VARCHAR, "varchar", SqlTypes.VARCHAR, true);
}

@Test
public void shouldNotMatchIfColumnDefinitionNotEqualsToActualTypeNameButInferredTypeCodeEqualsToActualTypeCode() {
assertHasMatchingType("integer not null default 0", SqlTypes.BIGINT, "bigint", SqlTypes.BIGINT, false);
}

private void assertHasMatchingType(String columnDefinition, int inferredTypeCode, String actualTypeName, int actualTypeCode, boolean matching) {
Column column = new Column();
Value value = mock();
given(value.getType()).willReturn(JavaObjectType.INSTANCE);
column.setValue(value);
column.setColumnDefinition(columnDefinition);
column.setSqlType(columnDefinition);
column.setSqlTypeCode(inferredTypeCode);

ColumnInformation columnInformation = new ColumnInformationImpl(
null,
null,
actualTypeCode,
actualTypeName,
255,
0,
TruthValue.TRUE
);

Metadata metadata = new MetadataSources( ServiceRegistryUtil.serviceRegistry() ).buildMetadata();
assertThat(ColumnDefinitions.hasMatchingType(column, columnInformation, metadata, metadata.getDatabase().getDialect()),
is(matching));
}

}