persistence@glassfish.java.net

Re: Issue 1454 and FieldDefinition.typeName?

From: Gordon Yorke <gordon.yorke_at_oracle.com>
Date: Mon, 12 Mar 2007 11:25:29 -0400
Wonseok, This looks good.
--Gordon


Wonseok Kim wrote:
Hi Tom, Gordon

I fixed the columnDefinition problem as the attached fix. Please review the fix.

The summary of fix:

FieldDefinition.java
* Add typeDefinition property which contains complete type definition string like "VARCHAR2(50) UNIQUE NOT NULL".
* Clarify the meaning of typeName - generic database type name which is translated based on platform.
* Modify appendDBString() to handle typeDefinition, typeName properly.

I removed FieldDefinition.appendTypeString () method which was mostly duplicate code with appendDBString() and was not being used anywhere. Let me know if this method is required.

TableDefinition.java
* Remove typeName -> type conversion code, which is now done in FieldDefinition.appendDBString() while generating type definition. Dynamic conversion is better way because it does not modify field values of user-given FieldDefinition internally.

DefaultTableGenerator.java
* The column definition value is now set to typeDefinition property instead of typeName of FieldDefinition.

I ran a local test which contains columnDefinition with MySQL and entity-persistence-tests. The entity-persistence unit testsuite is hitting 2 failures on Derby, but this is not the result of this fix as I said in a separate e-mail.

Thanks,
-Wonseok

On 3/10/07, Gordon Yorke <gordon.yorke@oracle.com> wrote:
Hello Wonseok,
    I'm not sure if Tom responded to you or not I haven't received the email through the persistence mailing list yet.
    typeName in TopLink is used as a place for a user to define definitions using generic database types (well mostly generic).  Those types are then translated to a particular database type based on platform as you mentioned.  This code is from the original implementation of the TopLink schema framework before ddl generation was written.  I think the issue here is that columnDefinition is being stored in the typeName attribute.  ColumnDefinition should really have its own property that overrides both type and typeName.
--Gordon
-----Original Message-----
From: Wonseok Kim [mailto:guruwons@gmail.com]
Sent: Friday, March 09, 2007 8:05 AM
To: persistence@glassfish.dev.java.net
Subject: Issue 1454 and FieldDefinition.typeName?

Hi Tom,

I'm looking into issue 1454 columnDefinition="TEXT" does not apply in table generation.
I'm fixing it, but I'm having problem in understanding the exact meaning of typeName field in FieldDefinition class.

DatabaseField has 'columnDefinition' field corresponding to user-defined column definition, and which is set to FieldDefinition's typeName(in this case FieldDefinition's type is null).
But as you see below code, the typeName is used to set its 'type' field and it is set to null!

TableDefinition.java
    /**
     * INTERNAL:
     * Build the field types based on the given name read in from the file.
     * Build the foriegn key constraints as well.
     */
    protected void buildFieldTypes(AbstractSession session) {        
        buildForeignFieldTypes(session);
    }

    private void buildForeignFieldTypes(final AbstractSession session) {
        Hashtable fieldTypes = session.getPlatform().getClassTypes();
        FieldDefinition field = null;
        
        // The ForeignKeyConstraint object is the newer way of doing things.
        // We support FieldDefinition.getForeignKeyFieldName() due to backwards compatibility
        // by converting it. To allow mixing both ways, we just add converted one to foreignKeys list.
        
        for (Enumeration enumtr = getFields().elements(); enumtr.hasMoreElements();) {
            field = (FieldDefinition)enumtr.nextElement();
            if (field.getForeignKeyFieldName() != null) {
                addForeignKeyConstraint(buildForeignKeyConstraint(field, session.getPlatform()));

            }
//FROM HERE - It's removing typeName(columnDefinition)!
            if ( field.getType() == null) {
                field.setType((Class)fieldTypes.get(field.getTypeName()));
                if (field.getType() != null) {
                    field.setTypeName(null);
                }
            }
        }

    }

This does not keep user-defined column definition. I thought the code is wrong and removed it. With some other changes, I could see the user-specified column definition is used properly.
But, I found that the code is used for another purpose in entity-persistence-tests. EPT is directly using FieldDefinition to generate tables like below.

        FieldDefinition fieldSTREET = new FieldDefinition();
        fieldSTREET.setName("STREET");
        fieldSTREET.setTypeName("VARCHAR2");
        fieldSTREET.setSize(60);
        fieldSTREET.setSubSize(0);
        fieldSTREET.setIsPrimaryKey(false);
        fieldSTREET.setIsIdentity(false);
        fieldSTREET.setUnique(false);
        fieldSTREET.setShouldAllowNull(true);
        table.addField(fieldSTREET);

As you know typeName "VARCHAR2" is only for Oracle, so it could not be used for other databases. The above type-related code is converting this 'typeName' to corresponding Java type which is converted to real database type later according to DatabasePlatform.

What is the exact meaning of 'typeName'?
If it is type definition analogous to @Column.columnDefinition, then tableCreators of EPT is abusing it - instead setType() should be used to be portable.
If it should be translated according to DatabasePlatform, it should not be used for @ Column.columnDefinition.

Could you explain?

Thanks
-Wonseok


Index: entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/DefaultTableGenerator.java =================================================================== RCS file: /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/DefaultTableGenerator.java,v retrieving revision 1.14 diff -c -w -r1.14 DefaultTableGenerator.java *** entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/DefaultTableGenerator.java 7 Mar 2007 02:26:57 -0000 1.14 --- entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/DefaultTableGenerator.java 12 Mar 2007 05:10:39 -0000 *************** *** 496,502 **** if (dbField.getColumnDefinition().length() > 0) { // This column definition would include the complete definition of the // column like type, size, "NULL/NOT NULL" clause, unique key clause ! fieldDef.setTypeName(dbField.getColumnDefinition()); } else { Class fieldType = dbField.getType(); --- 496,502 ---- if (dbField.getColumnDefinition().length() > 0) { // This column definition would include the complete definition of the // column like type, size, "NULL/NOT NULL" clause, unique key clause ! fieldDef.setTypeDefinition(dbField.getColumnDefinition()); } else { Class fieldType = dbField.getType(); Index: entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/FieldDefinition.java =================================================================== RCS file: /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/FieldDefinition.java,v retrieving revision 1.5 diff -c -w -r1.5 FieldDefinition.java *** entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/FieldDefinition.java 4 Jan 2007 14:32:10 -0000 1.5 --- entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/FieldDefinition.java 12 Mar 2007 05:10:39 -0000 *************** *** 22,27 **** --- 22,29 ---- package oracle.toplink.essentials.tools.schemaframework; import java.io.*; + import java.util.Hashtable; + import oracle.toplink.essentials.internal.helper.*; import oracle.toplink.essentials.internal.databaseaccess.*; import oracle.toplink.essentials.exceptions.*; *************** *** 40,47 **** --- 42,62 ---- */ public class FieldDefinition implements Serializable, Cloneable { protected String name; + /** + * Java type class for the field. + * Particular database type is generated based on platform from this. + */ protected Class type; + /** + * Generic database type name for the field, which can be used instead of the Java class 'type'. + * This is translated to a particular database type based on platform. + */ protected String typeName; + /** + * Database-specific complete type definition like "VARCHAR2(50) UNIQUE NOT NULL". + * If this is given, other additional type constraint fields(size, unique, null) are meaningless. + */ + protected String typeDefinition; protected int size; protected int subSize; protected boolean shouldAllowNull; *************** *** 100,117 **** * Append the database field definition string to the table creation statement. */ public void appendDBString(Writer writer, AbstractSession session, TableDefinition table) throws ValidationException { FieldTypeDefinition fieldType; ! if (getType() != null) { fieldType = session.getPlatform().getFieldTypeDefinition(getType()); if (fieldType == null) { throw ValidationException.javaTypeIsNotAValidDatabaseType(getType()); } ! } else { fieldType = new FieldTypeDefinition(getTypeName()); } ! try { ! writer.write(getName()); ! writer.write(" "); String qualifiedName = table.getFullName() + '.' + getName(); session.getPlatform().printFieldTypeSize(writer, this, fieldType, session, qualifiedName); session.getPlatform().printFieldUnique(writer, isUnique(), session, qualifiedName); --- 115,152 ---- * Append the database field definition string to the table creation statement. */ public void appendDBString(Writer writer, AbstractSession session, TableDefinition table) throws ValidationException { + try { + writer.write(getName()); + writer.write(" "); + + if (getTypeDefinition() != null) { //apply user-defined complete type definition + writer.write(getTypeDefinition()); + + } else { + // compose type definition - type name, size, unique, identity, constraints... FieldTypeDefinition fieldType; ! ! if (getType() != null) { //translate Java 'type' fieldType = session.getPlatform().getFieldTypeDefinition(getType()); if (fieldType == null) { throw ValidationException.javaTypeIsNotAValidDatabaseType(getType()); } ! } else if (getTypeName() != null) { //translate generic type name ! Hashtable fieldTypes = session.getPlatform().getClassTypes(); ! Class type = (Class)fieldTypes.get(getTypeName()); ! if (type == null) { // if unknown type name, use as it is fieldType = new FieldTypeDefinition(getTypeName()); + } else { + fieldType = session.getPlatform().getFieldTypeDefinition(type); + if (fieldType == null) { + throw ValidationException.javaTypeIsNotAValidDatabaseType(type); } ! } ! } else { ! // both type and typeName is null ! throw ValidationException.javaTypeIsNotAValidDatabaseType(null); ! } ! String qualifiedName = table.getFullName() + '.' + getName(); session.getPlatform().printFieldTypeSize(writer, this, fieldType, session, qualifiedName); session.getPlatform().printFieldUnique(writer, isUnique(), session, qualifiedName); *************** *** 130,177 **** if (getAdditional() != null) { writer.write(" " + getAdditional()); } - } catch (IOException ioException) { - throw ValidationException.fileError(ioException); - } - } - - /** - * INTERNAL: - * Append the database field definition string to the type creation statement. - * Types do not support constraints. - */ - public void appendTypeString(Writer writer, AbstractSession session) throws ValidationException { - FieldTypeDefinition fieldType; - if (getType() != null) { - fieldType = session.getPlatform().getFieldTypeDefinition(getType()); - if (fieldType == null) { - throw ValidationException.javaTypeIsNotAValidDatabaseType(getType()); - } - } else { - fieldType = new FieldTypeDefinition(getTypeName()); - } - try { - writer.write(getName()); - writer.write(" "); - writer.write(fieldType.getName()); - if ((fieldType.isSizeAllowed()) && ((getSize() != 0) || (fieldType.isSizeRequired()))) { - writer.write("("); - if (getSize() == 0) { - writer.write(new Integer(fieldType.getDefaultSize()).toString()); - } else { - writer.write(new Integer(getSize()).toString()); - } - if (getSubSize() != 0) { - writer.write(","); - writer.write(new Integer(getSubSize()).toString()); - } else if (fieldType.getDefaultSubSize() != 0) { - writer.write(","); - writer.write(new Integer(fieldType.getDefaultSubSize()).toString()); - } - writer.write(")"); - } - if (getAdditional() != null) { - writer.write(" " + getAdditional()); } } catch (IOException ioException) { throw ValidationException.fileError(ioException); --- 165,170 ---- *************** *** 251,258 **** /** * PUBLIC: ! * Return the type of the field. ! * This is the exact DB type name, which can be used instead of the Java class. */ public String getTypeName() { return typeName; --- 244,252 ---- /** * PUBLIC: ! * Return the type name of the field. ! * This is the generic database type name, which can be used instead of the Java class 'type'. ! * This is translated to a particular database type based on platform. */ public String getTypeName() { return typeName; *************** *** 260,265 **** --- 254,269 ---- /** * PUBLIC: + * Return the type definition of the field. + * This is database-specific complete type definition like "VARCHAR2(50) UNIQUE NOT NULL". + * If this is given, other additional type constraint fields(size, unique, null) are meaningless. + */ + public String getTypeDefinition() { + return typeDefinition; + } + + /** + * PUBLIC: * Answer whether the receiver is an identity field. * Identity fields are Sybase specific, * they insure that on insert a unique sequencial value is store in the row. *************** *** 382,392 **** /** * PUBLIC: ! * Set the type of the field. ! * This is the exact DB type name, which can be used instead of the Java class. */ public void setTypeName(String typeName) { this.typeName = typeName; } /** --- 386,407 ---- /** * PUBLIC: ! * Set the type name of the field. ! * This is the generic database type name, which can be used instead of the Java class 'type'. ! * This is translated to a particular database type based on platform. */ public void setTypeName(String typeName) { this.typeName = typeName; + } + + /** + * PUBLIC: + * Set the type definition of the field. + * This is database-specific complete type definition like "VARCHAR2(50) UNIQUE NOT NULL". + * If this is given, other additional type constraint fields(size, unique, null) are meaningless. + */ + public void setTypeDefinition(String typeDefinition) { + this.typeDefinition = typeDefinition; } /** Index: entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/TableDefinition.java =================================================================== RCS file: /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/TableDefinition.java,v retrieving revision 1.8 diff -c -w -r1.8 TableDefinition.java *** entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/TableDefinition.java 4 Jan 2007 14:32:11 -0000 1.8 --- entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/TableDefinition.java 12 Mar 2007 05:10:39 -0000 *************** *** 357,371 **** /** * INTERNAL: ! * Build the field types based on the given name read in from the file. ! * Build the foriegn key constraintsas well. */ protected void buildFieldTypes(AbstractSession session) { - buildForeignFieldTypes(session); - } - - private void buildForeignFieldTypes(final AbstractSession session) { - Hashtable fieldTypes = session.getPlatform().getClassTypes(); FieldDefinition field = null; // The ForeignKeyConstraint object is the newer way of doing things. --- 357,365 ---- /** * INTERNAL: ! * Build the foriegn key constraints. */ protected void buildFieldTypes(AbstractSession session) { FieldDefinition field = null; // The ForeignKeyConstraint object is the newer way of doing things. *************** *** 376,388 **** field = (FieldDefinition)enumtr.nextElement(); if (field.getForeignKeyFieldName() != null) { addForeignKeyConstraint(buildForeignKeyConstraint(field, session.getPlatform())); - - } - if (field.getType() == null) { - field.setType((Class)fieldTypes.get(field.getTypeName())); - if (field.getType() != null) { - field.setTypeName(null); - } } } --- 370,375 ----