Hi Tom,
Please see my comments inline.
On 11/2/06, Tom Ware <tom.ware_at_oracle.com> wrote:
>
> Hi Wonsoek,
>
> Answers inline.
>
> <snip>
>
> >
> > - 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.
>
> Here's an example on Oracle. I am wondering if we can support creating
> constraints on tables that are defined as follows?
>
> CREATE TABLE A_TABLE (ID INTEGER NOT NULL, B_NAME VARCHAR(20), B_TYPE
> INTEGER, PRIMARY KEY (ID))
>
> CREATE TABLE B_TABLE (ID INTEGER NOT NULL, NAME VARCHAR(20), TYPE
> INTEGER, PRIMARY KEY (ID), CONSTRAINT NAME_TYPE UNIQUE (NAME, TYPE))
>
> In this case, the combination of NAME and TYPE is a candidate key. The
> constraint would look something like the following:
>
> ALTER TABLE A_TABLE ADD CONSTRAINT B_CONSTRAINT FOREIGN KEY (B_NAME,
> B_TYPE) REFERENCES B_TABLE (NAME, TYPE)
>
> I realize that JPA probably will not currently create the Unique
> Constraint on TABLE_B, but I think it should be possible to create it
> using base-TopLink API. (TableDefinition.addUniqueKeyConstraint())
>
> Again, Perhaps I am missing something fundmental, but if possible I
> would like to avoid disallowing creating tables of that kind.
In that case, TableDefinition.addForeignKeyConstraint() can be used. The
method we are discussing is default foreign key constraints generator based
on ForeignReferenceMappings. Can ForeignReferenceMapping reference candidate
keys instead of primary keys of the target entity?
>
> >
> > > * 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?
>
>
> Good point about the public use of these methods. Go ahead and change
> the methods as you have suggested.
Do you mean I can treat them as INTERNAL methods or not? Actually I don't
see any reason they are PUBLIC methods, because there is already
addForeignKeyConstraint(). If required, we can add "Collection
getForeignKeyConstraints()" for public getter.
-Wonseok
-Tom
>
> >
> > 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.ddlgenerationpackage.
> > > * 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>
> > > <mailto: 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>>
> > > > <mailto: 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
> > > >
> > > >
> > >
> > >
> >
> >
>