Hi Wonseok,
A couple of comments inline.
Wonseok Kim wrote:
> Hi Tom, TopLink team,
>
> Could you review this fix(attached) for issue 1072?
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1072
>
> Here is the summary,
> * DefaultTableGenerator makes use of ForeignKeyConstraint object
> instead of FieldDefinition.foreginKeyFieldName to support a composite key.
I have some questions and comments about the addForeignKeyConstraint()
method.
- Although TopLink Essentials code does not tend to use the "final"
keyword in argument names, we tend to try to avoid directly modifying
the arguments passed into the method unless it is necessary. It looks
like the fkFields argument is modified throughout the method. Would it
be possible to make use of a local variable to do this?
- Will the addForeignKeyConstraint() method work if the foreign keys do
not map exactly to the primary keys of the target table? Will it work
if they, instead map to some candidate keys, or if they map to only part
of the primary key table? Perhaps I am missing something here and I
just need some explantation.
> * Deprecated FieldDefinition.foreginKeyFieldName.
> * When making ForeignKeyConstraint the key order is reordered as the
> primary key order of target table - if the order is different,
> databases don't accept it.
> * Changed the data structure of foreign keys in TableDefinition from
> Vector to Map to avoid same foreign key constraint being added - this
> occurs for bidirectional relationships.
I am concerned about this change. It does not appear to be completely
implemented. TableDefinition.getForeignKeys() and
TableDefinition.setForeignKeys() still operate on vectors. I think we
need to choose. Either this data structure is dealt with as a Vector or
as a HashMap.
>
> This contains also a fix for issue 1392 - Wrong primary key constraint
> if one-to-one and many-to-many relationships have same join column
> name. It is included because it is simple fix. See
> DefaultTableGenerator.resolveDatabaseField().
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1392
>
> For tests,
> I would like to add separate models for ddl generation purpose.
> * Added a new models in new
> oracle.toplink.essentials.testing.models.cmp3.ddlgeneration package.
> * Added a new test DDLGenerationJUnitTestSuite which now contains a
> test for issue 1392.
> * DDL generation doesn't throw exceptions, so I checked logs manually.
> * Tested single field foreign key constraint by running ddl generation
> for entity-persistence-tests models. There was no failure during ddl
> generation.
>
> Updating TableCreators to use ForeignKeyConstraint object instead of
> deprecated foreignKeyFieldName will be done after review, how about
> making this another enhancement issue?
>
> Regards,
> -Wonseok
>
> On 10/31/06, *Tom Ware* <tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> Hi Wonseok,
>
> Sorry for not replying sooner. I was out of the office for the last
> few days.
>
> First, a brief explanation:
>
> There are some historical reasons in TopLink why both
> foreignKeyFieldName and the ForeignKeyConstraint object exist. The
> ForeignKeyConstraintObject is the newer way of doing things, but
> due to
> backwards compatibility requirements we have chosen to keep both
> ways of
> doing things. In the past, some of the tooling that shipped with
> TopLink relied on the foreignKeyFieldName. I am in the process of
> determining if that is still the case and I will let you know the
> result.
>
> I think the ideal approach to this problem is as follows:
>
> 1. deprecate setForeignKeyFieldName and getForeignKeyFieldName on
> FieldDefinition and update DefaultTableGenerator to make use of
> ForeignKeyConstraint instead.
> 2. Make the changes you suggest below to allow both types of foreign
> keys to be used TableDefinition.buildForeignFieldTypes ().
> 3. Update the entity-persistence-tests to use the non-deprecated
> API. I
> realize this is a bit tedious, but I think we need to make that change
> in order to completely fix the issue.
>
> I assume there are more changes required to fix issue 1072.
>
> As for the use of Generics in the schema generation code, the simple
> answer is that you may now use generics in that code. For those that
> have been working on the TopLink Essentials code for some time, a
> bit of
> further explanation is required. Until fairly recently, we only
> allowed
> Java 5-specific code in a few of our packages due to a requirement to
> support core TopLink functionality on JDK 1.4. That requirement no
> longer exists and as a result, Java 5 code can now appear in our core
> packages.
>
> -Tom
>
> Wonseok Kim wrote:
>
> > TopLink Team, please shed light on this so I can go ahead.
> > I added some comments in the below,
> >
> > On 10/26/06, *Wonseok Kim* < guruwons_at_gmail.com
> <mailto:guruwons_at_gmail.com>
> > <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>>> wrote:
> >
> > Hi Tom,
> >
> > I'm trying to fix issue 1072 - ddl generation of composite
> foreign
> > key constraints.
> > I think the ForeignKeyConstraints class is the right one to
> use if
> > composite foreign key, but there is another foreign key
> setting in
> > FieldDefinition.
> >
> > public class FieldDefinition implements Serializable,
> Cloneable {
> > ...
> > protected String foreignKeyFieldName;//fully-qualified
> foreign
> > key field name
> >
> > But current code shows that if above foreignKeyFieldName is set,
> > it ignores the TableDefinition.foreignKeys
> > (ForeignKeyConstraints). See below.
> >
> > TableDefinition:
> > private void buildForeignFieldTypes(final AbstractSession
> > session) {
> > Hashtable fieldTypes =
> session.getPlatform().getClassTypes();
> > FieldDefinition field = null;
> >
> > Vector foreignKeysClone =
> > (Vector)getForeignKeys().clone();
> > setForeignKeys(new Vector());
> > for (Enumeration enumtr = getFields().elements();
> > enumtr.hasMoreElements();) {
> > field = (FieldDefinition)enumtr.nextElement();
> > if (field.getForeignKeyFieldName () != null) {
> >
> > addForeignKeyConstraint(buildForeignKeyConstraint(field, this,
> > session.getPlatform()));
> >
> > }
> > if (field.getType() == null) {
> > field.setType ((Class)fieldTypes.get(
> > field.getTypeName ()));
> > if (field.getType() != null) {
> > field.setTypeName(null);
> > }
> > }
> > }
> >
> > /* bug #2997188 constraints not getting generated when
> > creating/replacing tables via TableCreator
> > had to add the next few lines instead of removing the
> > above code for backwards compatibility */
> > if (getForeignKeys().isEmpty()) {
> > //if foreignKeys is empty then we know we are
> not in 2.5
> > setForeignKeys(foreignKeysClone);
> > }
> > }
> >
> > I'm curious, should we use either foreignKeyFieldName or
> > foreignKeys (ForeignKeyConstraints)? We could not mix both of
> > them? Is there any reason for not allowing this?
> > entity-persistence-tests table creators are using only
> > foreignKeyFieldName now, but If I would like to add composite
> > foreign key constraints, how can I mix them?
> >
> >
> > If there is no problem, I would like to support for both
> > ForeignKeyConstraint object and foreignKeyFieldName by removing
> > setForeignKeys(new Vector()) line and below lines.
> >
> > /* bug #2997188 constraints not getting generated when
> > creating/replacing tables via TableCreator
> > had to add the next few lines instead of removing the
> above
> > code for backwards compatibility */
> > if (getForeignKeys().isEmpty()) {
> > //if foreignKeys is empty then we know we are not in 2.5
> > setForeignKeys(foreignKeysClone);
> > }
> >
> > Then we can use both FieldDefinition.foreignKeyFieldName (for simple
> > foreign key) and addForeignKeyConstraint (for composite foreign
> key).
> > What do you think?
> >
> > Also, I don't know there is any policy about code, but could
> I use
> > Java SE 5 Generics in schema generation codes? This makes us
> avoid
> > type-mismatch mistakes and easy to read code.
> >
> > Regards,
> > - Wonseok
> >
> >
>
>