Hi Tom,
Thanks for reviewing this. Please see comments inline.
On 11/1/06, Tom Ware <tom.ware_at_oracle.com> wrote:
>
> 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?
Okay, I will use local variables.
- 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.
Foreign key and primary key list come from ForeignReferenceMapping. So I
assumed that they are correct - fkFields map exactly to pkFields and
pkFields contain all composite primary keys of the target table.
Is is possible that they are not? Is there any case that composite foreign
keys do not map to the primary keys of taget table? Or the part of composite
primary keys are referenced? I couldn't think of such a case. Could you tell
me an example if it's possible?
If a user specify incorrect join columns, it might lead to such a situation.
Validation may be required, but I think schema generation is not good place
to do it, because in that case ForeignReferenceMappings are already invalid
and the problem will occur in runtime too if schema generation is not used.
Without validation an incorrect ForeignKeyConstraint may cause underlying
DatabaseException.
What do you mean by candidate keys? The method will be called in
postInitTableSchema() phase after initTableSchema() construct primary keys.
So it assume that targetTableDefinition contain complete primary keys.
> * 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.
First of all I would like to know whether getForeignKeys() and
setForeignKeys() are PUBLIC or INTERNAL method. I thought they are PUBLIC
methods so I didn't modify their signature because they may be being used by
existing code. If they are INTERNAL methods, I will modify their signature
to use Map. How about others such as get/setFields() and
get/setUniqueKeys()? Are they all INTERNAL methods?
Thanks,
-Wonseok
>
> > 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
> > >
> > >
> >
> >
>